Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix Basis is_orthogonal and is_rotation methods, add is_orthonormal #83229

Merged
merged 1 commit into from Dec 8, 2023
Merged

Fix Basis is_orthogonal and is_rotation methods, add is_orthonormal #83229

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
23 changes: 19 additions & 4 deletions core/math/basis.cpp
View file
Open in desktop
Expand Up @@ -89,13 +89,26 @@ Basis Basis::orthogonalized() const {
return c;
}

// Returns true if the basis vectors are orthogonal (perpendicular), so it has no skew or shear, and can be decomposed into rotation and scale.
// See https://en.wikipedia.org/wiki/Orthogonal_basis
bool Basis::is_orthogonal() const {
Basis identity;
Basis m = (*this) * transposed();
const Vector3 x = get_column(0);
const Vector3 y = get_column(1);
const Vector3 z = get_column(2);
return Math::is_zero_approx(x.dot(y)) && Math::is_zero_approx(x.dot(z)) && Math::is_zero_approx(y.dot(z));
}

return m.is_equal_approx(identity);
// Returns true if the basis vectors are orthonormal (orthogonal and normalized), so it has no scale, skew, or shear.
// See https://en.wikipedia.org/wiki/Orthonormal_basis
bool Basis::is_orthonormal() const {
const Vector3 x = get_column(0);
const Vector3 y = get_column(1);
const Vector3 z = get_column(2);
return Math::is_equal_approx(x.length_squared(), 1) && Math::is_equal_approx(y.length_squared(), 1) && Math::is_equal_approx(z.length_squared(), 1) && Math::is_zero_approx(x.dot(y)) && Math::is_zero_approx(x.dot(z)) && Math::is_zero_approx(y.dot(z));
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are the pros and cons of doing this versus the old transpose + identity check? Is one faster than the other / handle more cases etc? 🤔

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The old check ensured that $M M^T = I$, where $M$ is the input matrix and $I$ is the identity matrix. This check allows $M M^T$ to be any diagonal matrix, such as diag(2, 2, 2) or diag(1, 2, 3).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That said, I question whether this method has any practical applications. At least, in math and physics, I have never needed to check whether a matrix was orthogonal in this sense, whereas checking whether or not a matrix is orthonormal (ie: a rotation) or conformal (ie: a uniformly scaled rotation) is fairly common. It's perhaps worth noting that $M M^T$ is a positive matrix and therefore diagonal in some basis. In other words, it might not be diagonal in the standard x, y, z basis, but it will be diagonal in some rotated basis.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need a real is_orthgonal method in the GLTF code. That's why I opened this PR.

I am using this method to check if a Basis is decomposable into TRS.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By TRS do you mean translation, rotation, scale? If so, I believe every matrix admits such a decomposition. Ignoring the T part for now so we just are just dealing with the square (ie 3 x 3) part of the matrix, every square matrix can be written as a rotation times a scale according to the polar decomposition https://en.m.wikipedia.org/wiki/Polar_decomposition. You can then take the square matrix and combine it with the translation to recover the original transformation. In case I'm mistaken, did you run into an example in the wild of a matrix that was not decomposable in the way you wanted?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on another comment, it does seem like "scale" in this context is being used to mean a diagonal matrix, not a diagonalizable matrix (or more precisely a positive-semi-definite matrix) as I initially presumed it to mean. I would be very careful with this terminology as it suggests that stretching an image along say, the 45 degree line is not a "scale" transformation, but stretching an image along the 0 degree or 90 degree line is.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what you mean by "diagonal in some orthonormal basis". Let's go with a simple example:

shear

This cannot be decomposed into rotation and scale.

Proof by degrees of freedom: In 2D, rotation is a single angle number, and scale is 2 numbers, so a total of 3 numbers. A 2x2 matrix has 4 numbers. So there is necessarily one degree of freedom that cannot be represented by rotation and scale. For 3D, rotation has 3 degrees of freedom (Euler angles), and scale also has 3, so 6 total numbers. A 3x3 matrix has 9 numbers. So there are 3 degrees of freedom in the matrix that cannot be represented by rotation and scale.

Copy link
Contributor

@nlupugla nlupugla Dec 6, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand now. The discussion is perhaps related to this post? https://math.stackexchange.com/questions/2022588/multiplying-trs-affine-transformations

When I say a matrix $M$ is "diagonal in some orthonormal basis" I mean that you can find an orthonormal matrix (ie: a rotation) $R$ such that $R M R^T$ is a diagonal matrix.

In math and physics, there is no "preferred" axis/orientation to look at things in, so we tend to care more about whether a matrix is diagonal in some basis rather than diagonal in the particular basis we happened to choose. That said, maybe things are different in computer graphics as the fact that your monitor is oriented a particular way does mean there is a preferred axis.

I agree that if by "scale" you mean a diagonal matrix, then you can't decompose every matrix as R S. However, I'm doubtful how useful such a representation is anyway. As that post on stack exchange shows, there isn't a straightforward way to update such a representation after a new transformation.

Part of the reason I'm harping on this is that Godot seems to try to keep it's APIs pretty lean, so I'd want to be sure this method deserves it's spot on the API.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How things relate to the monitor or computer graphics is not the problem here. The problem is with what we are calling rotation and scale.

You are thinking of scale as a diagonal matrix, but I am thinking of scale as a Vector3. The other values aren't 0, they don't exist. You can convert a Vector3 scale into a diagonal matrix, but a scale is not the same as a scale matrix, at least this is how it works in game engines. If a matrix can't be losslessly converted to a scale Vector3 (except for floating point error) then it's not a pure scale matrix and therefore the matrix contains more than just scale.

Similarly, you are thinking of rotation as a matrix. A rotation can be represented as Euler angles, a normalized Quaternion, and a matrix, and you can convert between these, but a rotation is not the same as a rotation matrix. If a matrix can't be losslessly converted to Euler angles (except for floating point error) then it's not a pure rotation matrix and therefore the matrix contains more than just rotation.

Copy link
Contributor

@nlupugla nlupugla Dec 6, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking around a bit, it does seem there are at least a few contexts where "scaling" does seem to refer to scaling the x, y, and z axes specifically. I'm kind of annoyed people take it to mean that, but I'll defer to that definition if that is how people commonly understand it, especially in game engines.

As for why I'm annoyed, it's because I would consider a transformation like this (produced from https://web.ma.utexas.edu/users/ysulyma/matrix/):

image

a "scale" because you obtain the transformation by stretching everything aligned with the 45 degree axis by a factor of 2.

Personally, I would think of the "scale" of a transformation as the eigenvalues of its corresponding transformation matrix. That said, it's not a hill I'm willing to die on :)


// Returns true if the basis is conformal (orthogonal, uniform scale, preserves angles and distance ratios).
// See https://en.wikipedia.org/wiki/Conformal_linear_transformation
bool Basis::is_conformal() const {
const Vector3 x = get_column(0);
const Vector3 y = get_column(1);
Expand All @@ -104,15 +117,17 @@ bool Basis::is_conformal() const {
return Math::is_equal_approx(x_len_sq, y.length_squared()) && Math::is_equal_approx(x_len_sq, z.length_squared()) && Math::is_zero_approx(x.dot(y)) && Math::is_zero_approx(x.dot(z)) && Math::is_zero_approx(y.dot(z));
}

// Returns true if the basis only has diagonal elements, so it may only have scale or flip, but no rotation, skew, or shear.
bool Basis::is_diagonal() const {
return (
Math::is_zero_approx(rows[0][1]) && Math::is_zero_approx(rows[0][2]) &&
Math::is_zero_approx(rows[1][0]) && Math::is_zero_approx(rows[1][2]) &&
Math::is_zero_approx(rows[2][0]) && Math::is_zero_approx(rows[2][1]));
}

// Returns true if the basis is a pure rotation matrix, so it has no scale, skew, shear, or flip.
bool Basis::is_rotation() const {
return Math::is_equal_approx(determinant(), 1, (real_t)UNIT_EPSILON) && is_orthogonal();
return is_conformal() && Math::is_equal_approx(determinant(), 1, (real_t)UNIT_EPSILON);
}

#ifdef MATH_CHECKS
Expand Down
1 change: 1 addition & 0 deletions core/math/basis.h
View file
Open in desktop
Expand Up @@ -138,6 +138,7 @@ struct _NO_DISCARD_ Basis {
_FORCE_INLINE_ Basis operator*(const real_t p_val) const;

bool is_orthogonal() const;
bool is_orthonormal() const;
bool is_conformal() const;
bool is_diagonal() const;
bool is_rotation() const;
Expand Down
94 changes: 94 additions & 0 deletions tests/core/math/test_basis.h
View file
Open in desktop
Expand Up @@ -324,6 +324,100 @@ TEST_CASE("[Basis] Is conformal checks") {
CHECK_FALSE_MESSAGE(
Basis(Vector3(Math_SQRT12, Math_SQRT12, 0), Vector3(0, 1, 0), Vector3(0, 0, 1)).is_conformal(),
"Basis with the X axis skewed 45 degrees should not be conformal.");

CHECK_MESSAGE(
Basis(0, 0, 0, 0, 0, 0, 0, 0, 0).is_conformal(),
"Edge case: Basis with all zeroes should return true for is_conformal (because a 0 scale is uniform).");
}

TEST_CASE("[Basis] Is orthogonal checks") {
CHECK_MESSAGE(
Basis().is_orthogonal(),
"Identity Basis should be orthogonal.");

CHECK_MESSAGE(
Basis::from_euler(Vector3(1.2, 3.4, 5.6)).is_orthogonal(),
"Basis with only rotation should be orthogonal.");

CHECK_MESSAGE(
Basis::from_scale(Vector3(-1, -1, -1)).is_orthogonal(),
"Basis with only a flip should be orthogonal.");

CHECK_MESSAGE(
Basis::from_scale(Vector3(1.2, 3.4, 5.6)).is_orthogonal(),
"Basis with only scale should be orthogonal.");

CHECK_MESSAGE(
Basis(Vector3(3, 4, 0), Vector3(4, -3, 0), Vector3(0, 0, 5)).is_orthogonal(),
"Basis with a flip, rotation, and uniform scale should be orthogonal.");

CHECK_FALSE_MESSAGE(
Basis(Vector3(Math_SQRT12, Math_SQRT12, 0), Vector3(0, 1, 0), Vector3(0, 0, 1)).is_orthogonal(),
"Basis with the X axis skewed 45 degrees should not be orthogonal.");

CHECK_MESSAGE(
Basis(0, 0, 0, 0, 0, 0, 0, 0, 0).is_orthogonal(),
"Edge case: Basis with all zeroes should return true for is_orthogonal, since zero vectors are orthogonal to all vectors.");
}

TEST_CASE("[Basis] Is orthonormal checks") {
CHECK_MESSAGE(
Basis().is_orthonormal(),
"Identity Basis should be orthonormal.");

CHECK_MESSAGE(
Basis::from_euler(Vector3(1.2, 3.4, 5.6)).is_orthonormal(),
"Basis with only rotation should be orthonormal.");

CHECK_MESSAGE(
Basis::from_scale(Vector3(-1, -1, -1)).is_orthonormal(),
"Basis with only a flip should be orthonormal.");

CHECK_FALSE_MESSAGE(
Basis::from_scale(Vector3(1.2, 3.4, 5.6)).is_orthonormal(),
"Basis with only scale should not be orthonormal.");

CHECK_FALSE_MESSAGE(
Basis(Vector3(3, 4, 0), Vector3(4, -3, 0), Vector3(0, 0, 5)).is_orthonormal(),
"Basis with a flip, rotation, and uniform scale should not be orthonormal.");

CHECK_FALSE_MESSAGE(
Basis(Vector3(Math_SQRT12, Math_SQRT12, 0), Vector3(0, 1, 0), Vector3(0, 0, 1)).is_orthonormal(),
"Basis with the X axis skewed 45 degrees should not be orthonormal.");

CHECK_FALSE_MESSAGE(
Basis(0, 0, 0, 0, 0, 0, 0, 0, 0).is_orthonormal(),
"Edge case: Basis with all zeroes should return false for is_orthonormal, since the vectors do not have a length of 1.");
}

TEST_CASE("[Basis] Is rotation checks") {
CHECK_MESSAGE(
Basis().is_rotation(),
"Identity Basis should be a rotation (a rotation of zero).");

CHECK_MESSAGE(
Basis::from_euler(Vector3(1.2, 3.4, 5.6)).is_rotation(),
"Basis with only rotation should be a rotation.");

CHECK_FALSE_MESSAGE(
Basis::from_scale(Vector3(-1, -1, -1)).is_rotation(),
"Basis with only a flip should not be a rotation.");

CHECK_FALSE_MESSAGE(
Basis::from_scale(Vector3(1.2, 3.4, 5.6)).is_rotation(),
"Basis with only scale should not be a rotation.");

CHECK_FALSE_MESSAGE(
Basis(Vector3(2, 0, 0), Vector3(0, 0.5, 0), Vector3(0, 0, 1)).is_rotation(),
"Basis with a squeeze should not be a rotation.");

CHECK_FALSE_MESSAGE(
Basis(Vector3(Math_SQRT12, Math_SQRT12, 0), Vector3(0, 1, 0), Vector3(0, 0, 1)).is_rotation(),
"Basis with the X axis skewed 45 degrees should not be a rotation.");

CHECK_FALSE_MESSAGE(
Basis(0, 0, 0, 0, 0, 0, 0, 0, 0).is_rotation(),
"Edge case: Basis with all zeroes should return false for is_rotation, because it is not just a rotation (has a scale of 0).");
}

} // namespace TestBasis
Expand Down