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 758 - operator* allows premultiplication of a Quaterniond by a scalar and returns invalid results
Summary: operator* allows premultiplication of a Quaterniond by a scalar and returns i...
Status: RESOLVED FIXED
Alias: None
Product: Eigen
Classification: Unclassified
Component: Geometry (show other bugs)
Version: 3.2
Hardware: All All
: High Wrong Result
Assignee: Nobody
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 459 3.3 560
  Show dependency treegraph
 
Reported: 2014-03-11 23:58 UTC by adampbry
Modified: 2016-01-31 13:05 UTC (History)
4 users (show)



Attachments

Description adampbry 2014-03-11 23:58:51 UTC
The following code should either not compile or give v1 = 4 * v2

Quaterniond q1 = Quaterniond::Identity();
Vector3d v1 = 4 * q1 * Vector3d::UnitZ();
Vector3d v2 = q1 * Vector3d::UnitZ();

Instead we get:

v1
0
0
0
v2
0
0
1
Comment 1 Christoph Hertzberg 2014-03-12 00:15:34 UTC
The problem is that scaling currently scales a quaternion, but Quaternion * Vector3 assumes that the Quaternion is of unit length.
Bug 459 would solve this. Note that you still could not express reflections (i.e. multiplying by -1.0 would not change anything)

Somewhat related, but contradicting is the idea of Bug 560. For consistency, this would rather result in (4*q)*v == 16*(q*v); [so it would be a good idea not to overload GeneralizedQuaternion * Vector3]
Comment 2 Christoph Hertzberg 2014-03-12 14:27:49 UTC
Sorry, my assumptions were wrong; the error appears to be much more subtle. 

What happens is that Scalar * RotationBase calls
  operator*(DiagonalMatrix, RotationBase)
and DiagonalMatrix has a constructor from type Index.

Your case should actually result in a runtime assertion (if not disabled), but these examples would not:
  3 * q;
  3.2 * q; 
both result in DiagonalMatrix<double,3>(3) * q which is bogus but valid.
Another DiagonalMatrix constructor which I think should fail is
  DiagonalMatrix<int, 3>(3, 1);
Furthermore, for DiagonalMatrix<double, 1>(1.0) I think it would make more sense to initialize the matrix entry by 1.0, instead of constructing an uninitialized matrix.

The easiest solution to make your code not compile is to mark the corresponding constructor as explicit, which I think would be a good idea anyways.
However, this results in a page full of "template argument deduction/substitution failed" messages.

To make your code compile and work properly, we could add a 
  friend operator*(const Scalar& s, const RotationBase& r);
to RotationBase which returns a scaled rotation (i.e. s * r.toRotationMatrix()).
For consistency, we should then also implement 
  RotationBase::operator*(const Scalar& s);
with the same result.
Comment 3 Gael Guennebaud 2016-01-31 13:05:09 UTC
Looks like this problem has already been partly fixed as "4 * q1 * Vector3d::UnitZ()" does not compile anymore in the 3.3 branch. This is because DiagonalMatrix(Index) is declared explicit now.

In 3.2 it raises a runtime assertion, so actually, "4 * q1 * Vector3d::UnitZ()" has always been invalid.

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