Details
Diff Detail
- Repository
- rB Blender
- Branch
- instances_to_points (branched from master)
- Build Status
Buildable 18106 Build 18106: arc lint + arc unit
Event Timeline
| source/blender/nodes/geometry/nodes/node_geo_instances_to_points.cc | ||
|---|---|---|
| 46 | Nitpick, I'd suggest just going with "selection", since "selection" and "mask" are redundant | |
| 58 | This and the vector can be replaced with a call to "materialize" on the virtual array. instance_positions->materialize(selection, {(float3*)pointcloud->co, pointcloud->totpoint}); | |
| 65 | Ah, I see what you meant with your question earlier-- This looks right to me, at least until this is exposed as an attribute on the instances component. | |
| 73 | This part is a little subtle, but this needs to keep the instances component as well, because of nested instancing, since this geometry set might be the owner of other geometry sets (via the instances component) that are being processed in parallel. | |
| source/blender/nodes/geometry/nodes/node_geo_instances_to_points.cc | ||
|---|---|---|
| 37 | unused variable | |
| 52 | Add a position and radius input, similar to the Mesh to Points node. | |
| 80 | I still think the node should only work on first-level-instances for now. Creating points for nested instances seems like a different use case. People would have to use a Separate Components node afterwards to get just the points for the first level instances, which is annoying. We can add an option later on that allows doing this recursively for all nested instances. Not necessary right now, but if we were to perform the operation on all instances recursively, the geometry_set.keep_only part would have to come after the if statement below. Otherwise it doesn't work in general. | |
| source/blender/nodes/geometry/nodes/node_geo_instances_to_points.cc | ||
|---|---|---|
| 112 | This also outputs the original geometry. The output should be either empty or only contain points. | |
Looks mostly good, just found one crash.
| source/blender/nodes/geometry/nodes/node_geo_instances_to_points.cc | ||
|---|---|---|
| 88 | This currently crashes when the number of points is zero, because then the attribute is not created. The best fix is to just return early further up when the index mask is empty. | |
Thanks for the patch!
I'll commit this with a few small cleanups:
- Rename variables and functions, mostly to make the names a little bit shorter
- The two things I commented about inline
- Pass the fields directly into the conversion function, use std::move (avoiding Field copy constructor)
| source/blender/nodes/geometry/nodes/node_geo_instances_to_points.cc | ||
|---|---|---|
| 69–70 | This was unused, for some reason it wasn't giving a warning though. | |
| 93 | for (const int i : selection) is a nice way to iterate directly over the indices in the mask to avoid the need for selection[i] below. | |
| source/blender/nodes/geometry/nodes/node_geo_instances_to_points.cc | ||
|---|---|---|
| 93 | And I was wrong about this again, since the unused indices are being skipped here. I'll probably work on a way to do this better at some point. | |