Details

Reviewers

Hans Goudey (HooglyBoogly)
Jacques Lucke (JacquesLucke)

Commits

rB9c004864511f: Geometry Nodes: Separate and Delete Geometry for fields

Summary

Delete Geometry:
This adds a copy of the old node in the legacy folder and updates the node to
work with fields. The invert option is removed, because it something that should
be very easy with fields, and to be consistent with other nodes which have a
selection. There is also a dropdown to select the domain, because the domain
can't be determined from the field input. When the domain does not belong on
any of the components an info message is displayed.

Separate Geometry:
A more general version of the old Point Separate node. The "inverted" output is the
same as using the delete geometry node.

Possible things to change:

The name of the outputs of the Separate Geometry node
Add option for "smooth" D10979: Smoother Mask Modifier . This is probably best for a dedicated node since it only applies to meshes.

Diff Detail

Repository

rB Blender

Branch

geo_sep_and_del (branched from master)

Build Status

Buildable 17369
Build 17369: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

I tested the patch yesterday, and it worked well! Didn't find any bug in my simple tests.

source/blender/nodes/geometry/nodes/node_geo_delete_geometry.cc
86	Copying every attribute for every element separately like that is quite inefficient. Better transfer entire attributes one by one. Besides that, I'm a bit confused that the source and destination index is both `i_src`.
841–842	Use `int` instead of `uint`. I know that the mask modifier code is still using `uint`, but that should be cleaned up.
1110	`r_error_message` isn't really an error message. Maybe something like `r_is_error` would be better. I'm generally not sure if this is the best way to do error handling here, but should be fine for now.

This revision now requires changes to proceed.Sep 24 2021, 1:48 PM

We talked about this a bit today in the meeting:

Having the separate Delete and Separate Geometry node is fine for now. In the long term the Separate Geometry node should be able to separate the input geometry into more than two geometries based on an index input.
The nodes should not realize instances. Instead they should evaluate the selection field on the point domain of the instances component and work on entire instances.

Don't realize instances

If the geometry has instances these are no longer made real. The code
I wrote for handling the instances seems to be working but I feel like
I'm missing something there.

Harbormaster completed remote builds in B17318: Diff 42437.Sep 26 2021, 1:25 PM

Wannes Malfait (Wannes) marked an inline comment as done.Sep 26 2021, 1:30 PM

Wannes Malfait (Wannes) added inline comments.

source/blender/nodes/geometry/nodes/node_geo_delete_geometry.cc
86	I went with this temporary solution, because I didn't find a way to copy it over all at once. I tried reading the CustomData API but just got confused even more.

Copy over whole attributes at once when possible.
Add description to sockets

Harbormaster completed remote builds in B17369: Diff 42542.Sep 28 2021, 12:45 PM

Can you merge master again? I moved the deprecated nodes into the legacy folder in a separate commit.

source/blender/nodes/geometry/nodes/node_geo_delete_geometry.cc
84	Would be good if you could use `gather_attributes_for_propagation`. This will take care of skipping some attributes that shouldn't be propagated (e.g. anonymous attributes that are not used anymore).
118	The same approach should be used for all the other attribute transfers.

This revision now requires changes to proceed.Sep 28 2021, 1:08 PM

Rebase master
Use gather_attributes_for_propagation

Harbormaster completed remote builds in B17389: Diff 42571.Sep 28 2021, 5:14 PM

Rebase master

Harbormaster completed remote builds in B17411: Diff 42625.Sep 29 2021, 9:11 AM

Couldn't fine any issues while testing this. Good job.
Only thing I'm thinking about again right now is whether this should have a separate instances mode and should work on instances recursively like e.g. the Curve Fill node. I'm fine with the current behavior as well though.

source/blender/nodes/geometry/nodes/node_geo_delete_geometry.cc
121	Pass `attributes` as const reference. Same below.
291	This looks correct to me.
354	Please add a comment that says that this deletes points inside of a spline. It does not split the spline apart (which I thought at first for some reason). Both behaviors seem reasonable but I'm fine if the current behavior is the one we use for now. Would be relatively easy to add the other one later on if necessary.

Cleanup: use const reference + comments

Harbormaster completed remote builds in B17435: Diff 42656.Sep 29 2021, 2:57 PM

Change instance handling to match other nodes

Harbormaster completed remote builds in B17452: Diff 42685.Sep 29 2021, 6:48 PM

Nice job with this. One design comment here, and relatively smaller comments inline.

I'm skeptical about the separate Domain and Mode inputs. Using two separate properties that depend on each other seems to be less usable than just a single list.
It gives you two things to change and it makes adding more functionality in the future more complicated. For example, it's not clear where an "Instances" mode would go.

Here's a single list more like the existing edit mode delete menu:

Points (Works on point clouds, meshes, and curves)
Edges
Faces
Only Faces
Only Edges & Faces
Splines

This leads room for future additions:

Instances
Dissolve Points (Works on curves and meshes)
Dissolve Edges
Dissolve Faces

To me this seems much more intuitive. What do you think?

source/blender/nodes/geometry/nodes/node_geo_delete_geometry.cc
54–58	Looks like the default is still true here, that should be changed.
91–100	An alternative is to take the vector by reference and return an `IndexMask`. I think this is preferable-- a bit simpler, and fewer lines for the caller. const Vector<int64_t> indices; const IndexMask mask = index_mask_indices(selection, invert, indices);
340	`Span` is small and should always be passed by value rather than by reference.
420–421	I think it's good practice to avoid passing redundant arguments where possible. Here the `in_component` argument can be retrieved from the geometry set inside the function. Though actually, when the geometry set needs to be passed anyway, I think a pattern where there isn't a second geometry set is simpler: separate_point_cloud_selection(geometry_set, selection_field, invert); const PointCloudComponent &src_points = geometry_set.get_component_for_read<PointCloudComponent>(); ... PointCloud pointcloud = BKE_pointcloud_new_nomain(total); PointCloudComponent dst_points; dst_points.replace(pointcloud, GeometryOwnershipType::Editable); ... geometry_set.replace_pointcloud(pointcloud);
423	`Field` contains a shared pointer, so to avoid the need for reference counting changes and the small overhead associated with them, it should be passed by reference where possible.
432	This can simply use `get_evaluated_as_mask(0)`
1098	Does this have to run for all methods? Seems like just the "Only Faces" mode needs this.
1144	Extra white-space here.
1182	Instead of building `some_component` here, this could use `geometry_set.has_realized_data()`

This revision now requires changes to proceed.Sep 29 2021, 9:53 PM

Cleanup: address small comments
Refactor to use only one geometry set

Harbormaster completed remote builds in B17491: Diff 42778.Oct 1 2021, 2:08 PM

I'm skeptical about the separate Domain and Mode inputs. Using two separate properties that depend on each other seems to be less usable than just a single list.
It gives you two things to change and it makes adding more functionality in the future more complicated. For example, it's not clear where an "Instances" mode would go.

Here's a single list more like the existing edit mode delete menu:
Points (Works on point clouds, meshes, and curves)
Edges
Faces
Only Faces
Only Edges & Faces
Splines
This leads room for future additions:
Instances
Dissolve Points (Works on curves and meshes)
Dissolve Edges
Dissolve Faces
To me this seems much more intuitive. What do you think?

I don't actually think that there is a "good" way to do it. The reason I went with this is, that doing "Only Faces" works differently when evaluated on vertices edges or faces. If there is only one Enum then there would be only one (hard-coded) domain to evaluate the field on for each mode. I see the first enum like the options in edit mode: "Vertex select mode", "Edge select mode" or "Face select mode", these are also separated there from the deletion option.

At the same time I feel like there is a better way to do this than the one I went with. Like you said adding more options would be difficult. It's also possible that this is because the node is trying to be "too general", i.e. it should be split into smaller nodes that have more specific functionality. I feel this way because the second enum is only relevant for meshes.

For the "Instances" mode I would just place it in the "domain" enum.

source/blender/nodes/geometry/nodes/node_geo_delete_geometry.cc
54–58	In D12574#329824, @Zijun Zhou (Eary) wrote: Just thought I would mention it here. I posted on devtalk just now that I believe the node should delete everything by default, because nobody would add a delete node but not deleting anything, while deleting everthing is actually a use case. https://devtalk.blender.org/t/delete-geometry-vs-separate-geometry/20522/17?u=eary @Hans Goudey (HooglyBoogly) Because of this I left the default as `true`. I don't think there is any compelling reason to set the default to `false`.
432	I would use it, but I need to be able to invert the selection, which is not possible when using `get_evaluated_as_mask(0)`. I also didn't want to change the function to support inverting because that feels like it's something outside the scope of this patch.
1098	I think this is necessary in all cases, except maybe the "Only Faces" mode since that doesn't touch any edges or vertices. But maybe I misunderstood what a "loose edge" is.

Rebase master

Harbormaster completed remote builds in B17511: Diff 42817.Oct 2 2021, 5:45 PM

Rebase master

Harbormaster completed remote builds in B17551: Diff 42895.Oct 4 2021, 5:13 PM

Hans Goudey (HooglyBoogly) mentioned this in D12551: Geometry Nodes: Fields Version of Point Separate.Oct 6 2021, 2:08 PM

I think the second output of the Separate Geometry (Inverted) is not working at the moment:

separate-geometry.blend135 KBDownload

As for their names maybe:

Selected
Non-Selected

Fix bug on second output of Separate Geometry

@Dalai Felinto (dfelinto) This should now be fixed. In the example file you need to change the random type to boolean for it to work. If you really want to use floats I think setting the min and max to -1 and 1 should work, because negatives are considered falsey.

Harbormaster completed remote builds in B17622: Diff 43005.Oct 7 2021, 11:36 AM

Sorry for the delay in responding. I see your point here, the idea of the separate domain and mode choice does help keep the complexity down in that way.

However, I think only the mode options for the current "domain" should display. That would probably be simplest to implement if there was a separate mode property for each domain.

This revision now requires changes to proceed.Oct 7 2021, 3:59 PM

Cleanup: Unused variables
Only show the mode for relevant domains

Currently there is only a mode that is specific to meshes. In the future
when there are more domains with specific modes this can be changed.

Harbormaster completed remote builds in B17630: Diff 43014.Oct 7 2021, 4:45 PM

I found an assert with curve inputs and the delete node. Other than that, this seems mostly fine to me.

Delete Geometry Assert.blend838 KBDownload

source/blender/nodes/geometry/nodes/node_geo_delete_geometry.cc
54–58	I think that reason is more of a hack honestly-- we shouldn't really design to support work-arounds like that. But I don't feel too strongly, and "nobody would add a delete node but not deleting anything," is a bit convincing.

This revision now requires changes to proceed.Oct 9 2021, 8:32 PM

Fix splines using wrong selection index

Bad copy paste from the old attribute system led to problems when there
were multiple splines: they all looked at the start of the selection,
instead of their actual place in the selection.

Harbormaster completed remote builds in B17685: Diff 43095.Oct 10 2021, 10:47 AM

Hans Goudey (HooglyBoogly) accepted this revision.Oct 11 2021, 3:31 PM

This revision is now accepted and ready to land.Oct 11 2021, 3:31 PM

Closed by commit rB9c004864511f: Geometry Nodes: Separate and Delete Geometry for fields (authored by Wannes Malfait (Wannes), committed by Hans Goudey (HooglyBoogly)). · Explain WhyOct 11 2021, 3:38 PM

This revision was automatically updated to reflect the committed changes.

Hans Goudey (HooglyBoogly) added a commit: rB9c004864511f: Geometry Nodes: Separate and Delete Geometry for fields.

Geometry Nodes: Separate + Delete Geometry for fields
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 42542

release/scripts/startup/nodeitems_builtins.py

source/blender/blenkernel/BKE_node.h

source/blender/blenkernel/intern/node.cc

source/blender/makesdna/DNA_node_types.h

source/blender/makesrna/RNA_enum_items.h

source/blender/makesrna/intern/rna_attribute.c

source/blender/makesrna/intern/rna_nodetree.c

source/blender/nodes/CMakeLists.txt

source/blender/nodes/NOD_geometry.h

source/blender/nodes/NOD_static_types.h

source/blender/nodes/geometry/node_geometry_util.hh

source/blender/nodes/geometry/nodes/legacy/node_geo_delete_geometry.cc

source/blender/nodes/geometry/nodes/node_geo_delete_geometry.cc

source/blender/nodes/geometry/nodes/node_geo_separate_geometry.cc

Geometry Nodes: Separate + Delete Geometry for fieldsClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 42542

release/scripts/startup/nodeitems_builtins.py

source/blender/blenkernel/BKE_node.h

source/blender/blenkernel/intern/node.cc

source/blender/makesdna/DNA_node_types.h

source/blender/makesrna/RNA_enum_items.h

source/blender/makesrna/intern/rna_attribute.c

source/blender/makesrna/intern/rna_nodetree.c

source/blender/nodes/CMakeLists.txt

source/blender/nodes/NOD_geometry.h

source/blender/nodes/NOD_static_types.h

source/blender/nodes/geometry/node_geometry_util.hh

source/blender/nodes/geometry/nodes/legacy/node_geo_delete_geometry.cc

source/blender/nodes/geometry/nodes/node_geo_delete_geometry.cc

source/blender/nodes/geometry/nodes/node_geo_separate_geometry.cc

Geometry Nodes: Separate + Delete Geometry for fields
ClosedPublic
Actions

Revision Contents
Changeset List