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] Fix 'Vertices', 'TextureCoordinates' and 'TriangleIndices' in 'ARFaceGeometry' #3090

Merged
merged 3 commits into from Dec 12, 2017
Merged

[arkit] Fix 'Vertices', 'TextureCoordinates' and 'TriangleIndices' in 'ARFaceGeometry' #3090

merged 3 commits into from Dec 12, 2017

Conversation

VincentDondain
Copy link
Contributor

@VincentDondain VincentDondain commented Dec 8, 2017 •
edited

  • 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 ().

@VincentDondain
[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.
@VincentDondain VincentDondain added the do-not-merge Do not merge this pull request label Dec 8, 2017
@VincentDondain
Copy link
Contributor Author

Need to address ARFaceGeometry's vertices and textureCoordinates too.

They're vector_float3 * and vector_float2 *

@spouliot spouliot self-requested a review December 10, 2017 20:28
spouliot
spouliot suggested changes Dec 10, 2017
View reviewed changes
src/arkit.cs Outdated
@@ -850,8 +850,17 @@ interface ARFaceGeometry : NSCopying {
[Export ("triangleCount")]
nuint TriangleCount { get; }

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

Choose a reason for hiding this comment

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

-> TriangleIndexes

src/ARKit/ARFaceGeometry.cs Outdated
var rv = new short [count];
var ptr = (short *) GetTriangleIndexes ();
for (int i = 0; i < count; i++)
rv [i] = *ptr++;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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).

@VincentDondain VincentDondain removed the do-not-merge Do not merge this pull request label Dec 11, 2017
@VincentDondain
[arkit] Fix 'Vertices' and 'TextureCoordinates' too
0334f1e 
- Obsolete `Vector3 Vertices`.
- Obsolete `Vector2 TextureCoordinates`.
- Introduced new `Vector3 [] GetVertices ()`.
- Introduced new `Vector2 [] GetTextureCoordinates ()`.
@VincentDondain VincentDondain force-pushed the arfacegeometry-triangleindices-fix branch from 4da24a2 to 0334f1e Compare December 11, 2017 19:48
spouliot
spouliot suggested changes Dec 11, 2017
View reviewed changes
src/arkit.cs Outdated
[Export ("vertices")]
[Sealed] // Just to avoid the duplicate selector error
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

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 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.

src/ARKit/ARFaceGeometry.cs Outdated
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 () {
Copy link
Contributor

Choose a reason for hiding this comment

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

minor: style { on next line

src/ARKit/ARFaceGeometry.cs Outdated
}

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

Choose a reason for hiding this comment

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

same

src/ARKit/ARFaceGeometry.cs Outdated
}

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

Choose a reason for hiding this comment

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

same

src/arkit.cs Outdated
[Export ("textureCoordinates")]
[Sealed] // Just to avoid the duplicate selector error
Copy link
Contributor

Choose a reason for hiding this comment

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

same

src/arkit.cs Outdated
[Export ("triangleIndices")]
[Sealed] // Just to avoid the duplicate selector error
Copy link
Contributor

Choose a reason for hiding this comment

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

same

@spouliot
Copy link
Contributor

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

@VincentDondain VincentDondain changed the title [arkit] Fix TriangleIndices to return 'short []' [arkit] Fix 'Vertices', 'TextureCoordinates' and 'TriangleIndices' in 'ARFaceGeometry' Dec 11, 2017
spouliot
spouliot approved these changes Dec 11, 2017
View reviewed changes
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.

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

@monojenkins
Copy link
Collaborator

Build success

@spouliot
Copy link
Contributor

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

Status is broken :(

@VincentDondain VincentDondain merged commit 8899158 into xamarin:master Dec 12, 2017
@VincentDondain VincentDondain deleted the arfacegeometry-triangleindices-fix branch December 12, 2017 17:33
VincentDondain added a commit to VincentDondain/xamarin-macios that referenced this pull request Jan 26, 2018
@VincentDondain
[arkit] Fix 'Vertices', 'TextureCoordinates' and 'TriangleIndices' in…
f893e40 
… 'ARFaceGeometry' (xamarin#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 ()`.
VincentDondain added a commit that referenced this pull request Jan 29, 2018
@VincentDondain
[backport][arkit] Fix 'Vertices', 'TextureCoordinates' and 'TriangleI…
5cc720f 
…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 ()`.
@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

@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.

None yet
5 participants
@VincentDondain @spouliot @monojenkins @dnfadmin @dnfclas