New user self-registration is disabled due to spam. Please email eigen-core-team @ lists.tuxfamily.org if you need an account.
Before reporting a bug, please make sure that your Eigen version is up-to-date!
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: 2016-01-29 11:45 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)

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