[arkit] Update for Xcode 9.3 beta 1 by VincentDondain · Pull Request #3343 · dotnet/macios

VincentDondain · 2018-01-26T22:40:10Z

Note: I chose to use GetVertices and friends instead of properties to match what was done in ARFaceGeometry (where the properties where wrong and unfortunately obsoleted).

Diff file: https://github.com/xamarin/xamarin-macios/wiki/ARKit-iOS-xcode9.3-beta1

dalexsoto

Very small naming detail, otherwise it looks good to me 👍

dalexsoto · 2018-01-26T23:30:24Z

+namespace XamCore.ARKit {
+	public partial class ARPlaneGeometry {
+
+		// Using GetXXX methods so it's similar to the ARFaceGeometry methods.


I think I prefer methods over props when the operation is "expensive" like interfacing with unmanaged code. Just a personal preference :)

dalexsoto · 2018-01-26T23:32:47Z

+		[Static]
+		[Export ("planeGeometryWithDevice:")]
+		[return: NullAllowed]
+		ARSCNPlaneGeometry CreatePlaneGeometry (IMTLDevice device);


Why not just Create? you already know it is a PlaneGeometry because of the type name.

Mmh that was good feedback for #2681 when ARSCNFaceGeometry.CreateFaceGeometry was introduced (:

I agree though, PlaneGeometry should be implied by the type's name.

I'll make CreateFaceGeometry [Obsolete] and move it to Create too for API consistency.

spouliot · 2018-01-27T02:56:24Z

+
+		[EditorBrowsable (EditorBrowsableState.Advanced)]
+		[Export ("vertices")]
+		IntPtr GetRawVertices ();


were not the previous one made internal or protected ? instead of public ?

looks like it was not (from the back port PR)

monojenkins · 2018-01-27T04:19:57Z

Build success

monojenkins · 2018-01-29T17:40:00Z

Build success

mandel-macaque · 2018-01-29T17:28:36Z

 		ExistingPlane = 1 << 3,
 		ExistingPlaneUsingExtent = 1 << 4,
+		[iOS (11,3)]
+		ExistingPlaneUsingGeometry = (1 << 5),


Why the (1 << 5) and not simply 1 << 5?

Because sharpie :P

I agree it's not needed, if I have to editor this PR again I'll fix it. It's minor though.

mandel-macaque · 2018-01-29T17:31:37Z

+
+		[EditorBrowsable (EditorBrowsableState.Advanced)]
+		[Export ("boundaryVertices")]
+		IntPtr GetRawBoundaryVertices ();


Why are GetRawTextureCoordinates, GetRawTextureCoordinates and GetRawTextureCoordinates methods and not properties?

At least in the diff we have:

@property (nonatomic, readonly) const vector_float2 *textureCoordinates NS_REFINED_FOR_SWIFT;

See the description of this PR (consistency with ARFaceGeometry), see #3090 (properties were incorrect and obsoleted) and see Alex's comment #3343 (comment) (Sebastien made the same comment).

mandel-macaque · 2018-01-29T17:37:51Z

+		public override IntPtr GetRawTriangleIndices ()
+		{
+			// Two triangles (set of 3 indices)
+			indices = new short [] { 1, 2, 3, 4, 5, 6 };


I'm a little confused, is this line creating an array of 6 shorts? can't you do new short [6]?? I might be missing something.

Because I'm setting values that I can then test. See https://github.com/xamarin/xamarin-macios/pull/3343/files#diff-58636f8c30f5f14ba7b2793d5900e7cfR129

I'm simulating the fact that there are actual indices.

mandel-macaque · 2018-01-29T17:39:26Z

+			var face = new ARPlaneGeometryPoker ();
+			var vertices = face.GetVertices ();
+			Assert.AreEqual (new VectorFloat3 (1, 2, 3), vertices [0]);
+			Assert.AreEqual (new VectorFloat3 (4, 5, 6), vertices [1]);


AFAIK we have as good practice to add a string if we have more than one assert in a test.

Oops, I missed that good practice (will remember for the next time). Minor comment though no (not sure it's blocking this PR)?

This wasn't mentioned when the first (similar) tests were added: #3090

mandel-macaque · 2018-01-29T17:39:36Z

+			var face = new ARPlaneGeometryPoker ();
+			var textureCoordinates = face.GetTextureCoordinates ();
+			Assert.AreEqual (new VectorFloat2 (1, 2), textureCoordinates [0]);
+			Assert.AreEqual (new VectorFloat2 (3, 4), textureCoordinates [1]);


Same as the previous one.

mandel-macaque · 2018-01-29T17:39:50Z

+			var face = new ARPlaneGeometryPoker ();
+			var boundaryVertices = face.GetBoundaryVertices ();
+			Assert.AreEqual (new VectorFloat3 (1, 2, 3), boundaryVertices [0]);
+			Assert.AreEqual (new VectorFloat3 (4, 5, 6), boundaryVertices [1]);


Same as previous comments about several asserts.

spouliot · 2018-01-29T20:26:28Z

+
+	[iOS (11,3)]
+	[BaseType (typeof(SCNGeometry))]
+	interface ARSCNPlaneGeometry {


the base SCNGeometry class does not have a default .ctor
and since there seems to be a requirement for a metal device it seems likely this type cannot be created with init

Not specified in the headers but I agree with you it makes sense (it's safer to ship it with [DisableDefaultCtor]. Also it's then similar to ARSCNFaceGeometry.

spouliot · 2018-01-29T20:28:33Z


+	[NoWatch, NoTV, NoMac, iOS (11,3)]
+	[BaseType (typeof(NSObject))]
+	interface ARReferenceImage : NSCopying, NSSecureCoding {


this is also not clear it can be created with init, the 2 .ctors don't accept nil images
easier to add it later than removing it (breaking changes)

spouliot · 2018-01-29T20:30:23Z

 	[BaseType (typeof(SCNGeometry))]
 	[DisableDefaultCtor]
 	interface ARSCNFaceGeometry {
+		[Obsolete ("Use the 'Create' static constructor instead.")]


put Obsolete under a !XAMCORE_4_0
also invert them, so the right one (this one) does not call the obsolete one
i.e. the bad one should suffer the extra call (and makes the !XAMCORE_4_0 compile

Makes sense

spouliot · 2018-01-29T20:30:32Z

+		[Wrap ("CreateFaceGeometry (device)")]
+		ARSCNFaceGeometry Create (IMTLDevice device);
+
+		[Obsolete ("Use the 'Create' static constructor instead.")]


spouliot · 2018-01-29T20:31:21Z


+	[NoWatch, NoTV, NoMac, iOS (11,3)]
+	[BaseType (typeof(ARAnchor))]
+	interface ARImageAnchor {


the base type ARAnchor has a [DisableDefaultCtor] and I don't see any reason for this subclass not to have it ?

Mmh so ARAnchor.h has:

/** Initializes a new anchor object. @param transform The transformation matrix that defines the anchor’s rotation, translation and scale in world coordinates. */ - (instancetype)initWithTransform:(matrix_float4x4)transform; /** Unavailable */ - (instancetype)init NS_UNAVAILABLE; + (instancetype)new NS_UNAVAILABLE;

ARImageAnchor and ARFaceAnchor (the only 2 subclasses) only have:

/** Unavailable */ - (instancetype)initWithTransform:(matrix_float4x4)transform NS_UNAVAILABLE;

Nothing is specified for init and new but given that initWithTransform is unavailable the must still have 1 constructor left (:

yes, init* are methods in objc so they are inherited
and that's why we need to inline some init* in subclasses
still it means no init - it's the `initWithTransform1 that must be duplicated

Oh yea ok so indeed init and new are also unavailable in the subclasses because they are inherited so [DisableDefaultCtor] makes sense (btw that could be caught by a test no? Unless the subclass in ObjC sometimes re-expose init and new even it's marked as unavailable in the parent?).

Now I don't follow your conclusion because initWithTransform is explicitly marked as unavailable. Looks like this type is not meant to be created by the user.

VincentDondain · 2018-01-30T15:22:46Z

@spouliot @mandel-macaque @dalexsoto please re-review (:

monojenkins · 2018-01-30T16:50:40Z

Build success

dalexsoto

LGTM 👍

Sorry Manuel but I have to dismiss your review to merge this PR. I addressed your changes (they were non-blocking)

[arkit] Update for Xcode 9.3 beta 1

28bedb8

VincentDondain requested a review from spouliot as a code owner January 26, 2018 22:40

dalexsoto requested changes Jan 26, 2018

View reviewed changes

spouliot reviewed Jan 27, 2018

View reviewed changes

rolfbjarne approved these changes Jan 29, 2018

View reviewed changes

VincentDondain added 2 commits January 29, 2018 11:11

Merge remote-tracking branch 'origin/xcode9.3' into xcode9.3-arkit-b1

97bec98

Use 'Create' for ARSCNPlaneGeometry and ARSCNFaceGeometry

72f1f07

mandel-macaque previously requested changes Jan 29, 2018

View reviewed changes

spouliot suggested changes Jan 29, 2018

View reviewed changes

Fixes based on comments

248bee7

spouliot approved these changes Jan 30, 2018

View reviewed changes

dalexsoto approved these changes Jan 30, 2018

View reviewed changes

VincentDondain merged commit 4c25aa9 into dotnet:xcode9.3 Jan 30, 2018

VincentDondain deleted the xcode9.3-arkit-b1 branch January 30, 2018 22:09

Uh oh!

Conversation

VincentDondain commented Jan 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dalexsoto left a comment

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!

monojenkins commented Jan 27, 2018

Uh oh!

monojenkins commented Jan 29, 2018

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!

VincentDondain Jan 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

VincentDondain Jan 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

VincentDondain Jan 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

VincentDondain commented Jan 30, 2018

Uh oh!

monojenkins commented Jan 30, 2018

Uh oh!

dalexsoto left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

VincentDondain commented Jan 26, 2018 •

edited

Loading

VincentDondain Jan 29, 2018 •

edited

Loading

VincentDondain Jan 29, 2018 •

edited

Loading

VincentDondain Jan 29, 2018 •

edited

Loading