Details

Reviewers

Commits

rB60a6fbf5b599: Curves: Port resample node to the new data-block

Summary

This commit re-implements the resample curve node to use the new curves
type instead of CurveEval. The largest changes come from the need to
keep track of offsets into the point array, and the fact that the
attributes for all curves are stored in a flat array.

Another difference is that a bit more of the logic is handled by
building on the field network inputs. The idea is to let the field
evaluator handle potential optimizations while making the rest of the
code simpler.

This also adds support for Catmull Rom curve inputs.

Diff Detail

Event Timeline

Hans Goudey (HooglyBoogly) requested review of this revision.Mar 24 2022, 4:51 AM

Hans Goudey (HooglyBoogly) created this revision.

Hans Goudey (HooglyBoogly) updated this revision to Diff 49718.Mar 25 2022, 5:04 PM

Hans Goudey (HooglyBoogly) retitled this revision from Curves: Port resample node to the new data-block (WIP) to Curves: Port resample node to the new data-block.

Hans Goudey (HooglyBoogly) edited the summary of this revision. (Show Details)

Hans Goudey (HooglyBoogly) added a reviewer: Jacques Lucke (JacquesLucke).

Hans Goudey (HooglyBoogly) added a parent revision: D14382: Curves: Add length cache, length paramerterize utility.Mar 25 2022, 5:07 PM

Hans Goudey (HooglyBoogly) edited the summary of this revision. (Show Details)Mar 25 2022, 9:23 PM

Hans Goudey (HooglyBoogly) set the repository for this revision to rB Blender.

Hans Goudey (HooglyBoogly) mentioned this in T95443: Refactor curve nodes to use new data structure.Mar 25 2022, 10:32 PM

Jacques Lucke (JacquesLucke) requested changes to this revision.Mar 27 2022, 2:14 PM

Jacques Lucke (JacquesLucke) added inline comments.

source/blender/nodes/geometry/nodes/node_geo_curve_resample.cc
251	Add comment saying that this is done because all curve attributes stay the same?
277	typo (`both both`)
279	Seems like it would be reasonable to have a `ResultSamples` for all the curves to avoid allocating many small vectors? I'm fine with doing that as a separate optimization.
307	Comment should mention why this is the way it works. Maybe there is a possible optimization for some curve types so that we don't need this double-interpolation? (when downsampling a curve, that could lead to a lot of overhead)
341	Should be cleared first. Otherwise this might unnecessarily copy the old bytes into the grown vector.
400–401	I'd argue that currently there is not much value in creating separate multi-functions for `great than zero` and `and`. Performance and readability might be better if you reduce the number of multi-functions. (same in `get_curve_count_field`)

This revision now requires changes to proceed.Mar 27 2022, 2:14 PM

Store sample indices and factors in separate arrays
Use a "for each group of curves: for each attribute: for each curve" pattern
Refactor gathering of attribute spans
Use the changes to the length parameterize API

source/blender/nodes/geometry/nodes/node_geo_curve_resample.cc
400–401	My idea was to make the functions simple so that it would be simple to transition them to a separate library of easier-to-build multi-functions. Sort of like how views from C++20 are combined. Maybe that's a bit preemptive though. For now I'll do what you suggest and make a larger function.

Jacques Lucke (JacquesLucke) added inline comments.Mar 30 2022, 12:47 PM

source/blender/nodes/geometry/nodes/node_geo_curve_resample.cc
224	typo, same below.
225–226	Generally, destructors shouldn't be able to fail. That's quite important for exception safety. While not strictly necessary here, I think it would be better to move the calls to `.save()` out of the destructor.
245	Add comment explaining why we have to retrieve attributes that are not interpolated.
277	typo (`curves`)
282	I don't understand this right now. Why "unselected"? Looks like the code is only doing something on selected curves.
311	typo (`the the`)
314	Maybe store `selection.slice(selection_range)` in a new variable?

Move saving out of destructor
Add comment
Store sliced selection in a avariable
Fix comments and typos
Merge branch 'master' into curves-resample-curves

source/blender/nodes/geometry/nodes/node_geo_curve_resample.cc
225–226	Right, I think you mentioned that to me before, sorry I forgot. Makes sense!

Seems to look fine now, haven't tested it in great detail though. Maybe check if tests run and also so the flower shop test before committing.

This revision is now accepted and ready to land.Mar 30 2022, 4:39 PM

Closed by commit rB60a6fbf5b599: Curves: Port resample node to the new data-block (authored by Hans Goudey (HooglyBoogly)). · Explain WhyMar 30 2022, 5:37 PM

This revision was automatically updated to reflect the committed changes.

Hans Goudey (HooglyBoogly) added a commit: rB60a6fbf5b599: Curves: Port resample node to the new data-block.

Curves: Port resample node to the new data-block
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 49631

source/blender/blenkernel/BKE_attribute_math.hh

source/blender/blenkernel/BKE_curves.hh

source/blender/blenkernel/intern/curve_bezier.cc

source/blender/blenkernel/intern/curves_geometry.cc

source/blender/blenkernel/intern/curves_geometry_test.cc

source/blender/blenlib/BLI_generic_span.hh

source/blender/blenlib/BLI_length_parameterize.hh

source/blender/blenlib/BLI_math_base.hh

source/blender/blenlib/CMakeLists.txt

source/blender/blenlib/tests/BLI_length_parameterize_test.cc

source/blender/editors/sculpt_paint/curves_sculpt_3d_brush.cc

source/blender/editors/sculpt_paint/curves_sculpt_snake_hook.cc

source/blender/functions/intern/field.cc

source/blender/makesdna/DNA_curves_types.h

source/blender/nodes/geometry/node_geometry_util.hh

source/blender/nodes/geometry/nodes/node_geo_curve_resample.cc

source/blender/nodes/geometry/nodes/node_geo_duplicate_elements.cc

source/blender/nodes/geometry/nodes/node_geo_input_spline_length.cc

Curves: Port resample node to the new data-blockClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 49631

source/blender/blenkernel/BKE_attribute_math.hh

source/blender/blenkernel/BKE_curves.hh

source/blender/blenkernel/intern/curve_bezier.cc

source/blender/blenkernel/intern/curves_geometry.cc

source/blender/blenkernel/intern/curves_geometry_test.cc

source/blender/blenlib/BLI_generic_span.hh

source/blender/blenlib/BLI_length_parameterize.hh

source/blender/blenlib/BLI_math_base.hh

source/blender/blenlib/CMakeLists.txt

source/blender/blenlib/tests/BLI_length_parameterize_test.cc

source/blender/editors/sculpt_paint/curves_sculpt_3d_brush.cc

source/blender/editors/sculpt_paint/curves_sculpt_snake_hook.cc

source/blender/functions/intern/field.cc

source/blender/makesdna/DNA_curves_types.h

source/blender/nodes/geometry/node_geometry_util.hh

source/blender/nodes/geometry/nodes/node_geo_curve_resample.cc

source/blender/nodes/geometry/nodes/node_geo_duplicate_elements.cc

source/blender/nodes/geometry/nodes/node_geo_input_spline_length.cc

Curves: Port resample node to the new data-block
ClosedPublic
Actions

Revision Contents
Changeset List