Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[arkit] Update for Xcode 9 GM #2681

Merged
merged 3 commits into from
Sep 14, 2017
Merged

[arkit] Update for Xcode 9 GM #2681

merged 3 commits into from
Sep 14, 2017

Conversation

VincentDondain
Copy link
Contributor

No description provided.

rolfbjarne
rolfbjarne requested changes Sep 13, 2017
View reviewed changes
Copy link
Member

@rolfbjarne rolfbjarne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One minor thing, otherwise 👍.

src/arkit.cs Outdated
[Export ("vertexCount")]
nuint VertexCount { get; }

// @property (readonly, nonatomic) const vector_float3 * _Nonnull vertices;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment can be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup missed. I'll remove in next PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rolfbjarne that's minor though, not PR blocking, I'll fix in next PR.

spouliot
spouliot suggested changes Sep 13, 2017
View reviewed changes
src/arkit.cs

[iOS (11,0)]
[NoWatch, NoTV, NoMac]
[BaseType (typeof(ARLightEstimate))]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

base type has a [DisableDefaultCtor] so the subclass should too

Copy link
Contributor Author

@VincentDondain VincentDondain Sep 13, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right ARDirectionalLightEstimate subclass doesn't have init

src/arkit.cs

[iOS (11,0)]
[NoWatch, NoTV, NoMac]
[BaseType (typeof(ARConfiguration))]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there's a [DisableDefaultCtor] on the base type
but in this case it might be valid but...
The abstract base class for AR session configurations. https://developer.apple.com/documentation/arkit/arconfiguration

so there's an [Abstract] missing from ARConfiguration

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ARConfiguration should indeed be [Abstract] and [DisableDefaultCtor] is right here (:

/** Unavailable */
- (instancetype)init NS_UNAVAILABLE;
+ (instancetype)new NS_UNAVAILABLE;

@monojenkins
Copy link
Collaborator

Build success

spouliot
spouliot suggested changes Sep 13, 2017
View reviewed changes
src/arkit.cs Outdated
@@ -505,6 +507,7 @@ interface ARWorldTrackingConfiguration {
[iOS (11,0)]
[NoWatch, NoTV, NoMac]
[BaseType (typeof(ARConfiguration))]
[DisableDefaultCtor]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry if it was confusing ;-)
but in this case it might be valid but...
meant that I did not knew if init made sense - because the base type did not have it but was abstract
It turns out it's valid
https://developer.apple.com/documentation/arkit/arorientationtrackingconfiguration/2923556-init?language=objc

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah sorry I misinterpreted that and thought you checked (:

src/arkit.cs Outdated
@@ -531,6 +534,7 @@ interface ARTrackable {
[iOS (11,0)]
[NoWatch, NoTV, NoMac]
[BaseType (typeof(ARConfiguration))]
[DisableDefaultCtor]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

double check headers/doc - I suspect it's available

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes https://developer.apple.com/documentation/arkit/arfacetrackingconfiguration/2928280-init?changes=latest_beta

src/arkit.cs Outdated
@@ -496,6 +497,7 @@ interface ARConfiguration : NSCopying {
[iOS (11,0)]
[NoWatch, NoTV, NoMac]
[BaseType (typeof (ARConfiguration))]
[DisableDefaultCtor]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

double check headers/doc - I suspect it's available

@monojenkins
Copy link
Collaborator

Build failure

@monojenkins
Copy link
Collaborator

Build failure

@spouliot
Copy link
Contributor

aborted build after 4 hours

@spouliot
Copy link
Contributor

build

@monojenkins
Copy link
Collaborator

Build success

@spouliot spouliot merged commit 05dde4c into xamarin:xcode9 Sep 14, 2017
@VincentDondain VincentDondain mentioned this pull request Jan 29, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rolfbjarne rolfbjarne rolfbjarne approved these changes

@spouliot spouliot spouliot approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@VincentDondain @monojenkins @spouliot @rolfbjarne @dnfclas