Details

Reviewers

Jacques Lucke (JacquesLucke)
Bastien Montagne (mont29)

Maniphest Tasks

T82765: Support for exposing object socket to the modifier

Commits

rB89f490932f2b: Geometry Nodes: Enable exposing object and collection sockets

Summary

This patch allows connecting wires for object and collection socket
types to the "Group Input" node, which exposes them to be adjusted
in the modifier.

Thanks a lot to @Alexander Gavrilov (angavrilov) for working on this. I was working on this
as well, but Alexander fixed a few more issues than I did, so his version
is here with some edits.

The patch is composed of a few different changes:

Add code to create pointer properties in the modifier settings for object and collection sockets, and also to draw them in the UI.
Also search through the modifier's IDProperty settings to find IDs used by the modifier.
Use the special pointer layout function to draw the pointer properties in the modifier.

Diff Detail

Repository

rB Blender

Branch

geometry-nodes-object-collection-in-modifier (branched from master)

Build Status

Buildable 12076
Build 12076: arc lint + arc unit

Event Timeline

Hans Goudey (HooglyBoogly) requested review of this revision.Jan 9 2021, 3:54 AM

Hans Goudey (HooglyBoogly) created this revision.

Harbormaster completed remote builds in B12062: Diff 32563.Jan 9 2021, 3:54 AM

Hans Goudey (HooglyBoogly) added a reviewer: Jacques Lucke (JacquesLucke).Jan 9 2021, 4:07 AM

Hans Goudey (HooglyBoogly) added a project: Geometry Nodes.

Hans Goudey (HooglyBoogly) added inline comments.Jan 9 2021, 6:40 AM

source/blender/modifiers/intern/MOD_nodes.cc
85–87	It's nice to not have to repeat `blender::bke::` everywhere. There are other places to change in this file to remove that prefix, but it could be done in another commit.

As I mentioned in chat, this needs to include one more simple fix to ensure depsgraph is properly updated when these properties are changed from the ui: https://developer.blender.org/rB8390e021896a5bbe2006391785b2181c90af2940

Apply fix from Alexander

Harbormaster completed remote builds in B12076: Diff 32583.Jan 10 2021, 6:48 PM

Jacques Lucke (JacquesLucke) added inline comments.Jan 11 2021, 11:51 AM

source/blender/makesrna/intern/rna_ID.c
1116	Do you have idea for why this was not editable before?
source/blender/modifiers/intern/MOD_nodes.cc
176	This comment can be removed now.
1055	Maybe call this `bmain_ptr`, that feels more descriptive to me.

Jacques Lucke (JacquesLucke) accepted this revision.Jan 11 2021, 11:52 AM

This revision is now accepted and ready to land.Jan 11 2021, 11:52 AM

Alexander Gavrilov (angavrilov) added inline comments.Jan 11 2021, 12:19 PM

source/blender/makesrna/intern/rna_ID.c
1116	Could be multiple reasons, from the simple fact that editable is on by default for numeric properties but not pointers, to some parts of the rna code needed for assignment not being implemented - note that there were bugs in rna_access that needed to be fixed.

Hans Goudey (HooglyBoogly) marked 4 inline comments as done.Jan 11 2021, 7:28 PM

Remove TODO comment, change variable name

Harbormaster completed remote builds in B12121: Diff 32641.Jan 11 2021, 7:30 PM

Hi @Bastien Montagne (mont29)! I think it's probably a good idea to get your input here before committing this, do you mind signing off on the changes to the RNA files?

Bastien Montagne (mont29) requested changes to this revision.Jan 12 2021, 2:48 PM

Bastien Montagne (mont29) added inline comments.

source/blender/makesrna/intern/rna_access.c
2293–2298	Please commit that kind of fix separately, not together with the rest of this patch.
2294	This line is not needed, change to an ID pointer should always trigger a depsgraph relations update, regardless of the context (unless `PROP_NO_DEG_UPDATE` is set, but that should only concern pure static RNA properties afaik, and I don't think it affects any ID pointer property anyway).
2295	Hrmmm, don't think this is valid check? A `PROP_POINTER` can point to many other things than an ID, at least in the 'runtime RNA' context. You probably want to call `RNA_property_pointer_type` here, and ensure returned type is actually an ID one (`RNA_struct_is_ID`)? TBH, the whole `\|\| (prop->flag & PROP_IDPROPERTY)` in main `if` entry check of this code block looks suspicious to me (from very old rB6d2754e07d0785), as for runtime RNA properties stored as IDProperties, regular RNA property handling should work? This exception case I would expect to be only needed for actual Custom Properties (defined outside of any RNA context)? But that goes beyond the scope of this patch.
3689	Cosmetic (and useless afaict?) change, remove this.
source/blender/modifiers/intern/MOD_nodes.cc
1106	`bmain`

This revision now requires changes to proceed.Jan 12 2021, 2:48 PM

Bastien Montagne (mont29) added inline comments.Jan 12 2021, 3:05 PM

source/blender/makesrna/intern/rna_access.c
3689	Scratch that. But adding comment that `rna_idproperty_check` is needed first to get fully valid `prop` would be nice then.
3689	And this should also obviously be committed separately in its own commit

Hans Goudey (HooglyBoogly) added a project: BF Blender (2.92).Jan 12 2021, 3:27 PM

Alexander Gavrilov (angavrilov) added subscribers: Brecht Van Lommel (brecht), Campbell Barton (campbellbarton).Jan 12 2021, 3:33 PM

Alexander Gavrilov (angavrilov) added inline comments.

source/blender/makesrna/intern/rna_access.c
2294	If you looked carefully, you would see that PROP_NO_DEG_UPDATE is only checked in the `is_rna` part of this same function, which deals with properties that have a proper RNA definition and aren't bare IDProperties. `!is_rna \|\| (prop->flag & PROP_IDPROPERTY)` specifically detects IDProperties. The clear fact is that without this addition changing bare IDProperty pointers in the UI does not trigger an update.
2295	It is in no way "beyond the scope", because bare editable pointer IDProperties is exactly what this patch needs and enables in order to use them in the Nodes modifier.

@Alexander Gavrilov (angavrilov) think you did not read or understand my comments, and did not even take into account what I said on the chat. Adding noise like that is not going to make reviewing this any more easier...

Fact is, that RNA part of of this patch is:
A) not valid as-is
B) should have been submitted as separate patches, since they affect general IDProperty/RNA code, and are by no means specific to the Geometry nodes project. It just happens that those issues were not noticed before.

In D10056#251939, @Bastien Montagne (mont29) wrote:

@Alexander Gavrilov (angavrilov) think you did not read or understand my comments, and did not even take into account what I said on the chat. Adding noise like that is not going to make reviewing this any more easier...

I'm refuting a provably incorrect statement that something is "not needed", when very simple testing demonstrates it is needed. I even double-checked right now by building with and without that part.

Edit: unless by "line is not needed" you mean the comment line, in which case I was indeed confused. I mainly included it to document where to reproduce the issue.

B) should have been submitted as separate patches, since they affect general IDProperty/RNA code, and are by no means specific to the Geometry nodes project. It just happens that those issues were not noticed before.

If an issue is only noticed with new functionality, and cannot be reproduced without either using it, or writing special custom code just to demonstrate the issue, I don't see how it can even be reviewed separately. Committing as separate commits is one thing, but separate patch review submission is a totally different issue.

Added an ID pointer type check and a comment to explain rna_idproperty_check.

Harbormaster completed remote builds in B12141: Diff 32683.Jan 12 2021, 4:44 PM

Alexander Gavrilov (angavrilov) mentioned this in D10098: RNA: allow editing pointer IDProperty values from the UI..Jan 12 2021, 7:11 PM

Alexander Gavrilov (angavrilov) added a parent revision: D10098: RNA: allow editing pointer IDProperty values from the UI..Jan 12 2021, 7:11 PM

Hans Goudey (HooglyBoogly) added a task: T82765: Support for exposing object socket to the modifier.Jan 12 2021, 9:19 PM