-
Notifications
You must be signed in to change notification settings - Fork 520
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.
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
SCNMatrix4 Breaking Change Breaks SCNNode.Transform #15094
Comments
This is rather unfortunate, and I'm sorry we got this wrong. Just to make sure I understand correctly, a potential fix would be to change this: to: result.Column3 = new SCNVector4 (x, y, z, 1); (and the equivalent change in the result of the Great bug report btw! |
Yes, exactly. Such changes to the construction methods would make them column-vector transformers. (Translation in the last column is how I always remember column vectors). Unfortunately, there are a lot of those construction methods. The Matrix multiply should be fine because it’s universal. But I have seen some matrix multiplies that ignore the skew elements of the matrix and that would make it different (because it’s not doing the full 4x4 multiply). So happy my bug report made sense to you. Sorry it was so late, I didn’t know SceneKit was being modified. |
I've looked a bit more into this, and oh dear Apple, they haven't made things easy. Step 1 is to try to answer the question: "does SCNMatrix4 really use a column-major layout?"
Conclusiona) |
Here's another argument for
|
Thank you for the analysis! I did mean to do some C/Swift examples to prove out the row/column ordering. Yeah, I’m pretty sure they’re column-major also, but I didn’t have facts.
Well that’s terrifying!!! Oh dear… As you discovered, experiments do seem to be the only way to prove this stuff out. I think the SCNNode tests are the most important. Once you get Node working, it’s pretty easy to get the matrix self-consistent. I wrote the next section without fully noting that you said you’re on Step 1 of solving this bug. I’ll leave it here but I understand you were just trying to answer 1 of the questions. Old EditI was more concerned with he construction methods (rotation, scale, translation) and less about the ordering since the ordering can be verified pretty easily:
That procedure will pretty clearly tell you the memory order of the translation elements. From there, we can decide if a left side vector multiplier or a right side multiplier:
The nice part of these tests is it allows you to answer these questions independent of our binding - Apple is dealing with the matrix 100% themselves. But their work can easily be read out of the class using the Position, Scale, Orientation properties. Thanks for taking the deep dive. Can’t wait for this to get fixed! |
When we changed SCNMatrix4 to be column-major instead of row-major in .NET, there were several other related changes we should have done but didn't do. In particular we should have made transformation operations based on column-vectors instead of row-vectors. In legacy Xamarin, a vector would be transformed by a transformation matrix by doing matrix multiplication like this: [ x y z w] * [ 11 21 31 41 ] | 12 22 32 42 | | 13 23 33 43 | [ 14 24 34 41 ] In this case the vector is a row-vector, and it's the left operand in the multiplication. When using column-major matrices, we want to use column-vectors, where the vector is the right operand, like this: [ 11 21 31 41 ] * [ x ] | 12 22 32 42 | | y | | 13 23 33 43 | | z | [ 14 24 34 41 ] [ w ] This affects numerous APIs in SCNMatrix4, SCNVector3 and SCNVector4: * The M## fields have been changed to make the first number the column and the second number the row, to reflect that it's a column-major matrix. * Functions that return a transformation matrix have been modified to return column-vector transformers. Technically this means that these matrices are transposed compared to legacy Xamarin. The functions involved are: * CreateFromAxisAngle * CreateRotation[X|Y|Z] * CreateTranslation * CreatePerspectiveFieldOfView * CreatePerspectiveOffCenter * Rotate * LookAt * Combining two column-vector transforming transformation matrices is done by multiplying them in the reverse order, so the Mult function (and the multiplication operator) have been modified to multiply the given matrices in the opposite order (this matches how the SCNMatrix4Mult function does it). To make things clearer I've changed the parameter names for XAMCORE_5_0. * Functions that transform a vector using a transformation matrix have been modified to do a column-vector transformation instead of a row-vector transformation. This involves the following functions: * SCNVector3.TransformVector * SCNVector3.TransformNormal * SCNVector3.TransformNormalInverse * SCNVector3.TransformPosition * SCNVector4.Transform * Numerous new tests. Fixes dotnet#15094.
@praeclarum I have a PR up that should fix this: #15160, could you review to see if it makes sense what I did? |
When we changed SCNMatrix4 to be column-major instead of row-major in .NET, there were several other related changes we should have done but didn't do. In particular we should have made transformation operations based on column-vectors instead of row-vectors. In legacy Xamarin, a vector would be transformed by a transformation matrix by doing matrix multiplication like this: [ x y z w] * [ 11 21 31 41 ] | 12 22 32 42 | | 13 23 33 43 | [ 14 24 34 41 ] In this case the vector is a row-vector, and it's the left operand in the multiplication. When using column-major matrices, we want to use column-vectors, where the vector is the right operand, like this: [ 11 21 31 41 ] * [ x ] | 12 22 32 42 | | y | | 13 23 33 43 | | z | [ 14 24 34 41 ] [ w ] This affects numerous APIs in SCNMatrix4, SCNVector3 and SCNVector4: * The M## fields have been changed to make the first number the column and the second number the row, to reflect that it's a column-major matrix (this is also how it's defined in the native SCNMatrix4 type). * Functions that return a transformation matrix have been modified to return column-vector transformers. Technically this means that these matrices are transposed compared to legacy Xamarin. The functions involved are: * CreateFromAxisAngle * CreateRotation[X|Y|Z] * CreateTranslation * CreatePerspectiveFieldOfView * CreatePerspectiveOffCenter * Rotate * LookAt * Combining two column-vector transforming transformation matrices is done by multiplying them in the reverse order, so the Mult function (and the multiplication operator) have been modified to multiply the given matrices in the opposite order (this matches how the SCNMatrix4Mult function does it). To make things clearer I've changed the parameter names for XAMCORE_5_0. * Functions that transform a vector using a transformation matrix have been modified to do a column-vector transformation instead of a row-vector transformation. This involves the following functions: * SCNVector3.TransformVector * SCNVector3.TransformNormal * SCNVector3.TransformNormalInverse * SCNVector3.TransformPosition * SCNVector4.Transform * Numerous new tests. Fixes #15094.
I hope I got it right this time! |
When we changed SCNMatrix4 to be column-major instead of row-major in .NET, there were several other related changes we should have done but didn't do. In particular we should have made transformation operations based on column-vectors instead of row-vectors. In legacy Xamarin, a vector would be transformed by a transformation matrix by doing matrix multiplication like this: [ x y z w] * [ 11 21 31 41 ] | 12 22 32 42 | | 13 23 33 43 | [ 14 24 34 41 ] In this case the vector is a row-vector, and it's the left operand in the multiplication. When using column-major matrices, we want to use column-vectors, where the vector is the right operand, like this: [ 11 21 31 41 ] * [ x ] | 12 22 32 42 | | y | | 13 23 33 43 | | z | [ 14 24 34 41 ] [ w ] This affects numerous APIs in SCNMatrix4, SCNVector3 and SCNVector4: * The M## fields have been changed to make the first number the column and the second number the row, to reflect that it's a column-major matrix. * Functions that return a transformation matrix have been modified to return column-vector transformers. Technically this means that these matrices are transposed compared to legacy Xamarin. The functions involved are: * CreateFromAxisAngle * CreateRotation[X|Y|Z] * CreateTranslation * CreatePerspectiveFieldOfView * CreatePerspectiveOffCenter * Rotate * LookAt * Combining two column-vector transforming transformation matrices is done by multiplying them in the reverse order, so the Mult function (and the multiplication operator) have been modified to multiply the given matrices in the opposite order (this matches how the SCNMatrix4Mult function does it). To make things clearer I've changed the parameter names for XAMCORE_5_0. * Functions that transform a vector using a transformation matrix have been modified to do a column-vector transformation instead of a row-vector transformation. This involves the following functions: * SCNVector3.TransformVector * SCNVector3.TransformNormal * SCNVector3.TransformNormalInverse * SCNVector3.TransformPosition * SCNVector4.Transform * Numerous new tests. Fixes dotnet#15094.
When we changed SCNMatrix4 to be column-major instead of row-major in .NET, there were several other related changes we should have done but didn't do. In particular we should have made transformation operations based on column-vectors instead of row-vectors. In legacy Xamarin, a vector would be transformed by a transformation matrix by doing matrix multiplication like this: [ x y z w] * [ 11 21 31 41 ] | 12 22 32 42 | | 13 23 33 43 | [ 14 24 34 41 ] In this case the vector is a row-vector, and it's the left operand in the multiplication. When using column-major matrices, we want to use column-vectors, where the vector is the right operand, like this: [ 11 21 31 41 ] * [ x ] | 12 22 32 42 | | y | | 13 23 33 43 | | z | [ 14 24 34 41 ] [ w ] This affects numerous APIs in SCNMatrix4, SCNVector3 and SCNVector4: * The M## fields have been changed to make the first number the column and the second number the row, to reflect that it's a column-major matrix. * Functions that return a transformation matrix have been modified to return column-vector transformers. Technically this means that these matrices are transposed compared to legacy Xamarin. The functions involved are: * CreateFromAxisAngle * CreateRotation[X|Y|Z] * CreateTranslation * CreatePerspectiveFieldOfView * CreatePerspectiveOffCenter * Rotate * LookAt * Combining two column-vector transforming transformation matrices is done by multiplying them in the reverse order, so the Mult function (and the multiplication operator) have been modified to multiply the given matrices in the opposite order (this matches how the SCNMatrix4Mult function does it). To make things clearer I've changed the parameter names for XAMCORE_5_0. * Functions that transform a vector using a transformation matrix have been modified to do a column-vector transformation instead of a row-vector transformation. This involves the following functions: * SCNVector3.TransformVector * SCNVector3.TransformNormal * SCNVector3.TransformNormalInverse * SCNVector3.TransformPosition * SCNVector4.Transform * Numerous new tests. Fixes dotnet#15094.
…15297) When we changed SCNMatrix4 to be column-major instead of row-major in .NET, there were several other related changes we should have done but didn't do. In particular we should have made transformation operations based on column-vectors instead of row-vectors. In legacy Xamarin, a vector would be transformed by a transformation matrix by doing matrix multiplication like this: [ x y z w] * [ 11 21 31 41 ] | 12 22 32 42 | | 13 23 33 43 | [ 14 24 34 41 ] In this case the vector is a row-vector, and it's the left operand in the multiplication. When using column-major matrices, we want to use column-vectors, where the vector is the right operand, like this: [ 11 21 31 41 ] * [ x ] | 12 22 32 42 | | y | | 13 23 33 43 | | z | [ 14 24 34 41 ] [ w ] This affects numerous APIs in SCNMatrix4, SCNVector3 and SCNVector4: * The M## fields have been changed to make the first number the column and the second number the row, to reflect that it's a column-major matrix. * Functions that return a transformation matrix have been modified to return column-vector transformers. Technically this means that these matrices are transposed compared to legacy Xamarin. The functions involved are: * CreateFromAxisAngle * CreateRotation[X|Y|Z] * CreateTranslation * CreatePerspectiveFieldOfView * CreatePerspectiveOffCenter * Rotate * LookAt * Combining two column-vector transforming transformation matrices is done by multiplying them in the reverse order, so the Mult function (and the multiplication operator) have been modified to multiply the given matrices in the opposite order (this matches how the SCNMatrix4Mult function does it). To make things clearer I've changed the parameter names for XAMCORE_5_0. * Functions that transform a vector using a transformation matrix have been modified to do a column-vector transformation instead of a row-vector transformation. This involves the following functions: * SCNVector3.TransformVector * SCNVector3.TransformNormal * SCNVector3.TransformNormalInverse * SCNVector3.TransformPosition * SCNVector4.Transform * Numerous new tests. Fixes #15094. Backport of #15160 Co-authored-by: Rolf Bjarne Kvinge <rolf@xamarin.com>
…15296) When we changed SCNMatrix4 to be column-major instead of row-major in .NET, there were several other related changes we should have done but didn't do. In particular we should have made transformation operations based on column-vectors instead of row-vectors. In legacy Xamarin, a vector would be transformed by a transformation matrix by doing matrix multiplication like this: [ x y z w] * [ 11 21 31 41 ] | 12 22 32 42 | | 13 23 33 43 | [ 14 24 34 41 ] In this case the vector is a row-vector, and it's the left operand in the multiplication. When using column-major matrices, we want to use column-vectors, where the vector is the right operand, like this: [ 11 21 31 41 ] * [ x ] | 12 22 32 42 | | y | | 13 23 33 43 | | z | [ 14 24 34 41 ] [ w ] This affects numerous APIs in SCNMatrix4, SCNVector3 and SCNVector4: * The M## fields have been changed to make the first number the column and the second number the row, to reflect that it's a column-major matrix. * Functions that return a transformation matrix have been modified to return column-vector transformers. Technically this means that these matrices are transposed compared to legacy Xamarin. The functions involved are: * CreateFromAxisAngle * CreateRotation[X|Y|Z] * CreateTranslation * CreatePerspectiveFieldOfView * CreatePerspectiveOffCenter * Rotate * LookAt * Combining two column-vector transforming transformation matrices is done by multiplying them in the reverse order, so the Mult function (and the multiplication operator) have been modified to multiply the given matrices in the opposite order (this matches how the SCNMatrix4Mult function does it). To make things clearer I've changed the parameter names for XAMCORE_5_0. * Functions that transform a vector using a transformation matrix have been modified to do a column-vector transformation instead of a row-vector transformation. This involves the following functions: * SCNVector3.TransformVector * SCNVector3.TransformNormal * SCNVector3.TransformNormalInverse * SCNVector3.TransformPosition * SCNVector4.Transform * Numerous new tests. Fixes #15094. Backport of #15160 Co-authored-by: Rolf Bjarne Kvinge <rolf@xamarin.com>
Short Version
TLDR; The recent change (#13695) in .NET 6 to make
SCNMatrix4
column-major breaks theTransform
property ofSCNNode
. The change correctly identified thatSCNMatrix4
was bound incorrectly, but the solution to the problem is currently incomplete and makes the class behave in inconsistent ways.Long Version
Here is a video of me walking through discovering and explaining the problem: https://www.twitch.tv/videos/1491014116?t=00h15m47s
Linear Algebra Primer
In linear algebra, the order or multiplication is important.
If one wishes to use a matrix for transforming points, one must choose if points (vectors) will be multiplied on the left hand side of the matrix or on the right. If the vector is on the left, it must, by dimensional analysis, be a row-vector (shaped 1x4). If it is on the right, it must be a column vector (4x1).
This has repercussions in how the matrices are composed. If vectors are multiplied on the left (row vectors)�, then matrices should be composed so the left most operation happens first. If however, vectors are multiplied on the right (column vectors), then matrices should be composed so the first operation is on the right and the last is on the left.
The Column-major vs Row-major Debate
Regardless of what's in a matrix and how it was composed and intended to be used, it can be stored in either row or column major format. This decision is orthogonal to whether left or right vector multiplication will be used for actual transforms.
A History Lesson
There is a strange history in .NET to favor row-vectors over column vectors. I say strange because all of science, engineering, and pretty much every 3D engine in the universe uses column vectors.
SceneKit also uses column vectors. Please note that I am saying column vectors and not column-major element ordering. It does also use column-major ordering, but let's table that for the moment.
When SceneKit was bound 8 years ago,
SCNMatrix4
should have been implemented as a Column-vector Column-major matrix. Sadly, it was implemented as a Row-vector Row-major matrix.A Modern Tragedy
This mistake was caught early on, but it wasn't understood that the mistake was two-fold.
It was noticed that if you add a
Transpose
to the matrix before it is passed to SceneKit (SCNNode.Transform
for example) then the matrix acted the same way as Apple's matrices (that are column-vector, column-major).Sadly, the conclusion was that the mistake was purely due to the column-major, row-major mixup.
It wasn't recognized that the matrices were actually constructed incorrectly. Because the matrices have a Row-vector interface for construction (for example,
SCNMatrix4.CreateTranslation
, etc.) the order in which they were composed was backwards (compared to the Apple examples).For .NET 6, the matrices were correctly switched to be Column-major element ordering. #13695
However the APIs were not fixed to make the matrices Column-vector transformers (they remain row-vector transformers). This puts them in a terribly awkward state where they act like row-vector transformers when used by themselves and with vectors. But when used with SceneKit that is expecting Column-vector, Column-major element ordering, they don't work at all.
To demonstrate this inconsistency I have written some unit tests. Allow me to explain them next.
Unit Tests to Demonstrate Inconsistencies
Demonstrate that SCNMatrix4 is Row-vector, Column-major
I wish to first demonstrate that the matrix class itself isn't broken, but is indeed constructed in a Row-vector manner. Let's first demonstrate that simple translation works fine:
That works. To demonstrate that the matrix assumes left-hand vector multiplication (which means the vectors have to be row vectors), let's compose two transformations, one after the other. We will do a translation followed by a scaling operation. This is a a good test because the order of these operations matters a lot and is very evident.
Note that the translation operation had to come before (on the left) the scale operation. This means it runs first, then the scale. This is proven out in the test because the X coordinate of the point is first translated then scaled:
X = (1 + T)*S
X = (1 + (-1))*10
X = 0*10
X = 0
If you instead put the scale on the left of the translation, then you note that the result is quite different.
X = 1*S + T
X = 1*10 + (-1)
X = 10 - 1
X = 9
We can see from this code and its results that the matrices are indeed constructed assuming row vectors. This is independent of the memory order of elements.
The memory order of the matrices doesn't really make a difference here because the algorithms correctly address the fields based on whether they're rows or columns.
The problem is: this hack doesn't work with SCNNode, because it's incomplete.
Demonstrate that the matrix does not work with
SCNNode
The .NET 6 matrix hack does not fix how matrices are used with
SCNNodes
. This is because, although effort was put into them being Column-major, no effort was put into making them Column-vector constructed. This means the matrices are still the transpose of what they should be. One problem was fixed, while the other, much bigger problem, was ignored.To demonstrate this confusing state of affairs. Let us now attempt to use one of the .NET 6 matrices with an
SCNNode
. We will do the simplest thing possible, translate the node using a translation matrix.This code fails even though we just showed that translation matrices work.
The problem is this: the matrix is constructed as a row-vector transformer. It is encoded column-major. Because it's column-major the Microsoft binding passes it directly to SceneKit. BUT SceneKit wants a column-vector transformer. The result is, the transform is not applied correctly (the translation is actually stored in the homogenous skew coordinates, not a good place).
This means that you still have to do a
Transpose
operation when using these matrices onSCNNodes
BUT you must use the normal (not transposed version) when transforming points.This is terribly inconsistent and makes writing 3D math impossible.
Proposed Solutions
The Easy Solution
Just go back to row-vector, row-major matrices. I know they're wrong. But they're wrong in an internally consistent way. They work the same way with point transforms as they do with
SCNNodes
and friends.Then there would be no broken breaking change and we can all get on with our lives.
The Correct Solution
SCNMatrix4
should be designed and implemented as a Column-vector, Column-major matrix (or a new struct should be introduced). It would match perfectly with SceneKit, OpenGL, mathematics, engineering, and basically everything else in the universe. It would be both internally consistent and work with SceneKit without any transpositions.I wish this was the breaking change introduced in .NET 6. Instead, the change we got is only a partial solution.
Workarounds
If you are coming into this bug, completely confused by .NET 6's matrices, let me give you this advice about how to work around its new design:
Conclusion
Thank you for coming to my TED talk. I know that everything I wrote can sound pedantic, but I hope I demonstrated through the tests and examples that the new (.NET 6)
SCNMatrix4
operates in a very inconsistent manner. This is due to not fully understanding why the original binding in SceneKit was incorrect and only doing a partial fix (fixed the element ordering but neglected to fix the construction and transformation methods).Attached Tests and Repro
SCNMatricNET6Boug.zip
The text was updated successfully, but these errors were encountered: