gilrs: Add code to unknown buttons and axes

[lkolbly]

Objective

I'm using a joystick to control my game (a flight simulator), so gilrs has not mapped all of the buttons. As discussed elsewhere, that's intentional by gilrs, and that's fine - but, gilrs does give us the code for the axis/button, which games can use to do their own mapping if we expose it.

Fixes #1916

Solution

I change the gilrs converter to use the (already existing) Other variant of the axis/button types to expose the code, so users can access them through events.

The code is given to us as a u32, but here I store it as a u8 just by casting. The presumption is that joysticks will not typically have more than 256 axes or buttons, indeed as of 2020 linux only supported 80 buttons. So I think it should be fine.

---

Changelog

• Exposed unmapped gamepad events

Migration Guide

I don't think there should be any action required for users who don't want this functionality.

[github-actions]

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

[alice-i-cecile]

Great, I don't see any harm in supporting this and this implementation looks like what I would expect.

[mockersf]

The code format is dependent on the platform, and on some (macOS and window/wgi) it's bigger than a u32. Converting it again to a u8 means that only windows/xinput and wasm won't lose data.

[lkolbly]

Ah, fair 'nuff, yeah I see now that's what gilrs does on mac.

Bevy's Other variant only stores a u8: https://github.com/bevyengine/bevy/blob/main/crates/bevy_input/src/gamepad.rs#L208 would it make sense to up that to a u32? That would increase the size of the enum, and would also require that users adjust what type they use for that (which admittedly would probably be a fairly simple refactor).

[alice-i-cecile]

I think we should bump it up for consistency: I don't think it makes sense to have an "other" variant without proper support.

[mockersf]

or don't use that into_u32 function that makes no guarantee on its meaning and keep the platform dependent EvCode? It's impossible to do anything cross platform with this value, at least it would make it clear

[lkolbly]

So, I tried storing a gilrs::ev::Code in the Other variant, but ran into two problems:

• bevy_input needs to depend directly on gilrs, and
• We need gilrs::ev::Code to implement FromReflect because GamepadAxisType implements FromReflect.

It also means that if someone were using serde to send gamepad bindings between platforms, which previously would work (and is cross-platform for every variant except Other), now it possibly won't deserialize properly. Which we probably want to stop them from doing that, maybe? Except that it is valid to do that for every variant except Other.

So I pushed code for now that uses u32, but definitely I would be open to trying other ways to get gilrs::ev::Code to work.

[mockersf]

The code is given to us as a u32, but here I store it as a u8 just by casting. The presumption is that joysticks will not typically have more than 256 axes or buttons

Even if there's no support for more than 256 axes/buttons, on some platforms the conversion to u32 is from two u16, each on its own bits of the u32. Converting to a u8 will just discard the second u16

[mockersf]

I'm opposed to converting to a u32. I think that's very misleading as it's not something that should be serialisable.

Looking at how other engines handles that, I would prefer to remove the Other option, and add an other "ManualInputDevice" (or similar name) that would just keep the code and doesn't try any conversion

[lkolbly]

Giving it some thought... so, if we split it, then even if you want to use a single button that isn't mapped, then you lose all of the support that gilrs gives you. For example, today bevy recognizes three of the axes on my joystick (a Logitech Extreme 3D Pro) and maps pitch/roll to the left stick and yaw to RightZ. So it provides a reasonable default, and my presumption is that it will on essentially any gamepad with gilrs mappings. It just doesn't detect the throttle axis.

Really, gamepads are a subset of controllers (they don't have any substantively different functionality, that I know of). So maybe it makes sense to have gamepad support be some sort of converter on top of a base functionality that just directly exposes axis and button number. And then the user can apply the converter to get the gamepad mappings.

Not sure how that would interact with the gilrs though.

[lkolbly]

Okay, I have a bit of time so I'm thinking about this again. If we want to directly expose the gilrs::ev::Code to the user, then either the user needs to directly use bevy_gilrs or we need bevy_input to depend on gilrs (currently, bevy_gilrs depends on bevy_input, and is largely hidden from the user).

If we make the user depend directly on bevy_gilrs, then we would have to reimplement all of the logic around Input<GamepadButton> in bevy_gilrs or else the user would lose that.

Maybe we could make the Other variant of GamepadButtonType conditional on the bevy_gilrs feature - if gilrs is enabled, we have that variant, otherwise we don't. This seems pretty cursed, and I'm not sure if Rust actually supports that. But, we could wrap the gilrs::ev::Code in a newtype (RawCode?), which we could conditionally compile to either gilrs::ev::Code or a u32 (or a u64). That way, if the user doesn't have gilrs enabled, they could use their own systems to create RawCodes.

We'd probably need that newtype anyway to implement reflection on it.

(and we can do a similar thing for GamepadAxisChangedEvent)

[ratmice]

Curious how your think went, I was thinking about it too a bit

One thing I would note is that in the w3c gamepad api, doesn't appear to encode any notion of type like this, there is a vector of axes, with each axis referred to by index. AFAICT it doesn't encode an axis type or button type at all, while gilrs is encoding each button with a different code where code seems to be a combination of both a button ID and it's type in one unhelpful value.

To me it doesn't make any sense to try and put the gilrs::ev::Code in bevy_input, or avoid conversion to u32, since the only method that Code actually has is into_u32() and bevy_input appears to be attempting to avoid depending upon any specific input layer. If we go the route of embedding input-system specific types into the other value I think it only makes sense if it becomes GameAxisType<T> { ..., Other(T), }, But I don't think anyone wants to see that?

Edit: The below doesn't seem like it would really work well, the gilrs axes and buttons functions don't appear to have stable indices, they just appear in whatever order buttons/axes events first arrive.

I think one thing worth considering is to make the Other value based around the index into return value of the following
GamepadState functions, buttons() axes() which would require an upstream change to gilrs to be able to get the index (probably from Code).

That is basically my current thoughts I suppose

[janhohenheim]

Triage: has merge conflicts
@lkolbly do you want to update this PR or should I tag it S-Needs-Adoption? :)

[lkolbly]

I just rebased and force-pushed.

I think one thing worth considering is to make the Other value based around the index into return value of the following
GamepadState functions, buttons() axes() which would require an upstream change to gilrs to be able to get the index (probably from Code).

I like the principle, but I will note that the buttons() method currently returns the order of a FnvHashMap. Although, maybe we could do something upstream in gilrs to have some guarantees around the order.

Another option we have is we could take all of the Codes from gilrs when the joystick is connected, and then sort them, and use that sorted list as the numbering.

e.g. I'm testing with a joystick here, and I'm getting codes like Other(66250) and Other(66251). We could relabel these as Other(0) and Other(1) if we wanted. As far as I'm concerned though, the actual numeric value doesn't matter.

In #18958, @clangdo mentions a possible performance cost of handling all axes:

I have some, potentially overwrought, concerns about registering all unknown axis events by default, because with HID devices that have a large number of axes, there could be knock-ons when bevy goes to cache everything, particularly when a new game developer runs their game for the first time with a full sim-pit connected and wonders why their input system is so slow. Still, putting this behind a flag, or perhaps allowing a whitelist for specific event codes could be a good step?

Handling all axes (and buttons) would increase the number of RawGamepad*ChangedEvent events generated, and the values do get put into a hashmap.

I'm opposed to converting to a u32. I think that's very misleading as it's not something that should be serialisable.

The gilrs::ev::Code is itself serializable, their docs also give the caveat that it's platform-specific.

[lkolbly]

The CI issues seem unrelated to this change.