|Summary:||Translation * Mapped vector does not compile|
|Product:||Eigen||Reporter:||Gael Guennebaud <gael.guennebaud>|
|Severity:||API Change||CC:||chtz, gael.guennebaud, hauke.heibel, jacob.benoit.1|
|Bug Depends on:|
Description Gael Guennebaud 2013-10-02 23:41:09 UTC
See: http://forum.kde.org/viewtopic.php?f=74&t=117764 The problem is that this product does not instanciate the right Translation::operator* overload. The fix consists in having a general overload for MatrixBase<> objects with meta-programming tricks to return the right type (Affine or Vector) with the right implementation more like Transform::operator*.
Comment 1 Christoph Hertzberg 2013-10-16 12:38:59 UTC
There are other Transform-related operations/conversions that don't work, e.g. AngleAxisd a; DiagonalMatrix<double,3> d; Affine3d T0 = d; // not ok Affine3d T1 = a * d; // ok Affine3d T2 = a * d.inverse(); // not ok Affine3d T3(a * d.inverse()); // ok (explicit conversion from Matrix3d) Affine3d T4 = a.toRotationMatrix(); // not ok I assume there are more examples. Maybe we should introduce a TransformBase from which all classes which can be converted to a Transform must inherit. Then we implement several Transform * Transform products which must return something derived from TransformBase (with a fall-back to Transform(a) * Transform(b) if no specialization is applicable). Alternatively, instead of TransformBase, introduce a transform_traits struct. That would also avoid having to avoid DiagonalBase (which should not depend on the Geometry module). I would also not limit x to be a vector in Transform * x. If it is a matrix, apply Transform to each column separately: AngleAxisd a; DiagonalMatrix<double,3> d; Matrix<double, 3, 2> m; // essentially, a pair of 3d vectors a * m; // does not work a * d * m; // ok a * (d * m); // does not work a.toRotationMatrix() * m; // ok There is a problem however, when multiplying Translation3d * Matrix3d. The latter could be interpreted as three separate vectors (resulting in 3 vectors) or a linear transformation (resulting in an affine transformation).
Comment 2 Christoph Hertzberg 2013-10-16 12:43:18 UTC
(In reply to comment #1) > Alternatively, instead of TransformBase, introduce a transform_traits struct. > That would also avoid having to avoid DiagonalBase (which should not depend on > the Geometry module). Sorry for the messed up last sentence. What I wanted to say is that this way DiagonalBase does not have to inherit from TransformBase (and the diagonal-traits can be defined inside the Geometry module).
Comment 3 Gael Guennebaud 2015-06-10 09:59:34 UTC
Looking at this general issue again, it also seems that the documentation of Transform * MatrixBase is misleading. Among the different possibilities for the rhs, it mentions: "a linear transformation matrix of size Dim x Dim," suggesting that the lhs L should in this case be interpreted as the following affine transformation: L 0 0 1 but, that's not the case as it is interpreted as a collection of Dim column vectors of size Dim+1 completed with 1 for the homogeneous coordinate: L 1 I think that the current behavior is the only one we could do, but then the documentation is wrong, and we also have a serious discrepancy between Translation and Transform: Translation3f T; Matrix3f L; T * Affine3f(L) == Affine3f(T) * Affine3f(L) // OK T * L == T * Affine3f(L) // might look ok at a first glance, but: Affine3f(T) * L != T * L // clearly not OK I don't known how to fix this without breaking existing code, perhaps even silently. This also complicates the generalisation of Translation::operator*. IMO, the "correct" fix would be to add a Linear<> decorator and always see a MatrixBase as a collection of column vectors and never as a rotation, linear or affine transform.
Comment 4 Gael Guennebaud 2015-12-10 17:09:11 UTC
Regarding the doc issue: https://bitbucket.org/eigen/eigen/commits/969affbdd8a
Comment 5 Christoph Hertzberg 2015-12-14 01:57:37 UTC
Regarding comment 3: I like the idea of a Linear<> decorator (or we could add Linear, Rotation, ScaledRotation, ... modes to Transform). An idea which at least would not silently break existing code: Transform<...> * MatrixBase<...> results in a Product<Transform, Matrix> object which can only be assigned to another MatrixBase object, but not to a Transform object and also is not multipliable by another Matrix; and Transform * Transform (including Translation/Rotation/...) results in a Product<Transform, Transform> object, which can only be assigned to a transform, or multiplied by either another Transform or a Matrix. Transform<...> T1, T2, T3; // or Translation/Scaling/Rotation/... Matrix M1, M2, M3; T1 = T2 * T3; // ok, works as before T1 = T2 * M1; // compile error! T1 = T2*Affine3d(M1); // ok, M1 is interpreted as linear operator M1 = T1 * M2; // ok, M2 is interpreted as set of vectors M1 = T1 * T2; // does not compile M1 = (T1 * T2) * M2; // ok, T1 and T2 result in a Transform // which is applied to M2 M1 = T1 * (T2 * M2); // ok, same result as above M1 = T1 * (M2 * M3); // ok, M2*M3 is evaluated first, then transformed M1 = (T1 * M2) * M3; // compile error, dubious whether M2 is a // set of vectors or a linear operator M1 = Matrix(T1*M2)*M3; // ok, result of T1*M2 is interpreted as matrix M1 = (T1*M2).matrix()*M3; // same result, without explicit temporary The not compiling cases can give reasonable compile errors and we might even provide a compile time flag to temporarily switch to the old behavior (as we did with EIGEN2_SUPPORT)
Comment 6 Nobody 2019-12-04 12:39:46 UTC
-- GitLab Migration Automatic Message -- This bug has been migrated to gitlab.com's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.com/libeigen/eigen/issues/669.