This bugzilla service is closed. All entries have been migrated to https://gitlab.com/libeigen/eigen
Bug 669 - Translation * Mapped vector does not compile
Summary: Translation * Mapped vector does not compile
Status: DECISIONNEEDED
Alias: None
Product: Eigen
Classification: Unclassified
Component: Geometry (show other bugs)
Version: unspecified
Hardware: All All
: Normal API Change
Assignee: Nobody
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 3.4
  Show dependency treegraph
 
Reported: 2013-10-02 23:41 UTC by Gael Guennebaud
Modified: 2019-12-04 12:39 UTC (History)
4 users (show)



Attachments

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.

Note You need to log in before you can comment on or make changes to this bug.