Geometry Nodes: new Split to Instances node #113083

Jacques Lucke · 2023-09-30T14:15:55+02:00

Jacques Lucke commented

2023-09-30 14:15:55 +02:00

This node allows splitting up a geometry into groups. A group is defined as all elements with the same group id. The output contains an instance per group. The Group ID output can be used for further deterministic processing.

The node supports meshes, curves, point clouds and instances. It only works on the top-level geometry, so it does not go into nested instances because it also generates new instances.

I also attached the file that I intend to add as a regression test.

This node allows splitting up a geometry into groups. A group is defined as all elements with the same group id. The output contains an instance per group. The `Group ID` output can be used for further deterministic processing. The node supports meshes, curves, point clouds and instances. It only works on the top-level geometry, so it does not go into nested instances because it also generates new instances. ![image](/attachments/a6a816dc-3990-494b-b38f-e699967ae3b0) I also attached the file that I intend to add as a regression test.

image.png

276 KiB

image.png

285 KiB

split_voronoi.blend

903 KiB

split_test.blend

1.0 MiB

❤️ 4 🎉 2

Jacques Lucke added 7 commits 2023-09-30 14:16:09 +02:00

Jacques Lucke added 7 commits 2023-09-30 16:20:26 +02:00

Jacques Lucke added 6 commits 2023-09-30 17:07:08 +02:00

Merge branch 'main' into split-geometry-groups a15c1be680

add comment 3add27575a

cleanup d7c78b64f4

optimize for cases when the selection is empty or there is only one group 6bdb7fd465

deduplicate common code 428127914a

cleanup 1d1ffc4a9f

Jacques Lucke added 1 commit 2023-09-30 17:35:13 +02:00

fix

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

Details

ed16e3ff8f

Jacques Lucke changed title from ~~WIP: Geometry Nodes: new Split Geometry Groups node~~ to Geometry Nodes: new Split Geometry Groups node

2023-09-30 17:58:16 +02:00

Jacques Lucke requested review from Lukas Tönne 2023-09-30 17:59:58 +02:00

Jacques Lucke requested review from Simon Thommes 2023-09-30 17:59:58 +02:00

Jacques Lucke requested review from Hans Goudey 2023-09-30 17:59:59 +02:00

Jacques Lucke commented

2023-09-30 18:05:51 +02:00

@blender-bot package

Blender Bot commented

2023-09-30 18:05:54 +02:00

Package build started. Download here when ready.

Package build started. [Download here](https://builder.blender.org/download/patch/PR113083) when ready.

Iliya Katushenock reviewed 2023-09-30 18:15:22 +02:00

source/blender/nodes/geometry/nodes/node_geo_split_geometry_groups.cc Outdated

						
				@ -0,0 +292,4 @@

				  Map<int, std::unique_ptr<GeometrySet>> geometry_by_group_id;

				  if (src_geometry.has_mesh() &&

Iliya Katushenock commented

2023-09-30 18:15:22 +02:00

What do you think about for loop on all supported components, evaluate fields / do common check, and after this pass all data to mesh/curve/... processing functions (as like in join geometry code)?

Jacques Lucke commented

2023-09-30 18:27:49 +02:00

I think I prefer the more explicit check as it is now . Right now that's easier to read and debug for me.

Brady Johnston commented

2023-10-01 03:23:28 +02:00

First-time contributor

In my testing, it has performed exceptionally well. The only enhancement I'd consider is perhaps incorporating an additional input. However, upon reflection, this might be best managed by a distinct set of instance-specific nodes.

Typically, I generate instances for animation purposes to optimize performance. It would be great to specify the origin point of the generated instance. Alternatively, having a toggle that situates the origin point at the 'center of mass' of the instance as opposed to the global origin for all instances would be advantageous.

I am just thinking in terms of rotation, or turning instances to points to then do physics calculations on, currently all instances have their origin at the world so all instances when turned to points are at the same location.

You can get around this using the Bounding Box and doing some per-instance calculations to then find where their centre of mass is, but having this be part of the initial node that creates the instances would be great.

In my testing, it has performed exceptionally well. The only enhancement I'd consider is perhaps incorporating an additional input. However, upon reflection, this might be best managed by a distinct set of instance-specific nodes. Typically, I generate instances for animation purposes to optimize performance. It would be great to specify the origin point of the generated instance. Alternatively, having a toggle that situates the origin point at the 'center of mass' of the instance as opposed to the global origin for all instances would be advantageous. ![Node screenshot](/attachments/2f88377d-67f3-438c-82b0-fcb574d0a4fb) I am just thinking in terms of rotation, or turning instances to points to then do physics calculations on, currently all instances have their origin at the world so all instances when turned to points are at the same location. You can get around this using the `Bounding Box` and doing some per-instance calculations to then find where their centre of mass is, but having this be part of the initial node that creates the instances would be great.

CleanShot 2023-10-01 at 09.22.09@2x.png

38 KiB

Jacques Lucke commented

2023-10-01 11:22:09 +02:00

Thanks for testing. Glad it works for you.

Yes, seems like setting the origin should better be done separately. I can't think of a good solution as part of this node that does the complexity of the problem justice. Simply using the average position is only one out of very many options.

Thanks for testing. Glad it works for you. Yes, seems like setting the origin should better be done separately. I can't think of a good solution as part of this node that does the complexity of the problem justice. Simply using the average position is only one out of very many options.

fwfeet commented

2023-10-01 12:31:02 +02:00

First-time contributor

It's a great node that was added, pretty interesting how it compares to this setup I was using before the node lol :)

It's a great node that was added, pretty interesting how it compares to this setup I was using before the node lol :) ![image](/attachments/584b2040-7fca-4e2f-8f3c-138dcf71c4d8)

image.png

358 KiB

Lukas Tönne commented

2023-10-02 13:48:55 +02:00

About finding origins: I agree with Jacques here, this should be a separate feature, if at all. There are too many different ways of setting an origin: centroid (center of mass), weighted centroid, median, minimum, bounds center, etc. etc. And the same applies to other attributes one might want to average over each group. The Accumulate Field node is quite useful to find a sum/average per group.

About finding origins: I agree with Jacques here, this should be a separate feature, if at all. There are too many different ways of setting an origin: centroid (center of mass), _weighted_ centroid, median, minimum, bounds center, etc. etc. And the same applies to other attributes one might want to average over each group. The _Accumulate Field_ node is quite useful to find a sum/average per group.

Lukas Tönne commented

2023-10-02 16:27:47 +02:00

I'm hitting an ASAN error with this file (just open and un-mute the split node).

FWIW i also hit an asan error in #113146, so it may not be caused by this particular patch.

I'm hitting an ASAN error with this file (just open and un-mute the split node). FWIW i also hit an asan error in #113146, so it may not be caused by this particular patch.

split_groups_01.blend

1.2 MiB

asan.txt

12 KiB

Jacques Lucke referenced this pull request

2023-10-02 17:08:48 +02:00

Fix: Geometry Nodes: dangling pointer in geometry after modifier evaluation #113160

Jacques Lucke commented

2023-10-02 17:09:24 +02:00

Good catch. It's indeed independent of this patch. I created a separate patch for the fix: #113160.

Jacques Lucke referenced this issue from a commit

2023-10-02 17:43:30 +02:00

Fix: Geometry Nodes: dangling pointer in geometry after modifier evaluation

Jacques Lucke added 1 commit 2023-10-02 20:17:31 +02:00

Merge branch 'main' into split-geometry-groups 0c5194a933

Simon Thommes reviewed 2023-10-02 21:08:58 +02:00

Simon Thommes left a comment

Very nice, behavior seems to work great!

There is some overlap in functionality with existing nodes, but that seems reasonable to me in this case.

To me the fact that the node splits the geometry into instances is more integral to the behavior imo than the fact that is uses group IDs to do so, so I'd propose the name Split Geometry into Instances, or even just Split into Instances though it's longer I feel like it's more in line with the naming scheme we have going in general. The fact that group IDs are used is more implied imo. (besides that the output should be called Instances)
It's not immediately clear to me why the Group ID output is needed, shouldn't with attribute capture it be possible to propagate to the instance domain? Or I'm guessing it would just stay an attribute on the geometry of each instance?

Very nice, behavior seems to work great! There is some overlap in functionality with existing nodes, but that seems reasonable to me in this case. - To me the fact that the node splits the geometry into instances is more integral to the behavior imo than the fact that is uses group IDs to do so, so I'd propose the name `Split Geometry into Instances`, or even just `Split into Instances` though it's longer I feel like it's more in line with the naming scheme we have going in general. The fact that group IDs are used is more implied imo. (besides that the output should be called `Instances`) - It's not immediately clear to me why the Group ID output is needed, shouldn't with attribute capture it be possible to propagate to the instance domain? Or I'm guessing it would just stay an attribute on the geometry of each instance?

Iliya Katushenock commented

2023-10-02 21:16:48 +02:00

It's not immediately clear to me why the Group ID output is needed, shouldn't with attribute capture it be possible to propagate to the instance domain? Or I'm guessing it would just stay an attribute on the geometry of each instance?

Probably due to the fact that we do not have a statistics node to transfer attributes from the internal geometry to the instance attribute.

> It's not immediately clear to me why the Group ID output is needed, shouldn't with attribute capture it be possible to propagate to the instance domain? Or I'm guessing it would just stay an attribute on the geometry of each instance? Probably due to the fact that we do not have a statistics node to transfer attributes from the internal geometry to the instance attribute.

Jacques Lucke commented

2023-10-02 22:55:13 +02:00

or even just Split into Instances

The name is fine with me. What do you think @HooglyBoogly?

It's not immediately clear to me why the Group ID output is needed, shouldn't with attribute capture it be possible to propagate to the instance domain? Or I'm guessing it would just stay an attribute on the geometry of each instance?

Yeah, basically what you said already. The group id exists on the geometry that is split. It's not an attribute on the instances on the output.

> or even just `Split into Instances` The name is fine with me. What do you think @HooglyBoogly? > It's not immediately clear to me why the Group ID output is needed, shouldn't with attribute capture it be possible to propagate to the instance domain? Or I'm guessing it would just stay an attribute on the geometry of each instance? Yeah, basically what you said already. The group id exists on the geometry that is split. It's not an attribute on the instances on the output.

👍 1

Hans Goudey commented

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

or even just Split into Instances

The name is fine with me. What do you think @HooglyBoogly?

The fact that the header is green and it's in the geometry category might imply the "Geometry" part of the name anyway, making it redundant. I'd go with "Split to Instances" though, "into" sounds a bit weird to me.

Regarding the "Group ID" output, my only concern is whether a more general solution might be needed. I'd imagine the need to transfer a "per-group" value to instances would be very common, especially for things like changing the origin point later on, or for further instance-domain processing.

Design-wise, I think the node is reasonable. For the example in the PR description though, I wonder if this solution is really better than a "Face Set Boundaries", "Split Edges", and "Set Position" combination though.

> > or even just `Split into Instances` > > The name is fine with me. What do you think @HooglyBoogly? The fact that the header is green and it's in the geometry category might imply the "Geometry" part of the name anyway, making it redundant. I'd go with "Split to Instances" though, "into" sounds a bit weird to me. --- Regarding the "Group ID" output, my only concern is whether a more general solution might be needed. I'd imagine the need to transfer a "per-group" value to instances would be very common, especially for things like changing the origin point later on, or for further instance-domain processing. --- Design-wise, I think the node is reasonable. For the example in the PR description though, I wonder if this solution is really better than a "Face Set Boundaries", "Split Edges", and "Set Position" combination though.

👍 1

Iliya Katushenock commented

2023-10-03 05:51:18 +02:00

Geometry Groups to Instances/

👍 1

Simon Thommes commented

2023-10-03 11:29:39 +02:00

Regarding the "Group ID" output, my only concern is whether a more general solution might be needed. I'd imagine the need to transfer a "per-group" value to instances would be very common, especially for things like changing the origin point later on, or for further instance-domain processing.

But this would be generally the ability to propagate attribute data from the geometry to the instancer, right? I think that would generally be useful, but it seems separate from this to me. I do agree with Hans that you might need this functionality for other generic attributes just as well, so singling out this data seems a bit strange.
I see why it exists though, so as long as we don't have a generic solution that's fine with me.

> Regarding the "Group ID" output, my only concern is whether a more general solution might be needed. I'd imagine the need to transfer a "per-group" value to instances would be very common, especially for things like changing the origin point later on, or for further instance-domain processing. But this would be generally the ability to propagate attribute data from the geometry to the instancer, right? I think that would generally be useful, but it seems separate from this to me. I do agree with Hans that you might need this functionality for other generic attributes just as well, so singling out this data seems a bit strange. I see why it exists though, so as long as we don't have a generic solution that's fine with me.

Jacques Lucke commented

2023-10-03 11:37:19 +02:00

What makes the Group ID special is that it is the only attribute we know to be the same on every element on the group. All other attributes can have variations, which also means that the user would need to decide how to reduce all the values into a single value (average, max, min, ...). Solving that more generally with a separate node seems totally reasonable.

What makes the `Group ID` special is that it is the only attribute we know to be the same on every element on the group. All other attributes can have variations, which also means that the user would need to decide how to reduce all the values into a single value (average, max, min, ...). Solving that more generally with a separate node seems totally reasonable.

👍 1

Jacques Lucke added 3 commits 2023-10-03 11:51:54 +02:00

Merge branch 'main' into split-geometry-groups 393dbb792f

rename to "Split to Instances" 1f94d86241

rename output to Instances 0cec75bdc9

Jacques Lucke changed title from ~~Geometry Nodes: new Split Geometry Groups node~~ to Geometry Nodes: new Split to Instances node

2023-10-03 11:51:54 +02:00

Simon Thommes approved these changes 2023-10-06 16:31:25 +02:00

Simon Thommes left a comment

From user side this is good to go for me now.

Just want to note that we should invest into methods to propagate attribute data from geometry to instancers.

From user side this is good to go for me now. Just want to note that we should invest into methods to propagate attribute data from geometry to instancers.

Hans Goudey added 3 commits 2023-10-06 23:14:46 +02:00

Progress a1e3ac7b32

Use IndexMask::from_groups instead of MultiValueMap 0dc0062da6

Simplify group id output 27005f017c

Do it in a separate loop. Even if it means iterating over the map twice,
this is much simpler.

Hans Goudey added 1 commit 2023-10-06 23:16:15 +02:00

Merge branch 'main' into split-geometry-groups bb0569e7ca

Hans Goudey commented

2023-10-06 23:17:11 +02:00

I'm on my laptop currently so those previous commits weren't so well tested. I'll look into this again when I'm back at my proper workstation.

Joseph Eagar referenced this issue from a commit

2023-10-07 21:46:17 +02:00

Fix: Geometry Nodes: dangling pointer in geometry after modifier evaluation

Joseph Eagar referenced this issue from a commit

2023-10-07 21:46:18 +02:00

Fix: Geometry Nodes: dangling pointer in geometry after modifier evaluation

Jacques Lucke added 1 commit 2023-10-13 14:48:17 +02:00

Merge branch 'main' into split-geometry-groups a719732880

Jacques Lucke added 2 commits 2023-10-13 15:31:48 +02:00

fix after merge aaaf59ff2d

cleanup

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

Details

a547bf3c1c

Jacques Lucke commented

2023-10-13 15:33:25 +02:00

@blender-bot build

Hans Goudey approved these changes 2023-10-16 22:06:31 +02:00

Hans Goudey left a comment

Definitely easy to do crazy terrible things! (terrible for viewport performance anyway :P)

But hopefully people using this node will already know about this sort of thing. I still think we might be able to eventually unify lists, layers, and instances to some extent. But for now this should be quite useful.

Though it would be good to improve the algorithmic complexity of the geometry copying algorithms, that can be done as a separate step.

Definitely easy to do crazy terrible things! (terrible for viewport performance anyway :P) ![image](/attachments/880bb716-0593-4ad2-80c1-b678af226bce) But hopefully people using this node will already know about this sort of thing. I still think we might be able to eventually unify lists, layers, and instances to some extent. But for now this should be quite useful. Though it would be good to improve the algorithmic complexity of the geometry copying algorithms, that can be done as a separate step.

image.png

46 KiB

source/blender/nodes/NOD_static_types.h Outdated

						
				@ -441,6 +441,7 @@ DefNode(GeometryNode, GEO_NODE_SET_SPLINE_CYCLIC, 0, "SET_SPLINE_CYCLIC", SetSpl

				DefNode(GeometryNode, GEO_NODE_SET_SPLINE_RESOLUTION, 0, "SET_SPLINE_RESOLUTION", SetSplineResolution, "Set Spline Resolution", "Control how many evaluated points should be generated on every curve segment")

				DefNode(GeometryNode, GEO_NODE_SIMULATION_INPUT, def_geo_simulation_input, "SIMULATION_INPUT", SimulationInput, "Simulation Input", "Input data for the simulation zone")

				DefNode(GeometryNode, GEO_NODE_SIMULATION_OUTPUT, def_geo_simulation_output, "SIMULATION_OUTPUT", SimulationOutput, "Simulation Output", " Output data from the simulation zone")

				DefNode(GeometryNode, GEO_NODE_SPLIT_TO_INSTANCES, 0, "Split to Instances", SplitToInstances, "Split to Instances", "Create separate geometries where each geometry contains the elements from the same group")

Hans Goudey commented

2023-10-16 22:00:09 +02:00

Simplification: "Create separate geometries containing the elements from the same group"

That's still clear IMO, but it could still say "where each contains" if you thought that helped.

Simplification: "Create separate geometries containing the elements from the same group" That's still clear IMO, but it could still say "where each contains" if you thought that helped.

JacquesLucke marked this conversation as resolved

source/blender/nodes/geometry/nodes/node_geo_split_to_instances.cc Outdated

						
				@ -0,0 +17,4 @@

				#include "UI_interface.hh"

				#include "UI_resources.hh"

				#include "DNA_pointcloud_types.h"

Hans Goudey commented

2023-10-06 18:51:44 +02:00

This is already included by BKE_pointcloud.h

This is already included by `BKE_pointcloud.h`

JacquesLucke marked this conversation as resolved

source/blender/nodes/geometry/nodes/node_geo_split_to_instances.cc Outdated

						
				@ -0,0 +39,4 @@

				      .description("All geometry groups as separate instances");

				  b.add_output<decl::Int>("Group ID")

				      .field_on_all()

				      .description("The group id of each group instance");

Hans Goudey commented

2023-10-16 22:00:39 +02:00

id -> ID

`id` -> `ID`

JacquesLucke marked this conversation as resolved

source/blender/nodes/geometry/nodes/node_geo_split_to_instances.cc

						
				@ -0,0 +229,4 @@

				      else {

				        group_curves = bke::curves_copy_curve_selection(src_curves, mask, propagation_info);

				      }

				      Curves *group_curves_id = bke::curves_new_nomain(std::move(*group_curves));

Hans Goudey commented

2023-10-16 22:05:13 +02:00

Seems to be a missing "empty" check here?

JacquesLucke marked this conversation as resolved

Hans Goudey reviewed 2023-10-16 22:23:41 +02:00

source/blender/blenkernel/intern/curves_geometry.cc

						
				@ -1232,3 +1232,3 @@

				  dst_curves.remove_attributes_based_on_types();

				  dst_curves.update_curve_types();

				  dst_curves.remove_attributes_based_on_types();