Fix CMatrix<T>::getScale returning negative scale

irr/include/matrix4.h

@@ -782,7 +782,7 @@ inline CMatrix4<T> &CMatrix4<T>::setScale(const vector3d<T> &scale)
 	return *this;
 }
 
-//! Returns the absolute values of the scales of the matrix.
+//! Returns the absolute values of the scales of the 3x3 submatrix.
 /**
 Note: You only get back original values if the matrix only set the scale.
 Otherwise the result is a scale you can use to normalize the matrix axes,
@@ -791,19 +791,15 @@ but it's usually no longer what you did set with setScale.
 template <class T>
 inline vector3d<T> CMatrix4<T>::getScale() const
 {
-	// See http://www.robertblum.com/articles/2005/02/14/decomposing-matrices
-
-	// Deal with the 0 rotation case first
-	// Prior to Irrlicht 1.6, we always returned this value.
-	if (core::iszero(M[1]) && core::iszero(M[2]) &&
-			core::iszero(M[4]) && core::iszero(M[6]) &&
-			core::iszero(M[8]) && core::iszero(M[9]))
-		return vector3d<T>(M[0], M[5], M[10]);
-
-	// We have to do the full calculation.
-	return vector3d<T>(sqrtf(M[0] * M[0] + M[1] * M[1] + M[2] * M[2]),
-			sqrtf(M[4] * M[4] + M[5] * M[5] + M[6] * M[6]),
-			sqrtf(M[8] * M[8] + M[9] * M[9] + M[10] * M[10]));
+	auto row_vector_length = [this](int col) {
+		int i = 4 * col;
+		return sqrtf(M[i] * M[i] + M[i+1] * M[i+1] + M[i+2] * M[i+2]);
+	};
+	return {
+		row_vector_length(0),
+		row_vector_length(1),
+		row_vector_length(2),
+	};
 }
 
 template <class T>
@@ -891,29 +887,15 @@ inline core::vector3d<T> CMatrix4<T>::getRotationDegrees(const vector3d<T> &scal
 
 //! Returns a rotation that is equivalent to that set by setRotationDegrees().
 template <class T>
-inline core::vector3d<T> CMatrix4<T>::getRotationDegrees() const
+inline vector3d<T> CMatrix4<T>::getRotationDegrees() const
 {
 	// Note: Using getScale() here make it look like it could do matrix decomposition.
 	// It can't! It works (or should work) as long as rotation doesn't flip the handedness
 	// aka scale swapping 1 or 3 axes. (I think we could catch that as well by comparing
 	// crossproduct of first 2 axes to direction of third axis, but TODO)
 	// And maybe it should also offer the solution for the simple calculation
 	// without regarding scaling as Irrlicht did before 1.7
-	core::vector3d<T> scale(getScale());
-
-	// We assume the matrix uses rotations instead of negative scaling 2 axes.
-	// Otherwise it fails even for some simple cases, like rotating around
-	// 2 axes by 180° which getScale thinks is a negative scaling.
-	if (scale.Y < 0 && scale.Z < 0) {
-		scale.Y = -scale.Y;
-		scale.Z = -scale.Z;
-	} else if (scale.X < 0 && scale.Z < 0) {
-		scale.X = -scale.X;
-		scale.Z = -scale.Z;
-	} else if (scale.X < 0 && scale.Y < 0) {
-		scale.X = -scale.X;
-		scale.Y = -scale.Y;
-	}
+	vector3d<T> scale(getScale());
 
 	return getRotationDegrees(scale);
 }

src/client/content_cao.cpp

@@ -1459,34 +1459,6 @@ void GenericCAO::updateBones(f32 dtime)
 		bone->setScale(props.getScale(bone->getScale()));
 	}
 
-	// search through bones to find mistakenly rotated bones due to bug in Irrlicht
-	for (u32 i = 0; i < m_animated_meshnode->getJointCount(); ++i) {
-		scene::IBoneSceneNode *bone = m_animated_meshnode->getJointNode(i);
-		if (!bone)
-			continue;
-
-		//If bone is manually positioned there is no need to perform the bug check
-		bool skip = false;
-		for (auto &it : m_bone_override) {
-			if (it.first == bone->getName()) {
-				skip = true;
-				break;
-			}
-		}
-		if (skip)
-			continue;
-
-		// Workaround for Irrlicht bug
-		// We check each bone to see if it has been rotated ~180deg from its expected position due to a bug in Irricht
-		// when using EJUOR_CONTROL joint control. If the bug is detected we update the bone to the proper position
-		// and update the bones transformation.
-		v3f bone_rot = bone->getRelativeTransformation().getRotationDegrees();
-		float offset = fabsf(bone_rot.X - bone->getRotation().X);
-		if (offset > 179.9f && offset < 180.1f) {
-			bone->setRotation(bone_rot);
-			bone->updateAbsolutePosition();
-		}
-	}
 	// The following is needed for set_bone_pos to propagate to
 	// attached objects correctly.
 	// Irrlicht ought to do this, but doesn't when using EJUOR_CONTROL.

src/unittest/test_irr_matrix4.cpp

@@ -66,4 +66,21 @@ SECTION("setRotationRadians") {
     }
 }
 
+SECTION("getScale") {
+    SECTION("correctly gets the length of each row of the 3x3 submatrix") {
+        matrix4 A(
+            1, 2, 3, 0,
+            4, 5, 6, 0,
+            7, 8, 9, 0,
+            0, 0, 0, 1
+        );
+        v3f scale = A.getScale();
+        CHECK(scale.equals(v3f(
+            v3f(1, 2, 3).getLength(),
+            v3f(4, 5, 6).getLength(),
+            v3f(7, 8, 9).getLength()
+        )));
+    }
+}
+
 }