[no squash] Misc. stuff around rotations

irr/include/matrix4.h

@@ -179,25 +179,25 @@ class CMatrix4
 	CMatrix4<T> &setRotationDegrees(const vector3d<T> &rotation);
 
 	//! Get the rotation, as set by setRotation() when you already know the scale used to create the matrix
-	/** NOTE: The scale needs to be the correct one used to create this matrix.
-		You can _not_ use the result of getScale(), but have to save your scale
-		variable in another place (like ISceneNode does).
+	/**
 	NOTE: No scale value can be 0 or the result is undefined.
 	NOTE: It does not necessarily return the *same* Euler angles as those set by setRotationDegrees(),
 		but the rotation will be equivalent,  i.e. will have the same result when used to rotate a vector or node.
 	NOTE: It will (usually) give wrong results when further transformations have been added in the matrix (like shear).
 	WARNING: There have been troubles with this function over the years and we may still have missed some corner cases.
 		It's generally safer to keep the rotation and scale you used to create the matrix around and work with those.
 	*/
-	vector3d<T> getRotationDegrees(const vector3d<T> &scale) const;
+	vector3d<T> getRotationRadians(const vector3d<T> &scale) const;
 
 	//! Returns the rotation, as set by setRotation().
 	/** NOTE: You will have the same end-rotation as used in setRotation, but it might not use the same axis values.
-		NOTE: This only works correct if no other matrix operations have been done on the inner 3x3 matrix besides
-			setting rotation (so no scale/shear). Thought it (probably) works as long as scale doesn't flip handedness.
+		NOTE: This only works correctly for TRS matrix products where S is a positive, component-wise scaling (see setScale).
 		NOTE: It does not necessarily return the *same* Euler angles as those set by setRotationDegrees(),
-		but the rotation will be equivalent,  i.e. will have the same result when used to rotate a vector or node.
+		but the rotation will be equivalent, i.e. will have the same result when used to rotate a vector or node.
 	*/
+	vector3d<T> getRotationRadians() const;
+
+	//! Same as getRotationRadians, but returns degrees.
 	vector3d<T> getRotationDegrees() const;
 
 	//! Make a rotation matrix from angle and axis, assuming left handed rotation.
@@ -425,6 +425,9 @@ class CMatrix4
 	bool equals(const CMatrix4<T> &other, const T tolerance = (T)ROUNDING_ERROR_f64) const;
 
 private:
+	template <bool degrees>
+	vector3d<T> getRotation(const vector3d<T> &scale) const;
+
 	//! Matrix data, stored in row-major order
 	T M[16];
 };
@@ -779,63 +782,60 @@ inline CMatrix4<T> &CMatrix4<T>::setRotationRadians(const vector3d<T> &rotation)
 	return *this;
 }
 
-//! Returns a rotation which (mostly) works in combination with the given scale
-/**
-This code was originally written by by Chev (assuming no scaling back then,
-we can be blamed for all problems added by regarding scale)
-*/
 template <class T>
-inline vector3d<T> CMatrix4<T>::getRotationDegrees(const vector3d<T> &scale_) const
+template <bool degrees>
+inline vector3d<T> CMatrix4<T>::getRotation(const vector3d<T> &scale_) const
 {
+	// Based on code by Chev
 	const CMatrix4<T> &mat = *this;
 	const vector3d<f64> scale(iszero(scale_.X) ? FLT_MAX : scale_.X, iszero(scale_.Y) ? FLT_MAX : scale_.Y, iszero(scale_.Z) ? FLT_MAX : scale_.Z);
 	const vector3d<f64> invScale(reciprocal(scale.X), reciprocal(scale.Y), reciprocal(scale.Z));
 
-	f64 Y = -asin(clamp(mat[2] * invScale.X, -1.0, 1.0));
-	const f64 C = cos(Y);
-	Y *= RADTODEG64;
+	f64 a = clamp(mat[2] * invScale.X, -1.0, 1.0);
+	f64 Y = -asin(a);
 
 	f64 rotx, roty, X, Z;
 
-	if (!iszero((T)C)) {
-		const f64 invC = reciprocal(C);
-		rotx = mat[10] * invC * invScale.Z;
-		roty = mat[6] * invC * invScale.Y;
-		X = atan2(roty, rotx) * RADTODEG64;
-		rotx = mat[0] * invC * invScale.X;
-		roty = mat[1] * invC * invScale.X;
-		Z = atan2(roty, rotx) * RADTODEG64;
+	if (!core::equals(std::abs(a), 1.0)) {
+		// abs(a) = abs(sin(Y)) = 1 <=> cos(Y) = 0
+		rotx = mat[10] * invScale.Z;
+		roty = mat[6] * invScale.Y;
+		X = atan2(roty, rotx);
+		rotx = mat[0] * invScale.X;
+		roty = mat[1] * invScale.X;
+		Z = atan2(roty, rotx);
 	} else {
 		X = 0.0;
-		rotx = mat[5] * invScale.Y;
-		roty = -mat[4] * invScale.Y;
-		Z = atan2(roty, rotx) * RADTODEG64;
+		rotx = mat[5];
+		roty = -mat[4];
+		Z = atan2(roty, rotx);
 	}
 
-	// fix values that get below zero
-	if (X < 0.0)
-		X += 360.0;
-	if (Y < 0.0)
-		Y += 360.0;
-	if (Z < 0.0)
-		Z += 360.0;
+	if (degrees) {
+		X *= core::RADTODEG64;
+		Y *= core::RADTODEG64;
+		Z *= core::RADTODEG64;
+	}
 
 	return vector3d<T>((T)X, (T)Y, (T)Z);
 }
 
-//! Returns a rotation that is equivalent to that set by setRotationDegrees().
 template <class T>
-inline vector3d<T> CMatrix4<T>::getRotationDegrees() const
+inline vector3d<T> CMatrix4<T>::getRotationRadians(const vector3d<T> &scale) 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
-	vector3d<T> scale(getScale());
+	return getRotation<false>(scale);
+}
 
-	return getRotationDegrees(scale);
+template <class T>
+inline vector3d<T> CMatrix4<T>::getRotationRadians() const
+{
+	return getRotationRadians(getScale());
+}
+
+template <class T>
+inline vector3d<T> CMatrix4<T>::getRotationDegrees() const
+{
+	return getRotation<true>(getScale());
 }
 
 //! Sets matrix to rotation matrix defined by axis and angle, assuming LH rotation

irr/src/CAnimatedMeshSceneNode.cpp

@@ -619,7 +619,7 @@ void CAnimatedMeshSceneNode::animateJoints(bool CalculateAbsolutePositions)
 
 				// Code is slow, needs to be fixed up
 
-				const core::quaternion RotationStart(PretransitingSave[n].getRotationDegrees() * core::DEGTORAD);
+				const core::quaternion RotationStart(PretransitingSave[n].getRotationRadians());
 				const core::quaternion RotationEnd(JointChildSceneNodes[n]->getRotation() * core::DEGTORAD);
 
 				core::quaternion QRotation;

irr/src/CGLTFMeshFileLoader.cpp

@@ -546,16 +546,6 @@ void SelfType::MeshExtractor::deferAddMesh(
 	});
 }
 
-// Base transformation between left & right handed coordinate systems.
-// This just inverts the Z axis.
-static const core::matrix4 leftToRight = core::matrix4(
-	1, 0, 0, 0,
-	0, 1, 0, 0,
-	0, 0, -1, 0,
-	0, 0, 0, 1
-);
-static const core::matrix4 rightToLeft = leftToRight;
-
 static core::matrix4 loadTransform(const tiniergltf::Node::Matrix &m, SkinnedMesh::SJoint *joint)
 {
 	// Note: Under the hood, this casts these doubles to floats.
@@ -570,14 +560,7 @@ static core::matrix4 loadTransform(const tiniergltf::Node::Matrix &m, SkinnedMes
 
 	auto scale = mat.getScale();
 	joint->Animatedscale = scale;
-	core::matrix4 inverseScale;
-	inverseScale.setScale(core::vector3df(
-			scale.X == 0 ? 0 : 1 / scale.X,
-			scale.Y == 0 ? 0 : 1 / scale.Y,
-			scale.Z == 0 ? 0 : 1 / scale.Z));
-
-	core::matrix4 axisNormalizedMat = inverseScale * mat;
-	joint->Animatedrotation = axisNormalizedMat.getRotationDegrees();
+	joint->Animatedrotation = mat.getRotationRadians(scale);
 	// Invert the rotation because it is applied using `getMatrix_transposed`,
 	// which again inverts.
 	joint->Animatedrotation.makeInverse();

irr/src/CXMeshFileLoader.cpp

@@ -1526,8 +1526,6 @@ bool CXMeshFileLoader::parseDataObjectAnimationKey(SkinnedMesh::SJoint *joint)
 				os::Printer::log("Line", core::stringc(Line).c_str(), ELL_WARNING);
 			}
 
-			// core::vector3df rotation = mat.getRotationDegrees();
-
 			AnimatedMesh->addRotationKey(joint, time, core::quaternion(mat.getTransposed()));
 			AnimatedMesh->addPositionKey(joint, time, mat.getTranslation());
 

src/client/clientenvironment.cpp

@@ -441,8 +441,8 @@ void ClientEnvironment::getSelectedActiveObjects(
 		GenericCAO* gcao = dynamic_cast<GenericCAO*>(obj);
 		if (gcao != nullptr && gcao->getProperties().rotate_selectionbox) {
 			gcao->getSceneNode()->updateAbsolutePosition();
-			const v3f deg = obj->getSceneNode()->getAbsoluteTransformation().getRotationDegrees();
-			collision = boxLineCollision(selection_box, deg,
+			const v3f rad = obj->getSceneNode()->getAbsoluteTransformation().getRotationRadians();
+			collision = boxLineCollision(selection_box, rad,
 				rel_pos, line_vector, &current_intersection, &current_normal, &current_raw_normal);
 		} else {
 			collision = boxLineCollision(selection_box, rel_pos, line_vector,

src/client/game.cpp

@@ -3245,9 +3245,10 @@ PointedThing Game::updatePointedThing(
 			hud->setSelectionPos(pos, camera_offset);
 			GenericCAO* gcao = dynamic_cast<GenericCAO*>(runData.selected_object);
 			if (gcao != nullptr && gcao->getProperties().rotate_selectionbox)
-				hud->setSelectionRotation(gcao->getSceneNode()->getAbsoluteTransformation().getRotationDegrees());
+				hud->setSelectionRotationRadians(gcao->getSceneNode()
+						->getAbsoluteTransformation().getRotationRadians());
 			else
-				hud->setSelectionRotation(v3f());
+				hud->setSelectionRotationRadians(v3f());
 		}
 		hud->setSelectedFaceNormal(result.raw_intersection_normal);
 	} else if (result.type == POINTEDTHING_NODE) {
@@ -3267,7 +3268,7 @@ PointedThing Game::updatePointedThing(
 		}
 		hud->setSelectionPos(intToFloat(result.node_undersurface, BS),
 			camera_offset);
-		hud->setSelectionRotation(v3f());
+		hud->setSelectionRotationRadians(v3f());
 		hud->setSelectedFaceNormal(result.intersection_normal);
 	}
 

src/client/hud.cpp

@@ -880,7 +880,7 @@ void Hud::drawSelectionMesh()
 	core::matrix4 translate;
 	translate.setTranslation(m_selection_pos_with_offset);
 	core::matrix4 rotation;
-	rotation.setRotationDegrees(m_selection_rotation);
+	rotation.setRotationRadians(m_selection_rotation_radians);
 	driver->setTransform(video::ETS_WORLD, translate * rotation);
 
 	if (m_mode == HIGHLIGHT_BOX) {

src/client/hud.h

@@ -74,9 +74,15 @@ class Hud
 
 	v3f getSelectionPos() const { return m_selection_pos; }
 
-	void setSelectionRotation(v3f rotation) { m_selection_rotation = rotation; }
+	void setSelectionRotationRadians(v3f rotation)
+	{
+		m_selection_rotation_radians = rotation;
+	}
 
-	v3f getSelectionRotation() const { return m_selection_rotation; }
+	v3f getSelectionRotationRadians() const
+	{
+		return m_selection_rotation_radians;
+	}
 
 	void setSelectionMeshColor(const video::SColor &color)
 	{
@@ -129,7 +135,7 @@ class Hud
 	std::vector<aabb3f> m_halo_boxes;
 	v3f m_selection_pos;
 	v3f m_selection_pos_with_offset;
-	v3f m_selection_rotation;
+	v3f m_selection_rotation_radians;
 
 	scene::IMesh *m_selection_mesh = nullptr;
 	video::SColor m_selection_mesh_color;

src/raycast.cpp

@@ -124,13 +124,13 @@ bool boxLineCollision(const aabb3f &box, const v3f start,
 	return false;
 }
 
-bool boxLineCollision(const aabb3f &box, const v3f rotation,
-	const v3f start, const v3f dir,
+bool boxLineCollision(const aabb3f &box, v3f rotation_radians,
+	v3f start, v3f dir,
 	v3f *collision_point, v3f *collision_normal, v3f *raw_collision_normal)
 {
 	// Inversely transform the ray rather than rotating the box faces;
 	// this allows us to continue using a simple ray - AABB intersection
-	core::quaternion rot(rotation * core::DEGTORAD);
+	core::quaternion rot(rotation_radians);
 	rot.makeInverse();
 
 	bool collision = boxLineCollision(box, rot * start, rot * dir, collision_point, collision_normal);

src/server/unit_sao.h

@@ -30,7 +30,7 @@ class UnitSAO : public ServerActiveObject
 		v3f res;
 		// First rotate by m_rotation, then rotate by the automatic rotate yaw
 		(core::quaternion(v3f(0, -m_rotation_add_yaw * core::DEGTORAD, 0))
-				* core::quaternion(rot.getRotationDegrees() * core::DEGTORAD))
+				* core::quaternion(rot.getRotationRadians()))
 				.toEuler(res);
 		return res * core::RADTODEG;
 	}

src/unittest/CMakeLists.txt

@@ -13,6 +13,8 @@ set (UNITTEST_SRCS
 	${CMAKE_CURRENT_SOURCE_DIR}/test_filesys.cpp
 	${CMAKE_CURRENT_SOURCE_DIR}/test_inventory.cpp
 	${CMAKE_CURRENT_SOURCE_DIR}/test_irrptr.cpp
+	${CMAKE_CURRENT_SOURCE_DIR}/test_irr_matrix4.cpp
+	${CMAKE_CURRENT_SOURCE_DIR}/test_irr_rotation.cpp
 	${CMAKE_CURRENT_SOURCE_DIR}/test_logging.cpp
 	${CMAKE_CURRENT_SOURCE_DIR}/test_lua.cpp
 	${CMAKE_CURRENT_SOURCE_DIR}/test_map.cpp
@@ -54,7 +56,6 @@ set (UNITTEST_CLIENT_SRCS
 	${CMAKE_CURRENT_SOURCE_DIR}/test_eventmanager.cpp
 	${CMAKE_CURRENT_SOURCE_DIR}/test_gameui.cpp
 	${CMAKE_CURRENT_SOURCE_DIR}/test_irr_gltf_mesh_loader.cpp
-	${CMAKE_CURRENT_SOURCE_DIR}/test_irr_matrix4.cpp
 	${CMAKE_CURRENT_SOURCE_DIR}/test_mesh_compare.cpp
 	${CMAKE_CURRENT_SOURCE_DIR}/test_keycode.cpp
 	PARENT_SCOPE)

src/unittest/test_irr_matrix4.cpp

@@ -2,6 +2,7 @@
 // SPDX-License-Identifier: LGPL-2.1-or-later
 
 #include "catch.h"
+#include "catch_amalgamated.hpp"
 #include "irrMath.h"
 #include "matrix4.h"
 #include "irr_v3d.h"
@@ -83,4 +84,40 @@ SECTION("getScale") {
 	}
 }
 
+SECTION("getRotationRadians") {
+	auto test_rotation_degrees = [](v3f rad, v3f scale) {
+		matrix4 S;
+		S.setScale(scale);
+		matrix4 R;
+		R.setRotationRadians(rad);
+		v3f rot = (R * S).getRotationRadians();
+		matrix4 B;
+		B.setRotationRadians(rot);
+		CHECK(matrix_equals(R, B));
+	};
+	SECTION("returns a rotation equivalent to the original rotation") {
+		test_rotation_degrees({1.0f, 2.0f, 3.0f}, v3f(1));
+		Catch::Generators::RandomFloatingGenerator<f32> gen_angle(0.0f, 2 * core::PI, Catch::getSeed());
+		Catch::Generators::RandomFloatingGenerator<f32> gen_scale(0.1f, 10, Catch::getSeed());
+		auto draw = [](auto gen) {
+			f32 f = gen.get();
+			gen.next();
+			return f;
+		};
+		auto draw_v3f = [&](auto gen) {
+			return v3f{draw(gen), draw(gen), draw(gen)};
+		};
+		for (int i = 0; i < 1000; ++i)
+			test_rotation_degrees(draw_v3f(gen_angle), draw_v3f(gen_scale));
+		for (f32 i = 0; i < 4; ++i)
+		for (f32 j = 0; j < 4; ++j)
+		for (f32 k = 0; k < 4; ++k) {
+			v3f rad = core::PI / 4.0f * v3f(i, j, k);
+			for (int l = 0; l < 100; ++l) {
+				test_rotation_degrees(rad, draw_v3f(gen_scale));
+			}
+		}
+	}
+}
+
 }