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.

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] Update framework to Xcode 12 beta 5. #9402

Merged
merged 5 commits into from Aug 21, 2020

Conversation

mandel-macaque
Copy link
Member

No description provided.

@mandel-macaque
Copy link
Member Author

Adding as a draft until we get the bump, but is a good way to move fwd while we wait for bots.

Copy link
Contributor

@spouliot spouliot left a comment

Choose a reason for hiding this comment

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

The .todo needs to be editing and it is not empty (previous beta?)

!missing-pinvoke! ARSkeletonJointNameForRecognizedPointKey is not bound

@monojenkins
Copy link
Collaborator

Build failure
Build failed or was aborted

Build succeeded
API Diff (from stable)

@monojenkins
Copy link
Collaborator

Build failure
Build failed or was aborted

@mandel-macaque mandel-macaque marked this pull request as ready for review August 20, 2020 15:59
public void UnknonwPointTest ()
{
using (var notKnownPoint = new NSString ("nariz"))
Assert.IsNull (ARSkeleton.CreateJointName (notKnownPoint));
Copy link
Member Author

Choose a reason for hiding this comment

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

Can't add the success until visio is done, tracked here: #9420

[Preserve (AllMembers = true)]
public class ARSkeletonTest {
[Test]
public void UnknonwPointTest ()
Copy link
Contributor

Choose a reason for hiding this comment

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

minor: typo

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
public void UnknonwPointTest ()
public void UnknownPointTest ()

[DllImport (Constants.ARKitLibrary)]
static extern IntPtr /* NSString */ ARSkeletonJointNameForRecognizedPointKey (/* NSString */ IntPtr recognizedPointKey);

[EditorBrowsable (EditorBrowsableState.Advanced)]
Copy link
Contributor

Choose a reason for hiding this comment

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

what's the normal alternative ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Use the enum from Visio or use the smart enum ARSkeletonJointName rather than create one from the key

Copy link
Contributor

Choose a reason for hiding this comment

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

I get that :-) but I mean what's the method/API alternative that the IDE won't try to hide from the developer ?

We normally use Advanced because there's a simpler/nicer alternative that we want developers to use (over the more complex one).

Copy link
Member Author

Choose a reason for hiding this comment

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

I need visio for that, I have create issue #9432

src/ARKit/ARSkeleton.cs Outdated Show resolved Hide resolved
nuint GetJointIndex (NSString? jointName);

[Wrap ("GetJointIndex (jointName.GetConstant()!)")]
nuint GetJointIndex (ARSkeletonJointName jointName);
Copy link
Contributor

Choose a reason for hiding this comment

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

what was the reason to remove [BindAs] ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because the new added C API allows to create new ARSkeletonJointName which are a typedef based on NSString, if we hide it then the new API makes so sense. The new binding exposes the NSString option as an advance option and then adds the wrap for our smart enum.

Copy link
Contributor

Choose a reason for hiding this comment

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

make sense, copy/paste that into the commit message before merging :)

src/arkit.cs Show resolved Hide resolved
src/arkit.cs Show resolved Hide resolved
src/arkit.cs Show resolved Hide resolved
@monojenkins
Copy link
Collaborator

Build failure
Build failed or was aborted

Build succeeded
API Diff (from stable)

src/ARKit/ARSkeleton.cs Outdated Show resolved Hide resolved
Co-authored-by: Rolf Bjarne Kvinge <rolf@xamarin.com>
@dalexsoto dalexsoto added this to the xcode12 milestone Aug 20, 2020
@dalexsoto dalexsoto added the note-highlight Worth calling out specifically in release notes label Aug 20, 2020
@monojenkins
Copy link
Collaborator

Build success
Build succeeded
API Diff (from stable)
⚠️ API Diff (from PR only) (🔥 breaking changes 🔥)
ℹ️ Generator Diff (please review changes)
Test run succeeded

@mandel-macaque mandel-macaque merged commit 91ce8b1 into xamarin:xcode12 Aug 21, 2020
@mandel-macaque mandel-macaque deleted the arkit-xcode12-beta5 branch August 21, 2020 14:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
note-highlight Worth calling out specifically in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants