Support rotation by multiplication of native types
Closed this issue · 5 comments
I would love to be able to rotate/transform native types (Eigen::Vector3d) by multiplication.
I implemented this by adding the following to RotationBase
/*! \brief Rotates a vector or a matrix column-wise.
* \returns the rotated vector or matrix
*/
template <typename internal::get_matrix3X<Derived_>::IndexType Cols>
typename internal::get_matrix3X<Derived_>::template Matrix3X<Cols> operator *(const typename internal::get_matrix3X<Derived_>::template Matrix3X<Cols>& matrix) const {
return internal::RotationTraits<RotationBase<Derived_,Usage_>>::rotate(this->derived(), matrix);
}
@HannesSommer , @remod , does that seem right? Will that cause problem?
I understand that it would be quite comfortable.
But it spoils a bit the generic (parametrization independent concept of RotationBase). Only for matrices this is a natural operations. For a quaternion one could equally well think of quaternion multiplied with a pure imaginary quaternion. And what if the right side is a rotation vector (stored as a Eigen Vector 3d)? Then a user could think of the composition of rotations, which is the generic interpretation of rotation multiplication.
Another problem is that one could then expect the * in A * B with A and B rotation objects to also stand for rotate. But B rotated by A is something different from A after B (as composition, the current meaning of * in kindr).
I would rather suggest the evaluation operator as an abbreviation for .rotate . (so operator ()). That suits much more the idea of an rotation. Doesn't it?
For what do you need that? For code migration? We could put that operator in a different header (SupportRotationByMultiplication) file (to be included in migrated code). Or restrict it to rotation matrices?
As far as I see it, we are using * as a composition operator that we explicitly define. It does not represent multiplication. Where is the error following this further to apply rotation to vectors?
I just want it because it seems natural...like a natural interface for "RotationBase".
I see a difference because composition is the "multiplication" in the rotation group - that is very usual. But rotating a vector is an application of the rotation on the vector (rotation action), which corresponds much more to the evaluation operator () - in my opinion.
Imagine we overlaod .rotate at some time to rotation object - very natural and useful. But having * for rotate would then clash with the composition (as I wrote they are different operations). That would be inconsistent.
I guess. I basically have the impression it could be valuable to stress that these operations are not the same but something quite different.
One more : From pure c++ design considerations I try to avoid using left to right associative binary operators like *, +, << for something that does not benefit from their strength in chaining. Because to me that is the only benefit truly strong enough to legitimate taking the risk that comes with any operator overloading (e.g. wrong assumption of the user how the compiler interprets the application graph). And composition has natural chaining demands while application does not except some very wired cases!
Anyway I think I've made my point and maybe for most of the users it would be as natural as for you. And I won't start crying when we keep it as a RotationBase operation :). Just wanted to warn about that possible future clash and possible confusions - which are probably rather unlikely I have to admit.
@gehrinch, @bloesch, @remod, @simonlynen, any comments?
I agree with Hannes. We should not implement it because it only makes 'sense' for rotation matrices.