Page MenuHome

Geometry Nodes: Curve Length Node
ClosedPublic

Authored by Johnny Matthews (guitargeek) on Jun 2 2021, 7:33 AM.

Details

Summary

An implementation of https://developer.blender.org/T88725

outputs the total length of all of the splines as a float.

Diff Detail

Repository
rB Blender

Event Timeline

Johnny Matthews (guitargeek) requested review of this revision.Jun 2 2021, 7:33 AM
Johnny Matthews (guitargeek) created this revision.

Fixing a couple of terrible variable names.

This way (with mode separation) if user will need both total length and per spline length attribute at the same time, two nodes will be required instead of one.

One thing I didn't think about in the design-- it might be clearer if the geometry output was hidden in "Total" mode. That way it's clearer that the node doesn't modify the curve at all.

source/blender/blenkernel/intern/node.cc
5070

Sort this alphabetically

source/blender/makesrna/intern/rna_nodetree.c
9851 ↗(On Diff #37714)

Suggestion: Output the combined length of all splines

9856–9857 ↗(On Diff #37714)

Suggestion: Store each spline's length in an attribute on the spline domain

source/blender/nodes/CMakeLists.txt
182

Alphabetical sorting here too

source/blender/nodes/NOD_geometry.h
70

Alphabetical sorting here too

source/blender/nodes/NOD_static_types.h
309

And here too

source/blender/nodes/geometry/nodes/node_geo_curve_length.cc
18–23

Unused includes

32

I don't think this is used

94

CurveEval variables are usually just named curve elsewhere.

94

You can use a reference instead of a pointer here since you already returned early if the geometry set didn't have a curve.
Even though the compiled code will be the same, this helps to show that we don't expect the pointer to be null.

const CurveEval &curve = *curve_set.get_curve_for_read();

97

profile_spline is probably a variable name you copied from elsewhere? Just spline is better here

98

You don't need the .get() in this case

107

Don't forget to set the geometry output in this case, like other early returns in node exec functions.

113–114

This can be Span<float> span = attribute.as_span<float>();

116

You can use a C++ style loop: for (const int i : span.index_range()) {

Hans Goudey (HooglyBoogly) requested changes to this revision.Jun 2 2021, 4:37 PM
This revision now requires changes to proceed.Jun 2 2021, 4:37 PM
Johnny Matthews (guitargeek) marked 15 inline comments as done.

Fixes per @Hans Goudey (HooglyBoogly). Alphabetizing entries, Adjusting Copy, Code fixes/style in main file.

Still need to remove geometry output on total mode.

source/blender/nodes/geometry/nodes/node_geo_curve_length.cc
116

Been out of the game too long. This is new C++ syntax for me!

Merged master into my branch, had one conflict.

source/blender/nodes/geometry/nodes/node_geo_curve_length.cc
105

Crashes on setting to radius, tilt, position,

no effect on resolution, cyclic (no crash)

Can create new attribute, and update a custom attribute

Johnny Matthews (guitargeek) edited the summary of this revision. (Show Details)

Remove geometry output when in "total" mode

Johnny Matthews (guitargeek) edited the summary of this revision. (Show Details)

Removed "Per Spline" mode per the design team's direction.

Hans Goudey (HooglyBoogly) requested changes to this revision.Jun 3 2021, 8:38 PM

Just two small comments this time.

source/blender/nodes/geometry/nodes/node_geo_curve_length.cc
38

Oops, maybe you mean params.set_output("Length", 0.0f);

57–58

This isn't needed anymore either.

This revision now requires changes to proceed.Jun 3 2021, 8:38 PM
Johnny Matthews (guitargeek) marked 2 inline comments as done.Jun 3 2021, 9:08 PM

Fix error condition
Remove storage call

This revision is now accepted and ready to land.Jun 4 2021, 1:17 AM
This revision was automatically updated to reflect the committed changes.