From 9aa68715db0465fe9583a80a292d1211155b351c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= Date: Sat, 18 Jan 2020 12:51:00 +0100 Subject: [PATCH] Math: preserve signs in the Matrix[34]::scaling() getter. This is a breaking change, but I think it is worth doing. Because now Matrix4::scaling(vec).scaling() == vec which was true also for translation and other. --- doc/changelog.dox | 10 ++++++++++ src/Magnum/Math/Matrix3.h | 18 +++++++++--------- src/Magnum/Math/Matrix4.h | 22 +++++++++++----------- src/Magnum/Math/Test/Matrix3Test.cpp | 12 ++++++++++-- src/Magnum/Math/Test/Matrix4Test.cpp | 12 ++++++++++-- 5 files changed, 50 insertions(+), 24 deletions(-) diff --git a/doc/changelog.dox b/doc/changelog.dox index bbb3cb75f..3f9425909 100644 --- a/doc/changelog.dox +++ b/doc/changelog.dox @@ -127,6 +127,12 @@ See also: in order to allow passing runtime-sized lists (see [mosra/magnum#403](https://github.com/mosra/magnum/pull/403)) +@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 + @subsubsection changelog-latest-changes-meshtools MeshTools library - Added @ref MeshTools::subdivideInPlace() that operates on a partially @@ -223,6 +229,10 @@ See also: @ref Animation library was changed to allow mutable access to the keys & values it references. Existing code needs to be changed to say @cpp TrackView @ce instead of @cpp TrackView @ce. +- @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. - @ref Platform::GlfwApplication::setMinWindowSize() / @ref Platform::GlfwApplication::setMaxWindowSize() and equivalent APIs in @ref Platform::Sdl2Application now premultiply the value with diff --git a/src/Magnum/Math/Matrix3.h b/src/Magnum/Math/Matrix3.h index 72a3593e8..d436a48db 100644 --- a/src/Magnum/Math/Matrix3.h +++ b/src/Magnum/Math/Matrix3.h @@ -430,11 +430,11 @@ template class Matrix3: public Matrix3x3 { /** * @brief Non-uniform scaling part of the matrix * - * 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[ + * *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[ * \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 \\ @@ -446,8 +446,8 @@ template class Matrix3: public Matrix3x3 { * * the resulting scaling vector is: @f[ * \boldsymbol{s} = \begin{pmatrix} - * | \boldsymbol{a} | \\ - * | \boldsymbol{b} | + * \operatorname{sgn}(a_x) | \boldsymbol{a} | \\ + * \operatorname{sgn}(a_y) | \boldsymbol{b} | * \end{pmatrix} * @f] * @@ -455,8 +455,8 @@ template class Matrix3: public Matrix3x3 { * @ref rotation() const, @ref Matrix4::scaling() const */ Vector2 scaling() const { - return {(*this)[0].xy().length(), - (*this)[1].xy().length()}; + 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)}; } /** diff --git a/src/Magnum/Math/Matrix4.h b/src/Magnum/Math/Matrix4.h index 630b9b357..b539b9457 100644 --- a/src/Magnum/Math/Matrix4.h +++ b/src/Magnum/Math/Matrix4.h @@ -684,11 +684,11 @@ template class Matrix4: public Matrix4x4 { /** * @brief Non-uniform scaling part of the matrix * - * 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[ + * *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[ * \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 \\ @@ -701,9 +701,9 @@ template class Matrix4: public Matrix4x4 { * * the resulting scaling vector is: @f[ * \boldsymbol{s} = \begin{pmatrix} - * | \boldsymbol{a} | \\ - * | \boldsymbol{b} | \\ - * | \boldsymbol{c} | + * \operatorname{sgn}(a_x) | \boldsymbol{a} | \\ + * \operatorname{sgn}(b_y) | \boldsymbol{b} | \\ + * \operatorname{sgn}(c_z) | \boldsymbol{c} | * \end{pmatrix} * @f] * @@ -711,9 +711,9 @@ template class Matrix4: public Matrix4x4 { * @ref rotation() const, @ref Matrix3::scaling() const */ Vector3 scaling() const { - return {(*this)[0].xyz().length(), - (*this)[1].xyz().length(), - (*this)[2].xyz().length()}; + 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)}; } /** diff --git a/src/Magnum/Math/Test/Matrix3Test.cpp b/src/Magnum/Math/Test/Matrix3Test.cpp index a0245e0a3..2fb8c57ba 100644 --- a/src/Magnum/Math/Test/Matrix3Test.cpp +++ b/src/Magnum/Math/Test/Matrix3Test.cpp @@ -526,10 +526,18 @@ 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 = + 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})); } void Matrix3Test::uniformScalingPart() { diff --git a/src/Magnum/Math/Test/Matrix4Test.cpp b/src/Magnum/Math/Test/Matrix4Test.cpp index 61a4eeff5..6ecf3276d 100644 --- a/src/Magnum/Math/Test/Matrix4Test.cpp +++ b/src/Magnum/Math/Test/Matrix4Test.cpp @@ -760,10 +760,18 @@ 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::uniformScalingPart() {