Page MenuHome

Curves: Port fillet node to the new data-block
ClosedPublic

Authored by Hans Goudey (HooglyBoogly) on Jul 2 2022, 5:07 PM.

Details

Summary

This commit ports the fillet curves node to the new curves data-block,
and moves the fillet node implementation to the geometry module to help
separate the implementation from the node.

The changes are similar to the subdivide node or resample node. I've
resused common utilities where it makes sense, though some things like
the iteration over attributes can be generalized further. The node
is now multi-threaded per-curve and inside each curve, and some buffers
are reused per curve to avoid many allocations.

The code is more explicit now, and though there is more boilerplate to
pass around many spans, the more complex logic should be more readable.
I think things could be improved more in the future, but I don't think
I'd go much further as part of this patch.

Diff Detail

Repository
rB Blender

Event Timeline

Hans Goudey (HooglyBoogly) retitled this revision from Curves: Port fillet node to the new data-block (WIP) to Curves: Port fillet node to the new data-block.
Hans Goudey (HooglyBoogly) edited the summary of this revision. (Show Details)
  • Fixes, cleanup, more comments
Jacques Lucke (JacquesLucke) requested changes to this revision.Jul 19 2022, 1:44 PM
Jacques Lucke (JacquesLucke) added inline comments.
source/blender/blenkernel/intern/curves_utils.cc
139 ↗(On Diff #53499)

Sounds like this function could easily be more generic, i.e. not specific to curves and points.

148 ↗(On Diff #53499)

Sounds like this comment makes assumptions about the calling function.

source/blender/blenlib/BLI_index_range.hh
207

That looks very wrong.

source/blender/geometry/GEO_fillet_curves.hh
14

Unnecessary const, same below.

source/blender/geometry/intern/fillet_curves.cc
90

Avoid using the count variable name twice.

509

Redundant check of use_bezier_mode.

This revision now requires changes to proceed.Jul 19 2022, 1:44 PM
Hans Goudey (HooglyBoogly) marked 6 inline comments as done.Jul 19 2022, 8:55 PM
Hans Goudey (HooglyBoogly) added inline comments.
source/blender/blenkernel/intern/curves_utils.cc
139 ↗(On Diff #53499)

I moved it to BKE_attribute.hh now. It seems to make sense there. There is some redundancy with some of the geometry component utilities, but that's okay IMO.

source/blender/geometry/intern/fillet_curves.cc
509

Good catch. I ended up deduplicating the Bezier and Poly loops, this area is simpler now.

Hans Goudey (HooglyBoogly) marked 2 inline comments as done.
  • Remove unnecessary change
  • Deduplicate loops for Bezier and poly modes
  • Simplify radius limiting
  • Small fixes
  • Remove unnecessary const
  • Make attribute span and writer extraction more generic
  • Add test for IndexRange function (and fix it)

Having some test file for this would be good. Also to compare old and new behavior.

source/blender/blenkernel/intern/attribute_access.cc
1033

Using ATTR_DOMAIN_POINT here seems wrong.

Generally, I'm not sure if this function is a great abstraction. I was thinking about how we could standardize gathering attributes to propagate a bit better, but didn't come to a conclusion yet. For now this is fine.

source/blender/blenkernel/intern/curves_utils.cc
138 ↗(On Diff #53826)

clang format

source/blender/geometry/intern/fillet_curves.cc
79

typo (offsets)

This revision is now accepted and ready to land.Jul 19 2022, 11:13 PM
source/blender/blenkernel/intern/attribute_access.cc
1033

Oh, thanks for catching. Yeah, I'm not convinced by it either, which is why my instinct was to keep it curve-specific. But something like this would improve a fair number of places, so maybe it's an okay first step

Hans Goudey (HooglyBoogly) marked 2 inline comments as done.Jul 20 2022, 1:49 AM