Page MenuHome

Geometry Nodes: Remove implicit realizing and conversion
ClosedPublic

Authored by Hans Goudey (HooglyBoogly) on Oct 1 2021, 12:30 AM.

Details

Summary

This commit removes the implicit conversion from points to a mesh
that used to happen before the next modifier. It also removes the
implicit realizing of instances that happened before another modifier.

Now we have specific nodes for both of these operations, the implicit
conversions make less sense, and implicit instance realizing has already
been removed in other nodes.

Versioning
This commit adds another geometry nodes modifier before modifiers
that would have realized instances implicitly before. The way this works
is currently a bit sketchy, and needs more testing.

Diff Detail

Repository
rB Blender

Event Timeline

Jacques Lucke (JacquesLucke) requested changes to this revision.Oct 1 2021, 12:39 PM

It's not that simple unfortunately. You can't just add these nodes to every node group, because (1) it may affect node groups that are also used in other node groups and (2) it makes instances real in cases when they don't have to be made real (e.g. in the procedural buildings demo file). Two possible solutions:

  • Add a checkbox in the modifier instead that allows enabling and disabling this behavior.
  • Add another geometry nodes modifier after the existing modifier that just converts points to vertices and realizes instances. This new modifier should only be added when there are actually other modifiers afterwards.
This revision now requires changes to proceed.Oct 1 2021, 12:39 PM
  • Add a modifier after the old modifier (still needs testing)
  • Merge branch 'master' into nodes-modifier-no-realize
  • Cleanup: Add comment
  • Fix build error

Another idea I had was adding a new modifier type that wasn't exposed in the UI for this, that's probably more complicated though, and has a longer term cost for a situation which probably isn't even that common.

Another idea I had was adding a new modifier type that wasn't exposed in the UI for this, that's probably more complicated though, and has a longer term cost for a situation which probably isn't even that common.

Don't think that would be any simpler.
I think the current approach is fine. Maybe it would be good if we would not create the node group multiple times but would reuse it.
Furthermore, I'm not sure if there are any caveats when adding new id datablocks in versioning code. Maybe try to find a similar case of ask someone else.

Would be nice if the order of inputs in the Join Geometry node could be changed a bit so that it looks more clean.

Jacques Lucke (JacquesLucke) requested changes to this revision.Oct 14 2021, 2:55 PM
This revision now requires changes to proceed.Oct 14 2021, 2:55 PM
Hans Goudey (HooglyBoogly) edited the summary of this revision. (Show Details)
  • Merge branch 'master' into nodes-modifier-no-realize
  • Reorder links for prettier node tree
  • Deselect the new nodes
  • Only add the node tree once
Jacques Lucke (JacquesLucke) requested changes to this revision.Oct 18 2021, 11:11 AM
Jacques Lucke (JacquesLucke) added inline comments.
source/blender/blenkernel/intern/lib_id.c
1126 ↗(On Diff #43326)

I wonder if is_locked_for_linking should already be false when calling do_versions_after_linking_300. The after linking suggests that linking is already done at this point. Not sure what kinds of errors one can run into when creating data blocks in versioning code. (I can see that removing them can be an issue though)

source/blender/blenloader/intern/versioning_300.c
572

I think the flag is unnecessary.

This revision now requires changes to proceed.Oct 18 2021, 11:11 AM
Hans Goudey (HooglyBoogly) marked an inline comment as done.
  • Merge branch 'master' into nodes-modifier-no-realize
  • Remove unnecessary flag, bump subversion again
source/blender/blenkernel/intern/lib_id.c
1126 ↗(On Diff #43326)

The reasoning makes sense, but the file reading code is not that logical-- it's still set to true in do_versions_after_linking.

source/blender/blenloader/intern/versioning_300.c
572

Right, good point, it was at one point but I forgot to remove it.

source/blender/blenkernel/intern/lib_id.c
1126 ↗(On Diff #43326)

I think this is wrongly handled currently, this exception makes no sense to me. Created a design task for it (T92333: BMain locking during the 'after liblink' doversion).

But for the time being think you will need that hack yes, fixing this is outside the scope of that patch.

Jacques Lucke (JacquesLucke) accepted this revision.EditedOct 19 2021, 2:27 PM
  • Please add a file for testing to the original post. Ideally one that has multiple objects that require versioning.
  • Mention T92333 in the commit message.
This revision is now accepted and ready to land.Oct 19 2021, 2:27 PM