I don't really like the fact that we store the cos and sin here as well. It can help sometimes and be worse in others (when it is computed even though it's not needed). I think this should be passed to functions separately or there should be something like struct AxisAngleWithSinCos that contains everything.

I think these using statements should be removed. At least to me they make reading the code below harder, because Map, Matrix and Stride are very generic. With the Eigen:: prefix I would have much more context for what is meant.

Your example does not compile. (MacOS + Clang 13.1.6)

You should get something like

/Users/clement/Blender/blender/source/blender/blenlib/tests/BLI_math_matrix_types_test.cc:298:32: error: no viable overloaded '+='
  mat_const.view<2, 2, 1, 1>() += 5;
  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^  ~
[...]
/Users/clement/Blender/blender/source/blender/blenlib/BLI_math_matrix_types.hh:566:12: note: candidate function not viable: 'this' argument has type 'const MatView<float, 2, 2, 4, 4, 1, 1>', but method is not marked const
  MatView &operator+=(T b)

This is because the view() method return either a const MatView or a MatView depending on the object const type.

So I am a bit hesitant to make the code way less understandable for something that should work.

I am not against changing it but I would like to express my opinion.

• I could not find a single use through the whole old rotation API that uses the angle as is without converting them to cos/sin representation.
• This representation is also not suited for interpolation so it is likely to be a corner case than the main usage of it.
• I doubt this is also a type to consider for storage. But a case could be made that that's always more semantically correct to store them as such. Also it might be better for data exchange with the GPU to use 16 bytes struct.
• Having more types make the API less trivial to understand and use.

These being protected members, makes it implementation detail and allows for changes to be made later if the case arise that we really need these.
But to be fair, the rotation API wasn't the goal of this patch and is going to be addressed in a separate differential.

There is an important difference between:

mat_const.view<2, 2, 1, 1>() += 5;

and

auto view = x.view<4, 4>();
view += 5;

The example you provided indeed does not work, but my example does and it has the const correctness problem.
The problem is that a const MatView can just be copied (it's just a pointer after all) and then it is not const anymore. This is what happens in my example. x.view() returns a const MatView, but view is just a MatView.

Does not look like this has been addressed.

Call this something like is_equal_approximate or so. It's never clear to me what a compare function returns.

Think it's better to define the binary operator functions as a friend function, or it doesn't even have to be inside the class.

These checks are not necessary since they are included in the checks below.

Assert that the other matrix has the same size as this matrix.
Also this should probably be implemented outside of the class. Then only one num-row and num-col template argument should be neccesary (but two start-col/row arguments).

Eigen only by default, not in general

This is in order to provide template specialization for common types. I couldn't find a nice way for friend functions to work with this.

This is already the case here. You were just confused by the long list of arguments.

Grammar

Wouldn't it make more sense to be specific about the rotation type here?
Or did you do it this way to support more rotation types in the future?

What do you think about adding a tidbit about when you would use pseudo_invert to the comment? Maybe others know about it, but it would be helpful to me anyway.

What do you think about the from_normalized_axes name? I've always wished I used that name since it's shorter and _data at the end is pretty useless

What about adding the from_ prefix to these, since they build matrices like the other functions above? Just a thought, totally fine if you don't think it's a good idea

Grammar. Appears elsewhere in this file too

I understand you might not have the energy for this at this point, but I think it would be great to see a description of the overall design in a comment at the top of the file like BLI_vector.hh.
This file will be looked at a lot, and that would probably save people lots of time in the long run

Could the "this only works for tightly packed T" be checked with a static assert instead?

Grammar

The idea was to keep the declarations generic enough so that specializations is implementation details.

For the moment only the AxisAngle version is implemented.

I have no idea... maybe a good one for @Sergey Sharybin (sergey) .

I am tempted to rename it to from_orthonormal_axes. If the given axes aren't orthonormal, the resulting matrix isn't a proper rotation matrix.

Also tempted to template it for any axis couple.

I don't think it is a good idea because it isn't another representation of a transform like EulerXYZ or Location. It would make sense if it was from_perspective_distances or something like that, but it is way longer and doesn't convey much more information.

Do we need to specify this is the MatBase<T,3,3> specialization?
(Same for orthonormal)

My c++-foo is weak, do we also need ?:

  MatBase(MatBase &&) = default;
  MatBase& operator=(const MatBase &) = default;
`

Can we make a slightly stronger hash by following FNV-hash and using XOR instead of ADD:

uint64_t hash() const
{
  uint64_t h = 14695981039346656037ULL;
  unroll<NumCol * NumRow>([&](auto i) {
    T value = (static_cast<const T *>(this))[i];
    h *= 1099511628211ULL;
    h ^= *reinterpret_cast<const as_uint_type<T> *>(&value);
  });
  return h;
}

(!OPTIONAL!)

As a personal preference, it's possible to write this slightly cleaner like so:

  friend std::ostream &operator<<(std::ostream &stream, const MatBase &mat)
  {
    const char *col_prefix = "(\n";
    unroll<NumCol>([&](auto i) {
      stream << col_prefix;
      col_prefix = "),\n(";
      const char *row_prefix = "(";
      unroll<NumRow>([&](auto j) {
        /** NOTE: j and i are swapped to follow mathematical convention. */
        stream << row_prefix << mat[j][i];
        row_prefix = ", ";
      });
    });
    stream << ")\n)\n";
    return stream;
  }
};

(question?)
Can we remove the negation by using the identity:

-math::cross(forward, up) === math::cross(up, forward)

TODO: The return type doesn't look quite right here, it should be possible to return a non-square matrix.
e.g. a MatBase<2,3> * MatBase<3,4> should return a MatBase<2,4> ?
(Or it's possible I have that transposed.)

To support non-square matrices, perhaps std::min(NumCol,NumRow)

This is unfortunately not possible to determine if a matrix is 2d affine (projective) or 3d rotation.

For this reason, I would leave this function as is for now. It could become templated with the dimensionality.
However, as it isn't present inside BLI C matrix API, I don't see the incentive to add it now.

That isn't required since none of the copy / move constructor/assignment are defined. Thus these are automatically defined.

I did it out of simplicity and avoiding to template this even more. I'll leave that as a TODO.

Good catch!

This patch uses the same hash as the old float4x4 library for compatibility. If we have to change it, it isn't in this patch.

I find that's a little less obvious.