Page MenuHome

Collection Info node for the moss use case
ClosedPublic

Authored by Sebastian Parborg (zeddb) on Jan 19 2021, 5:59 PM.

Details

Summary

Here is my initial implementation of the collection node from T84603
Note that the point scatter node does not yet support instanced geometry.

Changing the collection offset does not update properly until doing a manual refresh.
Backlog task: T85274

Diff Detail

Event Timeline

Sebastian Parborg (zeddb) requested review of this revision.Jan 19 2021, 5:59 PM
Sebastian Parborg (zeddb) updated this revision to Diff 32930.
Sebastian Parborg (zeddb) created this revision.
Sebastian Parborg (zeddb) edited the summary of this revision. (Show Details)
Sebastian Parborg (zeddb) edited the summary of this revision. (Show Details)
Jacques Lucke (JacquesLucke) requested changes to this revision.Jan 20 2021, 12:46 PM
Jacques Lucke (JacquesLucke) added inline comments.
source/blender/makesrna/intern/rna_nodetree.c
449

Tooltip mentions "object".

source/blender/nodes/NOD_static_types.h
292

Don't leave a newline here.

source/blender/nodes/geometry/nodes/node_geo_collection_info.cc
64

You'll also have to take the rotation and scale of the self object into account here.
The "Original" mode seems to work fine, but the "Relative" mode does not work as I'd expect. It's quite different from the Object Info node currently.
You should probably talk to @Dalai Felinto (dfelinto) about the exact behavior we want.

This revision now requires changes to proceed.Jan 20 2021, 12:46 PM
Sebastian Parborg (zeddb) marked 3 inline comments as done.

Updated based on feed back.

Probably need to discuss the tooltips a bit more.
I'm also not sure about the offset output in "relative" mode as I'm not sure what it would be used for.

This is much closer to what I'd expect than before, but not exactly I think. I'm not sure about the offset output either, I don't know when the user would use it.

When the mode is set to Relative, I'd expect that the Geometry does not depend on the offset of the input collection.

Right now it is. You can see that by the fact that the instance does not exactly match up with the other objects in the viewport anymore, once the offset is changed.

Updated based on feedback.

source/blender/makesrna/intern/rna_nodetree.c
449

Tooltip STILL mentions "object"

8902

Can you use the same tooltip as the object info node?

Hans Goudey (HooglyBoogly) added inline comments.
source/blender/makesrna/intern/rna_nodetree.c
449

There is no "input object" in this context. I believe this should discuss the collection offset instead

Dalai Felinto (dfelinto) requested changes to this revision.Jan 21 2021, 6:01 PM

When in Release mode, to scale the modified object should NOT scale the collection. However if you use non-uniform scaling it does affect.

This revision now requires changes to proceed.Jan 21 2021, 6:01 PM

Returning early when the collection is null can simplify this function nicely IMO.

To mention what I said in chat, the definition of rna_node_geometry_collection_info_transform_space_items should be moved inside of def_geo_collection_info since it isn't used elsewhere.

I think it's nice to have the enum separate from the object info node, just so we can be more specific here.

Also, the commit / patch shouldn't be titled "Fix TXXXX", since this is adding a new feature.

source/blender/nodes/geometry/nodes/node_geo_collection_info.cc
50–51

I think this should return early at the top of the function instead, so that the rest of the function can assume it is not null.

69

This can just use collection->instance_offset here, no need for a temporary variable.

Hans Goudey (HooglyBoogly) requested changes to this revision.Jan 21 2021, 6:15 PM

When in Release mode, to scale the modified object should NOT scale the collection. However if you use non-uniform scaling it does affect.

I created T85049 to tackle this issue.

Sebastian Parborg (zeddb) marked 5 inline comments as done.
Sebastian Parborg (zeddb) retitled this revision from Fix T84603: Collection Info node for the moss use case to Collection Info node for the moss use case.

Addressed feedback and updated to master.

source/blender/makesrna/intern/rna_nodetree.c
449

The object info mentions "object" as well.
I've modified the tooltip to be more clear that it is the modifer object transform.
(Collections do not have any transforms, only offset data)

Great work. This is now working as expected in terms of transformation. I have a feeling the tooltips could more closely match the ones from object info. But I don't want to hold back the patch because of that.

Next time please remember to keep the "patch description" up to date with the planned commit message. In this particular case it would be nice to mention the task that is intended to tackle the instancing (besides removing the mention about old tooltips). Even better if it covers WHY instancing is not supported by the distribution points node.

This still needs the dependency on the collection offset. I'm not sure how tricky it is to add that. I'm fine if this is skipped for this patch, but it should be mentioned in the commit message.

The code looks good to me. Like Dalai said it's important to clarify that this outputs instances and that instances are not supported by many nodes.
Also just to be clear, I'd suggest Geometry Nodes: Add Collection Info Node for the commit title, the specific use case we're working towards is not so important to users.

source/blender/makesrna/intern/rna_nodetree.c
8922–8927

I'm also not sure why these need to be so different from the object info tooltips, aside from the obvious differences.
With the patch as is, there is no coherence between the descriptions on the two nodes.

I know this is usually not a deal breaker, but we're adding the node right now, this is the time to get it right.

Something like these mirrors the existing tooltips much more closely.
Output the geometry relative to the input object transforms and the collection offset
Bring the input object geometry into the modified object, maintaining the relative position between the objects in the scene

source/blender/makesrna/intern/rna_nodetree.c
8922–8927

The issue is that I think those two are wrong.

Let me explain:
Output the geometry relative to the input object transforms and the collection offset

We are not outputting the geometry relative to the input object. We are outputting the geometry with the input object transform.
IE we are taking the raw geometry data and applying the modifier objects obmat on it.

So it is not relative, it is literately the input object transform.
So here simply replacing "relative to" with "with" so:
Output the geometry with the input object transforms and the collection offset

Is more correct and is more verbose I feel, relatively speaking ;)

Bring the input object geometry into the modified object, maintaining the relative position between the objects in the scene

"relative position" is wrong here.
If it were relative, it would move when the modifier object moves.
Otherwise it is not maintaining the relative distance between the two objects.
Now the position it has stays the same no matter how you move the modifier object.
This is more of maintaining an absolute position if nothing else.

The correct tip would be:
Apply the modifier object transform and then its inverse to make it keep its original transform.

However something along the lines of "Keep it as is" tip is easier to understand.

After writing all this down, I realized that I think we should rethink the "Original/Relative" labels again.
This is essentially Apply/Keep transform. I don't feel there is any original/relative about these to in this case.

However we could also see it as a selector for which objects transform we should use.
So we could have something like "Modifier/Input Object" so you select which transform to use.

@Sebastian Parborg (zeddb) could we wrap this up and focus on something else instead? If tooltip is still under debate please solve this in a synchronous way, which is faster.

As for the labels Original/Relative I understand your concern. But I think we should own the decision we made as a team and move on.

This was clearly a hard naming issue to crack and I already believe there will be no name that will please everyone. If you do have a suggestion by all means. But just to encourage the discussion to restart without an actual suggestion is not the best use of everyone's time in my opinion. Specially if it means blocking this patch.

Sebastian Parborg (zeddb) edited the summary of this revision. (Show Details)
Sebastian Parborg (zeddb) marked an inline comment as done.
Sebastian Parborg (zeddb) edited the summary of this revision. (Show Details)
source/blender/makesrna/intern/rna_nodetree.c
8923

This still talks about an "input object"...

source/blender/makesrna/intern/rna_nodetree.c
8923

Isn't this talking about the modifier object though?
The collection does not have any transform (expect for the offset parameter).

source/blender/makesrna/intern/rna_nodetree.c
8923

@Sebastian Parborg (zeddb) if you look at the object info node you can see that there is "input object" and "modified object":

Relative: "Bring the input object geometry, location, rotation and scale into the modified object, maintaining the relative position between the two objects in the scene".

source/blender/makesrna/intern/rna_nodetree.c
8923

That is for the relative tool tip and not for the "original" tool tip that were a discussing now.

Other tip reads:
"Output the geometry relative to the input object transform, and the location, rotation and scale relative to the world origin."

We don't use the info node input object transform in anyway, so we must be talking to about the modifier object here.
But I synced the tooltips as requested.

However if I'm wrong then I would like to have some suggestions on what to change it to.

ORIGINAL
Output the geometry relative to the collection offset.

RELATIVE
Bring the input collection geometry into the modified object, maintaining the relative position between the objects in the scene.

I assume only the tooltips changed since I last tested it.

This revision was not accepted when it landed; it landed in state Needs Review.Feb 3 2021, 5:54 PM
This revision was automatically updated to reflect the committed changes.