Bug 206033 – Standard gamepad mapping for GameControllerGamepads

• Component: WebKit2
• Status: RESOLVED FIXED

[James Howard]

The gamepad spec defines a standard mapping for certain gamepads[1]. As of iOS 13/macOS 10.15, extended gamepads via GameController.framework can be mapped in a straightforward manner to the standard mapping.

There are two benefits to moving these gamepads to the standard mapping, vs the ad-hoc empty string mapping they have now:

1. Buttons which are currently inaccessible in the current implementation will become accessible (thumbstick buttons, start and select buttons, and the home/guide button).
2. Web applications that recognize or require the standard mapping will start working.

[1] https://www.w3.org/TR/gamepad/#remapping

[James Howard]

I have a patch, which I will attach as soon as I can correctly follow the steps to do so.

[James Howard]

Created attachment 387272 [details]
Patch

[James Howard]

Created attachment 387406 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/58504913>

[Dean Jackson]

Comment on attachment 387406 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=387406&action=review

Almost there. I'd like to see some comments in the change log, and rationale for including the home button.

> Source/WebCore/ChangeLog:7
> +        Standard gamepad mapping for GameControllerGamepads
> +        https://bugs.webkit.org/show_bug.cgi?id=206033
> +
> +        Reviewed by NOBODY (OOPS!).
> +

Please explain what you did in the patch here, and why.

For example, I see you swapped some axis mappings. Was that a bug?

Also, it seems you added the mapping field, but we only support the canonical standard mapping. Do we plan to add labels for the types of controllers we do support?

> Source/WebCore/platform/gamepad/cocoa/GameControllerGamepad.mm:36
> +@interface GCExtendedGamepad (ButtonHome)
> +
> +@property (readonly, nonatomic) GCControllerButtonInput *_buttonHome; // Internal in macOS 10.15, iOS 13.0
> +
> +@end

I don't think we should expose this. It's currently reserved for system use. Is there a good reason you want it?

> Source/WebCore/platform/gamepad/cocoa/GameControllerGamepad.mm:105
> +    if (@available(macOS 10.15.0, iOS 13.0, *)) {

We compile per-platform, so you'll have to use:
#if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101500) || (PLATFORM(IOS_FAMILY) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 130000)

> Source/WebCore/platform/gamepad/cocoa/GameControllerGamepad.mm:111
> +    if (@available(macOS 10.14.1, iOS 12.1, *)) {

Ditto here? Did they really only get added in a .1 version? (checks notes.... they did but I think it will be ok to just use  __MAC_OS_X_VERSION_MIN_REQUIRED >= 101400 since we compile with a newer SDK)

Note that you don't have to have the iOS clause because iOS 12 is the minimum version we support at the moment.

> Source/WebCore/platform/gamepad/cocoa/GameControllerGamepad.mm:118
> +    // Home/Guide
> +    if ([m_extendedGamepad.get() respondsToSelector:@selector(_buttonHome)])
> +        bindButton(m_extendedGamepad.get()._buttonHome, 16);

Noted above: I don't think we should expose this.

[James Howard]

Created attachment 387596 [details]
Patch

[James Howard]

Hey dino! Thanks for the review! I've attached an updated patch incorporating your feedback.

A couple of notes:

> For example, I see you swapped some axis mappings. Was that a bug?

I updated the changelog entry to cover this, but in brief, when using the empty string gamepad mapping, anything goes in terms of axis coordinate systems. When using the "standard" mapping, however, the expectation is that the Y axis is treated as -1 = up, 1 = down [1], which is flipped from what GCControllerAxisInput provides.

> rationale for including the home button

My initial rationale was that the button exists on popular gamepads, the "standard" mapping reserves an index (16) in the buttons array for it, and Chrome on Mac does map this button when using Xbox and PlayStation gamepads. 

However, the spec is a little vague on this point [2]. It states that "up to 17 buttons" can be included, and while the binding is shown in the standard_gamepad.svg, the table in the remapping section doesn't list it.

Given that it's not an API on iOS, and that it is a button that is often reserved for system use elsewhere, I removed it from the patch and left the buttons array at 16 elements only, rather than 17.

I do worry a little bit about web apps that assume that the buttons array will always be 17 elements long, so maybe there is a case to be made that the 16th index should be present but just always 0, but I didn't do that for now. It's an easy change to make if we want to do that.

[1] https://w3c.github.io/gamepad/standard_gamepad.svg
[2] https://w3c.github.io/gamepad/#remapping

[James Howard]

> Also, it seems you added the mapping field, but we only support the canonical standard mapping. 

There are two supported mappings, "" and "standard" [1]. Prior to my patch, WebKit only offered the empty string mapping. After my patch, we continue to provide empty string mapping for gamepads whose physical layout we're either unaware of, or that don't correspond to the standard layout (HID gamepads on Mac, micro gamepads on iOS). For now, I'm just providing the standard mapping for the GCExtendedGamepads, because it's very straightforward.

As for what we should do for Mac, we could start recognizing certain vendor / product ID tuples in HIDGamepad and do the standard mapping for them. We'd end up having to bundle some sort of database of these controllers, similar to what SDL does for instance. The other alternative would be to make GameController the preferred API for gamepads on macOS 10.15+ and move off of the HIDGamepad implementation. Personally, I like the simplicity of using GameController.framework as it pushes the controller database issue off to them and unifies the code between iOS and Mac. I'm happy to contribute either approach in a follow-up patch if you have a stance on which approach you'd like to see.

> Do we plan to add labels for the types of controllers we do support?

I'm not sure what you mean. Gamepads have an id field, which is just a string identifier. With GameController.framework examples of these are "Nimbus Extended Gamepad" or "Xbox Wireless Controller Extended Gamepad". I didn't change that in this patch. In terms of the mapping, there are currently just two in the spec: "" and "standard" [1].

[1] https://w3c.github.io/gamepad/#gamepadmappingtype-enum

[James Howard]

> Did they really only get added in a .1 version? (checks notes.... they did but I think it will be ok to just use  __MAC_OS_X_VERSION_MIN_REQUIRED >= 101400 since we compile with a newer SDK)

GCExtendedGamepad.h:108:68: note: 'leftThumbstickButton' has been marked as being introduced in macOS 10.14.1 here, but the deployment target is macOS 10.14.0

Looks like we weren't able to get away with 101400 :/

How should I handle this? If we can't use the @available approach, then pragma disable Wunguarded-availability-new and then check respondsToSelector for these?

[James Howard]

Created attachment 387677 [details]
Patch

[Dean Jackson]

(In reply to James Howard from comment #8)
> > Also, it seems you added the mapping field, but we only support the canonical standard mapping. 
> 
> There are two supported mappings, "" and "standard" [1]. Prior to my patch,
> WebKit only offered the empty string mapping. After my patch, we continue to
> provide empty string mapping for gamepads whose physical layout we're either
> unaware of, or that don't correspond to the standard layout (HID gamepads on
> Mac, micro gamepads on iOS). For now, I'm just providing the standard
> mapping for the GCExtendedGamepads, because it's very straightforward.
> 
> As for what we should do for Mac, we could start recognizing certain vendor
> / product ID tuples in HIDGamepad and do the standard mapping for them. We'd
> end up having to bundle some sort of database of these controllers, similar
> to what SDL does for instance. The other alternative would be to make
> GameController the preferred API for gamepads on macOS 10.15+ and move off
> of the HIDGamepad implementation. Personally, I like the simplicity of using
> GameController.framework as it pushes the controller database issue off to
> them and unifies the code between iOS and Mac. I'm happy to contribute
> either approach in a follow-up patch if you have a stance on which approach
> you'd like to see.

I agree that using GameController.framework is best.

> > Do we plan to add labels for the types of controllers we do support?
> 
> I'm not sure what you mean. Gamepads have an id field, which is just a
> string identifier. With GameController.framework examples of these are
> "Nimbus Extended Gamepad" or "Xbox Wireless Controller Extended Gamepad". I
> didn't change that in this patch. In terms of the mapping, there are
> currently just two in the spec: "" and "standard" [1].
> 
> [1] https://w3c.github.io/gamepad/#gamepadmappingtype-enum

Got it!

[Dean Jackson]

(In reply to James Howard from comment #7)

> > rationale for including the home button
> 
> My initial rationale was that the button exists on popular gamepads, the
> "standard" mapping reserves an index (16) in the buttons array for it, and
> Chrome on Mac does map this button when using Xbox and PlayStation gamepads. 
> 
> However, the spec is a little vague on this point [2]. It states that "up to
> 17 buttons" can be included, and while the binding is shown in the
> standard_gamepad.svg, the table in the remapping section doesn't list it.
> 
> Given that it's not an API on iOS, and that it is a button that is often
> reserved for system use elsewhere, I removed it from the patch and left the
> buttons array at 16 elements only, rather than 17.

Great. I don't think we should expose this while it isn't public API on the system.

> 
> I do worry a little bit about web apps that assume that the buttons array
> will always be 17 elements long, so maybe there is a case to be made that
> the 16th index should be present but just always 0, but I didn't do that for
> now. It's an easy change to make if we want to do that.

Let's see if it causes problems.

> 
> [1] https://w3c.github.io/gamepad/standard_gamepad.svg
> [2] https://w3c.github.io/gamepad/#remapping

[Dean Jackson]

Comment on attachment 387677 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=387677&action=review

> Source/WebCore/platform/gamepad/cocoa/GameControllerGamepad.mm:106
> +    if (@available(macOS 10.14.1, iOS 12.1, *)) {

Please remove this statement.

[James Howard]

> Please remove this statement.

And replace it with this?

#if HAVE(GCEXTENDEDGAMEPAD_BUTTONS_THUMBSTICK)
#pragma clang diagnostic push
#pragma clang diagnostic ignored "-Wunguarded-availability"
    if ([m_extendedGamepad.get() respondsToSelector:@selector(leftThumbstickButton)]) {
        bindButton(m_extendedGamepad.get().leftThumbstickButton, 10);
        bindButton(m_extendedGamepad.get().rightThumbstickButton, 11);
    }
#pragma clang diagnostic pop
#endif


Our options are disable the unguarded-availability warning or use @available. We can't set the HAVE_ flag to __MAC_OS_X_VERSION_MIN_REQUIRED >= 101401 since the Mojave build has it set to 101400, which means it would be compiled out incorrectly there, and also WebKit style check won't allow a point release in a HAVE_ flag.

[Dean Jackson]

(In reply to James Howard from comment #14)
> > Please remove this statement.
> 
> And replace it with this?
> 
> #if HAVE(GCEXTENDEDGAMEPAD_BUTTONS_THUMBSTICK)
> #pragma clang diagnostic push
> #pragma clang diagnostic ignored "-Wunguarded-availability"
>     if ([m_extendedGamepad.get()
> respondsToSelector:@selector(leftThumbstickButton)]) {
>         bindButton(m_extendedGamepad.get().leftThumbstickButton, 10);
>         bindButton(m_extendedGamepad.get().rightThumbstickButton, 11);
>     }
> #pragma clang diagnostic pop
> #endif
> 
> 
> Our options are disable the unguarded-availability warning or use
> @available. We can't set the HAVE_ flag to __MAC_OS_X_VERSION_MIN_REQUIRED
> >= 101401 since the Mojave build has it set to 101400, which means it would
> be compiled out incorrectly there, and also WebKit style check won't allow a
> point release in a HAVE_ flag.

This would be the first use of @available in WebKit, so I'm just trying
to avoid it.

I think the code above, while ugly, is ok for now. Although please make
a note that this is required because the API was added in 10.14.1 (so
we can possibly remove the check once we drop support for that).

[James Howard]

Created attachment 389296 [details]
Patch

[James Howard]

Created attachment 389394 [details]
Patch

[James Howard]

Updated the patch. Previous one I had forgotten to add the comment about 10.14.1.

[WebKit Commit Bot]

Comment on attachment 389394 [details]
Patch

Clearing flags on attachment: 389394

Committed r256215: <https://trac.webkit.org/changeset/256215>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.