FIX: Correct north, east, south, west bindings for Switch Pro Controller to match the physical layout of the device #2208
Conversation
… buttons Also includes new state, for any devices which use the Nintendo-y button bindings
vallerieknight-unity
changed the title
vallerieknight-unity
changed the title
Codecov ReportAttention: Patch coverage is
@@ Coverage Diff @@
## develop #2208 +/- ##
===========================================
+ Coverage 68.11% 68.12% +0.01%
===========================================
Files 367 367
Lines 53603 53623 +20
===========================================
+ Hits 36512 36531 +19
- Misses 17091 17092 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes 🚀 New features to boost your workflow:
|
vallerieknight-unity
changed the title
vallerieknight-unity
changed the title
|
iOS CI issues appear to be instabilities also occurring on head of |
| /// <summary> | ||
| /// A Button for a Nintendo Switch Pro Controller. | ||
| /// If querying via script, ensure you cast the device to SwitchProControllerHID, rather than using the Gamepad class. | ||
| /// The gamepad class will return the state of buttonSouth, whereas this class returns the state of buttonEast | ||
| /// </summary> | ||
| public new ButtonControl aButton => buttonEast; | ||
|
|
||
| /// <summary> | ||
| /// A Button for a Nintendo Switch Pro Controller. | ||
| /// If querying via script, ensure you cast the device to SwitchProControllerHID, rather than using the Gamepad class. | ||
| /// The gamepad class will return the state of buttonEast, whereas this class returns the state of buttonSouth | ||
| /// </summary> | ||
| public new ButtonControl bButton => buttonSouth; | ||
|
|
||
| /// <summary> | ||
| /// A Button for a Nintendo Switch Pro Controller. | ||
| /// If querying via script, ensure you cast the device to SwitchProControllerHID, rather than using the Gamepad class. | ||
| /// The gamepad class will return the state of buttonNorth, whereas this class returns the state of buttonWest | ||
| /// </summary> | ||
| public new ButtonControl yButton => buttonWest; | ||
|
|
||
| /// <summary> | ||
| /// A Button for a Nintendo Switch Pro Controller. | ||
| /// If querying via script, ensure you cast the device to SwitchProControllerHID, rather than using the Gamepad class. | ||
| /// The gamepad class will return the state of buttonWest, whereas this class returns the state of buttonNorth | ||
| /// </summary> | ||
| public new ButtonControl xButton => buttonNorth; | ||
|
|
There was a problem hiding this comment.
This is a change impacting all platforms, right? The Changelog gives the idea we only changed this for iOS,.
There was a problem hiding this comment.
Ah, that's a good point; thanks! If it sounds OK to you, I can add a new changelog item to reference the aliases being corrected and state that this is updated for both iOS and standalone.
There was a problem hiding this comment.
Just added new changelog item to help clear up why this was fixed here. If you think this should be a second PR, let me know - I combined it together, just as the actual issue there impacts both iOS & Standalone :)
| /// A Switch Pro Controller connected to an iOS device. | ||
| /// If you use InputSystem.GetDevice, you must query for this class rather than Gamepad in order for aButton, bButton, yButton and xButton to be correct | ||
| /// </summary> | ||
| [InputControlLayout(stateType = typeof(iOSGameControllerStateSwappedFaceButtons), displayName = "iOS Switch Pro Controller Gamepad")] | ||
| public class SwitchProControlleriOS : Gamepad |
There was a problem hiding this comment.
Do we need a new state? I wonder if we could actually create a similar pattern of what we do here
Because the remapping seems to be done below, as we already did in SwitchProControllerHID.cs.
Maybe we would need to create a SwitchProController class. Which would be inherited by SwitchProControllerHID and SwitchProControlleriOS.
There was a problem hiding this comment.
Currently, if you query buttonSouth, you'll get the A button's state, despite the fact buttonSouth is on the east on a Pro Controller. The new state maps this all correctly, such that buttonSouth will actually return the south face button.
The changes below are intended for situations when you want a button by label, regardless of where it is on the controller (e.g. some users will query this, and their UI in a tutorial will show "Press (A)").
If it makes this easier to land currently, I can split the changes to update aButton => buttonEast and friends into a new PR. This PR would then only focus on getting in the new state that rotates around the buttons for the Pro Controller.
Also agreed on the SwitchProController class as a root class; I'm just about to log off for the weekend, so I can make that change on Monday.
There was a problem hiding this comment.
Just pushed change that adds new SwitchProController device :)
There was a problem hiding this comment.
@vallerieknight-unity asking a lot here but a refactor like the SwitchProController for DualSense, DualShock, Xbox etc. is also much needed. We have so much boiler plate code as we otherwise get errors that specific controller features are not available on platform XY. We would expect that this is handled by the input system with an overall class per controller. Then if feature XY is not available on target XY it just does not perform the action and does not throw compile errors (e.g., PlayStation light bar). The same goes for creating actions and having to manually add targets to it due to different controller classes per platform. See the attached action we have for the select button on controllers to open a journal. You will notice we need several of the same controllers in there to make it work on all platforms the controller is used. Would bring a huge QoL improvement to using the input system.
Description
aButton,bButton,zButton,yButtonbinding to be correct for the Switch Pro ControllerTesting status & QA
Overall Product Risks
iOS : This could break som projects if the project already has mitigations for this behaviour, but, the behaviour was incorrect previously, and customers in this position can disable the new device.
Standalone: Less likely to break customer projects, but, could break UI / tutorialisation if a user is assuming
.aButtonis always the same as button south. Happy to revert the standalone change, but if you are a Unity employee, please reach out on Slack as I can explain this decision making process further.Comments to reviewers
See above notes on some potential upgrade pain - would be good to have some feedback / thoughts from other folks on both of these risks.
Checklist
Before review:
Changed,Fixed,Addedsections.Area_CanDoX,Area_CanDoX_EvenIfYIsTheCase,Area_WhenIDoX_AndYHappens_ThisIsTheResult.During merge:
NEW: ___.FIX: ___.DOCS: ___.CHANGE: ___.RELEASE: 1.1.0-preview.3.After merge: