An endpoint selection input node to replace the curve endpoint node. Start and End size determine how much of the end of the curve is
selected.
Details
Details
Diff Detail
Diff Detail
- Repository
- rB Blender
Event Timeline
Comment Actions
Just a couple basic comments inline, I think the idea is great though.
| source/blender/nodes/geometry/nodes/node_geo_curve_endpoint_selection.cc | ||
|---|---|---|
| 41–46 | I know the cost is probably very minimal, but I'd suggest using two loops instead of checking start for each iteration. | |
| 43 | It's important to only control control_point_offsets once, since the return value is an array. In other words, it has to allocate an array the size of the number of splines every time it's called. | |
Comment Actions
Other than the comment inline, this looks great to me.
| source/blender/nodes/geometry/nodes/node_geo_curve_endpoint_selection.cc | ||
|---|---|---|
| 83–88 | control_point_offsets is called down here too, it's probably best to pass it to select_by_point | |
Comment Actions
I'm not quite convinced by the general usefulness of this node. Feels like something we could easily ship as a node group at some point. Until then it does not seem to have enough use-cases to justify adding a built-in node for it. Maybe you can convince me otherwise with some better examples.
Comment Actions
Combined with the selection input to the instance on points node, this node is meant to replace the curve endpoints node. The use case there was instancing something else at the beginning and end of each spline, which is actually a very use case.
This node generalizes that idea a bit, and works well with the rest of the fields system for curves. You can instance directly on them with the same method as the "interior" points as well.
Comment Actions
Ok, I'd like to have one of the following two changes:
- Add a Both output that contains the start and end endpoints.
- Give the node an enum to change between start/end.
Comment Actions
It's fine for now, but it feels like an unnecessary limitation that the inputs are not fields that can be changed per spline.
Comment Actions
- Merge branch 'master' into endpoints
- Make the start and end lengths into fields
- Add 'both' output
Comment Actions
Looking at this again, it feels like the "Start" and "End" output should be removed and the "Both" output should be renamed to "Selection".
If someone wants to have a separate selection for start and end, two seperate nodes can be used.
What do you think?
Comment Actions
Looks pretty good, it's definitely simpler now. Just a few changes inline.
| source/blender/blenkernel/BKE_node.h | ||
|---|---|---|
| 1542 | Small thing, but it's best to just add a new line here instead of replacing the old one, so whitespace remains at the end of the list. | |
| source/blender/nodes/geometry/nodes/node_geo_curve_endpoint_selection.cc | ||
| 33 | How about the splines -> each spline? I think that's a bit clearer. | |
| 42 | Might as well use the same word-- sizes | |
| 95 | This can return early a bit above, like the other early returns. That avoids the need to indent the rest of the function, which is an improvement IMO. | |
| 97–98 | These two lines can be combined: Array<bool> selection(point_size, false); | |
| 122 | Since Field has a hash function, it doesn't need to be cast here. | |