feat(Math): Extract the correct negative scale from matrices (#883)

src/math/Mat4.js

@@ -283,14 +283,11 @@ export class Mat4 {
 	getRotation(scale) {
 		if (!scale) scale = this.getScale();
 
-		let sX = scale.x;
-		if (this.getDeterminant() < 0) sX = -sX;
-
 		let m00 = this.values[0][0]; let m10 = this.values[0][1]; let m20 = this.values[0][2];
 		let m01 = this.values[1][0]; let m11 = this.values[1][1]; let m21 = this.values[1][2];
 		let m02 = this.values[2][0]; let m12 = this.values[2][1]; let m22 = this.values[2][2];
 
-		const invSX = 1 / sX;
+		const invSX = 1 / scale.x;
 		const invSY = 1 / scale.y;
 		const invSZ = 1 / scale.z;
 
@@ -334,10 +331,26 @@ export class Mat4 {
 	}
 
 	getScale() {
-		const sx = (new Vec3(this.values[0])).magnitude;
-		const sy = (new Vec3(this.values[1])).magnitude;
-		const sz = (new Vec3(this.values[2])).magnitude;
-		return new Vec3(sx, sy, sz);
+		const scale = new Vec3(
+			(new Vec3(this.values[0])).magnitude,
+			(new Vec3(this.values[1])).magnitude,
+			(new Vec3(this.values[2])).magnitude,
+		);
+		if (this.getDeterminant() < 0) {
+			const xNegative = this.values[0][0] < 0;
+			const yNegative = this.values[1][1] < 0;
+			const zNegative = this.values[2][2] < 0;
+			if (xNegative && yNegative && zNegative) {
+				scale.multiplyScalar(-1);
+			} else if (yNegative) {
+				scale.y *= -1;
+			} else if (zNegative) {
+				scale.z *= -1;
+			} else {
+				scale.x *= -1;
+			}
+		}
+		return scale;
 	}
 
 	/**

test/unit/src/core/Entity/transformations.test.js

@@ -1,7 +1,7 @@
 import { assertEquals, assertThrows } from "std/testing/asserts.ts";
 import { assertSpyCalls, spy, stub } from "std/testing/mock.ts";
 import { Entity, Mat4, Quat, Vec3 } from "../../../../../src/mod.js";
-import { assertMatAlmostEquals, assertVecAlmostEquals } from "../../../shared/asserts.js";
+import { assertMatAlmostEquals, assertQuatAlmostEquals, assertVecAlmostEquals } from "../../../shared/asserts.js";
 
 // ==== Local transformations ==================================================
 
@@ -554,6 +554,50 @@ Deno.test({
 	},
 });
 
+Deno.test({
+	name: "The local scale and rotation of negatively scaled entities doesn't change when setting the rotation",
+	fn() {
+		const root = new Entity("root");
+		const entity = root.add(new Entity());
+		entity.scale.set(-1, 1, 1);
+		assertMatAlmostEquals(entity.worldMatrix, [-1, 0, 0, 0, 0, 1, 0, 0, 0, 0, 1, 0, 0, 0, 0, 1]);
+
+		// When we rotate the object slightly, the world scale and rot match the local transformation.
+		entity.rot.set(Quat.fromAxisAngle(0, 1, 0, 0.1));
+		assertVecAlmostEquals(entity.worldScale, [-1, 1, 1]);
+		assertQuatAlmostEquals(entity.worldRot, Quat.fromAxisAngle(0, 1, 0, 0.1));
+		assertVecAlmostEquals(entity.scale, [-1, 1, 1]);
+		assertQuatAlmostEquals(entity.rot, Quat.fromAxisAngle(0, 1, 0, 0.1));
+
+		// But when we rotate too far, the worldScale will change.
+		// This is because the worldScale and worldRot get extracted from the world matrix,
+		// which doesn't contain information about which axis the object was scaled on.
+		// The local rotation and scale, however, should never be changed in this case.
+		// The only thing that is allowed to change local position/rotation/scale is when setting
+		// the localMatrix or worldMatrix.
+		entity.rot.set(Quat.fromAxisAngle(0, 1, 0, 2));
+		assertVecAlmostEquals(entity.worldScale, [1, 1, -1]);
+		assertQuatAlmostEquals(entity.worldRot, Quat.fromAxisAngle(0, 1, 0, 2 - Math.PI));
+		assertVecAlmostEquals(entity.scale, [-1, 1, 1]);
+		assertQuatAlmostEquals(entity.rot, Quat.fromAxisAngle(0, 1, 0, 2));
+	},
+});
+
+Deno.test({
+	name: "setting the a local matrix with a negative scale maintains that scale where possible",
+	fn() {
+		const entity = new Entity();
+		entity.localMatrix.set(Mat4.createScale(new Vec3(-1, 1, 1)));
+		assertVecAlmostEquals(entity.scale, [-1, 1, 1]);
+		entity.localMatrix.set(Mat4.createScale(new Vec3(1, -1, 1)));
+		assertVecAlmostEquals(entity.scale, [1, -1, 1]);
+		entity.localMatrix.set(Mat4.createScale(new Vec3(1, 1, -1)));
+		assertVecAlmostEquals(entity.scale, [1, 1, -1]);
+		entity.localMatrix.set(Mat4.createScale(new Vec3(-1, -1, -1)));
+		assertVecAlmostEquals(entity.scale, [-1, -1, -1]);
+	},
+});
+
 Deno.test({
 	name: "compute worldMatrix when entity is added as child",
 	fn() {

test/unit/src/math/Mat4.test.js

@@ -147,6 +147,64 @@ Deno.test({
 	},
 });
 
+Deno.test({
+	name: "When the scale is negative getScale() returns a scale that results in the least amount of rotation",
+	fn() {
+		const rotations = [
+			Quat.identity,
+			Quat.fromAxisAngle(1, 0, 0, 0.5),
+			Quat.fromAxisAngle(1, 0, 0, -0.5),
+			Quat.fromAxisAngle(0, 1, 0, 0.5),
+			Quat.fromAxisAngle(0, 1, 0, -0.5),
+			Quat.fromAxisAngle(0, 0, 1, 0.5),
+			Quat.fromAxisAngle(0, 0, 1, -0.5),
+			Quat.fromAxisAngle(1, 1, 1, 0.5),
+			Quat.fromAxisAngle(1, 1, 1, -0.5),
+		];
+
+		const scales = [
+			new Vec3(-1, 1, 1),
+			new Vec3(1, -1, 1),
+			new Vec3(1, 1, -1),
+			new Vec3(-1, -1, -1),
+		];
+
+		for (const rotation of rotations) {
+			for (const scale of scales) {
+				const mat = Mat4.createPosRotScale(Vec3.zero, rotation, scale);
+				assertVecAlmostEquals(mat.getScale(), scale);
+			}
+		}
+	},
+});
+
+Deno.test({
+	name: "decomposing and then recreating should not change the matrix",
+	fn() {
+		const tests = [
+			Mat4.createPosRotScale(new Vec3(1, 2, 3), Quat.identity, new Vec3(-1, -1, -1)),
+			Mat4.createPosRotScale(new Vec3(3, 4, 5), Quat.identity, new Vec3(-1, 1, 1)),
+			Mat4.createPosRotScale(new Vec3(0, 0, 0), Quat.identity, new Vec3(1, -1, 1)),
+			Mat4.createPosRotScale(new Vec3(1, 2, 3), Quat.identity, new Vec3(1, 1, -1)),
+			Mat4.createPosRotScale(new Vec3(1, 2, 3), Quat.identity, new Vec3(1, -1, -1)),
+			Mat4.createPosRotScale(new Vec3(1, 2, 3), Quat.identity, new Vec3(-1, 1, -1)),
+			Mat4.createPosRotScale(new Vec3(1, 2, 3), Quat.fromAxisAngle(0, 1, 0, Math.PI), new Vec3(-1, 1, -1)),
+			Mat4.createPosRotScale(new Vec3(1, 2, 3), Quat.fromAxisAngle(0, 1, 0, Math.PI), new Vec3(1, 1, -1)),
+			Mat4.createPosRotScale(new Vec3(1, 2, 3), Quat.fromAxisAngle(0, 1, 0, Math.PI), Vec3.one),
+			Mat4.createPosRotScale(Vec3.zero, Quat.fromAxisAngle(0, 1, 0, Math.PI), Vec3.one),
+			Mat4.createPosRotScale(Vec3.zero, Quat.fromAxisAngle(0, 1, 0, Math.PI), Vec3.one),
+			Mat4.createPosRotScale(Vec3.zero, Quat.identity, Vec3.one),
+			// Mat4.createPosRotScale(Vec3.zero, Quat.identity, Vec3.zero),
+			// Mat4.createPosRotScale(Vec3.one, Quat.identity, Vec3.zero),
+		];
+		for (const mat of tests) {
+			const { pos, rot, scale } = mat.decompose();
+			const mat2 = Mat4.createPosRotScale(pos, rot, scale);
+			assertMatAlmostEquals(mat, mat2);
+		}
+	},
+});
+
 Deno.test({
 	name: "multiplyMatrices",
 	fn() {