Geometry Nodes: Add selection and depth options to realize instances #104832

Angus Stanton · 2023-02-16T18:03:57+01:00

Angus Stanton commented

2023-02-16 18:03:57 +01:00

First-time contributor

Adds selection and depth options to realize instances.

Angus Stanton added 14 commits 2023-02-16 18:04:09 +01:00

21f96afc7e Add selection field to realize instances

Works by only realizing instances in selection, and copying instances
not in selection over to new geometry set

Differential Revision: https://developer.blender.org/D16743

9ec14f7446 const correctness

2a9df9b46d Address style comments

b062b4bb8c Fixes:

- Style changes to auto
- Don't use selection at depths > 0

115fc990b2 Add depth option to realize instances

- Moves join geometry code from node to GEO_join_geometry.hh and
  join_geometry.cc

ab69f6d66b Make transforms VArray const ref

c531488033 Merge branch 'master' into add-selection-realize-instances

4fc0c2da73 Merge branch 'master' into add-selection-realize-instances

242dc8a407 Update to use attribute propagation

b8ea715239 Formatting

a28a9bc0df Address comments

e492b2de48 Merge branch 'master' into add-selection-realize-instances

f1d361266e Merge branch 'main' into add-selection-realize-instances

ffa53a4fb2 Fix default value of depth

Hans Goudey added the

labels 2023-02-16 18:13:41 +01:00

Hans Goudey added this to the Nodes & Physics project 2023-02-16 18:13:47 +01:00

Hans Goudey requested review from Jacques Lucke 2023-02-16 18:13:56 +01:00

Hans Goudey requested review from Hans Goudey 2023-02-16 18:13:56 +01:00

Hans Goudey approved these changes 2023-02-16 19:21:44 +01:00

Hans Goudey left a comment

This is working well and my tests, and the implementation seems reasonable.
I'll add a couple more reviewers though.

This is working well and my tests, and the implementation seems reasonable. I'll add a couple more reviewers though.

source/blender/geometry/intern/realize_instances.cc

						
				@ -1455,0 +1502,4 @@

				    GeometrySet &new_geometry_set,

				    const bke::AnonymousAttributePropagationInfo &propagation_info)

				{

				  const Instances *instances = geometry_set.get_instances_for_read();

Hans Goudey commented

2023-02-16 19:10:06 +01:00

Add a null check or use a reference here with *

Add a null check or use a reference here with `*`

ab_stanton marked this conversation as resolved

source/blender/nodes/geometry/nodes/node_geo_join_geometry.cc

						
				@ -3,3 +2,1 @@

				#include "GEO_realize_instances.hh"

				#include "BKE_instances.hh"

				#include "GEO_join_geometry.hh"

Hans Goudey commented

2023-02-16 19:01:29 +01:00

Missing newline in between license and includes

ab_stanton marked this conversation as resolved

source/blender/nodes/geometry/nodes/node_geo_realize_instances.cc

						
				@ -12,6 +14,8 @@ namespace blender::nodes::node_geo_realize_instances_cc {

				static void node_declare(NodeDeclarationBuilder &b)

				{

				  b.add_input<decl::Geometry>(N_("Geometry"));

				  b.add_input<decl::Bool>(N_("Selection")).default_value(true).hide_value().supports_field();

Hans Goudey commented

2023-02-16 19:05:36 +01:00

Suggest a description like Which top-level instances to realize

Suggest a description like `Which top-level instances to realize`

ab_stanton marked this conversation as resolved

source/blender/nodes/geometry/nodes/node_geo_realize_instances.cc

						
				@ -13,2 +15,4 @@

				{

				  b.add_input<decl::Geometry>(N_("Geometry"));

				  b.add_input<decl::Bool>(N_("Selection")).default_value(true).hide_value().supports_field();

				  b.add_input<decl::Int>(N_("Depth")).default_value(0).supports_field();

Hans Goudey commented

2023-02-16 19:06:41 +01:00

Maybe How many levels of nested instances to realize at each top-level instance. Beyond the depth, further instances won't be realized

Though that's probably a bit too long.

Maybe `How many levels of nested instances to realize at each top-level instance. Beyond the depth, further instances won't be realized` Though that's probably a bit too long.

Angus Stanton commented

2023-02-16 19:45:10 +01:00

First-time contributor

Just removed the last sentence, seems intuitive enough to me to leave it out

HooglyBoogly marked this conversation as resolved

source/blender/nodes/geometry/nodes/node_geo_realize_instances.cc

						
				@ -43,0 +48,4 @@

				  Field<bool> selection_field = params.extract_input<Field<bool>>("Selection");

				  Field<int> depth_field = params.extract_input<Field<int>>("Depth");

				  const bke::Instances &instances = *geometry_set.get_instances_for_read();

Hans Goudey commented

2023-02-16 19:00:49 +01:00

The whitespace here doesn't really help IMO. I'd suggest putting all of these in one block:

  const bke::InstancesFieldContext field_context{instances};
  fn::FieldEvaluator evaluator{field_context, instances.instances_num()};
  evaluator.set_selection(params.extract_input<Field<bool>>("Selection"));
  evaluator.add(params.extract_input<Field<int>>("Depth"));
  evaluator.evaluate();
  const IndexMask selection = evaluator.get_evaluated_selection_as_mask();
  const VArray<int> depths = evaluator.get_evaluated<int>(0);

This is subjective, but generally blank lines are only helpful when used a bit more sparingly, to separate the code into meaningful sections. Just like paragraphs of prose I guess.

The whitespace here doesn't really help IMO. I'd suggest putting all of these in one block: ``` const bke::InstancesFieldContext field_context{instances}; fn::FieldEvaluator evaluator{field_context, instances.instances_num()}; evaluator.set_selection(params.extract_input<Field<bool>>("Selection")); evaluator.add(params.extract_input<Field<int>>("Depth")); evaluator.evaluate(); const IndexMask selection = evaluator.get_evaluated_selection_as_mask(); const VArray<int> depths = evaluator.get_evaluated<int>(0); ``` This is subjective, but generally blank lines are only helpful when used a bit more sparingly, to separate the code into meaningful sections. Just like paragraphs of prose I guess.

ab_stanton marked this conversation as resolved

Hans Goudey requested review from Simon Thommes 2023-02-16 19:22:01 +01:00

Angus Stanton added 1 commit 2023-02-16 19:44:07 +01:00

7416cfe064 Address comments

Simon Thommes reviewed 2023-02-17 11:55:32 +01:00

Simon Thommes left a comment

This sounds like a good idea to me overall! I don't have time to properly test it this week though, but I can take a look next week.

Simon Thommes requested changes 2023-02-20 12:01:05 +01:00

Simon Thommes left a comment

Testing the node I'm getting a crash in the attached file, when trying to set the depth higher than 0.

negative depth doesn't make any sense in this context, so there should be a soft min
to me intuitively the depth 0 implies no realization at all, I would propose to define the default and current behavior as 1. I think it would be fine to also allow the input 0 and use it like a selection. I can see that being useful in certain cases (similar to Duplicate Elements)

Testing the node I'm getting a crash in the attached file, when trying to set the depth higher than 0. - negative depth doesn't make any sense in this context, so there should be a soft min - to me intuitively the depth 0 implies no realization at all, I would propose to define the default and current behavior as 1. I think it would be fine to also allow the input 0 and use it like a selection. I can see that being useful in certain cases (similar to `Duplicate Elements`)

crash_realize_depth.blend

914 KiB

Simon Thommes commented

2023-02-20 12:06:06 +01:00

I just noticed that I built the patch on top of the release branch. Let me know if you can replicate the crash. If you can't it might be that and I'll try again properly.

Angus Stanton commented

2023-02-20 12:51:59 +01:00

First-time contributor

I agree re a depth of 0 seeming like it should be no realising, however doing 1 doesn't make sense to me either, as I think the user would expect all the instances to be realised by default

Simon Thommes commented

2023-02-20 13:50:57 +01:00

I agree re a depth of 0 seeming like it should be no realising, however doing 1 doesn't make sense to me either, as I think the user would expect all the instances to be realised by default

Right, I didn't consider this aspect, but that's absolutely correct.
Hm, the only options I see to resolve this dilemma then is to either

add an enum to input a depth or not, or to
add -1 as a default option that handles this.

To be honest I'm not a fan of either of these. Is there maybe a better solution that I'm missing?

Btw: I tried my test file again with the build based on main and it still crashes.

> I agree re a depth of 0 seeming like it should be no realising, however doing 1 doesn't make sense to me either, as I think the user would expect all the instances to be realised by default Right, I didn't consider this aspect, but that's absolutely correct. Hm, the only options I see to resolve this dilemma then is to either - add an enum to input a depth or not, or to - add -1 as a default option that handles this. To be honest I'm not a fan of either of these. Is there maybe a better solution that I'm missing? Btw: I tried my test file again with the build based on main and it still crashes.

Jacques Lucke requested changes 2023-02-23 14:05:36 +01:00

Jacques Lucke left a comment

I't currently possible to loose attributes with this node, consider the example below. The test attribute should be available on the vertices in the end. Note that this does work when I mute the first Realize Instances node.

I agree that a Depth of 0 should just not change the geometry at all. I think a boolean node property Realize All is reasonable and results in the least surprising behavior.

I't currently possible to loose attributes with this node, consider the example below. The `test` attribute should be available on the vertices in the end. Note that this does work when I mute the first `Realize Instances` node. ![image](/attachments/8e5fd0f1-144b-489e-88af-3bdba4b25c5c) I agree that a `Depth` of 0 should just not change the geometry at all. I think a boolean node property `Realize All` is reasonable and results in the least surprising behavior.

realize_instances_depth.blend

833 KiB

image.png

290 KiB

source/blender/nodes/geometry/nodes/node_geo_realize_instances.cc

						
				@ -43,0 +62,4 @@

				  evaluator.set_selection(selection_field);

				  evaluator.add(depth_field);

				  evaluator.evaluate();

				  const VArray<int> depths = evaluator.get_evaluated<int>(0);

Jacques Lucke commented

2023-02-23 14:00:11 +01:00

It looks like later you expect there to be a depth only per selected instance, but here you get an array that contains a depth for every instance (not only selected ones, although the unselected instances might be uninitialized).

source/blender/nodes/geometry/nodes/node_geo_realize_instances.cc

						
				@ -43,0 +69,4 @@

				      "Geometry");

				  geometry_set = geometry::realize_instances(

				      geometry_set, {legacy_behavior, !legacy_behavior, selection, depths, propagation_info});