[SceneKit] Fix SCNMatrix4 in .NET. Fixes #15094. #15160

rolfbjarne · 2022-05-31T13:04:09Z

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.

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.

vs-mobiletools-engineering-service2 · 2022-06-03T12:56:01Z

📚 [PR Build] Artifacts 📚

Packages generated

View packages

Pipeline on Agent XAMBOT-1109.Monterey'
Hash: cc3a9291d733bb393f3063e7b4da4a03b18f680c

vs-mobiletools-engineering-service2 · 2022-06-03T12:58:47Z

📋 [PR Build] API Diff 📋

API diff (for current PR)

ℹ️ API Diff (from PR only) (please review changes)

API diff: vsdrops gist

Xamarin

iOS vsdrops gist
macOS vsdrops gist
tvOS vsdrops gist
watchOS vsdrops gist

.NET

dotnet-iOS vsdrops gist
dotnet-macOS vsdrops gist
dotnet-tvOS vsdrops gist
dotnet-MacCatalyst vsdrops gist

Xamarin vs .NET

dotnet-legacy-iOS vsdrops gist
dotnet-legacy-macOS vsdrops gist
dotnet-legacy-tvOS vsdrops gist

iOS vs Mac Catalyst (.NET)

dotnet-macCatiOS vsdrops gist

API diff (vs stable)

✅ API Diff from stable

API diff: vsdrops gist

Xamarin

iOS vsdrops gist
macOS vsdrops gist
tvOS vsdrops gist
watchOS vsdrops gist

.NET

dotnet-iOS vsdrops gist
dotnet-macOS vsdrops gist
dotnet-tvOS vsdrops gist
dotnet-MacCatalyst vsdrops gist

Xamarin vs .NET

dotnet-legacy-iOS vsdrops gist
dotnet-legacy-macOS vsdrops gist
dotnet-legacy-tvOS vsdrops gist

iOS vs Mac Catalyst (.NET)

dotnet-macCatiOS vsdrops gist

Generator diff

✅ Generator Diff (no change)

Pipeline on Agent XAMBOT-1023.Monterey
Hash: cc3a9291d733bb393f3063e7b4da4a03b18f680c

vs-mobiletools-engineering-service2 · 2022-06-03T13:01:29Z

💻 [PR Build] Tests on macOS Mac Catalina (10.15) passed 💻

✅ All tests on macOS Mac Catalina (10.15) passed.

Pipeline on Agent
Hash: cc3a9291d733bb393f3063e7b4da4a03b18f680c

vs-mobiletools-engineering-service2 · 2022-06-03T13:05:09Z

❌ [PR Build] Tests on macOS M1 - Mac Big Sur (11.5) failed ❌

Failed tests are:

monotouch-test

Pipeline on Agent
Hash: cc3a9291d733bb393f3063e7b4da4a03b18f680c

vs-mobiletools-engineering-service2 · 2022-06-03T19:27:21Z

❌ [CI Build] Tests failed on VSTS: simulator tests iOS ❌

Tests failed on VSTS: simulator tests iOS.

Test results

1 tests failed, 147 tests passed.

Failed tests

introspection/watchOS 32-bits - simulator/Debug: Crashed

Pipeline on Agent XAMBOT-1109.Monterey'
Merge cc3a929 into 5614519

chamons

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.

chamons · 2022-06-06T12:34:31Z

src/SceneKit/SCNMatrix4_dotnet.cs

+#if XAMCORE_5_0
+		public static SCNMatrix4 Mult (SCNMatrix4 firstTransformation, SCNMatrix4 secondTransformation)
+#else
+		public static SCNMatrix4 Mult(SCNMatrix4 left, SCNMatrix4 right)


Suggested change

public static SCNMatrix4 Mult(SCNMatrix4 left, SCNMatrix4 right)

public static SCNMatrix4 Mult (SCNMatrix4 left, SCNMatrix4 right)

chamons · 2022-06-06T12:34:45Z

src/SceneKit/SCNMatrix4_dotnet.cs

+#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)


Suggested change

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)

praeclarum · 2022-06-07T15:53:42Z

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:

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.

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

praeclarum · 2022-06-07T15:59:16Z

@rolfbjarne One question regarding multiplication: I’m OK with the Mult function being a bit backwards, but can you verify that the operator A * B does in fact A*B and not B*A? I think it’s OK for a function to be backwards but the operator should do the correct thing.

rolfbjarne · 2022-06-08T09:10:02Z

@praeclarum

@rolfbjarne One question regarding multiplication: I’m OK with the Mult function being a bit backwards, but can you verify that the operator A * B does in fact A*B and not B*A? I think it’s OK for a function to be backwards but the operator should do the correct thing.

I changed the * operator too, to make it behave like Mult, because:

That way it matches how the native SCNMatrix4Mult function works.
Your sample here wouldn't work otherwise:

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 (S is the scale matrix, v is the vector):

$$S*v$$

and then you translate it (T):

$$T*(S*v)$$

it's the same as:

$$(T*S)*v$$

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 * operator do multiplication the way you'd intuitively think, every code location that uses it would have to be changed to switch the operands.

praeclarum · 2022-06-10T01:32:11Z

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:

P*V*M*v = projectedPoint

The vector is first transformed by the Model matrix, then View, then the Projection. If you define * backwards, now you have to write:

v*M*V*P = projectedPoint

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:

Defining * to be backwards is obviously wrong and is going to create much confusion for people who know matrix math.
Making it backwards does mean this breaking change should allow old code to work thus saving me a lot of time.

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.

rolfbjarne · 2022-06-15T13:13:52Z

I'm leaning towards keeping the behavior I implemented (i.e. reversed multiplication), because of the reasons mentioned:

It's how SCNMatrix4Mult works. This can be quite helpful when porting Swift/Objective-C code to managed code.
Apple seems to be favoring the new simd matrices (such as matrix_float4x4), and these matrices multiply in the correct order (and that's how we've bound them too). This means that eventually the problem might go away if/when Apple deprecates/removes all these APIs.
Existing legacy Xamarin code would just work in .NET.

Unless there are any objections, I'll merge this PR by the end of the week.

dalexsoto · 2022-06-15T15:46:58Z

I'm leaning towards keeping the behavior I implemented (i.e. reversed multiplication), because of the reasons mentioned:

It's how SCNMatrix4Mult works. This can be quite helpful when porting Swift/Objective-C code to managed code.

Apple seems to be favoring the new simd matrices (such as matrix_float4x4), and these matrices multiply in the correct order (and that's how we've bound them too). This means that eventually the problem might go away if/when Apple deprecates/removes all these APIs.

Existing legacy Xamarin code would just work in .NET.

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

rolfbjarne · 2022-06-17T18:17:22Z

/sudo backport release/6.0.4xx

rolfbjarne · 2022-06-17T18:17:29Z

/sudo backport release/6.0.3xx

vs-mobiletools-engineering-service2 · 2022-06-17T18:17:54Z

Backport Job to branch release/6.0.4xx Created! The magic is happening here

vs-mobiletools-engineering-service2 · 2022-06-17T18:17:55Z

Backport Job to branch release/6.0.3xx Created! The magic is happening here

vs-mobiletools-engineering-service2 · 2022-06-17T18:19:56Z

Hooray! Backport succeeded! Please see https://devdiv.visualstudio.com/DevDiv/_build/results?buildId=6301704 for more details.

vs-mobiletools-engineering-service2 · 2022-06-17T18:20:09Z

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 <[email protected]>

…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 <[email protected]>

This comment has been minimized.

Sign in to view

rolfbjarne added 3 commits June 1, 2022 11:46

[tests] Add renaming to fix build errors.

e0c15c5

[tests] Fix xammac_tests.csproj.

534b80a

Merge branch 'main' into scnmatrix-columns

1703fb8

This comment has been minimized.

Sign in to view

rolfbjarne added 2 commits June 3, 2022 13:11

Fix preparation for test builds.

74a510f

Merge remote-tracking branch 'origin/main' into scnmatrix-columns

cc3a929

chamons approved these changes Jun 6, 2022

View reviewed changes

rolfbjarne marked this pull request as ready for review June 7, 2022 08:12

rolfbjarne requested a review from dalexsoto as a code owner June 7, 2022 08:12

rolfbjarne mentioned this pull request Jun 7, 2022

SCNMatrix4 Breaking Change Breaks SCNNode.Transform #15094

Closed

dalexsoto approved these changes Jun 7, 2022

View reviewed changes

mandel-macaque approved these changes Jun 15, 2022

View reviewed changes

rolfbjarne merged commit 42c1c66 into xamarin:main Jun 17, 2022

rolfbjarne deleted the scnmatrix-columns branch June 17, 2022 18:17

vs-mobiletools-engineering-service2 mentioned this pull request Jun 17, 2022

[release/6.0.4xx] [SceneKit] Fix SCNMatrix4 in .NET. Fixes #15094. #15296

Merged

vs-mobiletools-engineering-service2 mentioned this pull request Jun 17, 2022

[release/6.0.3xx] [SceneKit] Fix SCNMatrix4 in .NET. Fixes #15094. #15297

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SceneKit] Fix SCNMatrix4 in .NET. Fixes #15094. #15160

[SceneKit] Fix SCNMatrix4 in .NET. Fixes #15094. #15160

rolfbjarne commented May 31, 2022

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.

vs-mobiletools-engineering-service2 commented Jun 3, 2022

vs-mobiletools-engineering-service2 commented Jun 3, 2022

vs-mobiletools-engineering-service2 commented Jun 3, 2022

vs-mobiletools-engineering-service2 commented Jun 3, 2022

vs-mobiletools-engineering-service2 commented Jun 3, 2022

Failed tests

chamons left a comment

chamons Jun 6, 2022

chamons Jun 6, 2022

praeclarum commented Jun 7, 2022

praeclarum commented Jun 7, 2022

rolfbjarne commented Jun 8, 2022

praeclarum commented Jun 10, 2022 •

edited

Loading

rolfbjarne commented Jun 15, 2022

dalexsoto commented Jun 15, 2022

rolfbjarne commented Jun 17, 2022

rolfbjarne commented Jun 17, 2022

vs-mobiletools-engineering-service2 commented Jun 17, 2022

vs-mobiletools-engineering-service2 commented Jun 17, 2022

vs-mobiletools-engineering-service2 commented Jun 17, 2022

vs-mobiletools-engineering-service2 commented Jun 17, 2022

	public static SCNMatrix4 Mult(SCNMatrix4 left, SCNMatrix4 right)
	public static SCNMatrix4 Mult (SCNMatrix4 left, SCNMatrix4 right)

	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)

[SceneKit] Fix SCNMatrix4 in .NET. Fixes #15094. #15160

[SceneKit] Fix SCNMatrix4 in .NET. Fixes #15094. #15160

Conversation

rolfbjarne commented May 31, 2022

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.

vs-mobiletools-engineering-service2 commented Jun 3, 2022

📚 [PR Build] Artifacts 📚

Packages generated

vs-mobiletools-engineering-service2 commented Jun 3, 2022

📋 [PR Build] API Diff 📋

API diff (for current PR)

API diff (vs stable)

Generator diff

vs-mobiletools-engineering-service2 commented Jun 3, 2022

💻 [PR Build] Tests on macOS Mac Catalina (10.15) passed 💻

vs-mobiletools-engineering-service2 commented Jun 3, 2022

❌ [PR Build] Tests on macOS M1 - Mac Big Sur (11.5) failed ❌

vs-mobiletools-engineering-service2 commented Jun 3, 2022

❌ [CI Build] Tests failed on VSTS: simulator tests iOS ❌

Test results

Failed tests

chamons left a comment

Choose a reason for hiding this comment

chamons Jun 6, 2022

Choose a reason for hiding this comment

chamons Jun 6, 2022

Choose a reason for hiding this comment

praeclarum commented Jun 7, 2022

praeclarum commented Jun 7, 2022

rolfbjarne commented Jun 8, 2022

praeclarum commented Jun 10, 2022 • edited Loading

rolfbjarne commented Jun 15, 2022

dalexsoto commented Jun 15, 2022

rolfbjarne commented Jun 17, 2022

rolfbjarne commented Jun 17, 2022

vs-mobiletools-engineering-service2 commented Jun 17, 2022

vs-mobiletools-engineering-service2 commented Jun 17, 2022

vs-mobiletools-engineering-service2 commented Jun 17, 2022

vs-mobiletools-engineering-service2 commented Jun 17, 2022

praeclarum commented Jun 10, 2022 •

edited

Loading