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 9aa68715db.
6 years ago · c85b537937
5 changed files with 99 additions and 44 deletions
--- 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.
--- a/src/Magnum/Math/Matrix3.h
+++ b/src/Magnum/Math/Matrix3.h
@ -431,11 +431,11 @@ template<class T> class Matrix3: public Matrix3x3<T> {
        /**
         * @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 T> class Matrix3: public Matrix3x3<T> {
         *
         * 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<T> 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()};
        }

        /**
--- a/src/Magnum/Math/Matrix4.h
+++ b/src/Magnum/Math/Matrix4.h
@ -685,11 +685,11 @@ template<class T> class Matrix4: public Matrix4x4<T> {
        /**
         * @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 T> class Matrix4: public Matrix4x4<T> {
         *
         * 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<T> 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()};
        }

        /**
--- 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() {
--- 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() {