[SIMD] Implement Simd double types (VectorDouble3/MatrixDouble4x4).

[dalexsoto]

The following types will be used by ModelIO bindings, this is based on Rolf's commit :)

[rolfbjarne]

Just one minor thing, otherwise it would have been perfect 😄

[rolfbjarne]

I think delta should be double too (and there are more cases below too).

[migueldeicaza]

Why are we introducing new vector/matrix data types when we already have perfectly suitable data types for for Vecto2d, Vector3d, Vector4d and Matrix4x4d in OpenTK?

The OpenTK data types are already baked into Xamarin.iOS.dll, and we already consume them in many places in our Xamarin.iOS API.

And even if we had a good case (say, the internal layout is incompatible), then in that case, we should copy the code from OpenTK which has many existing capabilities and comes with inline API documentation.

[monojenkins]

Build success

[dalexsoto]

@migueldeicaza The issue is better explained here #2571 this is just an extension from PR #2622

[monojenkins]

Build success

[rolfbjarne]

@migueldeicaza For the matrix types, the internal layout is incompatible, so we don't have much choice (technically we can convert matrices when going from native to managed (and vice versa), but this will complicate a lot of code, it won't be ready for iOS 11, and it will be slow).

Regarding copying the OpenTK types, I looked at the ones we have, and found that:

• The API is buggy. Some matrices got left and right operands reversed for multiplication (and I didn't look at most of the API): https://github.com/xamarin/xamarin-macios/blob/96067ef1c1ff84280f5aac7011fa3c8d60a0b7ad/tests/monotouch-test/Simd/MatrixFloat3x3Test.cs#L152
• The API is inconsistent. The fields for Matrix3 are R0C0..R2C2, while for Matrix4 it's Row0..Row3. This is just ugly.
• There's a lot of obsolete API.
• Most of the operations are implemented using the Row* fields, since that's obviously the fastest. Changing the Row* fields to be Columns* instead (and re-implementing Row* to be properties that compute the return value based on the columns), would make those Row* based implementations very slow. This is obviously fixable by changing the implementation to use the Column* fields instead, but it requires writing a significant amount of tests (unfortunately the upstream OpenTK project has very few tests) to not get it wrong (and that takes time).
• I thought we didn't care about the inline API documentation, since it's not copied to our own documentation.

So I decided to implement the matrices with only a few simple operations, but correctly and well-tested. We can add more operations later, and in any case it's a lossless (explicit) conversion to and from the OpenTK matrices, so the operations there can still be used.

At the time it didn't occur to me to look to the upstream OpenTK types (https://github.com/opentk/opentk/tree/develop/src/OpenTK/Math), and it looks like some of my concerns are addressed (the API is consistent (Row0..RowX), there's less obsolete stuff (but still some)), but the rest of the points still stand.

If we still want to copy the OpenTK types, a minimum effort would be:

• Copy the upstream OpenTK types.
• Change the layout to what we need it to be (i.e. change the Row0-X fields to be Column0-X, and change the Column0-X properties to be Row0-X).
• Remove all obsolete API.
• Don't change anything else, and hope nothing is wrong in their implementations. This means most of the API will be quite slow, but this is fixable later.

Regarding the vector types: the OpenTK.Vector3 does not have the size it should have (it's 12 bytes, when it should be 16 bytes due to padding). While this is also technically fixable when we go from managed to native (and vice versa), it's not at all trivial to get it right (most of the time we get it wrong the first time), and it's also slow (if an API takes an array of Vector3, we can't just pass a reference to the managed array, we have to copy every single vector element into a new array, converting them, and pass that to native code - and now I just looked at Apple's API to see if any of it takes a vector_float3 array, and it turns out they do, and we bound it incorrectly (#59380)).

[migueldeicaza]

Excellent, so we have to go with the secondary part of the proposal, which is given that the layout is different, we should adjust the layout. If we happen to fix bugs in the process we should fix those clearly.

We can certainly try the approach of just using what we have and later fill the gaps for the case of Matrix (given the risk of swapping the fields), I would like us to avoid having two gratuitously different APIs nor resort to "Cast to the OpenTK and operate there and convert back" - because that is 128 byte copies per Matrix4d. I think we can wait on that part.

That said, I do not think the risk aversion applies to the Vector types.

Things that are gratuitously different from OpenTK (and previous efforts that have copied those, SceneKit and Urho):

• The name (Matrix4d, vs Matrix4x4; Vector2Double vs Vector2d)
• Naming of the "m" fields, OpenTK is zero-indexed based, ours is 1-based - this alone looks like a nightmakre for our developers to wrap our heads around and find bugs on

[spouliot]

@migueldeicaza I'm not sure what is the secondary part of the proposal but the main issue with we should adjust the layout was that OpenTK structures are [Serializable] so changing their layout will break existing code (e.g. cross platform interop) and stored data.

There's also an (alignment) issue with Vector3, but afaik the other vector types are fine. We can special case only this one - that implies more mixing of OpenTK and new Simd types.

The names can be changed, I think the major issue was avoiding identically named types that would confuse the compiler between different namespaces (a Simd prefix?). There's no reason for the member names not to be 0-based.

[rolfbjarne]

The matrices in the System.Numerics.Vectors namespaces are 1-based: https://msdn.microsoft.com/en-us/library/system.numerics.matrix4x4(v=vs.111).aspx#Anchor_5

But either way works for me, this is easy to change.

Regarding the naming, I avoided using the same name as OpenTK on purpose, because it becomes annoying to have two types with the same name from different namespaces when writing code (and I felt it made most sense to choose something close to how Apple named the types).

[rolfbjarne]

In fact some of the OpenTK's types are 1-based too (but inconsistently so), which is where I got it from:

• Our OpenTK.Matrix4: https://github.com/xamarin/xamarin-macios/blob/54058bb8851dc018b5c878e7e85cab6ea5288b59/src/OpenGLES/OpenTK/Math/Matrix4.cs#L166-L244

• Although the constructor in the same type doesn't agree: https://github.com/xamarin/xamarin-macios/blob/54058bb8851dc018b5c878e7e85cab6ea5288b59/src/OpenGLES/OpenTK/Math/Matrix4.cs#L99-L103

• Our OpenTK.Matrix3 (and Matrix2) has a different scheme, a 0-based R_C_ format: https://github.com/xamarin/xamarin-macios/blob/54058bb8851dc018b5c878e7e85cab6ea5288b59/src/OpenGLES/OpenTK/Math/Matrix3.cs#L38-L63

• The upstream OpenTK's Matrix3 has changed to match their Matrix4: 0-based M-name in the constructor, 1-based M-name fields: https://github.com/opentk/opentk/blob/077d7c709814feea163105a45457d71f1c3ee685/src/OpenTK/Math/Matrix3.cs#L151-L194

Whatever we choose we should at least be consistent...

[spouliot]

For the type names I think we can do a prefix, e.g. Simd, like it was done with SCN for SceneKit. That removes the compiling issue (wrt namespaces) and any confusion about the mapping of those types.

[migueldeicaza]

@spouliot The suggestion was to take the existing code and replace the names and adjust the types to match what we need (for layout and size), so the [Serializable] would not be an issue, it is a new data type with a new name.

@rolfbjarne I agree that it is undesirable to have the same type names, because you need to resolve them with the namespace. What we could do is prefix them (like we did for SceneKit - SCNVectorX, SCNMatrixX) for this scenario.

It is a shame that the public constructors in OpenTK do not match the property names in the matrix type themselves, we should correct that.

But what I do not want to have is a scenario where our public properties are different, and we already have both the original OpenTK and the forked versions for SceneKit that are at least the same, we should not have one that is gratuitously different.

I would take what we have, because that is what we are consistent with.

Sadly, the upstream OpenTK was sort of a fork based on ours where they threw away backwards compatibility away. I rather be consistent with our existing data types, than with OpenTK's updated one (scenario: cast this new Matrix4d to our OpenTK Matrix4d to perform operations, but the value of M11 is different depending on which one you use).

I take it from the comments that we agree that the Vectors types should just then be the adjusted/ported versions of OpenTk at this point.

[rolfbjarne]

@spouliot / @migueldeicaza: OK, I think I have a plan now:

• Copy the OpenTK types we already have, like this:

  • OpenTK.Matrix2[d] -> OpenTK.SimdMatrix2[d]
  • OpenTK.Matrix3[d] -> OpenTK.SimdMatrix3[d]
  • OpenTK.Matrix4[d] -> OpenTK.SimdMatrix4[d]
  • OpenTK.Matrix4[d] -> OpenTK.SimdMatrix4x3[d] (OpenTK doesn't have a 4x3 matrix, so I'm copying the 4x4 matrix and modifying it accordingly).
  • OpenTK.Vector3[d] -> OpenTK.SimdVector3[d]

If we want to prefix the new types with Simd, it felt redundant to add them in a Simd namespace (Simd.SimdMatrix2 just looks ugly), and since there's no other applicable namespace, I put them in OpenTK.

In all cases I'll:

• Fix the memory layout as needed according to the corresponding native type.

• Remove any obsolete members.

• Fix the 0-based m## in constructors to match the 1-based M## fields.

• Keep all other API, and not change anything in the implementation unless it's to fix the known bug (the inverted matrix multiplication). This means that some operations will be significantly slower (but this can be fixed later). It also means that we might copy any existing bugs from OpenTK (since I won't have time to write tests to verify the existing implementations).

• This means that:

  • All 4-dimensional matrices will have the 1-based M## fields.
  • All 2- and 3-dimensional matrices will have the 0-based R#C# fields.
  • I'm assuming we're OK with this difference, since we consider it more important to be as similar as possible to the existing OpenTK matrices, than being consistent between the new types.

Other notes:

• I'm not copying Vector2 nor Vector4, since the memory layout is fine for those. This means our bindings will be using Vector2, SimdVector3 and Vector4, but we're fine with this since the alternative is to create otherwise duplicated/identical types.

Please confirm if this is OK.

[spouliot]

@rolfbjarne sounds good to me

[migueldeicaza]

I am ok with this plan, but I am also OK with the lower-impact plan which was to keep the code that is already part of this pull request, but:

(a) Adjust the public properties to match OpenTK ones

(b) Adjust the names to follow the convention. The only issue with using "Simd" in the prefix is that these types are not SIMD accelerated at all from C#, perhaps we can use the MIO prefix, if this is just for ModelIO interop?

[spouliot]

Apple use those types in ARKit, AVFoundation, ModelIO... they are not specific to a framework

[rolfbjarne]

OK, keeping the existing types as I implemented them, and just changing the type name and field/property names is much simpler. I'll do that instead.

Regarding the name: I've used the Simd prefix because that's where the equivalent types are in native code ("/usr/include/simd/simd.h"), but I'm fine with anything if there's a better idea. The problem is that these types are used in multiple frameworks.

[dalexsoto]

We could prefix them Native or N i.e. NativeVector3 or NVector3

[dalexsoto]

We have agreed to go with N prefix

[rolfbjarne]

I've created a PR (#2668) with the fixes as we agreed on for the Matrix/Vector types already present.

Once that's merged I'll fix this one and PR #2658 as well.

[rolfbjarne]

I've completed the requested changes for this PR locally, and I'll push them once PR #2668 is merged.

[rolfbjarne]

@migueldeicaza / @spouliot: This PR has now been updated with the new NMatrix/NVector names.

@dalexsoto: Please update PR #2651 accordingly.

[monojenkins]

Build failure

[monojenkins]

Build success