Bug 58599 - Matrix4 transforms used by ARKit (and SceneKit?) are transposed from Swift / Apple code
Summary: Matrix4 transforms used by ARKit (and SceneKit?) are transposed from Swift / ...
Alias: None
Product: iOS
Classification: Xamarin
Component: Xamarin.iOS.dll ()
Version: XI 10.99 (xcode9)
Hardware: PC Mac OS
: High critical
Target Milestone: Xcode9
Assignee: Rolf Bjarne Kvinge [MSFT]
Depends on:
Reported: 2017-08-04 22:11 UTC by Larry O'Brien
Modified: 2017-09-11 21:04 UTC (History)
4 users (show)

Is this bug a regression?: ---
Last known good build:

Notice (2018-05-24): bugzilla.xamarin.com is now in read-only mode.

Please join us on Visual Studio Developer Community and in the Xamarin and Mono organizations on GitHub to continue tracking issues. Bugzilla will remain available for reference in read-only mode. We will continue to work on open Bugzilla bugs, copy them to the new locations as needed for follow-up, and add the new items under Related Links.

Our sincere thanks to everyone who has contributed on this bug tracker over the years. Thanks also for your understanding as we make these adjustments and improvements for the future.

Please create a new report on Developer Community or GitHub with your current version information, steps to reproduce, and relevant error messages or log files if you are hitting an issue that looks similar to this resolved bug and you do not yet see a matching new report.

Related Links:

Description Larry O'Brien 2017-08-04 22:11:48 UTC
In Swift code demonstrating ARKit, you can extract the position from a 4x4 matrix using: 

        static func positionFromTransform(_ transform: matrix_float4x4) -> SCNVector3 {
            let pFromColumn3 = SCNVector3Make(transform.columns.3.x, transform.columns.3.y, transform.columns.3.z)
            return pFromColumn3

(Ref: https://developer.apple.com/sample-code/wwdc/2017/PlacingObjects.zip / Utilities.Swift.PositionFromTransform )

The straight-across port for this would seem to be:

    public static SCNVector3 PositionFromTransform(Matrix4 transform){
	var pFromColumn3 = new SCNVector3(transform.Column3.X, transform.Column3.Y, transform.Column3.Z);
	return pFromComponents;

But, in fact, that code always returns [0,0,0], while _this_ code returns the proper elements:
    var pFromComponents = new SCNVector3(transform.M41, transform.M42, transform.M43);

So, it appears to me that our `Matrix4` is transposed / rotated from the `matrix_float4x4`. 

At the risk of making things even more confusing, I note that `matrix_float4x4` seems to be hard-coded to treat its arguments as column-major, while ARKit seems to be row-major. Are we perhaps "fixing" the order when converting to an OpenTK.Matrix? 

Also, https://stackoverflow.com/questions/44732476/arkit-vs-scenekit-coordinates seems possibly relevant.
Comment 1 Sebastien Pouliot 2017-08-08 15:54:24 UTC
@Alex, @Rolf have you seen this while doing ARKit bindings / registrar work ?
Comment 2 Rolf Bjarne Kvinge [MSFT] 2017-08-08 18:32:08 UTC
Yeah, OpenTK.Matrix is row-major: https://github.com/xamarin/xamarin-macios/blob/master/src/OpenGL/OpenTK/Math/Matrix4.cs#L39-L54, and matrix_float4x4 is column-major:

typedef simd_float4x4 matrix_float4x4;
typedef struct { simd_float4 columns[4]; } simd_float4x4;

Which means we have two choices: we can define a new Matrix type which is column-major (to match the native definition), or we can convert parameters/return values on every method call.

This is further complicated by the fact that we already have bindings for signatures with matrix_float4x4 in master, which means that if we start converting parameters/return values we'd have to somehow skip those bindings. Due to this I'm leaning towards declaring a new MatrixSimD (or whatever we want to call it) type.
Comment 3 Sebastien Pouliot 2017-08-08 19:08:43 UTC
Either `MatrixSimd4` or, closer to the original, `SimdFloat4x4`.

We'll need to review all published API and add compatibility wrappers for `Matrix4` (in case someone use them with extra transformations).
Comment 4 Rolf Bjarne Kvinge [MSFT] 2017-08-10 15:08:37 UTC
I'll look at this.
Comment 5 Sebastien Pouliot 2017-08-10 15:10:18 UTC
Thanks! The data from sharpie/xtro should be able to pinpoint the existing, incorrect API
Comment 6 Rolf Bjarne Kvinge [MSFT] 2017-08-25 06:00:21 UTC
@Larry, can you attach/link to a sample I can use to test my changes?
Comment 7 Rolf Bjarne Kvinge [MSFT] 2017-08-31 12:22:46 UTC
Comment 8 Rolf Bjarne Kvinge [MSFT] 2017-09-05 13:40:50 UTC
Fixed: https://github.com/xamarin/xamarin-macios/commit/15808c4693d67f4112b8b275c7304cbb1e00c286

@Larry, this change requires a few changes in the sample (there are new matrix types now), could you please make sure the sample works as expected now? The sooner we find broken things the sooner we can fix them :)
Comment 9 Larry O'Brien 2017-09-11 21:02:10 UTC
Translation and rotation work as expected. I have updated our ARKit samples to compile and work with this new type.
Comment 10 Rolf Bjarne Kvinge [MSFT] 2017-09-11 21:04:22 UTC
Great, thanks for confirming!