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

Authored by Hans Goudey (HooglyBoogly) on Jul 1 2022, 6:16 AM.

Details

Reviewers

Jacques Lucke (JacquesLucke)

Maniphest Tasks

T95443: Refactor curve nodes to use new data structure

Commits

rB9435ee8c6519: Curves: Port subdivide node to the new data-block

Summary

This commit moves the subdivide curve node implementation to the
geometry module, changes it to work on the new curves data-block,
and adds support for Catmull Rom curves. Internally I also added
support for a curve domain selection. That isn't used, but it's
nice to have the option anyway.

The code uses a similar structure to the resample node (60a6fbf5b599)
and the set type node (9e393fc2f125). I've iterated a bit more on a
couple areas though, and the resample curves node can be restructured
to be more similar to this.

In general working on the curve domain feels slightly like a crutch that
complicates the whole algorithm. It might be interesting to making it work
on a per-segment level in the future.

Diff Detail

Repository: rB Blender

Event Timeline

Hans Goudey (HooglyBoogly) requested review of this revision.Jul 1 2022, 6:16 AM

Hans Goudey (HooglyBoogly) created this revision.

Hans Goudey (HooglyBoogly) mentioned this in T95443: Refactor curve nodes to use new data structure.

Hans Goudey (HooglyBoogly) retitled this revision from Geometry Nodes: Port subdivide node to the new data-block to Curves: Port subdivide node to the new data-block.Jul 1 2022, 6:22 AM

Merge master

Will continue the review another day.

source/blender/blenkernel/intern/curve_catmull_rom.cc
161	Are we not interpolating colors on purpose? Also makes me wonder whether you should use `attribute_math::convert_to_static_type` or the lower level API on `CPPType`. It seems wrong to use `attribute_math::convert_to_static_type` when you actually want a different set of types.
source/blender/geometry/GEO_subdivide_curves.hh
23	Wouldn't mention that here tbh, not sure if that helps anyone. It would only help if you could explicitly search for such places.
source/blender/geometry/intern/subdivide_curves.cc
53	typo (offsets)
65	unnecessary parenthesis, same below
88	typo (retrieve)
363	Still find it a bit hard to understand what's in this buffer exactly. Maybe a "diagram" showing what the value at every index means would help.

AgentA1cr added a subscriber: AgentA1cr.Jul 2 2022, 8:12 PM

HEYPictures (HEYPictures) awarded a token.Jul 4 2022, 9:16 AM

Hans Goudey (HooglyBoogly) marked 5 inline comments as done.Jul 4 2022, 4:53 PM

Hans Goudey (HooglyBoogly) added inline comments.

source/blender/geometry/GEO_subdivide_curves.hh
23	Maybe it could be a todo comment, but I'll just remove it. We'll change this stuff relatively soon anyway, so I don't think I'll forget it.
source/blender/geometry/intern/subdivide_curves.cc
363	I added a comment, hopefully it helps. BTW, currently the Bezier evaluated offsets are stored without the leading zero, curve offsets are stored with the zero, accumulated lengths are stored without the leading zero. Currently I prefer storing the leading zero, but I'll probably try to unify things, at least for the index offsets.

Hans Goudey (HooglyBoogly) updated this revision to Diff 53227.Jul 4 2022, 7:32 PM

Hans Goudey (HooglyBoogly) marked 2 inline comments as done.

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

Hans Goudey (HooglyBoogly) added inline comments.

source/blender/blenkernel/intern/curve_catmull_rom.cc
161	I'd like to use `attribute_math::mix4` for the basis function, but that doesn't exist yet. If it's okay, I'd like to resolve this TODO in a separate step. Now that we agreed on using `mix4`, it should be relatively simple.