Skip to content

Conversation

@jfreire-unity
Copy link
Collaborator

@jfreire-unity jfreire-unity commented Feb 19, 2025

Description

Fixes https://jira.unity3d.com/browse/ISXB-810.

Dpad presses for up-left were not being detected with IsPressed() function because we always compare it against their default state first to avoid calling the EvaluateMagnitude() virtual method.

On Xbox Controllers that I tested with (Xbox One Conteoller, Model 1708, Bluetooth), their default state is 0 for Dpad values. And byte value 8 was being considered as such before, which in fact corresponds to Dpad up-left presses. This PR fixes that.

Testing status & QA

@stefanunity and @Pauliusd01 could you test this issue all the Xbox controllers you can find, with both Wired and Wireless connections? Just to make sure there are no devices with a different default state for DPAD presses.

Overall Product Risks

Please rate the potential complexity and halo effect from low to high for the reviewers. Note down potential risks to specific Editor branches if any.

  • Complexity: 0
  • Halo Effect: 0

Comments to reviewers

There's a chance that at some point the default DPAD state was 8 but I can't really tell as I haven't used that much Xbox controllers on macOS.
I did validate that Dualshock 4 uses value 8 as the default state for DPAD.

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.

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, I wonder though if this changed with some Xbox firmware update or if the backend changed behavior or if this has always been broken?

Copy link
Collaborator

@Pauliusd01 Pauliusd01 left a comment

Choose a reason for hiding this comment

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

LGTM, all controls worked fine in play mode and player. Although I only had 1 controller that I checked with (1914) and only on bluetooth as wired isn't supported there natively

[InputControl(name = "rightTrigger", format = "USHT", parameters = "normalize,normalizeMin=0,normalizeMax=0.01560998")]
[FieldOffset(11)] public ushort rightTrigger;

[InputControl(name = "dpad", format = "BIT", layout = "Dpad", sizeInBits = 4, defaultState = 8)]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a history of this (defaultState = 8)? Why was this implemented like this in the first place? I think the change makes totally sense if this is just derived from another control.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I didn't find any history in this.
I'm not sure this was a copy paste issue from controls such as the DualShock controllers who actualkly have default state = 8. I guess we need to wait and see if there are any Xbox controllers with such default state...

@jfreire-unity jfreire-unity force-pushed the isxb-810-fix-defaultstate-dpad-xboxgamepadmacos branch from 22dd02a to f8d8270 Compare March 18, 2025 08:21
@jfreire-unity
Copy link
Collaborator Author

Changes look good to me, I wonder though if this changed with some Xbox firmware update or if the backend changed behavior or if this has always been broken?

I cannot answer if it was a firmware update and didn't find any history regarding this. But there's a chance that have might been the case. Let's monitor this.
But I also wouldn't completely disregard this as always being broken, as it only happens for IsPressed() calls. But I'm just guessing here.

The default state for zero value of sticks was not being set through default state.

We only set a defaultState if there's at least a control with defaultState set. Otherwise, we seem to be bypassing it. For some reason, it seems we were deriving the default state of the sticks through the parameters control have but I cannot say for sure without spending much more time on this.
This test was previously enabled for 2021 LTS only due to a bug. But the bug was already fixed so I decided to make sure this runs on all Unity versions.
@jfreire-unity jfreire-unity force-pushed the isxb-810-fix-defaultstate-dpad-xboxgamepadmacos branch from cfd7623 to f290685 Compare March 28, 2025 15:10
@jfreire-unity jfreire-unity merged commit e6254ab into develop Mar 31, 2025
110 checks passed
@jfreire-unity jfreire-unity deleted the isxb-810-fix-defaultstate-dpad-xboxgamepadmacos branch March 31, 2025 10:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants