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.

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

Added gamepad support using Gilrs #280

Merged
merged 2 commits into from
Sep 18, 2020
Merged

Conversation

simpuid
Copy link
Contributor

@simpuid simpuid commented Aug 21, 2020

Implements #94
Partially implements/uses a temporary implementation for #59

I have added crate bevy_gilrs which contains the system to:

  • Generate connect/disconnect event using resource Event<GamepadEvent>
  • Store gamepad's buttons state using resource Input<GamepadButton>
  • Store gamepad's axes state using resource Axis<GamepadAxis>

Added a temporary implementation of Axis<T> as there are other PRs open for that. (I will rebase this PR after they are merged or should I implement them here?)

Added Gamepad, GamepadButton, GamepadAxis, GamepadEvent, GamepadEventType, ButtonCode and AxisCode to crate bevy_input. These structs are exposed to the user. These structs are modified/generated using the systems inside bevy_gilrs.

Suggestions Needed:

  • Regarding names of these structs.
  • gilrs::Gilrs struct implements !Send, so I wrapped it inside an Arc<Mutex<T>> and implements Send unsafely. Are there any other approach to add gilrs::Gilrs as a Resource?

Check the example examples/input/gamepad_input.rs for usage.

Note: Need more polish before merge(comments), until then, review it.

@simpuid simpuid force-pushed the bevy_gilrs branch 2 times, most recently from 3f3d604 to 2114a18 Compare August 21, 2020 22:30
@karroffel karroffel added C-Feature A new feature, making something new possible A-Input Player input via keyboard, mouse, gamepad, and more labels Aug 22, 2020
@memoryruins memoryruins mentioned this pull request Sep 7, 2020
Copy link
Member

@CleanCut CleanCut left a comment

Choose a reason for hiding this comment

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

First, thanks! This looks fantastic and I'm going to try it out more in a personal project this weekend.

Second, some feedback! I tested this out on macOS. It appears to mostly work as advertised. I've only casually skimmed through the code, so don't consider this a thorough review. Here's what I noticed:

  • gilrs support on macOS is incomplete. If there isn't a better alternative, investing in contributions to gilrs may be necessary to gain feature parity across the OS's Bevy supports.
    • Most importantly, the dpad isn't yet supported since gilrs doesn't yet support hat events on macOS.
    • Force feedback is not supported
    • Power information is not supported
  • RightZ and LeftZ axes were not detected, though RightTrigger2 and LeftTrigger2 buttons worked fine. So the analog portion of triggers isn't working -- this might be another macOS difference, but I haven't been able to confirm that yet, which is why it isn't included above as a sub-item.
  • I quite like the overall design of the separate Bevy gamepad layer vs the gilrs driver layer.
  • Notwithstanding the previous point, I suggest that bevy_gilrs be refactored into a submodule of bevy_input. I don't think the gilrs part even needs to be pub, much less be in the top-level.
  • This appears to add about 14 dependencies on macOS, based off of a quick comparison of the number of crates compiled during cargo run --example gamepad_input on this branch vs cargo run --example keyboard_input on master. Is that an okay amount of dependencies to add for the one-time portion of compiling? I don't have a suggestion here, I'm just asking the question. (I'm guessing yes, it's okay.)

Finally, gilrs is a terrible name because Google autocorrects it to "girls" and gives you irrelevent results. 😝

@cart cart mentioned this pull request Sep 10, 2020
Copy link
Member

@cart cart left a comment

Choose a reason for hiding this comment

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

I think this is ready to go as soon as we merge #227 and update this.

crates/bevy_gilrs/src/gilrs_system.rs Outdated Show resolved Hide resolved
@simpuid
Copy link
Contributor Author

simpuid commented Sep 12, 2020

@cart If gilrs seems heavy, then there is gilrs-core. However we have to implement a high amount of gilrs code in bevy but that would give us the flexibility to implement that in a way most suitable for bevy.

@simpuid simpuid force-pushed the bevy_gilrs branch 2 times, most recently from c0013a4 to c9ef1ab Compare September 12, 2020 06:59
@cart
Copy link
Member

cart commented Sep 14, 2020

i think using the gilrs is worth it because it gives us a bunch of controller mappings "for free" / allows for nice integration with steam mappings (via environment variables).

We can pare it down later if we need to.

@CleanCut
Copy link
Member

FYI, I've been playing with this branch on a little project and it is working pretty well. I've almost got a patch for gilrs dpad support on mac working--I've plumbed a response all the way through to Bevy, but I've still got to differentiate the directions.

@simpuid The force pushes after my review make it difficult for me to tell exactly what changed since the last time I reviewed the code without reading the entire pull request again. 😢 I'll look for some time to re-review later this week.

@simpuid
Copy link
Contributor Author

simpuid commented Sep 15, 2020

@simpuid The force pushes after my review make it difficult for me to tell exactly what changed since the last time I reviewed the code without reading the entire pull request again. cry I'll look for some time to re-review later this week.

I was following the 1 commit per PR rule, sorry I should have squashed the commits at the end of the merge. The changes I have made are:

  • Gamepad, GamepadButton and GamepadAxis are now tuple structs instead of normal structs. Now there is no need of T::new function to construct them.
  • Renamed EventType to GamepadEventType, GamepadButtonCode to GamepadButtonType, GamepadAxisCode to GamepadAxisType.
  • bevy_gilrs is now an optional dependency. And I added GilrsPlugin to add_default_plugins
  • Added the Install udev step to ci.yml (Not sure if that's the correct place, because CI builds are stalling but local builds are fine) If somebody has any suggestions about it then please reply.

I think we can merge this PR after rebasing it over the 'Axis API` PR. After that we can focus on controller rumble feature.

@cart
Copy link
Member

cart commented Sep 18, 2020

I think I'm just going to merge this directly, rather than merging the axis pr. I'm not super convinced we should throw mouse state into the axis api. I'd prefer it if we can make the assumption that an axis is normalized to [-1, 1] for now.

@simpuid
Copy link
Contributor Author

simpuid commented Sep 18, 2020

Alright then, do you have any suggestions for the Axis<T> struct? The current implementation is a quick and dirty one with more focus on the use case with controller axes.

@cart
Copy link
Member

cart commented Sep 18, 2020

I dont think it needs to be fancy for now. If anything, I think we might want to remove the add() function because i think it encourages "relative" instead of "absolute" behavior, but other than that I think its good to go.

@cart
Copy link
Member

cart commented Sep 18, 2020

I want this in the 0.2 release, which im hoping to drop tomorrow.

@simpuid
Copy link
Contributor Author

simpuid commented Sep 18, 2020

There is no add() function in this implementation, so I think we are good to go. Good Luck for the 0.2 release 👍

@cart cart merged commit 19d4694 into bevyengine:master Sep 18, 2020
@cart cart mentioned this pull request Oct 1, 2020
mrk-its pushed a commit to mrk-its/bevy that referenced this pull request Oct 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Input Player input via keyboard, mouse, gamepad, and more C-Feature A new feature, making something new possible
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants