Fix: Apply tilt in curves data-block normals calculation
ClosedPublic
Actions

Authored by Hans Goudey (HooglyBoogly) on Apr 14 2022, 11:21 PM.

Details

Reviewers

Jacques Lucke (JacquesLucke)

Commits

rB47d961a4b1c1: Fix: Apply tilt in curves data-block normals calculation

Summary

The ported normal calculation from ceed37fc5cbb466a0 neglected to
use the tilt attribute to rotate the normals around the tangents.
This commit adds that behavior back, adding a new math header file
to avoid duplicating the rotation function for normalized axes.

Diff Detail

Repository: rB Blender

Event Timeline

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

Hans Goudey (HooglyBoogly) created this revision.

Jacques Lucke (JacquesLucke) added inline comments.Apr 14 2022, 11:43 PM

source/blender/blenkernel/intern/curves_geometry.cc
759	Why not use an accessor methods for tilt like for other attributes?
source/blender/blenlib/BLI_math_rotation.hh
15	In a header that should be `inline`. Although this function looks like it doesn't have to be inlined.

Hans Goudey (HooglyBoogly) mentioned this in D14642: Curves: Port curve to mesh node to the new data-block.Apr 14 2022, 11:45 PM

Hans Goudey (HooglyBoogly) added a child revision: D14642: Curves: Port curve to mesh node to the new data-block.

Move function to a .cc file

source/blender/blenkernel/intern/curves_geometry.cc
759	I guess it was a decision in favor of a longer term solution (a proper attribute API on `CurvesGeometry`) rather than adding accessor methods for every single named attribute. Tilt is a pretty generic attribute in the scheme of things, which makes me think the accessor methods are not really necessary for it long term. However I don't feel strongly about that at all, I'll add it if you prefer for sure.

Jacques Lucke (JacquesLucke) accepted this revision.Apr 15 2022, 8:41 AM

Jacques Lucke (JacquesLucke) added inline comments.

source/blender/blenkernel/intern/curves_geometry.cc
759	Well, tilt has an impact on `evaluated_normals`, so imo it is at least as important as those. I think having a separate accessor would be good.
source/blender/blenlib/intern/math_rotation.cc
14	How about using something like `BLI_ASSERT_UNIT_V3`? If inputs are expected to be normalized, that should be mentioned in a comment on the declaration.