Refactor: Changes to new curves data-block
ClosedPublic
Actions

Authored by Hans Goudey (HooglyBoogly) on Feb 1 2022, 11:21 PM.

Details

Reviewers

Jacques Lucke (JacquesLucke)
Brecht Van Lommel (brecht)

Maniphest Tasks

T95355: New Curves data block

Commits

rBf59767ff9729: Curves: Changes to the new curves data-block

Summary

This patch refactors the "Hair" data-block, which will soon be renamed to "Curves".
The larger change is switching from an array of HairCurve to find indices in the
points array to simply storing an array of offsets. Using a single integer instead of
two halves the amount of memory for that particular array. I've also found using
offsets directly works well in CurveEval::control_point_offsets() and
evaluated_point_offsets.

Besides that, there are some other changes in this patch:

Split the data-structure to a separate CurveGeometry DNA struct so it is usable for grease pencil too.
Update naming to be more pleasant, and more aligned with newer code and the style guide.
Add direct access to arrays in RNA
- Radius is now retrieved as a regular attribute.
- HairPoint has been replaced with CurvePoint
- HairCurve has been replaced with CurveSlice
Add comments to the struct in DNA.

The next steps are renaming Hair -> Curves, and adding support for other
curve types, Bezier, Poly, and NURBS.

Ref T95355

Diff Detail

Event Timeline

Hans Goudey (HooglyBoogly) requested review of this revision.Feb 1 2022, 11:21 PM

Hans Goudey (HooglyBoogly) created this revision.

Looks like a step in the right direction for me.
For the python api we could consider keeping some utility to get the number of points of a curve without having to do the subtraction manually. But that can be added later. Raw access to the data is necessary either way.

While some of the decisions made here are debatable (regarding naming and data layout), I'm fine with the decisions made.

source/blender/blenkernel/intern/hair.cc
290	This does not normalize in-place.

This revision is now accepted and ready to land.Feb 2 2022, 11:54 AM

Jacques Lucke (JacquesLucke) added 1 blocking reviewer(s): Brecht Van Lommel (brecht).Feb 2 2022, 11:55 AM

This revision now requires review to proceed.Feb 2 2022, 11:55 AM

Fix math::normalize issue in previous cleanup commit

Brecht Van Lommel (brecht) requested changes to this revision.Feb 3 2022, 1:07 PM

Brecht Van Lommel (brecht) added inline comments.

intern/cycles/blender/curves.cpp
653	No need to convert to ustring, just compare `b_attribute.name() == "radius"` directly.
source/blender/blenkernel/intern/customdata.cc
1796–1799	Just remove it, no need to deprecate. Since all of the custom data will be lost (since it moves to `.geometry`), there is no chance of such layers being read from .blend files.
source/blender/makesrna/intern/rna_hair.c
184–190	`point_size` and `curve_size` feels inconsistent with other data structures in our API, where you usually just use `len()` on something. I'd prefer to keep: hair.points hair.curves And then have also arrays for fast access: hair.position_data hair.curve_offset_data I'd don't want to give you a lot of work, but I feel we should get this API right when we make it public.

This revision now requires changes to proceed.Feb 3 2022, 1:07 PM

Use direct comparison to "name"
Add back "points" and "curves" to RNA (Eventually we will have curves.curves :P)
Rename some properties

Brecht Van Lommel (brecht) accepted this revision.Feb 3 2022, 5:02 PM

Brecht Van Lommel (brecht) added inline comments.

intern/cycles/blender/curves.cpp
694–695	Remove debug prints.

This revision is now accepted and ready to land.Feb 3 2022, 5:02 PM

Hans Goudey (HooglyBoogly) added inline comments.Feb 3 2022, 5:11 PM

intern/cycles/blender/curves.cpp
694–695	Oops, thanks.
source/blender/blenkernel/intern/customdata.cc
1796–1799	I wish I could, but I don't think I can remove it without changing the values of other custom data types, since this array is meant to be indexed with the type.

Closed by commit rBf59767ff9729: Curves: Changes to the new curves data-block (authored by Hans Goudey (HooglyBoogly)). · Explain WhyFeb 3 2022, 5:50 PM

This revision was automatically updated to reflect the committed changes.

Hans Goudey (HooglyBoogly) added a commit: rBf59767ff9729: Curves: Changes to the new curves data-block.

Brecht Van Lommel (brecht) added inline comments.Feb 3 2022, 6:29 PM

source/blender/blenkernel/intern/customdata.cc
1796–1799	Ah, well at least it should be safe to reuse the indices for something else then. So they could be renamed to something like `CD_UNUSED1` with a comment.