Merge pull request #7438 from tagcup/matrix3_rotate_fix

Fix the order in which additional transformations are applied

core/math/matrix3.cpp

@@ -133,16 +133,18 @@ Matrix3 Matrix3::transposed() const {
 	return tr;
 }
 
+// Multiplies the matrix from left by the scaling matrix: M -> S.M
+// See the comment for Matrix3::rotated for further explanation.
 void Matrix3::scale(const Vector3& p_scale) {
 
 	elements[0][0]*=p_scale.x;
-	elements[1][0]*=p_scale.x;
+	elements[0][1]*=p_scale.x;
-	elements[2][0]*=p_scale.x;
+	elements[0][2]*=p_scale.x;
-	elements[0][1]*=p_scale.y;
+	elements[1][0]*=p_scale.y;
 	elements[1][1]*=p_scale.y;
-	elements[2][1]*=p_scale.y;
+	elements[1][2]*=p_scale.y;
-	elements[0][2]*=p_scale.z;
+	elements[2][0]*=p_scale.z;
-	elements[1][2]*=p_scale.z;
+	elements[2][1]*=p_scale.z;
 	elements[2][2]*=p_scale.z;
 }
 
@@ -154,8 +156,13 @@ Matrix3 Matrix3::scaled( const Vector3& p_scale ) const {
 }
 
 Vector3 Matrix3::get_scale() const {
-
+	// We are assuming M = R.S, and performing a polar decomposition to extract R and S.
-	return Vector3(
+	// FIXME: We eventually need a proper polar decomposition.
+	// As a cheap workaround until then, to ensure that R is a proper rotation matrix with determinant +1
+	// (such that it can be represented by a Quat or Euler angles), we absorb the sign flip into the scaling matrix.
+	// As such, it works in conjuction with get_rotation().
+	real_t det_sign = determinant() > 0 ? 1 : -1;
+	return det_sign*Vector3(
 		Vector3(elements[0][0],elements[1][0],elements[2][0]).length(),
 		Vector3(elements[0][1],elements[1][1],elements[2][1]).length(),
 		Vector3(elements[0][2],elements[1][2],elements[2][2]).length()
@@ -163,18 +170,40 @@ Vector3 Matrix3::get_scale() const {
 
 }
 
-// Matrix3::rotate and Matrix3::rotated return M * R(axis,phi), and is a convenience function. They do *not* perform proper matrix rotation.
+// Multiplies the matrix from left by the rotation matrix: M -> R.M
-void Matrix3::rotate(const Vector3& p_axis, real_t p_phi) {
+// Note that this does *not* rotate the matrix itself.
-	// TODO: This function should also be renamed as the current name is misleading: rotate does *not* perform matrix rotation.
+//
-	// Same problem affects Matrix3::rotated.
+// The main use of Matrix3 is as Transform.basis, which is used a the transformation matrix
-	// A similar problem exists in 2D math, which will be handled separately.
+// of 3D object. Rotate here refers to rotation of the object (which is R * (*this)),
-	// After Matrix3 is renamed to Basis, this comments needs to be revised.
+// not the matrix itself (which is R * (*this) * R.transposed()).
-	*this = *this * Matrix3(p_axis, p_phi);
+Matrix3 Matrix3::rotated(const Vector3& p_axis, real_t p_phi) const {
+	return Matrix3(p_axis, p_phi) * (*this);
 }
 
-Matrix3 Matrix3::rotated(const Vector3& p_axis, real_t p_phi) const {
+void Matrix3::rotate(const Vector3& p_axis, real_t p_phi) {
-	return *this * Matrix3(p_axis, p_phi);
+	*this = rotated(p_axis, p_phi);
+}
 
+Matrix3 Matrix3::rotated(const Vector3& p_euler) const {
+	return Matrix3(p_euler) * (*this);
+}
+
+void Matrix3::rotate(const Vector3& p_euler) {
+	*this = rotated(p_euler);
+}
+
+Vector3 Matrix3::get_rotation() const {
+	// Assumes that the matrix can be decomposed into a proper rotation and scaling matrix as M = R.S,
+	// and returns the Euler angles corresponding to the rotation part, complementing get_scale().
+	// See the comment in get_scale() for further information.
+	Matrix3 m = orthonormalized();
+	real_t det = m.determinant();
+	if (det < 0) {
+		// Ensure that the determinant is 1, such that result is a proper rotation matrix which can be represented by Euler angles.
+		m.scale(Vector3(-1,-1,-1));
+	}
+
+	return m.get_euler();
 }
 
 // get_euler returns a vector containing the Euler angles in the format
@@ -363,7 +392,7 @@ int Matrix3::get_orthogonal_index() const {
 	for(int i=0;i<3;i++) {
 		for(int j=0;j<3;j++) {
 
-			float v = orth[i][j];
+			real_t v = orth[i][j];
 			if (v>0.5)
 				v=1.0;
 			else if (v<-0.5)
@@ -398,9 +427,6 @@ void Matrix3::set_orthogonal_index(int p_index){
 
 
 void Matrix3::get_axis_and_angle(Vector3 &r_axis,real_t& r_angle) const {
-	// TODO: We can handle improper matrices here too, in which case axis will also correspond to the axis of reflection.
-	// See Eq. (52) in http://scipp.ucsc.edu/~haber/ph251/rotreflect_13.pdf for example
-	// After that change, we should fail on is_orthogonal() == false.
 	ERR_FAIL_COND(is_rotation() == false);
 
 

core/math/matrix3.h

@@ -55,7 +55,7 @@ public:
 	Matrix3 inverse() const;
 	Matrix3 transposed() const;
 
-	_FORCE_INLINE_ float determinant() const;
+	_FORCE_INLINE_ real_t determinant() const;
 
 	void from_z(const Vector3& p_z);
 
@@ -73,6 +73,10 @@ public:
 	void rotate(const Vector3& p_axis, real_t p_phi);
 	Matrix3 rotated(const Vector3& p_axis, real_t p_phi) const;
 
+	void rotate(const Vector3& p_euler);
+	Matrix3 rotated(const Vector3& p_euler) const;
+	Vector3 get_rotation() const;
+
 	void scale( const Vector3& p_scale );
 	Matrix3 scaled( const Vector3& p_scale ) const;
 	Vector3 get_scale() const;
@@ -226,7 +230,7 @@ Vector3 Matrix3::xform_inv(const Vector3& p_vector) const {
 	);
 }
 
-float Matrix3::determinant() const {
+real_t Matrix3::determinant() const {
 
 	return elements[0][0]*(elements[1][1]*elements[2][2] - elements[2][1]*elements[1][2]) -
 	       elements[1][0]*(elements[0][1]*elements[2][2] - elements[2][1]*elements[0][2]) +

core/math/transform.cpp

@@ -54,7 +54,8 @@ void Transform::invert() {
 }
 
 Transform Transform::inverse() const {
-
+	// FIXME: this function assumes the basis is a rotation matrix, with no scaling.
+	// Transform::affine_inverse can handle matrices with scaling, so GDScript should eventually use that.
 	Transform ret=*this;
 	ret.invert();
 	return ret;
@@ -63,12 +64,12 @@ Transform Transform::inverse() const {
 
 void Transform::rotate(const Vector3& p_axis,real_t p_phi) {
 
-	*this = *this * Transform( Matrix3( p_axis, p_phi ), Vector3()  );
+	*this = rotated(p_axis, p_phi);
 }
 
 Transform Transform::rotated(const Vector3& p_axis,real_t p_phi) const{
 
-	return *this * Transform( Matrix3( p_axis, p_phi ), Vector3()  );
+	return Transform(Matrix3( p_axis, p_phi ), Vector3()) * (*this);
 }
 
 void Transform::rotate_basis(const Vector3& p_axis,real_t p_phi) {
@@ -113,7 +114,7 @@ void Transform::set_look_at( const Vector3& p_eye, const Vector3& p_target, cons
 
 }
 
-Transform Transform::interpolate_with(const Transform& p_transform, float p_c) const {
+Transform Transform::interpolate_with(const Transform& p_transform, real_t p_c) const {
 
 	/* not sure if very "efficient" but good enough? */
 

core/math/transform.h

@@ -86,7 +86,7 @@ public:
 	void operator*=(const Transform& p_transform);
 	Transform operator*(const Transform& p_transform) const;
 
-	Transform interpolate_with(const Transform& p_transform, float p_c) const;
+	Transform interpolate_with(const Transform& p_transform, real_t p_c) const;
 
 	_FORCE_INLINE_ Transform inverse_xform(const Transform& t) const {
 

doc/base/classes.xml

@@ -20696,7 +20696,8 @@
 		3x3 matrix datatype.
 	</brief_description>
 	<description>
-		3x3 matrix used for 3D rotation and scale. Contains 3 vector fields x,y and z. Can also be accessed as array of 3D vectors. Almost always used as orthogonal basis for a [Transform].
+		3x3 matrix used for 3D rotation and scale. Contains 3 vector fields x,y and z as its columns, which can be interpreted as the local basis vectors of a transformation. Can also be accessed as array of 3D vectors. Almost always used as orthogonal basis for a [Transform].
+		For such use, it is composed of a scaling and a rotation matrix, in that order (M = R.S).
 	</description>
 	<methods>
 		<method name="Matrix3">
@@ -20705,7 +20706,7 @@
 			<argument index="0" name="from" type="Quat">
 			</argument>
 			<description>
-				Create a matrix from a quaternion.
+				Create a rotation matrix from the given quaternion.
 			</description>
 		</method>
 		<method name="Matrix3">
@@ -20716,7 +20717,7 @@
 			<argument index="1" name="phi" type="float">
 			</argument>
 			<description>
-				Create a matrix which rotates around the given axis by the specified angle.
+				Create a rotation matrix which rotates around the given axis by the specified angle.
 			</description>
 		</method>
 		<method name="Matrix3">
@@ -20743,26 +20744,29 @@
 			<return type="Vector3">
 			</return>
 			<description>
-				Return euler angles (in the XYZ convention: first Z, then Y, and X last) from the matrix. Returned vector contains the rotation angles in the format (third,second,first).
+				Return Euler angles (in the XYZ convention: first Z, then Y, and X last) from the matrix. Returned vector contains the rotation angles in the format (third,second,first).
+				This function only works if the matrix represents a proper rotation.
 			</description>
 		</method>
 		<method name="get_orthogonal_index">
 			<return type="int">
 			</return>
 			<description>
+				This function considers a discretization of rotations into 24 points on unit sphere, lying along the vectors (x,y,z) with each component being either -1,0 or 1, and returns the index of the point best representing the orientation of the object. It is mainly used by the grid map editor. For further details, refer to Godot source code.
 			</description>
 		</method>
 		<method name="get_scale">
 			<return type="Vector3">
 			</return>
 			<description>
+				Assuming that the matrix is the combination of a rotation and scaling, return the absolute value of scaling factors along each axis.
 			</description>
 		</method>
 		<method name="inverse">
 			<return type="Matrix3">
 			</return>
 			<description>
-				Return the affine inverse of the matrix.
+				Return the inverse of the matrix.
 			</description>
 		</method>
 		<method name="orthonormalized">
@@ -20779,6 +20783,9 @@
 			</argument>
 			<argument index="1" name="phi" type="float">
 			</argument>
+			<description>
+				Introduce an additional rotation around the given axis by phi. Only relevant when the matrix is being used as a part of [Transform].
+			</description>
                 </method>
 		<method name="scaled">
 			<return type="Matrix3">
@@ -20786,7 +20793,7 @@
 			<argument index="0" name="scale" type="Vector3">
 			</argument>
 			<description>
-				Return the scaled version of the matrix, by a 3D scale.
+				Introduce an additional scaling specified by the given 3D scaling factor. Only relevant when the matrix is being used as a part of [Transform].
 			</description>
 		</method>
 		<method name="tdotx">
@@ -20829,7 +20836,7 @@
 			<argument index="0" name="v" type="Vector3">
 			</argument>
 			<description>
-				Return a vector transformed by the matrix and return it.
+				Return a vector transformed (multiplied) by the matrix and return it.
 			</description>
 		</method>
 		<method name="xform_inv">
@@ -20838,7 +20845,7 @@
 			<argument index="0" name="v" type="Vector3">
 			</argument>
 			<description>
-				Return a vector transformed by the transposed matrix and return it.
+				Return a vector transformed (multiplied) by the transposed matrix and return it. Note that this is a multiplication by inverse only when the matrix represents a rotation-reflection.
 			</description>
 		</method>
 	</methods>
@@ -20895,6 +20902,7 @@
 			<return type="Matrix32">
 			</return>
 			<description>
+				Return the inverse of the matrix.
 			</description>
 		</method>
 		<method name="basis_xform">
@@ -31484,7 +31492,7 @@
 		Quaternion.
 	</brief_description>
 	<description>
-		Quaternion is a 4 dimensional vector that is used to represent a rotation. It mainly exists to perform SLERP (spherical-linear interpolation) between to rotations obtained by a Matrix3 cheaply. Multiplying quaternions also cheaply reproduces rotation sequences, however quaternions need to be often normalized, or else they suffer from precision issues.
+		Quaternion is a 4 dimensional vector that is used to represent a rotation. It mainly exists to perform SLERP (spherical-linear interpolation) between two rotations. Multiplying quaternions also cheaply reproduces rotation sequences. However quaternions need to be often renormalized, or else they suffer from precision issues.
 	</description>
 	<methods>
 		<method name="Quat">
@@ -43040,7 +43048,7 @@
 		3D Transformation.
 	</brief_description>
 	<description>
-		Transform is used to store transformations, including translations. It consists of a Matrix3 "basis" and Vector3 "origin". Transform is used to represent transformations of any object in space. It is similar to a 4x3 matrix.
+		Transform is used to store translation, rotation and scaling transformations. It consists of a Matrix3 "basis" and Vector3 "origin". Transform is used to represent transformations of objects in space, and as such, determine their position, orientation and scale. It is similar to a 3x4 matrix.
 	</description>
 	<methods>
 		<method name="Transform">
@@ -43055,7 +43063,7 @@
 			<argument index="3" name="origin" type="Vector3">
 			</argument>
 			<description>
-				Construct the Transform from four Vector3. Each axis creates the basis.
+				Construct the Transform from four Vector3. Each axis corresponds to local basis vectors (some of which may be scaled).
 			</description>
 		</method>
 		<method name="Transform">
@@ -43084,7 +43092,7 @@
 			<argument index="0" name="from" type="Quat">
 			</argument>
 			<description>
-				Construct the Transform from a Quat. The origin will be Vector3(0, 0, 0)
+				Construct the Transform from a Quat. The origin will be Vector3(0, 0, 0).
 			</description>
 		</method>
 		<method name="Transform">
@@ -43093,21 +43101,21 @@
 			<argument index="0" name="from" type="Matrix3">
 			</argument>
 			<description>
-				Construct the Transform from a Matrix3. The origin will be Vector3(0, 0, 0)
+				Construct the Transform from a Matrix3. The origin will be Vector3(0, 0, 0).
 			</description>
 		</method>
 		<method name="affine_inverse">
 			<return type="Transform">
 			</return>
 			<description>
-				Returns the inverse of the transfrom, even if the transform has scale or the axis vectors are not orthogonal.
+				Returns the inverse of the transfrom, under the assumption that the transformation is composed of rotation, scaling and translation.
 			</description>
 		</method>
 		<method name="inverse">
 			<return type="Transform">
 			</return>
 			<description>
-				Returns the inverse of the transform.
+				Returns the inverse of the transform, under the assumption that the transformation is composed of rotation and translation (no scaling).
 			</description>
 		</method>
 		<method name="looking_at">
@@ -43136,7 +43144,7 @@
 			<argument index="1" name="phi" type="float">
 			</argument>
 			<description>
-				Rotate the transform locally. This introduces an additional pre-rotation to the transform, changing the basis to basis * Matrix3(axis, phi).
+				Rotate the transform around given axis by phi.
 			</description>
 		</method>
 		<method name="scaled">
@@ -43145,7 +43153,7 @@
 			<argument index="0" name="scale" type="Vector3">
 			</argument>
 			<description>
-				Scale the transform locally.
+				Scale the transform by the specified 3D scaling factors.
 			</description>
 		</method>
 		<method name="translated">
@@ -43154,7 +43162,7 @@
 			<argument index="0" name="ofs" type="Vector3">
 			</argument>
 			<description>
-				Translate the transform locally.
+				Translate the transform by the specified displacement.
 			</description>
 		</method>
 		<method name="xform">
@@ -43163,7 +43171,7 @@
 			<argument index="0" name="v" type="var">
 			</argument>
 			<description>
-				Transforms vector "v" by this transform.
+				Transforms the given vector "v" by this transform.
 			</description>
 		</method>
 		<method name="xform_inv">
@@ -43178,7 +43186,7 @@
 	</methods>
 	<members>
 		<member name="basis" type="Matrix3">
-			The basis contains 3 [Vector3]. X axis, Y axis, and Z axis.
+			The basis is a matrix containing 3 [Vector3] as its columns: X axis, Y axis, and Z axis. These vectors can be interpreted as the basis vectors of local coordinate system travelling with the object.
 		</member>
 		<member name="origin" type="Vector3">
 			The origin of the transform. Which is the translation offset.

scene/3d/spatial.cpp

@@ -83,9 +83,9 @@ void Spatial::_notify_dirty() {
 
 
 void Spatial::_update_local_transform() const {
-
+	data.local_transform.basis = Matrix3();
-	data.local_transform.basis.set_euler(data.rotation);
 	data.local_transform.basis.scale(data.scale);
+	data.local_transform.basis.rotate(data.rotation);
 
 	data.dirty&=~DIRTY_LOCAL;
 }
@@ -376,7 +376,7 @@ void Spatial::_set_rotation_deg(const Vector3& p_euler_deg) {
 void Spatial::set_scale(const Vector3& p_scale){
 
 	if (data.dirty&DIRTY_VECTORS) {
-		data.rotation=data.local_transform.basis.get_euler();
+		data.rotation=data.local_transform.basis.get_rotation();
 		data.dirty&=~DIRTY_VECTORS;
 	}
 
@@ -398,7 +398,8 @@ Vector3 Spatial::get_rotation() const{
 
 	if (data.dirty&DIRTY_VECTORS) {
 		data.scale=data.local_transform.basis.get_scale();
-		data.rotation=data.local_transform.basis.get_euler();
+		data.rotation=data.local_transform.basis.get_rotation();
+
 		data.dirty&=~DIRTY_VECTORS;
 	}
 
@@ -422,7 +423,8 @@ Vector3 Spatial::get_scale() const{
 
 	if (data.dirty&DIRTY_VECTORS) {
 		data.scale=data.local_transform.basis.get_scale();
-		data.rotation=data.local_transform.basis.get_euler();
+		data.rotation=data.local_transform.basis.get_rotation();
+
 		data.dirty&=~DIRTY_VECTORS;
 	}
 

tools/editor/plugins/multimesh_editor_plugin.cpp

@@ -238,9 +238,10 @@ void MultiMeshEditor::_populate() {
 
 		Matrix3 post_xform;
 
-		post_xform.rotate(xform.basis.get_axis(0),-Math::random(-_tilt_random,_tilt_random)*Math_PI);
-		post_xform.rotate(xform.basis.get_axis(2),-Math::random(-_tilt_random,_tilt_random)*Math_PI);
 		post_xform.rotate(xform.basis.get_axis(1),-Math::random(-_rotate_random,_rotate_random)*Math_PI);
+		post_xform.rotate(xform.basis.get_axis(2),-Math::random(-_tilt_random,_tilt_random)*Math_PI);
+		post_xform.rotate(xform.basis.get_axis(0),-Math::random(-_tilt_random,_tilt_random)*Math_PI);
+
 		xform.basis = post_xform * xform.basis;
 		//xform.basis.orthonormalize();
 

tools/editor/plugins/spatial_editor_plugin.cpp

@@ -61,8 +61,8 @@ void SpatialEditorViewport::_update_camera() {
 
 	Transform camera_transform;
 	camera_transform.translate(cursor.pos);
-	camera_transform.basis.rotate(Vector3(0, 1, 0), -cursor.y_rot);
 	camera_transform.basis.rotate(Vector3(1, 0, 0), -cursor.x_rot);
+	camera_transform.basis.rotate(Vector3(0, 1, 0), -cursor.y_rot);
 
 	if (orthogonal)
 		camera_transform.translate(0, 0, 4096);
@@ -474,8 +474,8 @@ Vector3 SpatialEditorViewport::_get_screen_to_space(const Vector3& p_pos) {
 
 	Transform camera_transform;
 	camera_transform.translate( cursor.pos );
-	camera_transform.basis.rotate(Vector3(0,1,0),-cursor.y_rot);
 	camera_transform.basis.rotate(Vector3(1,0,0),-cursor.x_rot);
+	camera_transform.basis.rotate(Vector3(0,1,0),-cursor.y_rot);
 	camera_transform.translate(0,0,cursor.distance);
 
 	return camera_transform.xform(Vector3( ((p_pos.x/get_size().width)*2.0-1.0)*screen_w, ((1.0-(p_pos.y/get_size().height))*2.0-1.0)*screen_h,-get_znear()));
@@ -1502,7 +1502,7 @@ void SpatialEditorViewport::_sinput(const InputEvent &p_event) {
 								Transform original=se->original;
 
 								Transform base=Transform( Matrix3(), _edit.center);
-								Transform t=base * (r * (base.inverse() * original));
+								Transform t=base * r * base.inverse() * original;
 
 								sp->set_global_transform(t);
 							}
@@ -1591,8 +1591,8 @@ void SpatialEditorViewport::_sinput(const InputEvent &p_event) {
 					Transform camera_transform;
 
 					camera_transform.translate(cursor.pos);
-					camera_transform.basis.rotate(Vector3(0,1,0),-cursor.y_rot);
 					camera_transform.basis.rotate(Vector3(1,0,0),-cursor.x_rot);
+					camera_transform.basis.rotate(Vector3(0,1,0),-cursor.y_rot);
 					Vector3 translation(-m.relative_x*pan_speed,m.relative_y*pan_speed,0);
 					translation*=cursor.distance/DISTANCE_DEFAULT;
 					camera_transform.translate(translation);
@@ -2804,20 +2804,9 @@ void SpatialEditor::_xform_dialog_action() {
 		scale[i]=xform_scale[i]->get_text().to_double();
 	}
 	
+	t.basis.scale(scale);
+	t.basis.rotate(rotate);
 	t.origin=translate;
-	for(int i=0;i<3;i++) {
-		if (!rotate[i])
-			continue;
-		Vector3 axis;
-		axis[i]=1.0;
-		t.basis.rotate(axis,rotate[i]); // BUG(?): Angle not flipped; please check during the review of PR #6865.
-	}
-
-	for(int i=0;i<3;i++) {
-		if (scale[i]==1)
-			continue;
-		t.basis.set_axis(i,t.basis.get_axis(i)*scale[i]);
-	}
 
 
 	undo_redo->create_action(TTR("XForm Dialog"));