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

FIX: Set macOS Xbox wireless gamepad button mappings based on PID/VID #2097

Merged
merged 6 commits into from
Jan 20, 2025
Merged

FIX: Set macOS Xbox wireless gamepad button mappings based on PID/VID #2097

merged 6 commits into from
Jan 20, 2025

Conversation

jfreire-unity
Copy link
Collaborator

@jfreire-unity jfreire-unity commented Dec 20, 2024 •
edited
Loading

Description

This PR fixes https://jira.unity3d.com/browse/ISXB-1264 by mapping "old" Xbox One wireless controllers based on their PID and VID.

Only for macOS since Windows uses XInput.

I also added the Version field to the Input Debugger device view. There might be some devices that get their firmware changed without a PID and VID change, so this could help us track down those devices.

Testing status & QA

Tested with a Xbox One wireless device which maps to a struct equivalent to Xbox Series.

More devices should be tested with, especially the ones that caused the ISS bug cc @ieuanrees .

Overall Product Risks

Unclear what other combinations of PID/VID we are missing.

  • Complexity: Minimal
  • Halo Effect: Low

Comments to reviewers

In general, I don't like this approach since I'm creating a lot of duplication. But I didn't find a better approach with the logic we have.

Also, we could probably expand the coverage by using this database. But I prefer for users to report their devices to us as we don't know if all those devices are still up to date.

A better alternative would be to stop using HID and rely on Apple Game Controller framework. But that requires platform code changes.

Checklist

Before review:

  • Changelog entry added.
    • Explains the change in Changed, Fixed, Added sections.
    • For API change contains an example snippet and/or migration example.
    • JIRA ticket linked, example (case %%). If it is a private issue, just add the case ID without a link.
    • Jira port for the next release set as "Resolved".
  • Tests added/changed, if applicable.
    • Functional tests Area_CanDoX, Area_CanDoX_EvenIfYIsTheCase, Area_WhenIDoX_AndYHappens_ThisIsTheResult.
    • Performance tests.
    • Integration tests.
  • Docs for new/changed API's.
    • Xmldoc cross references are set correctly.
    • Added explanation how the API works.
    • Usage code examples added.
    • The manual is updated, if needed.

During merge:

  • Commit message for squash-merge is prefixed with one of the list:
    • NEW: ___.
    • FIX: ___.
    • DOCS: ___.
    • CHANGE: ___.
    • RELEASE: 1.1.0-preview.3.

After merge:

  • Create forward/backward port if needed. If you are blocked from creating a forward port now please add a task to ISX-1444.

@jfreire-unity jfreire-unity force-pushed the isxb-1264-fix-different-xbox-macos-different-button-mapping branch from 7da428c to 41d5a3f Compare December 20, 2024 12:47
@jfreire-unity jfreire-unity force-pushed the isxb-1264-fix-different-xbox-macos-different-button-mapping branch from 59e6563 to 90f2587 Compare January 14, 2025 15:52
@jfreire-unity jfreire-unity marked this pull request as ready for review January 14, 2025 15:58
@jfreire-unity jfreire-unity changed the title FIX: WIP Set macOS Xbox gamepad based on PID/VID FIX: Set macOS Xbox gamepad based on PID/VID Jan 14, 2025
@jfreire-unity jfreire-unity changed the title FIX: Set macOS Xbox gamepad based on PID/VID FIX: Set macOS Xbox wireless gamepad button mappings based on PID/VID Jan 14, 2025
ritamerkl
ritamerkl approved these changes Jan 16, 2025
View reviewed changes
Copy link
Collaborator

@ritamerkl ritamerkl left a comment

Choose a reason for hiding this comment

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

Looks good to me - just one minor comment about the Assert message.

Packages/com.unity.inputsystem/InputSystem/Plugins/XInput/XboxGamepadMacOS.cs Outdated

public XInputControllerWirelessOSXStateV2 WithButton(Button button)
{
Debug.Assert((int)button < 32, $"Expected button < 32, so we fit into the 32 bit wide bitmask");
Copy link
Collaborator

Choose a reason for hiding this comment

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

This message is a bit odd and misses the unit "button < 32," here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agree, "A maximum of 32 buttons is supported for this layout". Which anyway can only happen if casting an integer (Out of valid range) into button and calling this function? So basically its an ArgumentOutOfRange exception.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah good point. This was essentially copy pasted.
I changed the messages but left it as Debug.Assert as I think this will be something to be caught in Debug builds.

Packages/com.unity.inputsystem/InputSystem/Plugins/XInput/XboxGamepadMacOS.cs Show resolved Hide resolved
Packages/com.unity.inputsystem/InputSystem/Editor/Debugger/InputDeviceDebuggerWindow.cs Show resolved Hide resolved
ekcoh
ekcoh approved these changes Jan 17, 2025
View reviewed changes
Copy link
Collaborator

@ekcoh ekcoh left a comment

Choose a reason for hiding this comment

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

Changes look good to me and thanks for expanding the test coverage. Its a bit unfortunate we need to add a whole class to fix something like this though.

Packages/com.unity.inputsystem/InputSystem/Plugins/XInput/XboxGamepadMacOS.cs Show resolved Hide resolved
ekcoh and others added 6 commits January 20, 2025 14:52
@ekcoh @jfreire-unity
Set different Xbox button mapping based on VID/PID
2d78745 
Only for macOS since Windows uses XInput.
Uses some PIDs sourced from SDL2 database.
Adds automated test to establish layout mapping.

We will need to update this based on reports from users in the future.
@jfreire-unity
Show HID version in Input Debugger device window
17b9431 
Will help track down devices that might have
a firmware version update that changes the
button bit mapping structure while maintaining
the same PID.
@jfreire-unity
Update CHANGELOG
6ae6742
@jfreire-unity
Fix test
fd28fa2
@jfreire-unity
Guard method with macOS platform define
96a8d1b
@jfreire-unity
Changed Debug.Assert message
2e68145
@jfreire-unity jfreire-unity force-pushed the isxb-1264-fix-different-xbox-macos-different-button-mapping branch from 217bedd to 2e68145 Compare January 20, 2025 13:52
stefanunity
stefanunity approved these changes Jan 20, 2025
View reviewed changes
Copy link
Collaborator

@stefanunity stefanunity left a comment

Choose a reason for hiding this comment

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

@Pauliusd01 says: Tried: Xbox, PS5, PS4 controllers on Win11 and MacOS player/playmode visualiser as well as the debugger. Seemed fine to me.

Also checked with Xbox pad I have and looked good.

@jfreire-unity jfreire-unity merged commit 29ca1f3 into develop Jan 20, 2025
77 checks passed
@jfreire-unity jfreire-unity deleted the isxb-1264-fix-different-xbox-macos-different-button-mapping branch January 20, 2025 16:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ekcoh ekcoh ekcoh approved these changes

@ritamerkl ritamerkl ritamerkl approved these changes

@stefanunity stefanunity stefanunity approved these changes

@ieuanrees ieuanrees Awaiting requested review from ieuanrees

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

Successfully merging this pull request may close these issues.
4 participants
@jfreire-unity @ekcoh @stefanunity @ritamerkl