|Summary:||operator* allows premultiplication of a Quaterniond by a scalar and returns invalid results|
|Severity:||Wrong Result||CC:||chtz, gael.guennebaud, hauke.heibel, jacob.benoit.1|
|Bug Depends on:|
|Bug Blocks:||459, 558, 560|
Description adampbry 2014-03-11 23:58:51 UTC
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.
Comment 4 Nobody 2019-12-04 13:04:13 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/758.