WIP: Geometry Nodes: make instance handling more explicit when modifying all real geometries #113933

Jacques Lucke · 2023-10-19T13:50:57+02:00

Jacques Lucke commented

2023-10-19 13:50:57 +02:00

When using modify_geometry_sets one had to be a bit careful before that one does not do any operations on the geometry-set that would cause a crash or race condition. For example, one was not allowed to modify/remove the instance references of the original instances.

With this refactor, the purpose of the function (processing all real geometries) becomes a bit more explicit. Furthermore, the instance handling is changed so that one does not have to be as careful anymore. That's because the original instances are removed before the callback is called and are added back afterwards.

TODO:

Figure out how to reuse the join geometry code in blenkernel.

When using `modify_geometry_sets` one had to be a bit careful before that one does not do any operations on the geometry-set that would cause a crash or race condition. For example, one was not allowed to modify/remove the instance references of the original instances. With this refactor, the purpose of the function (processing all real geometries) becomes a bit more explicit. Furthermore, the instance handling is changed so that one does not have to be as careful anymore. That's because the original instances are removed before the callback is called and are added back afterwards. TODO: * Figure out how to reuse the join geometry code in blenkernel.

👍 1

Jacques Lucke added 2 commits 2023-10-19 13:51:11 +02:00

cf755692a2 progress

dda93752cc improve naming

Jacques Lucke added 1 commit 2023-10-19 14:03:16 +02:00

buildbot/vexp-code-patch-coordinator Build done. Details

289bee4e96 cleanup

Jacques Lucke requested review from Hans Goudey 2023-10-19 14:03:47 +02:00

Jacques Lucke commented

2023-10-19 14:03:54 +02:00

@blender-bot build

Hans Goudey commented

2023-10-19 15:27:33 +02:00

Maybe just the join_instances and join_attributes code could move to blenkernel (instances.cc)?

Maybe just the `join_instances` and `join_attributes` code could move to blenkernel (`instances.cc`)?

Jacques Lucke commented

2023-10-20 13:56:59 +02:00

Hm, doesn't really sound like a scalable solution to me. Feels like with this strategy we'll just end up moving more and more code into blenkernel which (at least from my perspective) doesn't really help anyone.

Maybe we could move modify_geometry_sets to the geometry module instead. There is currently one use of it in blenkernel, but that could potentially be replaced with something else. Bit of a bummer in cases like this that we don't want to add the direct dependency from blenkernel to the geometry module, even though they depend on each other indirectly already probably.

Hm, doesn't really sound like a scalable solution to me. Feels like with this strategy we'll just end up moving more and more code into blenkernel which (at least from my perspective) doesn't really help anyone. Maybe we could move `modify_geometry_sets` to the geometry module instead. There is currently one use of it in blenkernel, but that could potentially be replaced with something else. Bit of a bummer in cases like this that we don't want to add the direct dependency from blenkernel to the geometry module, even though they depend on each other indirectly already probably.

Hans Goudey commented

2023-10-23 18:24:23 +02:00

I don't have a strong opinion honestly, other than that I find the one way dependency between geometry and blenkernel helpful, and I wish more of our dependencies were like that :)

I don't have a strong opinion honestly, other than that I find the one way dependency between `geometry` and `blenkernel` helpful, and I wish more of our dependencies were like that :)

buildbot/vexp-code-patch-coordinator Build done.

Details

This pull request has changes conflicting with the target branch.

source/blender/blenkernel/BKE_geometry_set.hh
source/blender/blenkernel/intern/bake_items.cc
source/blender/nodes/geometry/nodes/node_geo_curve_fill.cc
source/blender/nodes/geometry/nodes/node_geo_mean_filter_sdf_volume.cc
source/blender/nodes/geometry/nodes/node_geo_mesh_to_curve.cc
source/blender/nodes/geometry/nodes/node_geo_mesh_to_sdf_volume.cc
source/blender/nodes/geometry/nodes/node_geo_offset_sdf_volume.cc
source/blender/nodes/geometry/nodes/node_geo_points_to_sdf_volume.cc
source/blender/nodes/geometry/nodes/node_geo_points_to_volume.cc
source/blender/nodes/geometry/nodes/node_geo_separate_geometry.cc

View command line instructions.

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u modify-real-geometries:JacquesLucke-modify-real-geometries

git checkout JacquesLucke-modify-real-geometries

Sign in to join this conversation.

No reviewers

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

WIP: Geometry Nodes: make instance handling more explicit when modifying all real geometries #113933

Checkout