-
-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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.
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
gilrs: Add code to unknown buttons and axes #8560
base: main
Are you sure you want to change the base?
Conversation
Welcome, new contributor! Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, I don't see any harm in supporting this and this implementation looks like what I would expect.
crates/bevy_gilrs/src/converter.rs
Outdated
@@ -25,21 +25,22 @@ pub fn convert_button(button: gilrs::Button) -> Option<GamepadButtonType> { | |||
gilrs::Button::DPadDown => Some(GamepadButtonType::DPadDown), | |||
gilrs::Button::DPadLeft => Some(GamepadButtonType::DPadLeft), | |||
gilrs::Button::DPadRight => Some(GamepadButtonType::DPadRight), | |||
gilrs::Button::Unknown => None, | |||
gilrs::Button::Unknown => Some(GamepadButtonType::Other(code.into_u32() as u8)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should bump it up for consistency: I don't think it makes sense to have an "other" variant without proper support.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, I tried storing a gilrs::ev::Code
in the Other
variant, but ran into two problems:
bevy_input
needs to depend directly ongilrs
, and- We need
gilrs::ev::Code
to implementFromReflect
becauseGamepadAxisType
implementsFromReflect
.
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.
Even if there's no support for more than 256 axes/buttons, on some platforms the conversion to |
I'm opposed to converting to a Looking at how other engines handles that, I would prefer to remove the |
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 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. |
Okay, I have a bit of time so I'm thinking about this again. If we want to directly expose the If we make the user depend directly on Maybe we could make the We'd probably need that newtype anyway to implement reflection on it. (and we can do a similar thing for |
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 Edit: The below doesn't seem like it would really work well, the gilrs I think one thing worth considering is to make the That is basically my current thoughts I suppose |
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
Migration Guide
I don't think there should be any action required for users who don't want this functionality.