Implements the Curve Endpoints node.
Copies lots of code from the Curve to Points node.
Details
Details
- Reviewers
Hans Goudey (HooglyBoogly) - Group Reviewers
Geometry Nodes - Maniphest Tasks
- T89215: Curve Endpoints Node
- Commits
- rB63a8b3b9720c: Geometry Nodes: Curve Endpoints Node
Diff Detail
Diff Detail
- Repository
- rB Blender
- Branch
- T89215-curve-endpoints (branched from master)
- Build Status
Buildable 15446 Build 15446: arc lint + arc unit
Event Timeline
Comment Actions
Works well, nice job!
I may have found an issue with the "create_default_rotation_attribute" function in my test. Someone can work on solving that separately though, since it's shared with curve to points.
(I didn't expect to need the align rotation to vector node).
I think we can include something like this as an example in the manual.
| source/blender/nodes/geometry/nodes/node_geo_curve_endpoints.cc | ||
|---|---|---|
| 95 | Since this is the same between the two nodes, I think we can use the version in node_geo_curve_to_points.cc.
| |
| 156 | You can avoid looping over the splines twice and allocating the temporary bool array if you use a Vector as a return type instead. You can use reserve with the length of the input spline span to avoid the need for the vector to reallocate as it grows | |
| 192–196 | I think it's safe to assume that the first tilt value and the first radius value will be the same in the interpolated array. Therefore you shouldn't need to do the interpolation at all, you can just use start_data.radii[i] = spline.radii().first(); It is an assumption for sure, but I can't see why it would change. | |
| 198–201 | I don't really understand this comment. Sure, duplication isn't great, but this is not very complicated code anyway. | |
| 215 | Comments should start with /* , and end with a period. More info: https://wiki.blender.org/wiki/Style_Guide/C_Cpp | |
| 221–223 | You can make the same assumption I mentioned for the radii above. | |
| 239 | Since this function is exactly the same, I think we can generalize it slightly in the original node_geo_curve_to_points.cc file, add it to node_geometry_util.hh, and then use it here. Here's the signature I'd suggest: void curve_create_default_rotation_attribute(Span<float3> tangents, Span<float3> normals, MutableSpan<float3> rotations) | |
| 269 | Can be const Array<int> offsets | |
| source/blender/makesrna/intern/rna_nodetree.c | ||
|---|---|---|
| 9906 | Did not mean to include this in the diff | |
Comment Actions
This is quite close now, just some cleanup necessary.
| source/blender/nodes/geometry/nodes/node_geo_curve_endpoints.cc | ||
|---|---|---|
| 57–74 | These are unused now. | |
| 91 | Unused variable | |
| source/blender/nodes/geometry/nodes/node_geo_curve_to_points.cc | ||
| 141 ↗ | (On Diff #39006) | Needs clang format. I suggest setting up your editor to do that when you save a file. |
Comment Actions
Looks good, pretty simple really! There's a chance the attribute access to curves could be refactored in the future, but since this is so similar to the curve to points node, that should be easy.
I marked the places I changed when committing this in comments, just to leave a good record.
| source/blender/nodes/geometry/node_geometry_util.hh | ||
|---|---|---|
| 45 ↗ | (On Diff #39064) | I moved this down to be closer to the other functions at the bottom of this file. |
| source/blender/nodes/geometry/nodes/node_geo_curve_endpoints.cc | ||
| 86 | I changed the wording a little bit here, and used the /** format for the comment. | |
| 114 | I fixed the missing spaces in this comment. | |
| 170 | I removed this newline. | |
| 204 | And removed the newline here. | |