Details
Diff Detail
Event Timeline
| 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. | |
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.
| 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 | |
When in Release mode, to scale the modified object should NOT scale the collection. However if you use non-uniform scaling it does affect.
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. | |
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.
Addressed feedback and updated to master.
| source/blender/makesrna/intern/rna_nodetree.c | ||
|---|---|---|
| 449 | The object info mentions "object" as well. | |
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. 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. | |
| source/blender/makesrna/intern/rna_nodetree.c | ||
|---|---|---|
| 8922–8927 | The issue is that I think those two are wrong. Let me explain: We are not outputting the geometry relative to the input object. We are outputting the geometry with the input object transform. So it is not relative, it is literately the input object transform. 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. The correct tip would be: 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. However we could also see it as a selector for which objects transform we should 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.
| 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? | |
| 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: We don't use the info node input object transform in anyway, so we must be talking to about the modifier object here. 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.
