From 9f9d22ca332c3aca65b53162da316f3dd0e3a58d Mon Sep 17 00:00:00 2001 From: cutealien Date: Sat, 15 Oct 2022 15:46:03 +0000 Subject: [PATCH] Fix some problems with CMatrix4::getRotationDegrees - Only the getRotationDegrees without parameter is allowed to try fixing scale. My fault when I added a new function which takes scale parameter, that one is not allowed to be changed. On the up-side - we know have for the first time an option which works in cases only scale and rotation had been used and the user still has the correct scale. Before any solution for that was broken - getRotationDegrees fixes 2 places which caused wrong results due to floating point inaccuracies New test for that got added - Document the current restrains and problems of getRotationDegrees and getScale some more. - Improve docs for other matrix4 functions. - Add some comments about further improvements (I'll try if I find time) Note: Irrlicht still assumes in at least 2 places (getting bone animations and Collada loader) that matrix decomposing works. Which it doesn't yet for matrices which switch handedness (or have further transformations like skewing axes) The bone animation is mostly fine for now with recent workaround (but that might cause other problems as it may be used too often), haven't checked Collada yet in detail. TL/DR: This improves things with getRotationDegrees, but does not yet fix all troubles. git-svn-id: svn://svn.code.sf.net/p/irrlicht/code/trunk@6439 dfc29bdd-3216-0410-991c-e03cc46cb475 --- include/matrix4.h | 92 +++++++++++++++++++++++++++-------------------- 1 file changed, 54 insertions(+), 38 deletions(-) diff --git a/include/matrix4.h b/include/matrix4.h index e879c04..1245546 100644 --- a/include/matrix4.h +++ b/include/matrix4.h @@ -143,7 +143,7 @@ namespace core //! Set this matrix to the product of two matrices /** Calculate b*a, no optimization used, - use it if you know you never have a identity matrix */ + use it if you know you never have an identity matrix */ CMatrix4& setbyproduct_nocheck(const CMatrix4& other_a,const CMatrix4& other_b ); //! Multiply by another matrix. @@ -151,7 +151,8 @@ namespace core CMatrix4 operator*(const CMatrix4& other) const; //! Multiply by another matrix. - /** Calculate and return other*this */ + /** Like calling: (*this) = (*this) * other + */ CMatrix4& operator*=(const CMatrix4& other); //! Multiply by scalar. @@ -187,14 +188,25 @@ namespace core //! Make a rotation matrix from Euler angles. The 4th row and column are unmodified. CMatrix4& setRotationDegrees( const vector3d& rotation ); - //! Get the rotation, as set by setRotation() when you already know the scale. - /** If you already know the scale then this function is faster than the other getRotationDegrees overload. - NOTE: You will have the same end-rotation as used in setRotation, but it might not use the same axis values. + //! Get the rotation, as set by setRotation() when you already know the scale used to create the matrix + /** NOTE: The scale needs to be the correct one used to create this matrix. + You can _not_ use the result of getScale(), but have to save your scale + variable in another place (like ISceneNode does). + NOTE: No scale value can be 0 or the result is undefined. + NOTE: It does not necessarily return the *same* Euler angles as those set by setRotationDegrees(), + but the rotation will be equivalent, i.e. will have the same result when used to rotate a vector or node. + NOTE: It will (usually) give wrong results when further transformations have been added in the matrix (like shear). + WARNING: There have been troubles with this function over the years and we may still have missed some corner cases. + It's generally safer to keep the rotation and scale you used to create the matrix around and work with those. */ core::vector3d getRotationDegrees(const vector3d& scale) const; //! Returns the rotation, as set by setRotation(). /** NOTE: You will have the same end-rotation as used in setRotation, but it might not use the same axis values. + NOTE: This only works correct if no other matrix operations have been done on the inner 3x3 matrix besides + setting rotation (so no scale/shear). Thought it (probably) works as long as scale doesn't flip handedness. + NOTE: It does not necessarily return the *same* Euler angles as those set by setRotationDegrees(), + but the rotation will be equivalent, i.e. will have the same result when used to rotate a vector or node. */ core::vector3d getRotationDegrees() const; @@ -828,11 +840,9 @@ namespace core //! Returns the absolute values of the scales of the matrix. /** - Note that this returns the absolute (positive) values unless only scale is set. - Unfortunately it does not appear to be possible to extract any original negative - values. The best that we could do would be to arbitrarily make one scale - negative if one or three of them were negative. - FIXME - return the original values. + Note: You only get back original values if the matrix only set the scale. + Otherwise the result is a scale you can use to normalize the matrix axes, + but it's usually no longer what you did set with setScale. */ template inline vector3d CMatrix4::getScale() const @@ -895,33 +905,16 @@ namespace core } - //! Returns a rotation that is equivalent to that set by setRotationDegrees(). - /** This code was sent in by Chev. Note that it does not necessarily return - the *same* Euler angles as those set by setRotationDegrees(), but the rotation will - be equivalent, i.e. will have the same result when used to rotate a vector or node. - This code was originally written by by Chev. + //! Returns a rotation which (mostly) works in combination with the given scale + /** + This code was originally written by by Chev (assuming no scaling back then, + we can be blamed for all problems added by regarding scale) */ template inline core::vector3d CMatrix4::getRotationDegrees(const vector3d& scale_) const { const CMatrix4 &mat = *this; - core::vector3d scale(scale_); - // we need to check for negative scale on to axes, which would bring up wrong results - if (scale.Y<0 && scale.Z<0) - { - scale.Y =-scale.Y; - scale.Z =-scale.Z; - } - else if (scale.X<0 && scale.Z<0) - { - scale.X =-scale.X; - scale.Z =-scale.Z; - } - else if (scale.X<0 && scale.Y<0) - { - scale.X =-scale.X; - scale.Y =-scale.Y; - } + const core::vector3d scale(core::iszero(scale_.X) ? FLT_MAX : scale_.X , core::iszero(scale_.Y) ? FLT_MAX : scale_.Y, core::iszero(scale_.Z) ? FLT_MAX : scale_.Z); const core::vector3d invScale(core::reciprocal(scale.X),core::reciprocal(scale.Y),core::reciprocal(scale.Z)); f64 Y = -asin(core::clamp(mat[2]*invScale.X, -1.0, 1.0)); @@ -930,7 +923,7 @@ namespace core f64 rotx, roty, X, Z; - if (!core::iszero(C)) + if (!core::iszero((T)C)) { const f64 invC = core::reciprocal(C); rotx = mat[10] * invC * invScale.Z; @@ -957,14 +950,37 @@ namespace core } //! Returns a rotation that is equivalent to that set by setRotationDegrees(). - /** This code was sent in by Chev. Note that it does not necessarily return - the *same* Euler angles as those set by setRotationDegrees(), but the rotation will - be equivalent, i.e. will have the same result when used to rotate a vector or node. - This code was originally written by by Chev. */ template inline core::vector3d CMatrix4::getRotationDegrees() const { - return getRotationDegrees(getScale()); + // Note: Using getScale() here make it look like it could do matrix decomposition. + // It can't! It works (or should work) as long as rotation doesn't flip the handedness + // aka scale swapping 1 or 3 axes. (I think we could catch that as well by comparing + // crossproduct of first 2 axes to direction of third axis, but TODO) + // And maybe it should also offer the solution for the simple calculation + // without regarding scaling as Irrlicht did before 1.7 + core::vector3d scale(getScale()); + + // We assume the matrix uses rotations instead of negative scaling 2 axes. + // Otherwise it fails even for some simple cases, like rotating around + // 2 axes by 180° which getScale thinks is a negative scaling. + if (scale.Y<0 && scale.Z<0) + { + scale.Y =-scale.Y; + scale.Z =-scale.Z; + } + else if (scale.X<0 && scale.Z<0) + { + scale.X =-scale.X; + scale.Z =-scale.Z; + } + else if (scale.X<0 && scale.Y<0) + { + scale.X =-scale.X; + scale.Y =-scale.Y; + } + + return getRotationDegrees(scale); }