Rotation C++ API #104941

Merged
Clément Foucault merged 80 commits from fclem/blender:bli-math-rotation-cpp into main 2023-03-09 18:15:33 +01:00

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 and AngleFraction as
different angle types with the same interface. Each of them have
specific benefits / cons. See inline documentation for detail.

This adds Axis and AxisSigned class wrapped enums to increase type
safety 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 ambiguity
with DualQuat which come up quite often (even with namespace).

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` and `AngleFraction` as different angle types with the same interface. Each of them have specific benefits / cons. See inline documentation for detail. This adds `Axis` and `AxisSigned` class wrapped enums to increase type safety 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 ambiguity with `DualQuat` which come up quite often (even with namespace).
Clément Foucault added this to the 3.6 LTS milestone 2023-02-19 02:00:35 +01:00
Clément Foucault added the
Interest
Core
Module
Core
labels 2023-02-19 02:00:35 +01:00
Clément Foucault added 40 commits 2023-02-19 02:00:51 +01:00
5c8182f683 Quaternion: Fix internal component order and remove operator[]
# Conflicts:
#	source/blender/blenlib/CMakeLists.txt
fa5ffc3584 Quaternion: Optimize `from_triangle` by replacing matrix inversion
Replace by using quaternion conjugate and quaternion rotation.
Results seems to be the same.
28822926c9 Rotation: Add `eAxisSigned` and utilities
This adds better safety and semantic.
fe28377ec6 Quaternion: Add `from_vector` with simplification and optimization
This is the perf timings for 1.000.000 calls
comparing against `vec_to_quat` (Old):

|Build  | Old     | New     | Factor |
|-------|---------|---------|--------|
|Debug  | 200.8ms | 576.4ms | x2.88  |
|Release| 134.5ms | 92.9ms  | x0.69  |
767b1d3ecb EulerXYZ: Add wrapped_around()
This replace compatible_eul().
8500f0b4bd Rotation: Add rotate(RotationT, RotationT) to rotate rotation types
This is a simple composition function that adds more semantic info.
Also follows `rotate(MatT, MatT)`.
5300811751 AxisConversion: Move axis_conversion to a class to use it as a rotation
This make it more into the paradigm that rotations can be converted to
other rotations and used with `from_rotation` matrix template.
61f8eefc8a Cleanup: Replace `rotate` by `transform_point`
This avoid confusion between both. `rotate` being for rotating an
orientation.
5580f5002b AxisAngle: Refactor to use different angle representation
This make use of template to change how the angle is stored.
This avoid duplicating all the constructors.

The downside is that the setup cost is a bit hidden behind the AxisAngle
type.

The AngleSinCos type could be used in other cases such as 2D rotation.

Naming convention is up to debate.
Clément Foucault added 3 commits 2023-02-19 18:22:01 +01:00
Jacques Lucke requested changes 2023-02-19 18:37:19 +01:00
Jacques Lucke left a comment
Member

Only skimmed over it so far, generally seems reasonable. Will have to take a closer look and do some testing next week.

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.
Member

original code?

`original code`?
Author
Member

I looked up the meaning online and added more context to the comment.

I looked up the meaning online and added more context to the comment.
fclem marked this conversation as resolved
@ -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";
Member

Looks like the closing ) should move to the line below.

Looks like the closing `)` should move to the line below.
fclem marked this conversation as resolved
Jacques Lucke reviewed 2023-02-19 18:39:27 +01:00
@ -0,0 +23,4 @@
template<typename T, typename AngleT> struct AxisAngle {
using vec3_type = VecBase<T, 3>;
protected:
Member

Should probably be private. protected doesn't make sense when you don't derive from it.

Should probably be `private`. `protected` doesn't make sense when you don't derive from it.
fclem marked this conversation as resolved
Jacques Lucke reviewed 2023-02-19 18:40:03 +01:00
@ -0,0 +28,4 @@
AngleT angle_ = AngleT::identity();
/**
* A defaulted constructor would cause zero initialization instead of default initialization,
Member

I'm not sure about the purpose of this. I'd just use = default and make it public.

I'm not sure about the purpose of this. I'd just use `= default` and make it public.
fclem marked this conversation as resolved
Clément Foucault added 8 commits 2023-02-20 02:27:58 +01:00
Clément Foucault requested review from Sergey Sharybin 2023-02-20 12:13:37 +01:00
Sybren A. Stüvel added the
Interest
Animation & Rigging
label 2023-02-20 12:47:08 +01:00
Clément Foucault requested review from Sybren A. Stüvel 2023-02-20 13:11:57 +01:00
Clément Foucault requested review from Hans Goudey 2023-02-20 13:12:20 +01:00
Sybren A. Stüvel requested changes 2023-02-20 15:12:07 +01:00
Sybren A. Stüvel left a comment
Member

Some general questions about BLI_math_euler*, as this is the first bit I happened to look at.

const EulerXYZ<T> &eul = *this;

What's the reason to go for such a "this alias"? What's the added value of using eul.x() over this->x() or just x()?

  const T ti = eul.x() * T(0.5);
  const T tj = eul.y() * T(0.5);
  const T th = eul.z() * T(0.5);
  const T ci = math::cos(ti);
  const T cj = math::cos(tj);
  const T ch = math::cos(th);
  const T si = math::sin(ti);
  const T sj = math::sin(tj);
  const T sh = math::sin(th);
  const T cc = ci * ch;
  const T cs = ci * sh;
  const T sc = si * ch;
  const T ss = si * sh;

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 in BLI_math_euler_types.hh).

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.

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 that Euler3 can be in arbitrary order, but that isn't clear from the explanation.

For both types ijk() represent the rotation values in the order given to the constructor.

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 use i=Z, j=X, k=Y). But then there is this:

  Euler3(const eAxis axis, T angle, eEulerOrder order) : ijk_(0), order_(order)
  {
    ijk_[axis] = angle;
  }

where Euler3(eAxis::X, 3.14, ZXY) writes to the first element and not the second.

This to me is a blocking design issue.

Whereas xyz() is the order after shuffling and flipping axes signs but cannot be assigned.

The "but cannot be assigned" is unexpected. What does it mean? And what does it imply about the ijk() function?

blender::math::Euler3 shares the same limitations as blender::math::EulerXYZ.

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.

Some general questions about `BLI_math_euler*`, as this is the first bit I happened to look at. ```cpp const EulerXYZ<T> &eul = *this; ``` What's the reason to go for such a "`this` alias"? What's the added value of using `eul.x()` over `this->x()` or just `x()`? ```cpp const T ti = eul.x() * T(0.5); const T tj = eul.y() * T(0.5); const T th = eul.z() * T(0.5); const T ci = math::cos(ti); const T cj = math::cos(tj); const T ch = math::cos(th); const T si = math::sin(ti); const T sj = math::sin(tj); const T sh = math::sin(th); const T cc = ci * ch; const T cs = ci * sh; const T sc = si * ch; const T ss = si * sh; ``` 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 in `BLI_math_euler_types.hh`). > 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. 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 that `Euler3` can be in arbitrary order, but that isn't clear from the explanation. > For both types `ijk()` represent the rotation values in the order given to the constructor. 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 use `i=Z`, `j=X`, `k=Y`). But then there is this: ```cpp Euler3(const eAxis axis, T angle, eEulerOrder order) : ijk_(0), order_(order) { ijk_[axis] = angle; } ``` where `Euler3(eAxis::X, 3.14, ZXY)` writes to the first element and not the second. This to me is a blocking design issue. > Whereas `xyz()` is the order after shuffling and flipping axes signs but cannot be assigned. The "but cannot be assigned" is unexpected. What does it mean? And what does it imply about the `ijk()` function? > `blender::math::Euler3` shares the same limitations as `blender::math::EulerXYZ`. 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.
Author
Member

@dr.sybren

What's the reason to go for such a "this alias"? What's the added value of using eul.x() over this->x() or just x()?

This is in a conversion operator. Using this hides the type.

I know the style guide says "Local variables should be short and to the point." but I think this is too short.

This was copy paste from old code. Will improve the variable names. Also I think it uses ijh instead of ijk convention.

My guess is that EulerXYZ is always in XYZ order, and that Euler3 can be in arbitrary order, but that isn't clear from the explanation.

Yes, I'll reword that into a clearer explanation.

This to me is a blocking design issue.

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 for EulerXYZ and is quite ambiguous for Euler3. I will use ijk_shuffled() for internal use and document it properly.

Euler3(const eAxis axis, T angle, eEulerOrder order)

Concerning this constructor I think it just better to remove it.

The "but cannot be assigned" is unexpected. What does it mean? And what does it imply about the ijk() function?

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.

@dr.sybren > What's the reason to go for such a "this alias"? What's the added value of using eul.x() over this->x() or just x()? This is in a conversion operator. Using `this` hides the type. > I know the style guide says "Local variables should be short and to the point." but I think this is too short. This was copy paste from old code. Will improve the variable names. Also I think it uses `ijh` instead of `ijk` convention. > My guess is that EulerXYZ is always in XYZ order, and that Euler3 can be in arbitrary order, but that isn't clear from the explanation. Yes, I'll reword that into a clearer explanation. > This to me is a blocking design issue. 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 for `EulerXYZ` and is quite ambiguous for `Euler3`. I will use `ijk_shuffled()` for internal use and document it properly. ``` Euler3(const eAxis axis, T angle, eEulerOrder order) ``` Concerning this constructor I think it just better to remove it. > The "but cannot be assigned" is unexpected. What does it mean? And what does it imply about the ijk() function? 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.

@dr.sybren

What's the reason to go for such a "this alias"? What's the added value of using eul.x() over this->x() or just x()?

This is in a conversion operator. Using this hides the type.

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 that this is a pointer to a EulerXYZ<T>?

This to me is a blocking design issue.

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.

Regardless, it should be crystal clear what the meaning of these fields are.

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 for EulerXYZ and is quite ambiguous for Euler3. I will use ijk_shuffled() for internal use and document it properly.

I'm still uncertain what "shuffled" and "unshuffled" mean.

Euler3(const eAxis axis, T angle, eEulerOrder order)

Concerning this constructor I think it just better to remove it.

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 "but cannot be assigned" is unexpected. What does it mean? And what does it imply about the ijk() function?

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.

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.

> @dr.sybren > > What's the reason to go for such a "this alias"? What's the added value of using eul.x() over this->x() or just x()? > > This is in a conversion operator. Using `this` hides the type. 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 that `this` is a pointer to a `EulerXYZ<T>`? > > This to me is a blocking design issue. > > 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. Regardless, it should be crystal clear what the meaning of these fields are. > 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 for `EulerXYZ` and is quite ambiguous for `Euler3`. I will use `ijk_shuffled()` for internal use and document it properly. I'm still uncertain what "shuffled" and "unshuffled" mean. > ``` > Euler3(const eAxis axis, T angle, eEulerOrder order) > ``` > Concerning this constructor I think it just better to remove it. 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 "but cannot be assigned" is unexpected. What does it mean? And what does it imply about the ijk() function? > > 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. 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.
Hans Goudey requested changes 2023-02-20 20:42:00 +01:00
Hans Goudey left a comment
Member

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 the M_PI and FLT_EPSILON defines currently brought in from the old C headers. That could happen later though.

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 the `M_PI` and `FLT_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.
Member

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 documentation
  • Fast : Everything not slow. and Slow : 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 the Fast : list items.
  • Range : -> Range: Usually there isn't a space between labels and colons.
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 documentation_ - `Fast : Everything not slow.` and `Slow : 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 the `Fast :` list items._ - `Range :` -> `Range:` _Usually there isn't a space between labels and colons._
fclem marked this conversation as resolved
@ -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.
Member

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.

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.
fclem marked this conversation as resolved
@ -0,0 +298,4 @@
friend AngleCartesian operator/(const AngleCartesian &a, const T &b)
{
if (b == T(2)) {
/* Still costly but less than using `atan()`. */
Member

Grammar:

  • but less than -> but faster than
  • is range of -> is in the range
Grammar: - `but less than` -> `but faster than` - `is range of` -> `is in the range`
fclem marked this conversation as resolved
@ -0,0 +52,4 @@
*this = identity();
return;
}
else {
Member

else after return

else after return
fclem marked this conversation as resolved
@ -0,0 +5,4 @@
/** \file
* \ingroup bli
*
* A `blender::math::AxisAngle<T>` represent a rotation around a unit axis.
Member

Grammar:

  • represent a -> represents a
  • and thus faster -> and are thus faster
  • and they needs -> ; they must

Writing style:

  • it's not clear what the last paragraph is talking about. It it's referring to interpolation, it should be part of the same paragraph.

I'd also recommend putting this comment above the AxisAngle type.

Grammar: - `represent a` -> `represents a` - `and thus faster` -> `and are thus faster` - ` and they needs` -> `; they must` Writing style: - it's not clear what the last paragraph is talking about. It it's referring to interpolation, it should be part of the same paragraph. I'd also recommend putting this comment above the `AxisAngle` type.
fclem marked this conversation as resolved
@ -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.
Member

Grammar:

  • blender -> Blender
  • For cross product forward -> For cross product, forward
Grammar: - `blender` -> `Blender` - `For cross product forward` -> `For cross product, forward`
fclem marked this conversation as resolved
@ -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
Member

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 in fill_mesh_positions in curve_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.

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` in `fill_mesh_positions` in `curve_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.
Author
Member

Looking at the use of math::from_orthonormal_axes in fill_mesh_positions in curve_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.

> Looking at the use of math::from_orthonormal_axes in fill_mesh_positions in curve_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.
Author
Member

BevPoint have this orientation coming out of get_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.

`BevPoint` have this orientation coming out of `get_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 {
Member

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? and math::SignedAxis? Alternatively, if those names were too vague to people, they could get a 3D prefix or suffix.

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`? and `math::SignedAxis`? Alternatively, if those names were too vague to people, they could get a `3D` prefix or suffix.
Author
Member

Hell yeah, I hated thoses prefixes.

Hell yeah, I hated thoses prefixes.
fclem marked this conversation as resolved
@ -0,0 +57,4 @@
/** \name Axes utilities.
* \{ */
inline eAxis axis_unsigned(const eAxisSigned axis)
Member

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.

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.
fclem marked this conversation as resolved
@ -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.
Member

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
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`
fclem marked this conversation as resolved
@ -0,0 +202,4 @@
template<typename T> struct Euler3 {
public:
private:
/** Raw rotation values (in radian) as passed by the constructor. */
Member

Grammar: in radian -> in radians

Grammar: `in radian` -> `in radians`
fclem marked this conversation as resolved
@ -0,0 +334,4 @@
bool parity() const
{
switch (order_) {
default:
Member

I'd expect a BLI_assert_unreachable() in the default case, right?

I'd expect a `BLI_assert_unreachable()` in the default case, right?
fclem marked this conversation as resolved
@ -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);
Member

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.

`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.
fclem marked this conversation as resolved
@ -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. */
Member

Grammar: values it in -> values in it

Grammar: `values it in` -> `values in it`
fclem marked this conversation as resolved
@ -0,0 +87,4 @@
* \{ */
/**
* Transform \a p by rotation using the quaternion \a q .
Member

\a p -> \a v

`\a p` -> `\a v`
fclem marked this conversation as resolved
@ -0,0 +607,4 @@
*/
template<typename T>
[[nodiscard]] VecBase<T, 3> transform_point(const detail::DualQuaternion<T> &dq,
VecBase<T, 3> &point,
Member

It's a bit weird for this function to modify its argument and also return a new vector. Maybe I'm missing something?

It's a bit weird for this function to modify its argument and also return a new vector. Maybe I'm missing something?
fclem marked this conversation as resolved
@ -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
Member
  • transforming points is not -> transforming points is slower
  • information : -> information:
  • This mean the -> This means the
- `transforming points is not` -> `transforming points is slower` - `information :` -> `information:` - `This mean the` -> `This means the`
fclem marked this conversation as resolved
@ -246,3 +8,1 @@
/**
* Intermediate Types.
* Rotation Types
Member

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.

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.
Author
Member

I would think it was more for ease of use, discoverability and documentation purpose. But it can be removed if not wanted.

I would think it was more for ease of use, discoverability and documentation purpose. But it can be removed if not wanted.
Member

I guess this could be changed later if compile time is investigated

I guess this could be changed later if compile time is investigated
HooglyBoogly marked this conversation as resolved
@ -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.
Member

Grammar: more semantic informations and -> more semantic information and

Grammar: `more semantic informations and` -> `more semantic information and`
fclem marked this conversation as resolved
Falk David reviewed 2023-02-20 22:26:09 +01:00
@ -865,0 +987,4 @@
/**
* From:
* "Skinning with Dual Quaternions"
* Ladislav Kavan, Steven Collins, Jiri Zara, Carol OSullivan
Member

I'd avoid the unicode char and just make it Carol O'Sullivan

I'd avoid the unicode char `’` and just make it `Carol O'Sullivan`
fclem marked this conversation as resolved
Clément Foucault added 12 commits 2023-02-21 21:40:44 +01:00
Clément Foucault added 3 commits 2023-02-22 01:05:55 +01:00
Clément Foucault changed title from WIP: Rotation C++ API to Rotation C++ API 2023-02-22 01:25:52 +01:00
Clément Foucault requested review from Sybren A. Stüvel 2023-02-22 01:26:08 +01:00
Clément Foucault requested review from Jacques Lucke 2023-02-22 01:26:09 +01:00
Clément Foucault requested review from Hans Goudey 2023-02-22 01:26:10 +01:00
Author
Member

I got it to a point where I see no other changes to make.

EulerBase<AngleT> Template

I made use of AngleRadian<T> inside EulerBase 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 for EulerBase 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 a Axis::NULL for correct behavior?

The other topic is the dual-quaternions changes.

I got it to a point where I see no other changes to make. ### `EulerBase<AngleT>` Template I made use of `AngleRadian<T>` inside `EulerBase` 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 for `EulerBase` 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 a `Axis::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.

Some changes we typed while @fclem and I did a shoudler-to-shoulder IRL code dive.
Sybren A. Stüvel approved these changes 2023-03-06 17:48:20 +01:00
Sybren A. Stüvel left a comment
Member

To summarize on the above open questions:

Should AxisSigned cross(const AxisSigned a, const AxisSigned b) assert on failure and return a or b (or AxisSigned::NULL?

We picked BLI_assert() to make these cases visible in debug builds, and still returning an arbitrary axis that is perpendicular to both a and b so that there are no special cases as far as the calling code is concerned.

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. Is that reasonable change?

I think it's fine.

Approving now, with the note that @fclem got the patch of the stuff we discussed now 😻 :)

To summarize on the above open questions: > Should AxisSigned cross(const AxisSigned a, const AxisSigned b) assert on failure and return a or b (or AxisSigned::NULL? We picked `BLI_assert()` to make these cases visible in debug builds, and still returning an arbitrary axis that is perpendicular to both `a` and `b` so that there are no special cases as far as the calling code is concerned. > 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. Is that reasonable change? I think it's fine. Approving now, with the note that @fclem got the patch of the stuff we discussed now 😻 :)
Clément Foucault added 3 commits 2023-03-08 07:00:49 +01:00
buildbot/vexp-code-patch-coordinator Build done. Details
06fb4acad5
Use casting function instead of typecast
This is more inline with the general API design and allow more flexibility
in the semantic we use (`from`/`to`).
Author
Member

@blender-bot build

@blender-bot build
Clément Foucault added 1 commit 2023-03-08 09:38:17 +01:00
buildbot/vexp-code-patch-coordinator Build done. Details
777953cec8
Try to fix MSVC compile error
Author
Member

@blender-bot build

@blender-bot build
Clément Foucault added 1 commit 2023-03-08 09:54:23 +01:00
buildbot/vexp-code-patch-coordinator Build done. Details
e7a8c4a4da
try fix msvc
Author
Member

@blender-bot build

@blender-bot build
Clément Foucault added 2 commits 2023-03-08 11:22:46 +01:00
buildbot/vexp-code-patch-coordinator Build done. Details
dc905e55ca
Merge branch 'main' into bli-math-rotation-cpp
# Conflicts:
#	source/blender/blenlib/BLI_math_vector_types.hh
Author
Member

@blender-bot build

@blender-bot build
Clément Foucault added 1 commit 2023-03-08 11:34:36 +01:00
buildbot/vexp-code-patch-coordinator Build done. Details
47a7335e80
Fix missing explicit cast
Author
Member

@blender-bot build

@blender-bot build
Clément Foucault added 1 commit 2023-03-08 18:13:19 +01:00
Author
Member

@blender-bot build

@blender-bot build
Hans Goudey reviewed 2023-03-08 18:22:23 +01:00
@ -0,0 +38,4 @@
*/
class Axis {
public:
enum class Value : int {
Member

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.

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.
fclem marked this conversation as resolved
@ -0,0 +79,4 @@
int as_int() const
{
return static_cast<int>(axis_);
Member

Style guide mentions using functional casts for cases like this: int(axis_)

Style guide mentions using functional casts for cases like this: `int(axis_)`
fclem marked this conversation as resolved
Clément Foucault added 1 commit 2023-03-08 23:28:02 +01:00
buildbot/vexp-code-patch-coordinator Build done. Details
06006a51c1
Address review
Author
Member

@blender-bot build

@blender-bot build
Jacques Lucke approved these changes 2023-03-09 14:31:12 +01:00
@ -0,0 +41,4 @@
ZYX,
};
inline std::ostream &operator<<(std::ostream &stream, EulerOrder order)
Member

Can be moved to an implementation file.

Can be moved to an implementation file.
fclem marked this conversation as resolved
@ -0,0 +228,4 @@
EulerOrder order_;
/**
* Swizzle structure allowing to rotation ordered assignement.
Member

typo (assignement)

typo (assignement)
fclem marked this conversation as resolved
@ -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) */
Member

Looks unrelated.

Looks unrelated.
fclem marked this conversation as resolved
@ -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);
Member

Looks unrelated.

Looks unrelated.
fclem marked this conversation as resolved
Clément Foucault added 1 commit 2023-03-09 14:52:46 +01:00
buildbot/vexp-code-patch-coordinator Build done. Details
fdc8edba24
Improve const conrectness and variable names
Sergey Sharybin approved these changes 2023-03-09 14:58:26 +01:00
Author
Member

@blender-bot build

@blender-bot build
Hans Goudey requested changes 2023-03-09 15:21:22 +01:00
Hans Goudey left a comment
Member

Just noticed a few more small things

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.
Member

Grammar: There is no identities -> There are no identities

Grammar: `There is no identities` -> `There are no identities`
fclem marked this conversation as resolved
@ -0,0 +371,4 @@
} // namespace detail
/**
* A `blender::math::AngleFraction<T>` stores the radian angle as quotient.
Member

Grammar: the radian angle -> a radian angle

Grammar: `the radian angle` -> `a radian angle`
fclem marked this conversation as resolved
@ -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.
Member

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?

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?
Author
Member

I'm not sure how to be more explicit. Care to suggest anything better?

I'm not sure how to be more explicit. Care to suggest anything better?
Member

TBH I don't understand what this means. "Everything not slow" and "Slow: A, B, C" just don't fit logically

TBH I don't understand what this means. "Everything not slow" and "Slow: A, B, C" just don't fit logically
Author
Member

How about:

* - Slow : `cos()`, `sin()`, `tan()` for angles not optimized.
* - Fast : Everything else.
How about: ``` * - Slow : `cos()`, `sin()`, `tan()` for angles not optimized. * - Fast : Everything else. ```
Member

Much better!

Much better!
fclem marked this conversation as resolved
@ -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
Member

Typo: fo -> for

Typo: `fo` -> `for`
fclem marked this conversation as resolved
@ -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.
Member

Writing style: really -> very
Just sounds more professional IMO

Writing style: `really` -> `very` Just sounds more professional IMO
fclem marked this conversation as resolved
@ -0,0 +5,4 @@
/** \file
* \ingroup bli
*
* An `blender::math::CartesianBasis` represents an orientation that is aligned with the basis
Member

Similar to what we did elsewhere, it's probably better to have this comment about CartesianBasis right above the class itself.

Similar to what we did elsewhere, it's probably better to have this comment about `CartesianBasis` right above the class itself.
fclem marked this conversation as resolved
@ -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,
Member

Other enum classes in the codebase use camel case for each value. What do you think about doing that here? AxisSigned::XPos

Other enum classes in the codebase use camel case for each value. What do you think about doing that here? `AxisSigned::XPos`
Author
Member

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.

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.
fclem marked this conversation as resolved
@ -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.
Member

Grammar/Writing style: Way faster than -> This is much faster than

Grammar/Writing style: `Way faster than` -> `This is much faster than`
fclem marked this conversation as resolved
@ -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.
Member

Grammar: It is better filter -> It is better to filter

Grammar: `It is better filter` -> `It is better to filter`
fclem marked this conversation as resolved
@ -230,2 +230,4 @@
const VectorT up);
/**
* This return a version of \a mat with orthonormal basis axes.
Member

Grammar: This return a -> This returns a

However, just saying "Returns a..." is probably better than having "This" at the start

Grammar: `This return a` -> `This returns a` However, just saying "Returns a..." is probably better than having "This" at the start
fclem marked this conversation as resolved
@ -0,0 +5,4 @@
/** \file
* \ingroup bli
*
* A `blender::math::Quaternion<T>` represents either an orientation or a rotation.
Member

I also think these comments would fit better right above the relevant classes.

I also think these comments would fit better right above the relevant classes.
fclem marked this conversation as resolved
@ -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.
Member

Grammar: There is -> There are

Grammar: `There is` -> `There are`
fclem marked this conversation as resolved
Clément Foucault added 1 commit 2023-03-09 15:26:02 +01:00
buildbot/vexp-code-patch-coordinator Build done. Details
e73101918b
I'm too const for that
Author
Member

@blender-bot build

@blender-bot build
Clément Foucault added 1 commit 2023-03-09 16:06:09 +01:00
Hans Goudey approved these changes 2023-03-09 18:08:34 +01:00
Clément Foucault added 1 commit 2023-03-09 18:13:15 +01:00
Clément Foucault merged commit 28a581d6cb into main 2023-03-09 18:15:33 +01:00
Clément Foucault deleted branch bli-math-rotation-cpp 2023-03-09 18:15:34 +01:00
Sign in to join this conversation.
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
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#104941
No description provided.