Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

[GameController] Add Xcode 11 Beta 1-5 bindings#6589

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged

Conversation

@tj-devel709
Copy link
Member

No description provided.

@tj-devel709
Copy link
MemberAuthor

Here is a link to the issue:#6590
When adding attributes to the GCAcceleration, GCRotationRate, and GCQuaternion structs, it will not compile, but compiles fine without the attributes

@monojenkins
Copy link
Collaborator

Build failure
✅Build succeeded
✅API Diff (from stable)
ℹ️API Diff (from PR only) (please review changes)
ℹ️Generator Diff (please review changes)
🔥Test run failed 🔥

Test results

1 tests failed, 90 tests passed.

Failed tests

  • introspection/Mac Modern/Debug: Failed (Test run failed.)

usingFoundation;

namespaceGameController{
[Deprecated(PlatformName.MacOSX,10,15,message:"GCMicroGamepadSnapshot has been deprecated, use [GCController controllerWithMicroGamepad] instead")]
Copy link
Member

Choose a reason for hiding this comment

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

In deprecation messages that point elsewhere, we point to the managed name instead of the Objective-C name, since that's much more useful for our developers.

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Ah man looked right past that

publicfloat/* float_t = float */LeftTrigger;
publicfloat/* float_t = float */RightTrigger;
[iOS(12,2),Mac(10,14,4,onlyOn64:true),TV(12,2)]
publicboolsupportsClickableThumbsticks;
Copy link
Member

Choose a reason for hiding this comment

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

Public API shouldn't start with a lower-cased letter:SupportsClickableThumbsticks.

Same for the to other fields just below.

whitneyschmidt reacted with thumbs up emoji
Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Missed this one too, thanks!

// Removed the Headers for these structs because I could not compile with them.
// [iOS (13,0), Mac (10,15, onlyOn64: true), TV (13,0)]
publicstructGCAcceleration{

Copy link
Member

Choose a reason for hiding this comment

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

Not a big deal, but I think this looks better without a newline between each field.

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Changed!

GCControllerButtonInputRight{get;}

[Export("setValueForXAxis:yAxis:")]
voidSetValueForXAxis(floatxAxis,floatyAxis);
Copy link
Member

Choose a reason for hiding this comment

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

JustSetValue is enough here.

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, an ObjC API with two parameters looks likemethodParam1:Param2:
IOW there'sno: between the method name and the first parameter, so

  • method ->SetValueFor
  • parameter 1 ->xAxis
  • parameter 2 ->yAxis

and we remove suffix likeFor (andWith...) so it becomes

voidSetValue(floatxAxis,floatyAxis);

whitneyschmidt reacted with thumbs up emoji
Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Thanks, will change!

Copy link
Member

@rolfbjarnerolfbjarne left a comment

Choose a reason for hiding this comment

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

.


[Deprecated(PlatformName.MacOSX,10,15,message:"GCExtendedGamepadSnapshot has been deprecated, use [GCController capture] instead")]
[Deprecated(PlatformName.iOS,13,0,message:"GCExtendedGamepadSnapshot has been deprecated, use [GCController capture] instead")]
[Deprecated(PlatformName.TvOS,13,0,message:"GCExtendedGamepadSnapshot has been deprecated, use [GCController capture] instead")]
Copy link
Contributor

Choose a reason for hiding this comment

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

Hello, is this part of the messageGCExtendedGamepadSnapshot has been deprecated worth having ? I thought we just neededuse xxx instead

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, we can't use the text from header files
As much as possible (for direct replacements) we try to stick to this format:"Use 'XXX' instead.", whereXXX is themanaged name of the API providing the replacement

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Good catch! I will fix these, thanks!

Copy link
Contributor

@spouliotspouliot left a comment

Choose a reason for hiding this comment

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

Please review all deprecation messages to follow our format. Thanks!

// Buttons in the range [0.0, 1.0]
publicfloat/* float_t = float */LeftTrigger;
publicfloat/* float_t = float */RightTrigger;
[iOS(12,2),Mac(10,14,4,onlyOn64:true),TV(12,2)]
Copy link
Contributor

Choose a reason for hiding this comment

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

as#6595 will be merged soon please remove, onlyOn64: true from the PR

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Will do!

// [TV (12, 1), Mac (10, 14, 1, onlyOn64: true), iOS (12, 1)]
// public bool LeftThumbstickButton;
// [TV (12, 1), Mac (10, 14, 1, onlyOn64: true), iOS (12, 1)]
// public bool RightThumbstickButton;
Copy link
Contributor

Choose a reason for hiding this comment

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

^^^ please check this - it seems the API were not added for a reason
and the fact it's not[iOS (13,0]... is suspicious too

Copy link
MemberAuthor

@tj-devel709tj-devel709Jul 16, 2019•
edited
Loading

Choose a reason for hiding this comment

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

So for checking this, the link provided is dead. They also added the "SupportsClickableThumbsticks" which may have helped the functionality of those removed buttons. Where would you recommend checking if these should be here or not?

Copy link
Contributor

Choose a reason for hiding this comment

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

it's not dead - but you need to login (and have access) to trello to see it
Nothing much to see since Apple closed it as a duplicate. The original issue was that the structure was not backward compatible (since fields were added in the middle, not at end).

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Ah okay, so should I remove the LeftThumbsitckButton and RightThumbsitckButton but leave in the SuportsClickableThumbsticks?

Copy link
Contributor

Choose a reason for hiding this comment

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

no, if the new fields are present (in the headers) at the end of the structure then we must match the memory representation (so we must have the elements in the same order)

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Sorry I am a little confused. In the xcode11 headers, it seems that it should be in the order of SupportsClickableThumbsticks
LeftThumbstickButton
RightThumbstickButton.
So you are saying I should include these 3 and put them in this order?

Copy link
Contributor

Choose a reason for hiding this comment

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

I have not looked at the headers... but, since it's a structure, theymust match (same order, same size...) so that the code from both ObjC and C# can write to the (same) memory location (and make sense of what the other wrote)


namespaceGameController{

// Removed the Headers for these structs because I could not compile with them.
Copy link
Contributor

Choose a reason for hiding this comment

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

Removed the Headers ?
it looks likecommented the (availability) attributes

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Oops sorry, I need to get my vocab straight haha

spouliot reacted with laugh emoji
GCControllerButtonInputRight{get;}

[Export("setValueForXAxis:yAxis:")]
voidSetValueForXAxis(floatxAxis,floatyAxis);
Copy link
Contributor

Choose a reason for hiding this comment

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

yes, an ObjC API with two parameters looks likemethodParam1:Param2:
IOW there'sno: between the method name and the first parameter, so

  • method ->SetValueFor
  • parameter 1 ->xAxis
  • parameter 2 ->yAxis

and we remove suffix likeFor (andWith...) so it becomes

voidSetValue(floatxAxis,floatyAxis);

whitneyschmidt reacted with thumbs up emoji

[TV(13,0),Mac(10,15,onlyOn64:true),iOS(13,0)]
[Export("capture")]
GCControllerCapture{get;}
Copy link
Contributor

Choose a reason for hiding this comment

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

That sounds like a method - and not a property
Please check headers and docs (both ObjC and Swift)

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

This one, ControllerWithMicroGamepad, and ControllerWithExtendedGamepad were all methods but sharpie suggested changing them to properties since they just return a GCController, should I leave them as methods?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, they are not state (but actions) so they should be methods

side note: inold ObjC there was no syntax for property so sharpie does its best... but it's only a suggestion (and newer headers generally use@property so there's no/less need to guess properties).

whitneyschmidt reacted with eyes emoji
Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Okay thanks!

[TV(13,0),Mac(10,15,onlyOn64:true),iOS(13,0)]
[Static]
[Export("controllerWithMicroGamepad")]
GCControllerControllerWithMicroGamepad{get;}
Copy link
Contributor

Choose a reason for hiding this comment

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

No verb in method
I thinkGetMicroGamepadController is better

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Right now it is set as a property though and properties don't need a verb correct? I can revert to a method though.

Copy link
Contributor

Choose a reason for hiding this comment

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

It should be a method, as this is not a really a state of the current type.

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Changing!

[TV(13,0),Mac(10,15,onlyOn64:true),iOS(13,0)]
[Static]
[Export("controllerWithExtendedGamepad")]
GCControllerControllerWithExtendedGamepad{get;}
Copy link
Contributor

Choose a reason for hiding this comment

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

No verb in method
I thinkGetExtendedGamepadController () is better

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Same as above, Right now it is set as a property though and properties don't need a verb correct? I can revert to a method though.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, I was not clear - it should be a method (and have a verb)

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Changing!

@monojenkins
Copy link
Collaborator

Build failure
❌Build failed or was aborted

@monojenkins
Copy link
Collaborator

Build failure
❌Build failed or was aborted

[Deprecated(PlatformName.TvOS,13,0,message:"GCMicroGamepadSnapshot has been deprecated, use[GCControllercontrollerWithMicroGamepad] instead")]
[Deprecated(PlatformName.MacOSX,10,15,message:"Use[GCControllerControllerWithMicroGamepad] instead")]
[Deprecated(PlatformName.iOS,13,0,message:"Use[GCControllerControllerWithMicroGamepad] instead")]
[Deprecated(PlatformName.TvOS,13,0,message:"Use[GCControllerControllerWithMicroGamepad] instead")]
Copy link
Contributor

Choose a reason for hiding this comment

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

That[...] is the ObjC syntax - please use the managed name, like
Use 'GCController.GetMicroGamePadController()' instead.

note the. at the end :)

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Oh thank you

[Deprecated(PlatformName.MacOSX,10,15,message:"GCGamepad has been deprecated, use GCExtendedGamepad instead")]
[Deprecated(PlatformName.iOS,13,0,message:"GCGamepad has been deprecated, use GCExtendedGamepad instead")]
[Deprecated(PlatformName.TvOS,13,0,message:"GCGamepad has been deprecated, use GCExtendedGamepad instead")]
[Deprecated(PlatformName.MacOSX,10,15,message:"Use GCExtendedGamepad instead")]
Copy link
Contributor

Choose a reason for hiding this comment

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

need' around the type name
and. at the end of the string

@monojenkins
Copy link
Collaborator

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

Test results

1 tests failed, 90 tests passed.

Failed tests

  • introspection/Mac Modern/Debug: Failed (Test run failed.)

@monojenkins
Copy link
Collaborator

Build failure
❌Build failed or was aborted

🔥Build failed 🔥

@monojenkins
Copy link
Collaborator

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

Test results

1 tests failed, 90 tests passed.

Failed tests

  • introspection/Mac Modern/Debug: Failed (Test run failed.)

@monojenkins
Copy link
Collaborator

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

@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


[Deprecated(PlatformName.MacOSX,10,12,message:"Use GCExtendedGamepad instead.")]
[Deprecated(PlatformName.iOS,10,0,message:"Use GCExtendedGamepad instead.")]
[Deprecated(PlatformName.TvOS,10,0,message:"Use GCExtendedGamepad instead.")]
Copy link
Contributor

Choose a reason for hiding this comment

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

missing single' around the type name

[TV(13,0),Mac(10,15),iOS(13,0)]
[Static]
[Export("controllerWithMicroGamepad")]
GCControllerGetControllerWithMicroGamepad();
Copy link
Contributor

Choose a reason for hiding this comment

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

->GetMicroGamepadController sounds better

[TV(13,0),Mac(10,15),iOS(13,0)]
[Static]
[Export("controllerWithExtendedGamepad")]
GCControllerGetControllerWithExtendedGamepad();
Copy link
Contributor

Choose a reason for hiding this comment

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

->GetExtendedGamepadController sounds better


[Deprecated(PlatformName.MacOSX,10,15,message:"Use 'GCController.ControllerWithMicroGamepad()' instead.")]
[Deprecated(PlatformName.iOS,13,0,message:"Use 'GCController.ControllerWithMicroGamepad()' instead.")]
[Deprecated(PlatformName.TvOS,13,0,message:"Use 'GCControler.ControllerWithMicroGamepad()' instead.")]
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 not the managed name you have given (missingGet prefix)
Please review all attributes (since it was copy/pasted in many places)

// [TV (12, 1), Mac (10, 14, 1, onlyOn64: true), iOS (12, 1)]
// public bool LeftThumbstickButton;
// [TV (12, 1), Mac (10, 14, 1, onlyOn64: true), iOS (12, 1)]
// public bool RightThumbstickButton;
Copy link
Contributor

Choose a reason for hiding this comment

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

no, if the new fields are present (in the headers) at the end of the structure then we must match the memory representation (so we must have the elements in the same order)

@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

Copy link
Contributor

@whitneyschmidtwhitneyschmidt left a comment

Choose a reason for hiding this comment

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

👍 🎮 👾

// Buttons in the range [0.0, 1.0]
publicfloat/* float_t = float */LeftTrigger;
publicfloat/* float_t = float */RightTrigger;

Copy link
Contributor

Choose a reason for hiding this comment

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

Unneeded white space

@tj-devel709
Copy link
MemberAuthor

Okay this should be good now!@chamons@mandel-macaque@whitneyschmidt@spouliot@dalexsoto

@monojenkins
Copy link
Collaborator

Build failure
❌Build failed or was aborted

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

@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

[TV(12,2),Mac(10,14,4),iOS(12,2)]
publicboolRightThumbstickButton;

[Deprecated(PlatformName.MacOSX,10,15,message:"Use 'GCController.ControllerWithExtendedGamepad()' instead.")]
Copy link
Member

Choose a reason for hiding this comment

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

The managed name isGCController. GetExtendedGamepadController ()

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Thanks for catching this also!

// Apple inserted these in the middle of the struct previously so it broke the struct for backwards compatibility,
// but now they are at the end of the struct so they are allowed and do not prevent backwards compatibility
[iOS(12,2),Mac(10,14,4),TV(12,2)]
publicboolSupportsClickableThumbsticks;
Copy link
Member

Choose a reason for hiding this comment

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

I looked at the header, and these fields are not in theGCExtendedGamepadSnapShotDataV100 struct, they're only in theGCExtendedGamepadSnapshotData struct.

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Oh man big rookie mistake, thank you for catching this!

@tj-devel709tj-devel709force-pushed theTJ-xcode11-GameController branch from66e697c tofcaea88CompareJuly 29, 2019 14:58
@monojenkins
Copy link
Collaborator

Build failure
✅Build succeeded
✅API Diff (from stable)
ℹ️API Diff (from PR only) (please review changes)
ℹ️Generator Diff (please review changes)
🔥Test run failed 🔥

Test results

1 tests failed, 90 tests passed.

Failed tests

  • introspection/tvOS - simulator/Debug (tvOS 10.2): Failed

@tj-devel709tj-devel709 changed the title[GameController] Add Xcode 11 Beta 1-3 bindings[GameController] Add Xcode 11 Beta 1-5 bindingsJul 29, 2019
@monojenkins
Copy link
Collaborator

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

@tj-devel709tj-devel709 changed the title[GameController] Add Xcode 11 Beta 1-5 bindings[GameController] Add Xcode 11 Beta 1-4 bindingsJul 29, 2019
@mandel-macaque
Copy link
Contributor

There is a failing tests that might affect this PR that is unrelated:https://github.com/xamarin/maccore/issues/1903

@tj-devel709tj-devel709 changed the title[GameController] Add Xcode 11 Beta 1-4 bindings[GameController] Add Xcode 11 Beta 1-5 bindingsAug 5, 2019
@dalexsotodalexsoto merged commit9301deb intodotnet:xcode11Aug 5, 2019
spouliot pushed a commit to spouliot/xamarin-macios that referenced this pull requestAug 6, 2019
- Fix deprecation messages to point to an existing API;- Add setters for `Value` properties instead of adding `SetValue` methods;- Removing some `[TV (13,0)]` when the API was already available in previous tvOS versions. It's implied that type that existed in tvOS 9.0 (the first version) do not have (nor need) availability attributes. Decorating them as `13,0` sends an incorrect message to developers (since the API have been available for a while)- Adding some `[TV (13,0)]` when a new API only mention iOSreference:dotnet#6589
spouliot added a commit that referenced this pull requestAug 7, 2019
- Fix deprecation messages to point to an existing API;- Add setters for `Value` properties instead of adding `SetValue` methods;- Removing some `[TV (13,0)]` when the API was already available in previous tvOS versions. It's implied that type that existed in tvOS 9.0 (the first version) do not have (nor need) availability attributes. Decorating them as `13,0` sends an incorrect message to developers (since the API have been available for a while)- Adding some `[TV (13,0)]` when a new API only mention iOSreference:#6589
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@dalexsotodalexsotodalexsoto approved these changes

@VincentDondainVincentDondainAwaiting requested review from VincentDondain

@spouliotspouliotAwaiting requested review from spouliot

@rolfbjarnerolfbjarneAwaiting requested review from rolfbjarne

+3 more reviewers

@chamonschamonschamons left review comments

@jocontejocontejoconte left review comments

@whitneyschmidtwhitneyschmidtwhitneyschmidt approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@tj-devel709@monojenkins@mandel-macaque@dalexsoto@rolfbjarne@spouliot@chamons@joconte@whitneyschmidt
[8]ページ先頭
©2009-2025 Movatter.jp