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

[ARKit] different 'Transform' matrix presentation #4525

Closed
mykyta-bondarenko-gl opened this Issue Jul 30, 2018 · 12 comments

Comments

Projects
None yet
7 participants
@mykyta-bondarenko-gl

mykyta-bondarenko-gl commented Jul 30, 2018

After I set the Position property Transform matrixes look differently. Position vector is located under different indexes:

  • xamarin - position is under Row3
  • swift - position is under Column3

It looks like the same matrix, but one of them isn’t transposed.

Steps to Reproduce

  1. Open attached projects (swift and xamarin)
  2. Run them and looked to the output

Test code is located:

  • xamarin - GameViewController.cs - between 30 and 46 lines of code
  • swift - ViewController.swift - between 22 and 35 lines of code

Expected Behavior

The Transform matrixes are the same.

Actual Behavior

The Transform matrixes look differently.

Environment

  1. VSfM 7.6 Preview (7.6 build 2109)
    https://gist.github.com/mykyta-bondarenko-gl/aa42dcf103eb3a9b1259d6d67e0de428

Example Project

  1. Xamarin
  2. Swift

@spouliot spouliot added bug iOS labels Jul 30, 2018

@spouliot spouliot added this to the xcode10 milestone Jul 30, 2018

@spouliot

This comment has been minimized.

Contributor

spouliot commented Jul 30, 2018

@VincentDondain it sounds like the wrong type might have been used for the transform (please double check if other ARKit API might be in the same situation)

@lobrien

This comment has been minimized.

lobrien commented Jul 31, 2018

This definitely applies across ARKit and SceneKit scenarios. For instance,

var node = new SCNNode { Position = new SCNVector3(2, 3, 4) };  
var xform = node.Transform;
Console.WriteLine(xform);
// Output is: "(1, 0, 0, 0)\n(0, 1, 0, 0)\n(0, 0, 1, 0)\n(2, 3, 4, 1)"
xform.Column3
// Output is ""(0, 0, 0, 1)"
@lobrien

This comment has been minimized.

lobrien commented Jul 31, 2018

Note that in Swift:

let t = float4x4([float4(1, 1, 1, 0), float4(2,2,2,0), float4(3,3,3,0), float4(4, 4, 4, 1)])
// simd_float4x4([[1.0, 1.0, 1.0, 0.0)], [2.0, 2.0, 2.0, 0.0)], [3.0, 3.0, 3.0, 0.0)], [4.0, 4.0, 4.0, 1.0)]])
let v = float4(1, 0, 0, 0)
let v2 = t * v
// v2 == float4(1.0, 1.0, 1.0, 0.0)
@VincentDondain

This comment has been minimized.

Contributor

VincentDondain commented Aug 8, 2018

The native and managed matrices are identical.

(1, 0, 0, 0)
(0, 1, 0, 0)
(0, 0, 1, 0)
(0.25, 0.33, 0.44, 1)

However Apple think columns are rows and rows are columns (because I'm sorry but in my world Column3 is (0, 0, 0, 1). That's what Xamarin does.

So yea our code is doing the right thing but it's not matching the native behavior which I understand can be confusing.

I do not believe we can or should change the behavior of our code. Let's reopen if we think it's necessary.

@EgorBo

This comment has been minimized.

Member

EgorBo commented Aug 8, 2018

Column major matrices (where the matrix is basically a set of vectors=columns) is more efficient (or cache/SIMD friendly) than traditional row major matrices in some popular scenarios like
var foo = mat * vec

@VincentDondain

This comment has been minimized.

Contributor

VincentDondain commented Aug 8, 2018

Fair point.

Unfortunately if we swap rows and columns under the hood it will break everyone's assumptions. The only we could do is XAMCORE-ify everything for the "next" breaking change. Not even sure that is worth it.

@spouliot

This comment has been minimized.

Contributor

spouliot commented Aug 8, 2018

@VincentDondain let's add an [Advice ("This is a row major matrix representation.")]

@lobrien

This comment has been minimized.

lobrien commented Aug 8, 2018

Not changing it will confuse every single person doing SceneKit work. At the very least, I think we should have conversions to/from SCNMatrix4.

@migueldeicaza

This comment has been minimized.

Member

migueldeicaza commented Aug 9, 2018

@lobrien What do you suggest the cast to/from should be?

@VincentDondain

This comment has been minimized.

Contributor

VincentDondain commented Aug 9, 2018

@lobrien I'm confused, why do we need conversions for? This issue is about native going for a column major SCNMatrix4 and us going for a row major SCNMatrix4. The "conversion" method is Transpose. That will move from row major to column major.

Not changing it will confuse every single person doing SceneKit work

I agree it's confusing (though mostly for people porting stuff or reading Apple's doc) and annoying to have to transpose (if you want it the same way as native) but again if we change the behavior of SCNMatrix4 under the hood even though the APIs won't break customers' apps who were expecting rows instead of columns or were transposed will break.

VincentDondain added a commit to VincentDondain/xamarin-macios that referenced this issue Aug 9, 2018

[scenekit] Add advice to SCNMatrix4
In relation to xamarin#4525, adding an "advice" to remind everyone this is a row major matrix.
@VincentDondain

This comment has been minimized.

Contributor

VincentDondain commented Aug 9, 2018

@spouliot I added the advice attribute: #4605

VincentDondain added a commit that referenced this issue Aug 13, 2018

[scenekit] Add advice to SCNMatrix4 (#4605)
In relation to #4525, adding an "advice" to remind everyone this is a row major matrix.
@rolfbjarne

This comment has been minimized.

Member

rolfbjarne commented Aug 16, 2018

This is something we should fix for XAMCORE_4_0, it's a difference between Swift/ObjC that doesn't make sense (but which we can't change now due to backwards compatibility concerns)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment