From 4c215750451b0a7c0f7aac3ee93ac918c27dc012 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= <mosra@centrum.cz>
Date: Mon, 15 Jan 2024 17:44:48 +0100
Subject: [PATCH] Math: fix Matrix::isOrthogonal() to catch negative dot
 product values.

Also adjusting two tests which were calling rotation() on matrices that
actually didn't have a correct rotation part, and it only slipped
through because of the bug in isOrthogonal().

Co-authored-by: John Turner <7strbass@gmail.com>
---
 src/Magnum/Math/Matrix.h                | 2 +-
 src/Magnum/Math/Test/ComplexTest.cpp    | 4 ++--
 src/Magnum/Math/Test/MatrixTest.cpp     | 3 +++
 src/Magnum/Math/Test/QuaternionTest.cpp | 4 ++--
 4 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/src/Magnum/Math/Matrix.h b/src/Magnum/Math/Matrix.h
index aeab958de..74da25de0 100644
--- a/src/Magnum/Math/Matrix.h
+++ b/src/Magnum/Math/Matrix.h
@@ -469,7 +469,7 @@ template<std::size_t size, class T> bool Matrix<size, T>::isOrthogonal() const {
     /* Orthogonality */
     for(std::size_t i = 0; i != size-1; ++i)
         for(std::size_t j = i+1; j != size; ++j)
-            if(dot(RectangularMatrix<size, size, T>::_data[i], RectangularMatrix<size, size, T>::_data[j]) > TypeTraits<T>::epsilon())
+            if(std::abs(dot(RectangularMatrix<size, size, T>::_data[i], RectangularMatrix<size, size, T>::_data[j])) > TypeTraits<T>::epsilon())
                 return false;
 
     return true;
diff --git a/src/Magnum/Math/Test/ComplexTest.cpp b/src/Magnum/Math/Test/ComplexTest.cpp
index 438c53c68..6456d1966 100644
--- a/src/Magnum/Math/Test/ComplexTest.cpp
+++ b/src/Magnum/Math/Test/ComplexTest.cpp
@@ -519,10 +519,10 @@ void ComplexTest::matrixNotRotation() {
 
     std::ostringstream out;
     Error redirectError{&out};
-    /* Shear, using rotation() instead of rotationScaling() as that isn't
+    /* Shear, using rotationShear() instead of rotationScaling() as that isn't
        supposed to "fix" the shear */
     Complex::fromMatrix((Matrix3::scaling({2.0f, 1.0f})*
-                         Matrix3::rotation(45.0_degf)).rotation());
+                         Matrix3::rotation(45.0_degf)).rotationShear());
     /* Reflection, using rotation() instead of rotationScaling() as that isn't
        supposed to "fix" the reflection either */
     Complex::fromMatrix((Matrix3::scaling({-1.0f, 1.0f})*
diff --git a/src/Magnum/Math/Test/MatrixTest.cpp b/src/Magnum/Math/Test/MatrixTest.cpp
index 4f9b2db15..3709622bf 100644
--- a/src/Magnum/Math/Test/MatrixTest.cpp
+++ b/src/Magnum/Math/Test/MatrixTest.cpp
@@ -331,6 +331,9 @@ void MatrixTest::isOrthogonal() {
     CORRADE_VERIFY(!Matrix3x3(Vector3(1.0f, 0.0f, 0.0f),
                               Vector3(0.0f, 1.0f, 0.0f),
                               Vector3(0.0f, 1.0f, 0.0f)).isOrthogonal());
+    CORRADE_VERIFY(!Matrix3x3(Vector3(1.0f, 0.0f, 0.0f),
+                              Vector3(0.0f, 1.0f, 0.0f),
+                              Vector3(0.0f, -1.0f, 0.0f)).isOrthogonal());
     CORRADE_VERIFY(Matrix3x3(Vector3(1.0f, 0.0f, 0.0f),
                              Vector3(0.0f, 1.0f, 0.0f),
                              Vector3(0.0f, 0.0f, 1.0f)).isOrthogonal());
diff --git a/src/Magnum/Math/Test/QuaternionTest.cpp b/src/Magnum/Math/Test/QuaternionTest.cpp
index 793de30de..1a476864d 100644
--- a/src/Magnum/Math/Test/QuaternionTest.cpp
+++ b/src/Magnum/Math/Test/QuaternionTest.cpp
@@ -644,10 +644,10 @@ void QuaternionTest::matrixNotRotation() {
 
     std::ostringstream out;
     Error redirectError{&out};
-    /* Shear, using rotation() instead of rotationScaling() as that isn't
+    /* Shear, using rotationShear() instead of rotationScaling() as that isn't
        supposed to "fix" the shear */
     Quaternion::fromMatrix((Matrix4::scaling({2.0f, 1.0f, 1.0f})*
-                            Matrix4::rotationZ(45.0_degf)).rotation());
+                            Matrix4::rotationZ(45.0_degf)).rotationShear());
     /* Reflection, using rotation() instead of rotationScaling() as that isn't
        supposed to "fix" the reflection either */
     Quaternion::fromMatrix((Matrix4::scaling({-1.0f, 1.0f, 1.0f})*