From c85b537937937f2f0e6de82db6cf328d128adc6c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= Date: Fri, 14 Feb 2020 21:27:30 +0100 Subject: [PATCH] Revert "Math: preserve signs in the Matrix[34]::scaling() getter." Turns out this seriously broke properites that should always hold, such as that R*S = M for a R and S extracted out of M. Another way to fix this would be flipping signs in the rotation() / rotationShear() / ... queries as well, but that adds extra operations to both rotation() and scaling() and at that point it's just better to do what was done the whole decade before. To ensure this doesn't happen again and doesn't cost me a whole day of debugging in an end-user app (SceneGraph TRS transformations got broken because those assume that rotation() and scaling() *do* combine back, which is a reasonable thing to assume), the docs are extended to note this and there's an explicit test for both negative scale and large angle corner cases as well. This reverts commit 9aa68715db0465fe9583a80a292d1211155b351c. --- doc/changelog.dox | 7 ----- src/Magnum/Math/Matrix3.h | 26 ++++++++++++------ src/Magnum/Math/Matrix4.h | 30 ++++++++++++-------- src/Magnum/Math/Test/Matrix3Test.cpp | 39 ++++++++++++++++++++------ src/Magnum/Math/Test/Matrix4Test.cpp | 41 ++++++++++++++++++++++------ 5 files changed, 99 insertions(+), 44 deletions(-) diff --git a/doc/changelog.dox b/doc/changelog.dox index 2e724ee3d..3080399f0 100644 --- a/doc/changelog.dox +++ b/doc/changelog.dox @@ -172,9 +172,6 @@ See also: @subsubsection changelog-latest-changes-math Math library -- @ref Math::Matrix3::scaling() const and @ref Math::Matrix4::scaling() const - now return a signed scaling vector, taking into account negative scaling as - well - Functions in @ref Magnum/Math/FunctionsBatch.h now accept any type that's convertible to a @ref Corrade::Containers::StridedArrayView without having to add explicit casts or template parameters @@ -314,10 +311,6 @@ See also: - The 4-argument @ref GL::DynamicAttribute constructor was not marked as @cpp explicit @ce by mistake, it's done now to enforce readability in long expressions. -- @ref Math::Matrix3::scaling() const and @ref Math::Matrix4::scaling() const - now return a signed scaling vector instead of unsigned in order to prevent - information loss. To restore the old behavior, apply @ref Math::abs() on - the result. - The @ref Magnum/Math/FunctionsBatch.h header is no longer included from @ref Magnum/Math/Functions.h for backwards compatibility in order to speed up compile times. diff --git a/src/Magnum/Math/Matrix3.h b/src/Magnum/Math/Matrix3.h index 9beb0204c..a32d83413 100644 --- a/src/Magnum/Math/Matrix3.h +++ b/src/Magnum/Math/Matrix3.h @@ -431,11 +431,11 @@ template class Matrix3: public Matrix3x3 { /** * @brief Non-uniform scaling part of the matrix * - * *Signed* length of vectors in upper-left 2x2 part of the matrix. Use - * the faster alternative @ref scalingSquared() where possible. - * Assuming the following matrix, with the upper-left 2x2 part - * represented by column vectors @f$ \boldsymbol{a} @f$ and - * @f$ \boldsymbol{b} @f$: @f[ + * Length of vectors in upper-left 2x2 part of the matrix. Use the + * faster alternative @ref scalingSquared() where possible. Assuming + * the following matrix, with the upper-left 2x2 part represented by + * column vectors @f$ \boldsymbol{a} @f$ and @f$ \boldsymbol{b} @f$: + * @f[ * \begin{pmatrix} * \color{m-warning} a_x & \color{m-warning} b_x & t_x \\ * \color{m-warning} a_y & \color{m-warning} b_y & t_y \\ @@ -447,17 +447,25 @@ template class Matrix3: public Matrix3x3 { * * the resulting scaling vector is: @f[ * \boldsymbol{s} = \begin{pmatrix} - * \operatorname{sgn}(a_x) | \boldsymbol{a} | \\ - * \operatorname{sgn}(a_y) | \boldsymbol{b} | + * | \boldsymbol{a} | \\ + * | \boldsymbol{b} | * \end{pmatrix} * @f] * + * Note that the returned vector is sign-less and the signs are instead + * contained in @ref rotation() const / @ref rotationShear() const in + * order to ensure @f$ \boldsymbol{R} \boldsymbol{S} = \boldsymbol{M} @f$ + * for @f$ \boldsymbol{R} @f$ and @f$ \boldsymbol{S} @f$ extracted out + * of @f$ \boldsymbol{M} @f$. The signs can be extracted for example by + * applying @ref Math::sign() on a @ref diagonal(), but keep in mind + * that the signs can be negative even for pure rotation matrices. + * * @see @ref scalingSquared(), @ref uniformScaling(), * @ref rotation() const, @ref Matrix4::scaling() const */ Vector2 scaling() const { - return {(*this)[0].xy().length()*T((*this)[0][0] < T(0) ? -1 : 1), - (*this)[1].xy().length()*T((*this)[1][1] < T(0) ? -1 : 1)}; + return {(*this)[0].xy().length(), + (*this)[1].xy().length()}; } /** diff --git a/src/Magnum/Math/Matrix4.h b/src/Magnum/Math/Matrix4.h index 856d320ea..11da5a499 100644 --- a/src/Magnum/Math/Matrix4.h +++ b/src/Magnum/Math/Matrix4.h @@ -685,11 +685,11 @@ template class Matrix4: public Matrix4x4 { /** * @brief Non-uniform scaling part of the matrix * - * *Signed* length of vectors in upper-left 3x3 part of the matrix. Use - * the faster alternative @ref scalingSquared() where possible. - * Assuming the following matrix, with the upper-left 3x3 part - * represented by column vectors @f$ \boldsymbol{a} @f$, - * @f$ \boldsymbol{b} @f$ and @f$ \boldsymbol{c} @f$: @f[ + * Length of vectors in upper-left 3x3 part of the matrix. Use the + * faster alternative @ref scalingSquared() where possible. Assuming + * the following matrix, with the upper-left 3x3 part represented by + * column vectors @f$ \boldsymbol{a} @f$, @f$ \boldsymbol{b} @f$ and + * @f$ \boldsymbol{c} @f$: @f[ * \begin{pmatrix} * \color{m-warning} a_x & \color{m-warning} b_x & \color{m-warning} c_x & t_x \\ * \color{m-warning} a_y & \color{m-warning} b_y & \color{m-warning} c_y & t_y \\ @@ -702,19 +702,27 @@ template class Matrix4: public Matrix4x4 { * * the resulting scaling vector is: @f[ * \boldsymbol{s} = \begin{pmatrix} - * \operatorname{sgn}(a_x) | \boldsymbol{a} | \\ - * \operatorname{sgn}(b_y) | \boldsymbol{b} | \\ - * \operatorname{sgn}(c_z) | \boldsymbol{c} | + * | \boldsymbol{a} | \\ + * | \boldsymbol{b} | \\ + * | \boldsymbol{c} | * \end{pmatrix} * @f] * + * Note that the returned vector is sign-less and the signs are instead + * contained in @ref rotation() const / @ref rotationShear() const in + * order to ensure @f$ \boldsymbol{R} \boldsymbol{S} = \boldsymbol{M} @f$ + * for @f$ \boldsymbol{R} @f$ and @f$ \boldsymbol{S} @f$ extracted out + * of @f$ \boldsymbol{M} @f$. The signs can be extracted for example by + * applying @ref Math::sign() on a @ref diagonal(), but keep in mind + * that the signs can be negative even for pure rotation matrices. + * * @see @ref scalingSquared(), @ref uniformScaling(), * @ref rotation() const, @ref Matrix3::scaling() const */ Vector3 scaling() const { - return {(*this)[0].xyz().length()*T((*this)[0][0] < T(0) ? -1 : 1), - (*this)[1].xyz().length()*T((*this)[1][1] < T(0) ? -1 : 1), - (*this)[2].xyz().length()*T((*this)[2][2] < T(0) ? -1 : 1)}; + return {(*this)[0].xyz().length(), + (*this)[1].xyz().length(), + (*this)[2].xyz().length()}; } /** diff --git a/src/Magnum/Math/Test/Matrix3Test.cpp b/src/Magnum/Math/Test/Matrix3Test.cpp index 2fb8c57ba..3f008b687 100644 --- a/src/Magnum/Math/Test/Matrix3Test.cpp +++ b/src/Magnum/Math/Test/Matrix3Test.cpp @@ -90,6 +90,7 @@ struct Matrix3Test: Corrade::TestSuite::Tester { void rotationNormalizedPart(); void rotationNormalizedPartNotOrthogonal(); void scalingPart(); + void rotationScalingPartNegative(); void uniformScalingPart(); void uniformScalingPartNotUniform(); void vectorParts(); @@ -140,6 +141,7 @@ Matrix3Test::Matrix3Test() { &Matrix3Test::rotationNormalizedPart, &Matrix3Test::rotationNormalizedPartNotOrthogonal, &Matrix3Test::scalingPart, + &Matrix3Test::rotationScalingPartNegative, &Matrix3Test::uniformScalingPart, &Matrix3Test::uniformScalingPartNotUniform, &Matrix3Test::vectorParts, @@ -526,18 +528,39 @@ void Matrix3Test::scalingPart() { Matrix3 translationRotationScaling = Matrix3::translation({2.0f, -3.0f})* Matrix3::rotation(15.0_degf)* - Matrix3::scaling({0.5f, -3.5f}); + Matrix3::scaling({0.5f, 3.5f}); - CORRADE_COMPARE(translationRotationScaling.scaling(), (Vector2{0.5f, -3.5f})); + CORRADE_COMPARE(translationRotationScaling.scaling(), (Vector2{0.5f, 3.5f})); CORRADE_COMPARE(translationRotationScaling.scalingSquared(), (Vector2{0.25f, 12.25f})); +} - Matrix3 rotationScaling = +void Matrix3Test::rotationScalingPartNegative() { + /* Large angle */ + Matrix3 largeAngle = Matrix3::rotation(215.0_degf)* - Matrix3::scaling({-0.5f, 3.5f}); - - /* Sign inverted because of the rotation */ - CORRADE_COMPARE(rotationScaling.scaling(), (Vector2{0.5f, -3.5f})); - CORRADE_COMPARE(rotationScaling.scalingSquared(), (Vector2{0.25f, 12.25f})); + Matrix3::scaling({0.5f, 3.5f}); + CORRADE_COMPARE(Matrix3::from(largeAngle.rotation(), {}), + Matrix3::rotation(215.0_degf)); + CORRADE_COMPARE(largeAngle.scaling(), (Vector2{0.5f, 3.5f})); + /* The parts should combine back to the same matrix */ + CORRADE_COMPARE( + Matrix3::from(largeAngle.rotation(), {})* + Matrix3::scaling(largeAngle.scaling()), + largeAngle); + + /* The sign gets contained in the rotation */ + Matrix3 negativeScaling = + Matrix3::rotation(15.0_degf)* + Matrix3::scaling({0.5f, -3.5f}); + CORRADE_COMPARE(Matrix3::from(negativeScaling.rotation(), {}), + Matrix3::rotation(15.0_degf)* + Matrix3::scaling(Vector2::yScale(-1))); + CORRADE_COMPARE(negativeScaling.scaling(), (Vector2{0.5f, 3.5f})); + /* The parts should combine back to the same matrix */ + CORRADE_COMPARE( + Matrix3::from(negativeScaling.rotation(), {})* + Matrix3::scaling(negativeScaling.scaling()), + negativeScaling); } void Matrix3Test::uniformScalingPart() { diff --git a/src/Magnum/Math/Test/Matrix4Test.cpp b/src/Magnum/Math/Test/Matrix4Test.cpp index 6ecf3276d..3feb71379 100644 --- a/src/Magnum/Math/Test/Matrix4Test.cpp +++ b/src/Magnum/Math/Test/Matrix4Test.cpp @@ -105,6 +105,7 @@ struct Matrix4Test: Corrade::TestSuite::Tester { void rotationNormalizedPart(); void rotationNormalizedPartNotOrthogonal(); void scalingPart(); + void rotationScalingPartNegative(); void uniformScalingPart(); void uniformScalingPartNotUniform(); void normalMatrixPart(); @@ -173,6 +174,7 @@ Matrix4Test::Matrix4Test() { &Matrix4Test::rotationNormalizedPart, &Matrix4Test::rotationNormalizedPartNotOrthogonal, &Matrix4Test::scalingPart, + &Matrix4Test::rotationScalingPartNegative, &Matrix4Test::uniformScalingPart, &Matrix4Test::uniformScalingPartNotUniform, &Matrix4Test::normalMatrixPart, @@ -760,18 +762,39 @@ void Matrix4Test::scalingPart() { Matrix4 translationRotationScaling = Matrix4::translation({2.0f, 5.0f, -3.0f})* Matrix4::rotation(-74.0_degf, Vector3(-1.0f, 2.0f, 2.0f).normalized())* - Matrix4::scaling({0.5f, -3.5f, 1.2f}); + Matrix4::scaling({0.5f, 3.5f, 1.2f}); - CORRADE_COMPARE(translationRotationScaling.scaling(), (Vector3{0.5f, -3.5f, 1.2f})); + CORRADE_COMPARE(translationRotationScaling.scaling(), (Vector3{0.5f, 3.5f, 1.2f})); CORRADE_COMPARE(translationRotationScaling.scalingSquared(), (Vector3{0.25f, 12.25f, 1.44f})); +} - Matrix4 rotationScaling = - Matrix4::rotation(-174.0_degf, Vector3(-1.0f, 2.0f, 2.0f).normalized())* - Matrix4::scaling({-0.5f, 3.5f, 1.2f}); - - /* Sign inverted because of the rotation */ - CORRADE_COMPARE(rotationScaling.scaling(), (Vector3{0.5f, -3.5f, -1.2f})); - CORRADE_COMPARE(rotationScaling.scalingSquared(), (Vector3{0.25f, 12.25f, 1.44f})); +void Matrix4Test::rotationScalingPartNegative() { + /* Large angle */ + Matrix4 largeAngle = + Matrix4::rotationY(215.0_degf)* + Matrix4::scaling({0.5f, 3.5f, 1.2f}); + CORRADE_COMPARE(Matrix4::from(largeAngle.rotation(), {}), + Matrix4::rotationY(215.0_degf)); + CORRADE_COMPARE(largeAngle.scaling(), (Vector3{0.5f, 3.5f, 1.2f})); + /* The parts should combine back to the same matrix */ + CORRADE_COMPARE( + Matrix4::from(largeAngle.rotation(), {})* + Matrix4::scaling(largeAngle.scaling()), + largeAngle); + + /* The sign gets contained in the rotation */ + Matrix4 negativeScaling = + Matrix4::rotationY(15.0_degf)* + Matrix4::scaling({0.5f, -3.5f, 1.2f}); + CORRADE_COMPARE(Matrix4::from(negativeScaling.rotation(), {}), + Matrix4::rotationY(15.0_degf)* + Matrix4::scaling(Vector3::yScale(-1))); + CORRADE_COMPARE(negativeScaling.scaling(), (Vector3{0.5f, 3.5f, 1.2f})); + /* The parts should combine back to the same matrix */ + CORRADE_COMPARE( + Matrix4::from(negativeScaling.rotation(), {})* + Matrix4::scaling(negativeScaling.scaling()), + negativeScaling); } void Matrix4Test::uniformScalingPart() {