mosra / magnum

Lightweight and modular C++11 graphics middleware for games and data visualization
https://magnum.graphics/
Other
4.77k stars 439 forks source link

Quaternion::slerp returns invalid Quaternion given two identity Quaternions #117

Closed wivlaro closed 8 years ago

wivlaro commented 8 years ago
Magnum::Quaternion::slerp(Magnum::Quaternion(),Magnum::Quaternion(),0.25f)

Gives me a quaternion of not-a-numbers. I think it should probably return a unit quaternion again.

Squareys commented 8 years ago

That is because the angle between those quaternions is 0.0, and in Math::sclerp, we divide by sin(angle) = 0.0 which results in the Quaternion({nan, nan, nan}, nan).

I guess one simply needs to check if the angle is 0.0 in which case one can simply return the first quaternion. Should be simple, I will create a PR for this.

Squareys commented 8 years ago

Actually, since the quaternions are normalized anyway and (correct me, if I'm wrong) there is always only one specific unambiguous quaternion to describe a certain rotation, the only case where angle == 0.0 is when the two quaternions is equal. That should be cheeper than calculating angle = std::acos(dot(normalizedA, normalizedB)), right, @mosra ?

mosra commented 8 years ago

Thanks a lot for the report and the fix.

I think that one rotation can be described with two different quaternions, but I'm not sure. In that case a simple equality comparison would not be enough, but we can surely do a lot better than calling std::acos(). Doing something like TypeTraits<T>::equals(dot(normalizedA, normalizedB), T(1)) might be equivalent, I think? I need to check the math or the properties of dot product.

Oh... and couldn't similar problem affect also the dual quaternion sclerp()?

I'm completely braindead now, will think about this more deeply tomorrow.

Squareys commented 8 years ago

Alright, you are correct, indeed: Wikipedia says, that exactly two quaternions q and -q map to the same rotation matrix.

Which means, we can also check for equality with q and -q, in case that is cheeper than what you suggested.

Squareys commented 8 years ago

Oh, and yes, sclerp() is also affected by division by zero, but only for cases in which the difference of the interpolated dual quaternions is a dual quaternion with a vector v with |v| = 0.0 in the real part. That is because we need to use .lengthInverted(). I'm not sure when that is the case though, or how that would be interpreted geometrically.

Squareys commented 8 years ago

Okay, I found this: http://www.euclideanspace.com/maths/algebra/realNormedAlgebra/quaternions/slerp/ which suggests a good solution.

I modified the commit in the current pull request to implement that suggestion and included a check for interpotation of q and -q.

mosra commented 8 years ago

@Squareys thanks a lot for the investigation, reopening to not forget about the case in sclerp.

Squareys commented 8 years ago

It's actually the same case as in slerp:

Values Math::sclerp(DualQuaternion{}, DualQuaternion{}, 0.42f) and DualQuaternion{} are not the same,
        actual is
DualQuaternion({{nan, nan, nan}, nan}, {{nan, nan, nan}, nan}) 
        but expected
DualQuaternion({{0, 0, 0}, 1}, {{0, 0, 0}, 0})

Same solution might work for the rotation part. We do not need to check if the translation parts are the same, but since the division by zero still affects it when the rotations are the same, we will instead linearly interpolate it (which is equivalent in that case, if I'm correct).

Squareys commented 8 years ago

One catch though: lerp for quaternions does not behave as originally though. But in this case interpolating the vector part of the translation part is enough.

Pullrequest incoming.

mosra commented 8 years ago

Fixed with the two mentioned PRs. Thanks for the report and for the fixes.