Add a point selection to the fillet curve node so that it can be used as an IndexMask to skip filleting unselected points.
This addresses T100031.
Details
Details
- Reviewers
Hans Goudey (HooglyBoogly) - Maniphest Tasks
- T100031: Fillet Curve Node: Add a point selection input
Diff Detail
Diff Detail
- Repository
- rB Blender
- Branch
- fillet-curve-selection (branched from master)
- Build Status
Buildable 23823 Build 23823: arc lint + arc unit
Event Timeline
| source/blender/blenloader/intern/versioning_300.c | ||
|---|---|---|
| 1296 ↗ | (On Diff #56116) | I don't know how to test the versioning behavior, so not sure this is doing the right thing... Specifically,
|
| source/blender/geometry/intern/fillet_curves.cc | ||
| 82 | Inverting selection is counter intuitive, but is the most space efficient way I can think of. Open to suggestions though :) | |
Comment Actions
Sorry for the long delay. Thanks for looking into this!
Looking at the code now, it seems the curve selection is now redundant. In retrospect I probably shouldn't have added that.
What do you think about removing the curve selection from the code and replacing it with the point selection?
That might make the code a bit simpler.
| source/blender/blenlib/BLI_index_range.hh | ||
|---|---|---|
| 239 ↗ | (On Diff #56116) | Usually we try to be explicit about using this-> before member functions just to be clearer. |
| source/blender/blenloader/intern/versioning_300.c | ||
| 1296 ↗ | (On Diff #56116) | Generally I create a file with all of the inputs plugged into other nodes in an old version.
No, it doesn't the sockets are always there, just hidden. It's a bit weird.
Yes, definitely! I'd recommend nodeFindSocket. |
| source/blender/nodes/geometry/nodes/node_geo_curve_fillet.cc | ||
| 32–35 | On other nodes the selection goes right below the geometry input, it would probably make sense to be consistent here. | |
Comment Actions
Looking at the code now, it seems the curve selection is now redundant. In retrospect I probably shouldn't have added that.
What do you think about removing the curve selection from the code and replacing it with the point selection?
That might make the code a bit simpler.
Sounds good! I've removed the curve selection param. Seems simpler overall. PTAL.
| source/blender/blenloader/intern/versioning_300.c | ||
|---|---|---|
| 1296 ↗ | (On Diff #56116) | Niice thanks for the steps! Tried it out and seems to be working. The input to the radius socket in an older version now connects to radius and selection sockets. |
| source/blender/nodes/geometry/nodes/node_geo_curve_fillet.cc | ||
| 32–35 | Moved. | |
Comment Actions
Thanks for the update! Apart from calculate_overlapping_ranges being a bit hard to follow, this seems like a good solution.
My inline comments are mostly about code cleanups.
I found a crash in the "Bezier" mode that came with this message:
Code marked as unreachable has been executed. Please report this as a bug. Error found at source/blender/nodes/NOD_geometry_exec.hh:268 in get_input_index.
Can you reproduce that?
| source/blender/blenloader/intern/versioning_300.c | ||
|---|---|---|
| 1287 ↗ | (On Diff #57114) | It can be helpful to add a check like this so the versioning code doesn't do anything bad even if it ends up running twice somehow. if (nodeFindSocket(node, SOCK_IN, "Selection")) {
continue;
} |
| 1288 ↗ | (On Diff #57114) | /home/hans/Blender-Git/blender/source/blender/blenloader/intern/versioning_300.cc:1955:33: warning: unused variable ‘mode’ [-Wunused-variable]
1955 | GeometryNodeCurveFilletMode mode = GeometryNodeCurveFilletMode(storage.mode);
| ^~~~ |
| source/blender/geometry/intern/fillet_curves.cc | ||
| 73 | /home/hans/Blender-Git/blender/source/blender/geometry/intern/fillet_curves.cc:108:59: warning: unused parameter ‘radii’ [-Wunused-parameter]
108 | const VArray<float> &radii,
| ~~~~~~~~~~~~~~~~~~~~~^~~~~ | |
| 74 | ||
| 88 | src_curves.curves_range().slice(range)) is equivalent to just range The same thing appears a couple times in the patch | |
| 122–123 | Better type safety and less use of macros is always nice | |
| 124 | This should be equivalent to src_points.one_after_last() | |
| source/blender/nodes/geometry/nodes/node_geo_curve_fillet.cc | ||
| 113–114 | You could pass the selected_points variable here instead of retrieving it again, right? | |
Comment Actions
Addressed comments. PTAL :)
Attempted to improve calculate_overlapping_ranges documentation but not sure if I made it more confusing lol.
I found a crash in the "Bezier" mode that came with this message
Code marked as unreachable has been executed. Please report this as a bug.
Error found at source/blender/nodes/NOD_geometry_exec.hh:268 in get_input_index.
Yeah I got repro. Interesting, it seems to relate to the input socket definition ordering in node_geo_curve_fillet.cc. Moving "Selection" to right after "Curve" caused the crash, and moving "Selection" back fixed it.
The line throwing the assert error was in GeoNodeExecParams::check_input_access:
else if (found_socket->flag & SOCK_UNAVAIL) {
std::cout << "The socket corresponding to the identifier '" << identifier
<< "' is disabled.\n";
BLI_assert_unreachable();
}Any idea why the ordering matters?
| source/blender/blenloader/intern/versioning_300.c | ||
|---|---|---|
| 1288 ↗ | (On Diff #57114) | Oops thanks for catching it. Not sure why IDE didn't help me with that. Time to look into my IDE setup... :P |
| source/blender/nodes/geometry/nodes/node_geo_curve_fillet.cc | ||
|---|---|---|
| 55 | The order of sockets matters with this problem because of the socket order is used here to hide the "count" socket in Bezier mode IIRC. This is a common pattern in nodes that hide or show sockets. It's really brittle and we're hoping to improve it in the future by moving this info to the node declaration above | |
Comment Actions
Friendly ping here. I'd love to get this merged some time if it looks alright. All comments should have been addressed but let me know if anything needs updating. Thanks!
Comment Actions
Thank you for your work, I hope my comments will weak help to go to the day when it will be ready.
Not very specific to the implementation (using functions like extract_ranges_invert seems suspicious to me) so I think for now just add comments on the form.
And also, I think it will be useful to study this experience ralated OffsetIndices, like this : rB7fc395354cd1: Cleanup: Use offset indices arguments for curves utilities
| source/blender/geometry/intern/fillet_curves.cc | ||
|---|---|---|
| 91 | I'm not sure about this, so it's just a question. Is it possible to skip max() below if you just do 0 here? | |
| 108 | ||
| 120–130 | ||
| source/blender/geometry/intern/fillet_curves.cc | ||
|---|---|---|
| 120–130 | const IndexRange intersection = unselected_ranges.intersect(src_points); for (const int index : intersection.index_range().shift(src_points.start() - intersection.start())) { point_counts[index] = 1; } | |
Comment Actions
I haven't done performance tests. I start from simplicity and conciseness.
- Add IndexMask point selection to fillet_curves
- To calculate_result_offsets, perform initialization with filling 1. Then write down the number of fillets using the IndexMask. Calculate offsets based on this.
Given the existence of the implementation (this patch), it would be good either to see it with the merging with master (to test it) or just to find out the results of the check with my variant.
The main problem seems to me the processing of ranges. This looks like a weak optimization on a random selection. And at the same time, not cheap for memory. On the other hand, a filling by 1 is what is most likely to have a cost close to 0 (compared to any other algorithm).
| source/blender/nodes/geometry/nodes/node_geo_curve_fillet.cc | ||
|---|---|---|
| 84–121 | It seems that now more than one argument needs to be received in both places. Better to do it once. Other opinions are possible, just sensations | |
Comment Actions
The remaining thing I described badly in my last inline comment was ordering the selection input right after the geometry socket like other nodes. Then the retrieval of the specific socket in node_update needs to be updated.
| source/blender/blenloader/intern/versioning_300.c | ||
|---|---|---|
| 1289–1293 ↗ | (On Diff #57842) | Comment formatting: https://wiki.blender.org/wiki/Style_Guide/C_Cpp#Comments |