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.

Sign up for GitHub

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

Jump to bottom

[Merged by Bors] - Add Gamepads resource #3257

Closed
wants to merge 8 commits into from
Closed

[Merged by Bors] - Add Gamepads resource #3257

wants to merge 8 commits into from

Conversation

CrazyRoka
Copy link
Contributor

Objective

Fixes #3245

Solution

  • Move GamepadLobby to lib
  • Add connection_system to InputPlugin
  • Updated gamepad_input example

@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Dec 5, 2021
@alice-i-cecile alice-i-cecile added A-Input Player input via keyboard, mouse, gamepad, and more C-Usability A targeted quality-of-life change that makes Bevy easier to use C-Examples An addition or correction to our examples and removed S-Needs-Triage This issue needs to be labelled labels Dec 5, 2021
@alice-i-cecile
Copy link
Member

Looks great! Thanks for tackling this :) I would like to see some basic docs, since this is part of the public API now, but otherwise this LGTM.

@alice-i-cecile
Copy link
Member

I've added the needs-migration-guide label to this. It's not technically breaking, but users should probably replace their copy-pasted code, and use the built-in functionality instead.

@alice-i-cecile
Copy link
Member

Thinking about this a bit further, I think that connection_system should be renamed to gamepad_connection_system, in order to better communicate what it does out of context.

@alice-i-cecile
Copy link
Member

Great! You have my approval :) @CrazyRoka, you should click "Resolve Comment" on the comments that I've left so this PR's state is easier to follow for future reviewers.

MinerSebas
MinerSebas reviewed Dec 5, 2021
View reviewed changes
crates/bevy_input/src/lib.rs Outdated Show resolved Hide resolved
crates/bevy_input/src/gamepad.rs Outdated Show resolved Hide resolved
@alice-i-cecile alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review labels Dec 5, 2021
IceSentry
IceSentry approved these changes Dec 5, 2021
View reviewed changes
Copy link
Contributor

@IceSentry IceSentry left a comment

Choose a reason for hiding this comment

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

I don't really like the name GamepadLobby, but I can't figure out anything better either. Otherwise, looks good to me.

@CrazyRoka
Copy link
Contributor Author

@alice-i-cecile @MinerSebas Can you review my updates?

cart
cart reviewed Dec 8, 2021
View reviewed changes
crates/bevy_input/src/gamepad.rs Outdated
///
/// [Gamepad]s are registered and deregistered in [gamepad_connection_system]
pub struct GamepadLobby {
pub gamepads: HashSet<Gamepad>,
Copy link
Member

Choose a reason for hiding this comment

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

This is an implementation detail that we shouldn't expose directly to users (who could accidentally remove "active" gamepad and break guarantees that this list contains "active" gamepads). Instead, lets make this private and implement a method like GamepadLobby::contains(index: usize)

Copy link
Member

Choose a reason for hiding this comment

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

Eh actually we should probably accept the Gamepad type for consistency with other apis, but I think we should consider making the index private to encourage people to treat Gamepad as an opaque type that must be queried for. And if we don't want that behavior (and want user-specified ids to be a part of our api), then we should consider removing the Gamepad wrapper type.

Also, lets add GamepadLobby::iter() so users can iterate active gamepads.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • Made this field private
  • Added contains(gamepad: Gamepad) method
  • Added iter() method
  • Replaced usages with these methods
  • Added register and deregister private methods

cart
cart reviewed Dec 8, 2021
View reviewed changes
crates/bevy_input/src/gamepad.rs Outdated
/// Container of unique connected [Gamepad]s
///
/// [Gamepad]s are registered and deregistered in [gamepad_connection_system]
pub struct GamepadLobby {
Copy link
Member

Choose a reason for hiding this comment

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

I agree that GamepadLobby isn't the best name for this. Lets use the more descriptive ActiveGamepads unless someone can think of something better.

Copy link
Member

Choose a reason for hiding this comment

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

Or maybe just Gamepads

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I like Gamepads more because it is terser

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed it to Gamepads

MinerSebas
MinerSebas reviewed Dec 8, 2021
View reviewed changes
crates/bevy_input/src/gamepad.rs Outdated Show resolved Hide resolved
crates/bevy_input/src/gamepad.rs Outdated Show resolved Hide resolved
@cart
Copy link
Member

cart commented Dec 8, 2021

bors r+

@bors
Copy link
Contributor

bors bot commented Dec 8, 2021

Canceled.

@cart cart changed the title Added GamepadLobby to library together with connection_system Add Gamepads resource Dec 8, 2021
@cart
Copy link
Member

cart commented Dec 8, 2021

bors r+

bors bot pushed a commit that referenced this pull request Dec 8, 2021
@CrazyRoka @cart
Add Gamepads resource (#3257)
b896294 
# Objective

Fixes #3245 

## Solution

- Move GamepadLobby to lib
- Add connection_system to InputPlugin
- Updated gamepad_input example


Co-authored-by: CrazyRoka <[email protected]>
Co-authored-by: Carter Anderson <[email protected]>
@bors
Copy link
Contributor

bors bot commented Dec 8, 2021

Build failed:

@cart
Copy link
Member

cart commented Dec 8, 2021

bors r+

bors bot pushed a commit that referenced this pull request Dec 8, 2021
@CrazyRoka @cart
Add Gamepads resource (#3257)
01e2141 
# Objective

Fixes #3245 

## Solution

- Move GamepadLobby to lib
- Add connection_system to InputPlugin
- Updated gamepad_input example


Co-authored-by: CrazyRoka <[email protected]>
Co-authored-by: Carter Anderson <[email protected]>
@bors
Copy link
Contributor

bors bot commented Dec 8, 2021

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title Add Gamepads resource [Merged by Bors] - Add Gamepads resource Dec 8, 2021
@bors bors bot closed this Dec 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MinerSebas MinerSebas MinerSebas left review comments

@cart cart cart left review comments

@IceSentry IceSentry IceSentry approved these changes

@alice-i-cecile alice-i-cecile alice-i-cecile approved these changes

Assignees
No one assigned
Labels
A-Input Player input via keyboard, mouse, gamepad, and more C-Examples An addition or correction to our examples C-Usability A targeted quality-of-life change that makes Bevy easier to use S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Make GamepadLobby a built-in type
5 participants
@CrazyRoka @alice-i-cecile @cart @IceSentry @MinerSebas