Fix Basis is_orthogonal and is_rotation methods, add is_orthonormal

[aaronfranke]

I noticed that the behavior of these methods was not correct for all inputs. This PR adds unit tests for these methods and fixes the methods. If desired, I could expose them and write docs too.

The is_orthogonal method was not working for scaled inputs. The new changes also greatly simplify the code, we only need 3 dot products to check if the Basis is orthogonal, because the dot product of orthogonal vectors is zero. Without these changes, these unit tests would fail:

./tests/core/math/test_basis.h:340: ERROR: CHECK( Basis::from_scale(Vector3(1.2, 3.4, 5.6)).is_orthogonal() ) is NOT correct!
  values: CHECK( false )
  logged: Basis with only scale should be orthogonal.

./tests/core/math/test_basis.h:348: ERROR: CHECK( Basis(Vector3(3, 4, 0), Vector3(4, -3, 0), Vector3(0, 0, 5)).is_orthogonal() ) is NOT correct!
  values: CHECK( false )
  logged: Basis with a flip, rotation, and uniform scale should be orthogonal.

The is_rotation method was not taking into account squeeze matrices (a special form of non-uniform scale where the volume is preserved but the scale changes). Admittedly this was an edge case, but still, now it's fixed. Without this fix, this unit test would fail:

./tests/core/math/test_basis.h:374: ERROR: CHECK_FALSE( Basis(Vector3(2, 0, 0), Vector3(0, 0.5, 0), Vector3(0, 0, 1)).is_rotation() ) is NOT correct!
  values: CHECK_FALSE( true )
  logged: Basis with a squeeze should not be a rotation.

If anyone is wondering, here is the truth table of the Basis validation methods as of this PR:

Basis has...	is_rotation	is_orthonormal	is_conformal	is_orthogonal	is_diagonal
Rotation	✅ true	✅ true	✅ true	✅ true	❌ false
Flip	❌ false	✅ true	✅ true	✅ true	✅ true
Uniform Scale	❌ false	❌ false	✅ true	✅ true	✅ true
Non-Uniform Scale	❌ false	❌ false	❌ false	✅ true	✅ true
Skew/Shear	❌ false	❌ false	❌ false	❌ false	❌ false
Nothing/Identity	✅ true	✅ true	✅ true	✅ true	✅ true

[groud]

The is_orthogonal method was not working for scaled inputs.

I think that is expected. According to wikipedia: In linear algebra, an orthogonal matrix, or orthonormal matrix, is a real square matrix whose columns and rows are orthonormal vectors. And, still according to wikipedia, "orthonormal vectors" are perpendicular, but they are also supposed to be of length 1. This basically means that a Basis scaling its input is not orthogonal.

I wonder if that's an english specificity though. In french we differentiate "orthonormal" from "orthogonal", where one means unit vectors and the other does not.

[aaronfranke]

@groud Orthogonal and orthonormal are not the same thing. Orthonormal means normalized orthogonal. Orthogonal does not need to be normalized. We could add a new method for is_orthonormal. But anyway, I still need a real is_orthogonal method for the glTF transform logic.

The Wikipedia article you read is not correct. The article for Orthogonality does not mention being normalized: https://en.wikipedia.org/wiki/Orthogonality

[groud]

@groud Orthogonal and orthonormal are not the same thing.

Well, that's what I mentioned, I was not sure about the definition in English. Here is the article: https://en.wikipedia.org/wiki/Orthogonal_matrix

What I understand from the current is_orthogonal method, is that it does in fact returns whether or not the matrix is orthonormal. For me I think there's a difference but the wikipedia article about orthogonal/orthonormal matrices says the opposite.

If those are indeed different, then I think it would be good to add an is_orthonormal method for completeness, and fix is_orthogonal. This is however a compatibility breaking change that we need to mention. And if they are the same, then this fix is probably wrong. I think we probably need a more reliable source to decide though.

[aaronfranke]

@groud Actually, these methods are not exposed, so it's not a compatibility-breaking change.

I have opened a request for Wikipedia to rename that page. There are a lot of people on the talk page also confused by this, because the current page incorrectly uses the term "orthogonal".

[aaronfranke]

I updated the PR to add is_orthonormal and I updated the table in the OP.

[groud]

I have opened a request for Wikipedia to rename that page. There are a lot of people on the talk page also confused by this, because the current page incorrectly uses the term "orthogonal".

Yeah I can see that too, and it feels like the general consensus isn't here.

What I suggest if you need the two versions of the function, is that we add a comment explaining the difference between the two, and explain the naming choice we made here. That should avoid any future confusion. With that comment added I think the PR should be good to merge.

[aaronfranke]

@groud Also note, while Wikipedia has an article for "Orthogonal matrix" that is contested, it has this other article for "Orthonormal basis", and our type in Godot is Basis. https://en.wikipedia.org/wiki/Orthonormal_basis

Also, if the term "orthogonal" is confusing, we could instead call this method is_perpendicular, which is a bit less "mathematical" but it's still correct. Thankfully it's not exposed yet so we can rename without breaking compat.

As for writing a comment explaining these, I think that would be better to do in documentation after we expose these. For the moment I decided against exposing them in this PR in favor of just doing internal fixes, but we could expose them if that's desired (either in this PR or another PR later).

[fire]

My friend was mentioning the desynced gltf exporting bug to me and it is dependent on this pr Fix Basis is_orthogonal and is_rotation methods to work. #83231

@reduz I would prefer if you can take a look too.

[groud]

Also, if the term "orthogonal" is confusing, we could instead call this method is_perpendicular, which is a bit less "mathematical" but it's still correct. Thankfully it's not exposed yet so we can rename without breaking compat.

Hmm, I'd prefer keeping "orthogonal". I think I'd rather go for using a widespread term and explain the meaning we give to it than using an uncommon term. But well, I am not strongly against it either.

As for writing a comment explaining these, I think that would be better to do in documentation after we expose these. For the moment I decided against exposing them in this PR in favor of just doing internal fixes, but we could expose them if that's desired (either in this PR or another PR later).

Yeah, I agree that if it is exposed it's better to have this documented in the documentation. But if we choose not to expose them, then I think a comment is welcome to avoid any confusion. That would help prevent any future PRs claiming they would "fix" this function because their author would not agree with the meaning we gave it.

Now I am not against exposing them, I guess they can be useful functions. Though I don't think anyone asked for it yet so that might be a bit against our "the need shall come first" policy.

[lawnjelly]

Orthogonal etymology seems pretty clear:

"pertaining to or depending upon the use of right angles," 1570s, from French orthogonal, from orthogone, from Late Latin orthogonius, from Greek orthogonios "right-angled," from ortho- "straight" (see ortho-) + gōnia "angle, corner" (from PIE root *genu- (1) "knee; angle"). Related: Orthogonally; orthogonality.

Unfortunately if one influential person or teacher misinterprets a term, it can get regarded as canon. Personally I always go with what the derivation says. 😁 🍿

[dsnopek]

The end state of these changes makes sense to me!

However, if folks have existing code using is_orthongonal() which they depend on to also check that there isn't scaling, this change could break that. I don't know how prevalent such code is - I'd personally guess it's pretty rare, but it is technically a breaking change.

The problem with trying to do this in a non-breaking way is the unfortunate naming if we leave is_orthongonal() checking if a basis is orthonormal :-)

[aaronfranke]

@dsnopek Note that this method is not exposed to GDScript or GDExtension, so it only affects engine code, or third-party modules.

[dsnopek]

Oh, ok! Nevermind about that then :-)

[lawnjelly]

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? 🤔

[nlupugla]

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).

[nlupugla]

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.

[aaronfranke]

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.

[nlupugla]

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?

[nlupugla]

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.

[aaronfranke]

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

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.

[nlupugla]

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.

[aaronfranke]

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.

[nlupugla]

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/):

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 :)

[lawnjelly]

Barring the question about whether to have the transpose check or the new one for orthonormal I approve in principal. I know @groud has some misgivings, but the etymology is clear. I personally would prioritize correctness over mumbo jumbo. History (hopefully) eventually removes such language mistakes.

One problem is that if you do define orthogonal as normalized, then you need a new word to describe a non-normalized orthogonal basis. What do you call it? And why do you have two words orthogonal and orthonormal for the same thing? It is nonsense imo, and the logic is flawed.

The alternative if hotly disputed as aaron says is to remove all mention to orthogonal and replace in Godot with is_perpendicular or similar. But personally I have no problem in saying wikipedia is at best misleading, it should at least disambiguate clearly between incorrect (but common) use and correct use.

[groud]

Barring the question about whether to have the transpose check or the new one for orthonormal I approve in principal. I know @groud has some misgivings, but the etymology is clear. I personally would prioritize correctness over mumbo jumbo. History (hopefully) eventually removes such language mistakes.

I was just worried there would be induced confusion if we did not state clearly (in a comment) which definition of the names we used. But I think it's the right decision to change it, I was just worried about it being a source of confusion for those who would expect "orthogonality" to imply normalization.

Right now, I think the PR is good in its current state. The comments in the .cpp are enough, and the .h has the two function next to each other, so I guess it's clear enough now.

[kleonc]

Overall LGTM.

[YuriSizov]

Thanks!

[tagcup3]

@aaronfranke The name "orthogonal" comes from Lie groups. There is no confusion about it, and it is not contested it is only in your mind. An orthogonal matrix M is a matrix representation of an element of the orthogonal group $O(N)$ satisfying the condition $M M^T = I$, see wikipedia https://en.wikipedia.org/wiki/Orthogonal_group or any group theory book.

Please don't change long established mathematical definitions arbitrarily just because you don't know group theory and get confused by it. Ironically, any one who knows what an orthogonal matrix is going to be confused by your implementation.

@reduz @akien-mga I recommend reverting this commit. It is based on a misunderstand of what orthogonal matrix is.

This commit changes the correct implementation of is_orthogonal to an incorrect one, and introduces is_orthonormal which behaves like the original is_orthogonal but written in an unnecessarily complicated way.

[aaronfranke]

@tagcup3 An orthogonal basis is a basis which has orthogonal vectors: https://en.wikipedia.org/wiki/Orthogonal_basis

Even if you perfectly argue your case, the solution would not be to revert. We still need a method to check if the basis vectors are orthogonal/perpendicular, so at most you could rename this to is_perpendicular. The entire reason I opened this PR is because I need such a method to exist.

and introduces is_orthonormal which behaves like the original is_orthogonal but written in an unnecessarily complicated way.

It is not unnecessarily complicated. The original method was wrong, because it did not handle all cases.

[tagcup3]

If you need a function to determine whether a polar decomposition of the matrix is possible or not, you should open a separate PR for it.

For the function that you are looking for, "perpendicular matrix" is your words again and is not a good name. I suggest is_rotation_and_scale or something else that doesn't attempt to introduce new mathematical definitions out of nowhere.

is_valid is also a good name, given that Basis is by definition R.S matrix.

The original method was wrong, because it did not handle all cases.

You talk a lot but without any substance. What is wrong? If you are referring to scaling matrices in your original post as "all cases", those are not orthogonal matrices, and the original is_orthogonal is correctly saying so.

[nlupugla]

@tagcup3 I see where you are coming from. I have a math/physics background and the naming also bothered me initially. However, @aaronfranke explained their reasoning very well in a discussion you can read above. Personally, I think is_perpendicular is a nice middle ground between the two camps, but I don't have strong feelings either way.

[aaronfranke]

@tagcup3 I apologize, I did some testing, I had a few things mixed up before, some of my statements were not correct.

For is_orthonormal, I could not remember any specific case, so tried changing the contents of this method to the old is_orthogonal with the transpose-multiply-equals-identity approach and it passes all unit tests. I believe the old code was functioning as an orthonormality check perfectly fine and I was wrong to suggest there was incorrect behavior (even though the method name did not match what it was doing before).

Now, since both produce the correct behavior, we need to figure out which is faster. The code I have in this PR uses fewer approximate equality checks so I would assume it's faster. After benchmarking with looping the test cases one million times, running the old code took 381851 microseconds and the new code took 357442 microseconds, so the code in this PR is 6.4% faster (Apple M1 Pro arm64 macOS). I also tested with 10 million iterations with random normalized data (1183076µs -> 1093664µs, this PR is 7.5% faster in this case). and random non-normalized data (925739µs -> 820930µs, this PR is 11.3% faster in this case). In all cases the code in this PR is faster. So it's a good improvement.

When I mentioned incorrect behavior what I was thinking about before is the is_rotation method, which checks if the determinant is 1, and checks the old is_orthogonal method. With the new is_orthogonal method this was wrong. I believe I had changed is_orthogonal first and then noticed is_rotation was wrong and mistakenly come to the conclusion that is_rotation was wrong. But anyway, the new logic for is_rotation is faster.

As for is_orthogonal vs is_perpendicular vs is_rotation_and_scale, all seem fine to me. The last one seems fairly cumbersome of a name but it is highly accurate and describes its use case. I disagree with your assessment that is_perpendicular is unsuitable, it is likewise descriptive to me.

TL;DR: There will be no reverts. Everything is good in master except for bikeshedding about the naming of this method. If anyone proposes to change it to is_perpendicular or is_rotation_and_scale I am fine with that. The most important thing is that this method must exist because I need it for figuring out if a Basis is decomposable into rotation and scale.

[nlupugla]

By the way, are you planning to use this in a pattern like

if (is_orthogonal()) {
    get_scale()
}

or something?

If speed is a concern, you might want to consider that is_orthogonal is already doing some of the calculations you would need in a get_scale or get_rotation method, so you would be repeating your work. You could consider having get_scale and/or get_rotation return NaN if the basis isn't a scaled rotation. It makes the API a bit more awkward, but it probably halve the time it takes to use the pattern.

[tagcup3]

If you write is_orthogonal element-wise, it can be even faster, but becomes even less readable. But why expand matrix operations in vector form in one particular function, while many others are still in readable compact matrix form? This is reason I wrote it in the matrix form when I first introduced is_orthogonal.

Another point is, I noticed a test case for is_conformal in the diff. A matrix with all zero entries can't represent a conformal map, because it's not invertible (just like a scaling matrix with one or more zero scale is). Either the implementation (along with its documentation) of is_conformal needs to be corrected such that it checks if $M M^T = \lambda I$ can be satisfied for a non-zero $\lambda$, or the name should be changed to something else.

[nlupugla]

Another point is, I noticed a test case for is_conformal in the diff. A matrix with all zero entries can't represent a conformal map, because it's not invertible (just like a scaling matrix with one or more zero scale is). Either the implementation (along with its documentation) of is_conformal needs to be corrected such that it checks if $M M^T = \lambda I$ can be satisfied for a non-zero $\lambda$, or the name should be changed to something else.

Does a conformal map have to be invertible? Based on the Wikipedia article for Conformal Map, there seems to be different conventions https://en.wikipedia.org/wiki/Conformal_map."

The transformation is conformal whenever the Jacobian at each point is a positive scalar times a rotation matrix (orthogonal with determinant one). Some authors define conformality to include orientation-reversing mappings whose Jacobians can be written as any scalar times any orthogonal matrix.

In other words, it seems like some authors require $\lambda > 0$ while others don't.

For that reason, it seems like the "correct" choice in a computational setting is whichever one has simpler/faster code.

[aaronfranke]

@nlupugla My use case is in the GLTF export code, which is extremely non-performance-critical. It is much more important to have readable code that can be easily looked over to ensure the behavior is correct. Saving a few nanoseconds is not important here, it would be premature optimization for a non-hot code path.

@tagcup3 For my uses, it does not really matter what the result of is_conformal is for a zero matrix, because I would consider those to be invalid. So, whatever is fastest is fine. The test case primarily serves to solidify the current behavior, such that if anyone changes the behavior they would also need to update the test case to make it obvious that the behavior changes. But anyway, conceptually, distance ratios could be stated to be preserved (all become 0, 0/x == 0/y is true, and x/y == 0/0 is an undefined singularity but it cannot be definitively said to be false either).

Please also keep in mind that for game engines, being 100% accurate to the original math definitions is not the goal. Instead, being useful is the goal. This is why for example we have Vector2.cross.

[tagcup3]

@nlupugla Conformal maps need to be invertible (non-zero $\lambda$), what you're referring to is the convention about the orientation.

In physics, the rotation part is typically special orthogonal group $SO(N)$ or special unitary group $SU(N)$ which excludes reflections, but it sounds like in some fields, people absorb reflections into the rotation part by allowing it to be in the orthogonal group $O(N)$.

@aaronfranke Too many false dichotomies in your arguments. There is "not 100% accurate" and there is "conceptually incorrect, incoherent and broken". Vector2.cross seems to be a function with a correct implementation that should exist as is but it should be renamed (signed_area). is_conformal is a function with incorrect implementation. Finally, having a correct implementation doesn't make it less useful.

If you let $\lambda == 0$ through, it is going to cause problems elsewhere because it leads to a vanishing determinant. Basis::invert or all the other pieces of code in Basis implementation that relies on the assumption that determinant is non-zero.

Just fix it instead of coming up with random excuses.

[nlupugla]

@tagcup3 Do you have any references you can point to that suggest the convention I'm referring to requires non-zero $\lambda$? I'm on a university campus, so I have access to most popular textbooks if that's where you're pulling from.

If letting $\lambda$ be zero causes inconsistencies, that would certainly be an issue, but I don't quite follow your argument. Are you suggesting that someone will write code like
if basis.is_conformal(): basis.inverse()?

Also, it's nice to chat with other people that are passionate about getting these definitions right. Let's try to remember we're all on the same team here. People might be more inclined to heed your arguments if you use a bit of a warmer tone <3

[aaronfranke]

@tagcup3 is_conformal is not "conceptually incorrect, incoherent and broken". It is entirely correct for all non-zero inputs. Your opinions for what should happen with the edge case of an all-zero input do not change the fact that this function is correct, coherent, working, and most importantly, immensely useful for all real-world use cases that I know of.

As I've stated, what happens for this edge case is not important to me. You are welcome to propose an alternative implementation, but note that high performance for 99.999% of use cases is not worth sacrificing for purity with an arbitrary mathematical definition not based on practical use cases.