Rotation C++ API #104941
No reviewers
Labels
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset Browser
Interest
Asset Browser Project Overview
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
EEVEE & Viewport
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
EEVEE & Viewport
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Priority
High
Priority
Low
Priority
Normal
Priority
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
No Milestone
No project
No Assignees
6 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: blender/blender#104941
Loading…
Reference in New Issue
No description provided.
Delete Branch "fclem/blender:bli-math-rotation-cpp"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
This patch re-implement the whole C rotation API into a more type
oriented C++ API. See the #104444 design task for more details about
the goals.
The list of C to C++ equivalent syntax can be found attached.
This adds
AngleRadian
,AngleCartesian
andAngleFraction
asdifferent angle types with the same interface. Each of them have
specific benefits / cons. See inline documentation for detail.
This adds
Axis
andAxisSigned
class wrapped enums to increase typesafety with axes selection.
This adds
CartesianBasis
to represent orthonormal orientations.Added a weight accumulation to dual-quaternions to make normalization
error proof. Creates the overhead of summing the total weight twice
(which I think is negligible) and add an extra float.
Named the dual-quaternion
DualQuaternion
to avoid naming ambiguitywith
DualQuat
which come up quite often (even with namespace).Only skimmed over it so far, generally seems reasonable. Will have to take a closer look and do some testing next week.
@ -0,0 +246,4 @@
* This should be private in theory. */
/**
* Parity of axis permutation (even=0, odd=1) - 'n' in original code.
original code
?I looked up the meaning online and added more context to the comment.
@ -0,0 +350,4 @@
stream << " .trans = " << rot.trans << "\n";
if (rot.scale_weight != T(0)) {
stream << " .scale = " << rot.scale;
stream << " .scale_weight = " << rot.scale_weight << "\n)\n";
Looks like the closing
)
should move to the line below.@ -0,0 +23,4 @@
template<typename T, typename AngleT> struct AxisAngle {
using vec3_type = VecBase<T, 3>;
protected:
Should probably be
private
.protected
doesn't make sense when you don't derive from it.@ -0,0 +28,4 @@
AngleT angle_ = AngleT::identity();
/**
* A defaulted constructor would cause zero initialization instead of default initialization,
I'm not sure about the purpose of this. I'd just use
= default
and make it public.Some general questions about
BLI_math_euler*
, as this is the first bit I happened to look at.What's the reason to go for such a "
this
alias"? What's the added value of usingeul.x()
overthis->x()
or justx()
?I know the style guide says "Local variables should be short and to the point." but I think this is too short. Also the "ijk" convention is explained in a different file (the code is in
BLI_math_euler.hh
, whereas the explanation sits inBLI_math_euler_types.hh
).I'm not sure what the word "typical" means here. How is a typical (X, Y, Z) 3D rotation different from a non-typical one? And what does "runtime axis order" mean?
My guess is that
EulerXYZ
is always in XYZ order, and thatEuler3
can be in arbitrary order, but that isn't clear from the explanation.Not all constructors take a complete set of rotation values, so this is ambiguous.
What is the advantage of
ijk()
? I find it rather confusing that this apparently means that the contained rotations can be in any order (so ZXY rotation seems to usei=Z
,j=X
,k=Y
). But then there is this:where
Euler3(eAxis::X, 3.14, ZXY)
writes to the first element and not the second.This to me is a blocking design issue.
The "but cannot be assigned" is unexpected. What does it mean? And what does it imply about the
ijk()
function?I don't think this sentence add anything to the rest of the text. So far all the text has been about both types, so it might be clearer to just start the block of text with a statement that explains that, and then avoid repeating this.
@dr.sybren
This is in a conversion operator. Using
this
hides the type.This was copy paste from old code. Will improve the variable names. Also I think it uses
ijh
instead ofijk
convention.Yes, I'll reword that into a clearer explanation.
You are right.
ijk
is mostly for internal use of the type but given the functional style of the API, I cannot make it fully private.I had though about storing the values already shuffled. But that change the mindset quite a bit from the old API where data isn't shuffled.
I now understand that
xyz
has only meaning forEulerXYZ
and is quite ambiguous forEuler3
. I will useijk_shuffled()
for internal use and document it properly.Concerning this constructor I think it just better to remove it.
Well you cannot assign directly to a shuffled vector. The shuffled vector needs to be a copy. You can only assign to the stored order.
Edit: We could enable shuffled assignment, but that's a whole other level of dirtiness.
I don't understand. Literally two lines above that the function declaration is
template<typename T> EulerXYZ<T>::operator Quaternion<T>() const
-- how is it unclear thatthis
is a pointer to aEulerXYZ<T>
?Regardless, it should be crystal clear what the meaning of these fields are.
I'm still uncertain what "shuffled" and "unshuffled" mean.
Why? Was it buggy? Ill-defined? I don't like it when I use something as an example of uncertainty, and then the "solution" is to remove that example. It doesn't solve the underlying problem.
To me it would be preferred to have the whole "ijk" concept properly defined first, instead of renaming functions or removing code.
The problem was mostly linguistic. When you write "but cannot X" without ever saying anything about "X" to begin with, you kind of force the reader to re-read the previous parts again, now in the understanding that they may be able to "X" as well.
Overall it seems like you've put a lot of effort into building a solid foundation with types that can help us make good performance decisions in the future. The documentation is wonderful. Also, just having code that clearly has care put into it will help a lot as it grows in the future. Great job!
I don't have the best foundation with this type of math, so for my review I focused on the code quality and language aspects. I'm happy to take a look anywhere else if that would be helpful though.
I'd suggest that a new API based on
std::numeric_limits
or something else should replace theM_PI
andFLT_EPSILON
defines currently brought in from the old C headers. That could happen later though.@ -0,0 +8,4 @@
* Classes to represent rotation angles. They can be used as 2D rotation or as building blocks for
* other rotation types.
*
* Each `blender::math::Angle***<T>` implement the same interface and can be swapped easily.
Grammar:
implement
->implements
operations
->each operation's
group of angle
->group of angles
function overload
->function overloads
creating rotation
->creating rotations
2d
->2D
it is offers
->it offers
when needing trigonometric values of an angle and not
->when trigonometric values of an angle are required but not
Writing style:
your
->a
Subjective, but IMO this is a bit more formal and sounds more like proper technical documentationFast : Everything not slow.
andSlow : Everything not fast.
I think I can guess what you're getting at here, but the phrasing is weird, and they seem a bit contradictory to theFast :
list items.Range :
->Range:
Usually there isn't a space between labels and colons.@ -0,0 +14,4 @@
*
* This design allows some function overload to be more efficient with certain types.
*
* A `blender::math::AngleRadian<T>` is your typical radian angle.
I would suggest moving the section describing each type to directly above its definition.
The comment at the top of the file is about the general goals of the API, and the comment above each type is about the specifics of the type and when it should be used.
@ -0,0 +298,4 @@
friend AngleCartesian operator/(const AngleCartesian &a, const T &b)
{
if (b == T(2)) {
/* Still costly but less than using `atan()`. */
Grammar:
but less than
->but faster than
is range of
->is in the range
@ -0,0 +52,4 @@
*this = identity();
return;
}
else {
else after return
@ -0,0 +5,4 @@
/** \file
* \ingroup bli
*
* A `blender::math::AxisAngle<T>` represent a rotation around a unit axis.
Grammar:
represent a
->represents a
and thus faster
->and are thus faster
and they needs
->; they must
Writing style:
I'd also recommend putting this comment above the
AxisAngle
type.@ -0,0 +11,4 @@
* axes. This type of rotation is fast, precise and adds more meaning to the code that uses it.
*
* A practical reminder:
* - Forward is typically the positive Y direction in blender.
Grammar:
blender
->Blender
For cross product forward
->For cross product, forward
@ -0,0 +20,4 @@
* The basis changes for each space:
* - Object: X-right, Y-forward, Z-up
* - World: X-right, Y-forward, Z-up
* - Curve Tangent-Space: X-left, Y-up, Z-forward
It's sort of nice to nice to have stuff like
Curve Tangent-Space
documented here, but if it doesn't refer to some other area of code, it's hard to know what it's referring to really.Looking at the use of
math::from_orthonormal_axes
infill_mesh_positions
incurve_to_mesh_convert.cc
(which I'd say would be the canonical example at this point?) it's hard to tell if this is true exactly.And those a wrong even! I was super confused by them but I'll fix that in another PR.
It should be
math::from_orthonormal_axes_up_left<float4x4>(normals[i], tangents[i])
or something combined with the change of basis to then convert to blender world space.BevPoint
have this orientation coming out ofget_curve_points_from_idx
. So I think it is a pretty safe assumption to make.If another basis was used (chosen?) for geometry node curves it will have to be added here for sure.
@ -0,0 +33,4 @@
/** \name Conversion
* \{ */
enum eAxis {
In #98553 and related design tasks there was general agreement that the
e
prefix doesn't make sense. That hasn't been acted on yet, but it seems nice to follow that for these types.What about
math::Axis
? andmath::SignedAxis
? Alternatively, if those names were too vague to people, they could get a3D
prefix or suffix.Hell yeah, I hated thoses prefixes.
@ -0,0 +57,4 @@
/** \name Axes utilities.
* \{ */
inline eAxis axis_unsigned(const eAxisSigned axis)
What about using
enum class
for the axes and making this a constructor?I think
enum class
is generally considered best, and also allows making some of these functions into class methods, which might be nice for better auto-complete and namespacing.@ -0,0 +6,4 @@
* \ingroup bli
*
* A `blender::math::EulerXYZ` represent a typical (X, Y, Z) euler 3D rotation.
* A `blender::math::Euler3` represent a euler 3D rotation with runtime axis order.
Grammar:
a euler
->an Euler
ijk()` represent` ->
ijk()represents
is not suited for many application
->are not suited for many applications
stored as radian
->stored as radians
asignment
->assignment
@ -0,0 +202,4 @@
template<typename T> struct Euler3 {
public:
private:
/** Raw rotation values (in radian) as passed by the constructor. */
Grammar:
in radian
->in radians
@ -0,0 +334,4 @@
bool parity() const
{
switch (order_) {
default:
I'd expect a
BLI_assert_unreachable()
in the default case, right?@ -253,1 +262,4 @@
[[nodiscard]] inline detail::EulerXYZ<T> to_euler(const MatBase<T, 4, 4> &mat);
template<typename T, bool Normalized = false>
[[nodiscard]] inline detail::Euler3<T> to_euler(const MatBase<T, 3, 3> &mat,
const eEulerOrder order);
const eEulerOrder
->eEulerOrder
In headers (definitions)
const
is meaningless for arguments passed by value. and usually just takes up space. It only matters in the declaration.@ -922,0 +1091,4 @@
else {
detail::normalized_to_eul2(normalize(mat), eul1, eul2);
}
/* Return best, which is just the one with lowest values it in. */
Grammar:
values it in
->values in it
@ -0,0 +87,4 @@
* \{ */
/**
* Transform \a p by rotation using the quaternion \a q .
\a p
->\a v
@ -0,0 +607,4 @@
*/
template<typename T>
[[nodiscard]] VecBase<T, 3> transform_point(const detail::DualQuaternion<T> &dq,
VecBase<T, 3> &point,
It's a bit weird for this function to modify its argument and also return a new vector. Maybe I'm missing something?
@ -0,0 +9,4 @@
*
* Mainly used for rigging and armature deformations as they have nice mathematical properties
* (eg: smooth shortest path interpolation). A `blender::math::Quaternion<T>` is cheaper to combine
* than `MatBase<T, 3, 3>`. However, transforming points is not. Consider converting to a rotation
transforming points is not
->transforming points is slower
information :
->information:
This mean the
->This means the
@ -246,3 +8,1 @@
/**
* Intermediate Types.
* Rotation Types
Do you think we actually need the
BLI_math_rotation_types.hh
header? Seems like including the headers in here might be fine, and potentially better for compile times.I would think it was more for ease of use, discoverability and documentation purpose. But it can be removed if not wanted.
I guess this could be changed later if compile time is investigated
@ -249,2 +8,3 @@
* Rotation Types
*
* Some functions need to have higher precision than standard floats for some operations.
* They give more semantic informations and allow overloaded functions based on rotation type.
Grammar:
more semantic informations and
->more semantic information and
@ -865,0 +987,4 @@
/**
* From:
* "Skinning with Dual Quaternions"
* Ladislav Kavan, Steven Collins, Jiri Zara, Carol O’Sullivan
I'd avoid the unicode char
’
and just make itCarol O'Sullivan
WIP: Rotation C++ APIto Rotation C++ APII got it to a point where I see no other changes to make.
EulerBase<AngleT>
TemplateI made use of
AngleRadian<T>
insideEulerBase
so that we could make it templated if we wanted (since other angle types share the same interface). This is just a preemptive decision to avoid large refactor down the line.However I could not find a good incentive / use case for even having it templated. So maybe someone have ideas? Different
AngleT
could bring performance and precision difference but their limitation make them unfit to many uses.I did try to convert to use an
AngleT
template parameter forEulerBase
but the changes were too many and it seemed to make too much complex changes.In the meantime I don't think this is a blocking issue.
Euler3::ijk
euler3.ijk()
is now the 3 angles in rotation order. It makes so much more sense like this. I made it so that it is assignable (but I don't see a lot of usecase).We might consider making
Euler3::order
mutable but I would like to find a good use case for that first.Other points
There are left over discussion points in the PR message.
Namely the
cross(AxisSigned, AxisSigned)
behavior. Should we have aAxis::NULL
for correct behavior?The other topic is the dual-quaternions changes.
Some changes we typed while @fclem and I did a shoudler-to-shoulder IRL code dive.
To summarize on the above open questions:
We picked
BLI_assert()
to make these cases visible in debug builds, and still returning an arbitrary axis that is perpendicular to botha
andb
so that there are no special cases as far as the calling code is concerned.I think it's fine.
Approving now, with the note that @fclem got the patch of the stuff we discussed now 😻 :)
@blender-bot build
@blender-bot build
@blender-bot build
@blender-bot build
@blender-bot build
@blender-bot build
@ -0,0 +38,4 @@
*/
class Axis {
public:
enum class Value : int {
What about backing this with a smaller type like
int8_t
? That would help if these were stored in an array, which sounds like something that might happen. Same with the signed enum.@ -0,0 +79,4 @@
int as_int() const
{
return static_cast<int>(axis_);
Style guide mentions using functional casts for cases like this:
int(axis_)
@blender-bot build
@ -0,0 +41,4 @@
ZYX,
};
inline std::ostream &operator<<(std::ostream &stream, EulerOrder order)
Can be moved to an implementation file.
@ -0,0 +228,4 @@
EulerOrder order_;
/**
* Swizzle structure allowing to rotation ordered assignement.
typo (assignement)
@ -23,3 +23,3 @@
*/
struct DrawGroup {
/** Index of next #DrawGroup from the same header. */
/** Index of next #DrawGroup from the same header. (Only for CPU) */
Looks unrelated.
@ -197,3 +197,3 @@
{
ShadowTileMap tilemap(0 * SHADOW_TILEDATA_PER_TILEMAP);
tilemap.sync_cubeface(float4x4::identity(), 0.01f, 1.0f, Z_NEG, 0.0f);
tilemap.sync_cubeface(float4x4::identity(), 0.01f, 1.0f, eCubeFace::Z_NEG, 0.0f);
Looks unrelated.
@blender-bot build
Just noticed a few more small things
@ -0,0 +268,4 @@
* https://en.wikipedia.org/wiki/List_of_trigonometric_identities
* (see Angle_sum_and_difference_identities, Multiple-angle_formulae and Half-angle_formulae)
*
* There is no identities for (arbitrary) product or quotient of angles.
Grammar:
There is no identities
->There are no identities
@ -0,0 +371,4 @@
} // namespace detail
/**
* A `blender::math::AngleFraction<T>` stores the radian angle as quotient.
Grammar:
the radian angle
->a radian angle
@ -0,0 +374,4 @@
* A `blender::math::AngleFraction<T>` stores the radian angle as quotient.
* - Storage : `2 * sizeof(int64_t)`
* - Range : [-INT64_MAX..INT64_MAX] but angle must be expressed as fraction (be in Q subset).
* - Fast : Everything not slow.
Think I commented about this elsewhere, but it's confusing to read "everything not slow" and then "slow: ..." on the next line. Maybe the first line could use different wording?
I'm not sure how to be more explicit. Care to suggest anything better?
TBH I don't understand what this means. "Everything not slow" and "Slow: A, B, C" just don't fit logically
How about:
Much better!
@ -0,0 +377,4 @@
* - Fast : Everything not slow.
* - Slow : `cos()`, `sin()`, `tan()` for angles not optimized.
*
* It offers the best accuracy fo fractions of Pi radian angles. For instance
Typo:
fo
->for
@ -0,0 +379,4 @@
*
* It offers the best accuracy fo fractions of Pi radian angles. For instance
* `sin(AngleFraction::tau() * n - AngleFraction::pi() / 2)` will exactly return `-1` for any `n`
* within [-INT_MAX..INT_MAX]. This holds true even with really high radian values.
Writing style:
really
->very
Just sounds more professional IMO
@ -0,0 +5,4 @@
/** \file
* \ingroup bli
*
* An `blender::math::CartesianBasis` represents an orientation that is aligned with the basis
Similar to what we did elsewhere, it's probably better to have this comment about
CartesianBasis
right above the class itself.@ -0,0 +111,4 @@
enum class Value : int8_t {
/* Match #eTrackToAxis_Modes */
/* Must start at 0. Used as indices in tables and vectors. */
X_POS = 0,
Other enum classes in the codebase use camel case for each value. What do you think about doing that here?
AxisSigned::XPos
That's not part of the code-style and not documented anywhere I know of.
DNA still uses uppercase so I think it should follow that until we turn this style into a convention.
@ -0,0 +232,4 @@
/**
* Returns the cross direction from two basis direction using the right hand rule.
* Way faster than true cross product if the vectors are basis vectors.
Grammar/Writing style:
Way faster than
->This is much faster than
@ -0,0 +234,4 @@
* Returns the cross direction from two basis direction using the right hand rule.
* Way faster than true cross product if the vectors are basis vectors.
* Any ill-formed case will return a orthogonal axis to \a a but will also trigger an assert. It is
* better filter these cases upstream.
Grammar:
It is better filter
->It is better to filter
@ -230,2 +230,4 @@
const VectorT up);
/**
* This return a version of \a mat with orthonormal basis axes.
Grammar:
This return a
->This returns a
However, just saying "Returns a..." is probably better than having "This" at the start
@ -0,0 +5,4 @@
/** \file
* \ingroup bli
*
* A `blender::math::Quaternion<T>` represents either an orientation or a rotation.
I also think these comments would fit better right above the relevant classes.
@ -225,0 +396,4 @@
template<typename T> detail::Quaternion<T> to_quaternion(const CartesianBasis &rotation)
{
/**
* There is only 6 * 4 = 24 possible valid orthonormal orientations.
Grammar:
There is
->There are
@blender-bot build