[arkit] Fix 'Vertices', 'TextureCoordinates' and 'TriangleIndices' in 'ARFaceGeometry' by VincentDondain · Pull Request #3090 · dotnet/macios

VincentDondain · 2017-12-08T20:38:53Z

Fixes bug #61056: [ARKit] short TriangleIndices should be short[]
(https://bugzilla.xamarin.com/show_bug.cgi?id=61056)
Obsolete short TriangleIndices.
Obsolete Vector3 Vertices.
Obsolete Vector2 TextureCoordinates.
Introduced new short [] GetTriangleIndices ().
Introduced new Vector3 [] GetVertices ().
Introduced new Vector2 [] GetTextureCoordinates ().

- Fixes bug #61056: [ARKit] short TriangleIndices should be short[] (https://bugzilla.xamarin.com/show_bug.cgi?id=61056) - Obsolete `short TriangleIndices`. - Introduced new `short [] TriangleIndexes` property to avoid name collision.

VincentDondain · 2017-12-08T21:07:55Z

Need to address ARFaceGeometry's vertices and textureCoordinates too.

They're vector_float3 * and vector_float2 *

spouliot · 2017-12-10T20:29:21Z

 		nuint TriangleCount { get; }

+#if !XAMCORE_4_0
+		[Obsolete ("Use the 'GetTriangleIndices' method that return a 'short' array instead.")]


-> TriangleIndexes

spouliot · 2017-12-10T20:31:25Z

+				var rv = new short [count];
+				var ptr = (short *) GetTriangleIndexes ();
+				for (int i = 0; i < count; i++)
+					rv [i] = *ptr++;


not sure how many items there is usually but it might be better to use Marshal.Copy and drop the unsafe

I don't have an exact answer to this question, would need a test case. However this from the documentation: "Each 16-bit integer value in this buffer represents an index into the vertices and textureCoordinates buffers. Each set of three indices identifies the vertices comprising a single triangle in the mesh." makes me think it can be quite a lot (all the meshes representing a face).

Also I'd like to point out that we use the same code for ARPointCloud: https://github.com/xamarin/xamarin-macios/blob/master/src/ARKit/ARPointCloud.cs#L19-L39 (initially suggested by Rolf).

- Obsolete `Vector3 Vertices`. - Obsolete `Vector2 TextureCoordinates`. - Introduced new `Vector3 [] GetVertices ()`. - Introduced new `Vector2 [] GetTextureCoordinates ()`.

spouliot · 2017-12-11T19:52:18Z

+#if !XAMCORE_4_0
+		[Obsolete ("Use the 'GetVertices' method instead.")]
 		[Export ("vertices")]
+		[Sealed] // Just to avoid the duplicate selector error


that's a breaking change as it will remove virtual

since it's largely unusable just remove the [Export], which means moving it to ARCompat.cs, so the right thing can still be done on the new API.

Ah makes sense for [Sealed] an other option would have been to add [Sealed] to the new duplicated [Export] (looks like it's what we usually do). However I like the ARCompat.cs option better. Going for that.

spouliot · 2017-12-11T19:53:19Z

+	public partial class ARFaceGeometry {
+
+		// Going for GetXXX methods so we can keep the same name as the matching obsoleted property 'Vertices'.
+		public unsafe Vector3 [] GetVertices () {


minor: style { on next line

spouliot · 2017-12-11T19:53:24Z

+		}
+
+		// Going for GetXXX methods so we can keep the same name as the matching obsoleted property 'TextureCoordinates'.
+		public unsafe Vector2 [] GetTextureCoordinates () {


spouliot · 2017-12-11T19:53:29Z

+		}
+
+		// Going for GetXXX methods so we can keep the same name as the matching obsoleted property 'TriangleIndices'.
+		public unsafe short [] GetTriangleIndices () {


spouliot · 2017-12-11T19:57:14Z

+#if !XAMCORE_4_0
+		[Obsolete ("Use the 'GetTextureCoordinates' method instead.")]
 		[Export ("textureCoordinates")]
+		[Sealed] // Just to avoid the duplicate selector error


spouliot · 2017-12-11T19:57:23Z

+#if !XAMCORE_4_0
+		[Obsolete ("Use the 'GetTriangleIndices' method instead.")]
 		[Export ("triangleIndices")]
+		[Sealed] // Just to avoid the duplicate selector error


spouliot · 2017-12-11T19:58:04Z

PR title also needs to be updated to match the set of changes :)

spouliot

when merging update Introduced new short [] TriangleIndexes property to avoid name collision. since it's not a property anymore :)

monojenkins · 2017-12-12T02:33:43Z

Build success

spouliot · 2017-12-12T13:31:01Z

Link to successful build -> https://jenkins.mono-project.com/job/xamarin-macios-pr-builder/5756/

Status is broken :(

… 'ARFaceGeometry' (dotnet#3090) - Fixes bug #61056: [ARKit] TriangleIndices, Vertices and TextureCoordinates should be respectively short [], Vector3 [] and Vector2 [] (https://bugzilla.xamarin.com/show_bug.cgi?id=61056) - Obsolete `short TriangleIndices`. - Obsolete `Vector3 Vertices`. - Obsolete `Vector2 TextureCoordinates`. - Introduced new `short [] GetTriangleIndices ()`. - Introduced new `Vector3 [] GetVertices ()`. - Introduced new `Vector2 [] GetTextureCoordinates ()`.

…ndices' in 'ARFaceGeometry' (#3090) (#3342) - Fixes bug #61056: [ARKit] TriangleIndices, Vertices and TextureCoordinates should be respectively short [], Vector3 [] and Vector2 [] (https://bugzilla.xamarin.com/show_bug.cgi?id=61056) - Obsolete `short TriangleIndices`. - Obsolete `Vector3 Vertices`. - Obsolete `Vector2 TextureCoordinates`. - Introduced new `short [] GetTriangleIndices ()`. - Introduced new `Vector3 [] GetVertices ()`. - Introduced new `Vector2 [] GetTextureCoordinates ()`.

[arkit] Fix TriangleIndices to return 'short []'

08edf75

- Fixes bug #61056: [ARKit] short TriangleIndices should be short[] (https://bugzilla.xamarin.com/show_bug.cgi?id=61056) - Obsolete `short TriangleIndices`. - Introduced new `short [] TriangleIndexes` property to avoid name collision.

dnfclas added the cla-already-signed label Dec 8, 2017

VincentDondain added the do-not-merge Do not merge this pull request label Dec 8, 2017

spouliot self-requested a review December 10, 2017 20:28

spouliot suggested changes Dec 10, 2017

View reviewed changes

VincentDondain removed the do-not-merge Do not merge this pull request label Dec 11, 2017

[arkit] Fix 'Vertices' and 'TextureCoordinates' too

0334f1e

- Obsolete `Vector3 Vertices`. - Obsolete `Vector2 TextureCoordinates`. - Introduced new `Vector3 [] GetVertices ()`. - Introduced new `Vector2 [] GetTextureCoordinates ()`.

VincentDondain force-pushed the arfacegeometry-triangleindices-fix branch from 4da24a2 to 0334f1e Compare December 11, 2017 19:48

spouliot suggested changes Dec 11, 2017

View reviewed changes

VincentDondain changed the title ~~[arkit] Fix TriangleIndices to return 'short []'~~ [arkit] Fix 'Vertices', 'TextureCoordinates' and 'TriangleIndices' in 'ARFaceGeometry' Dec 11, 2017

More fixes

126930a

spouliot approved these changes Dec 11, 2017

View reviewed changes

dnfadmin removed the cla-already-signed label Dec 12, 2017

VincentDondain merged commit 8899158 into dotnet:master Dec 12, 2017

VincentDondain deleted the arfacegeometry-triangleindices-fix branch December 12, 2017 17:33

VincentDondain mentioned this pull request Jan 29, 2018

[arkit] Update for Xcode 9.3 beta 1 #3343

Merged

Uh oh!

Conversation

VincentDondain commented Dec 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

VincentDondain commented Dec 8, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spouliot commented Dec 11, 2017

Uh oh!

spouliot left a comment

Choose a reason for hiding this comment

Uh oh!

monojenkins commented Dec 12, 2017

Uh oh!

spouliot commented Dec 12, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

VincentDondain commented Dec 8, 2017 •

edited

Loading