-
Notifications
You must be signed in to change notification settings - Fork 511
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
[SceneKit] Fix SCNMatrix4 in .NET. Fixes #15094. #15160
Conversation
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 xamarin#15094.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
📋 [PR Build] API Diff 📋API diff (for current PR)ℹ️ API Diff (from PR only) (please review changes) .NETXamarin vs .NETAPI diff (vs stable)✅ API Diff from stable .NETXamarin vs .NETGenerator diff✅ Generator Diff (no change) Pipeline on Agent XAMBOT-1023.Monterey |
💻 [PR Build] Tests on macOS Mac Catalina (10.15) passed 💻✅ All tests on macOS Mac Catalina (10.15) passed. Pipeline on Agent |
❌ [PR Build] Tests on macOS M1 - Mac Big Sur (11.5) failed ❌Failed tests are:
Pipeline on Agent |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not qualified to review this matrix code without a lot of coffee and some reading, but outside of a few formatting nits I didn't see anything obviously wrong.
#if XAMCORE_5_0 | ||
public static SCNMatrix4 Mult (SCNMatrix4 firstTransformation, SCNMatrix4 secondTransformation) | ||
#else | ||
public static SCNMatrix4 Mult(SCNMatrix4 left, SCNMatrix4 right) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public static SCNMatrix4 Mult(SCNMatrix4 left, SCNMatrix4 right) | |
public static SCNMatrix4 Mult (SCNMatrix4 left, SCNMatrix4 right) |
#if XAMCORE_5_0 | ||
public static void Mult (ref SCNMatrix4 firstTransformation, ref SCNMatrix4 secondTransformation, out SCNMatrix4 result) | ||
#else | ||
public static void Mult(ref SCNMatrix4 left, ref SCNMatrix4 right, out SCNMatrix4 result) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public static void Mult(ref SCNMatrix4 left, ref SCNMatrix4 right, out SCNMatrix4 result) | |
public static void Mult (ref SCNMatrix4 left, ref SCNMatrix4 right, out SCNMatrix4 result) |
Wonderful! I haven’t verified your math but your description of the changes is exactly what I was hoping for. Thank you! I have a minor quibbled about:
My personal preference is to always name these fields according to M{Row}{Col} since that is the normal convention for sizing and indexing matrices (independent of the memory ordering). I think it’s less surprising that way. That said, it’s not a hill I’m going to die on. Certainly lots of matrix libraries like to keep you guessing about their naming convention :-) |
@rolfbjarne One question regarding multiplication: I’m OK with the Mult function being a bit backwards, but can you verify that the operator |
I changed the
void TranslateThenScalePoint ()
{
var point = new SCNVector3 (1, 2, 3);
var matrix =
SCNMatrix4.CreateTranslation (-1, 0, 0) *
SCNMatrix4.Scale (10, 1, 1);
var newPoint = SCNVector3.TransformPosition (point, matrix);
AssertEqual (new SCNVector3 (0, 2, 3), newPoint);
} You'd have to multiply in the reverse order to have it work as expected: void TranslateThenScalePoint ()
{
var point = new SCNVector3 (1, 2, 3);
var matrix =
SCNMatrix4.Scale (10, 1, 1) *
SCNMatrix4.CreateTranslation (-1, 0, 0);
var newPoint = SCNVector3.TransformPosition (point, matrix);
AssertEqual (new SCNVector3 (0, 2, 3), newPoint);
} This is because a column vector is on the right side of the multiplication, so if you scale first ( and then you translate it ( it's the same as: and the order is inversed: the translation is on the left, and the scale on the right. This effectively means that if I make the |
My sample was just trying to figure out how things are working and shouldn’t be considered how I want things to work. I wrote the operations in that order because that’s how they worked at the time. In column-vector transformers, it is expected that the right-most operation happens first. You can see this with CGContext’s and their transformers. They’re column transformers and therefore the first operation has to come last. If you pickup any OpenGL book you’ll see the transformation pipeline written as:
The vector is first transformed by the Model matrix, then View, then the Projection. If you define
Which makes it look like a row transformer (which you just went through a lot of trouble to fix). I think it’s pretty confusing doing the operation backwards. But here I am of two minds:
Ugh. Correctness or expediency? I really wish Apple hadn’t played that game. Again, I won’t die on this hill. But it seems so wrong to me. |
I'm leaning towards keeping the behavior I implemented (i.e. reversed multiplication), because of the reasons mentioned:
Unless there are any objections, I'll merge this PR by the end of the week. |
No objections here, I think this would be best moving forward too |
/sudo backport release/6.0.4xx |
/sudo backport release/6.0.3xx |
Backport Job to branch release/6.0.4xx Created! The magic is happening here |
Backport Job to branch release/6.0.3xx Created! The magic is happening here |
Hooray! Backport succeeded! Please see https://devdiv.visualstudio.com/DevDiv/_build/results?buildId=6301704 for more details. |
Hooray! Backport succeeded! Please see https://devdiv.visualstudio.com/DevDiv/_build/results?buildId=6301705 for more details. |
…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>
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:
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:
This affects numerous APIs in SCNMatrix4, SCNVector3 and SCNVector4:
second number the row, to reflect that it's a column-major matrix.
transformers. Technically this means that these matrices are transposed compared
to legacy Xamarin. The functions involved are:
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.
to do a column-vector transformation instead of a row-vector transformation. This
involves the following functions:
Fixes #15094.