From bdc36e5d7a1d1b673862dbccb1f06b8843825cd3 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= <mosra@centrum.cz>
Date: Fri, 24 Sep 2021 19:22:08 +0200
Subject: [PATCH] SceneGraph: properly decompose reflection in TRS transforms.

The test (that started asserting instead of failing after previous
commit) now passes.
---
 .../TranslationRotationScalingTransformation2D.h     | 12 +++++++++++-
 .../TranslationRotationScalingTransformation3D.h     | 12 +++++++++++-
 2 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/src/Magnum/SceneGraph/TranslationRotationScalingTransformation2D.h b/src/Magnum/SceneGraph/TranslationRotationScalingTransformation2D.h
index 450eb2162..ec55cd6cd 100644
--- a/src/Magnum/SceneGraph/TranslationRotationScalingTransformation2D.h
+++ b/src/Magnum/SceneGraph/TranslationRotationScalingTransformation2D.h
@@ -255,8 +255,18 @@ template<class T> Object<BasicTranslationRotationScalingTransformation2D<T>>& Ba
     /** @todo Do this in some common code so we don't need to include Object? */
     if(!static_cast<Object<BasicTranslationRotationScalingTransformation2D<T>>*>(this)->isScene()) {
         _translation = transformation.translation();
-        _rotation = Math::Complex<T>::fromMatrix(transformation.rotationShear());
         _scaling = transformation.scaling();
+        /* Using rotationShear() instead of rotation() here, as that'll lead to
+           an assert being fired in Quaternion::fromMatrix() instead of in
+           Matrix4::rotation(), which I suppose is a better place to hint at
+           the problem. */
+        /** @todo assert directly here? */
+        Math::Matrix2x2<T> rs = transformation.rotationShear();
+        if(rs.determinant() < T(0.0)) {
+            rs[0] *= T(-1.0);
+            _scaling[0] *= T(-1.0);
+        }
+        _rotation = Math::Complex<T>::fromMatrix(rs);
         static_cast<Object<BasicTranslationRotationScalingTransformation2D<T>>*>(this)->setDirty();
     }
 
diff --git a/src/Magnum/SceneGraph/TranslationRotationScalingTransformation3D.h b/src/Magnum/SceneGraph/TranslationRotationScalingTransformation3D.h
index 1973116d8..b3203fa88 100644
--- a/src/Magnum/SceneGraph/TranslationRotationScalingTransformation3D.h
+++ b/src/Magnum/SceneGraph/TranslationRotationScalingTransformation3D.h
@@ -344,8 +344,18 @@ template<class T> Object<BasicTranslationRotationScalingTransformation3D<T>>& Ba
     /** @todo Do this in some common code so we don't need to include Object? */
     if(!static_cast<Object<BasicTranslationRotationScalingTransformation3D<T>>*>(this)->isScene()) {
         _translation = transformation.translation();
-        _rotation = Math::Quaternion<T>::fromMatrix(transformation.rotationShear());
         _scaling = transformation.scaling();
+        /* Using rotationShear() instead of rotation() here, as that'll lead to
+           an assert being fired in Quaternion::fromMatrix() instead of in
+           Matrix4::rotation(), which I suppose is a better place to hint at
+           the problem. */
+        /** @todo assert directly here? */
+        Math::Matrix3x3<T> rs = transformation.rotationShear();
+        if(rs.determinant() < T(0.0)) {
+            rs[0] *= T(-1.0);
+            _scaling[0] *= T(-1.0);
+        }
+        _rotation = Math::Quaternion<T>::fromMatrix(rs);
         static_cast<Object<BasicTranslationRotationScalingTransformation3D<T>>*>(this)->setDirty();
     }