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

Add GamepadButtonInput event #9008

Merged
merged 7 commits into from
Aug 20, 2023

Conversation

bravely-beep
Copy link
Contributor

Objective

Add GamepadButtonInput event
Resolves #8988

Solution

  • Add GamepadButtonInput type
  • Emit GamepadButtonInput events whenever Input<GamepadButton> is written to
  • Update example

@github-actions
Copy link
Contributor

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 alice-i-cecile added C-Enhancement A new feature A-Input Player input via keyboard, mouse, gamepad, and more labels Jun 30, 2023
Copy link
Contributor

@nicopap nicopap left a comment

Choose a reason for hiding this comment

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

There is an issue with trigger buttons (they have a value that changes between 0.0 and 1.0) The Released event is sent every time the value change, yet it should only be sent once when it goes from Pressed to Released. This is also true when the value changes but stays within Pressed range.

Comment on lines 19 to 22
mut connection_events: EventReader<GamepadConnectionEvent>,
mut axis_changed_events: EventReader<GamepadAxisChangedEvent>,
mut button_changed_events: EventReader<GamepadButtonChangedEvent>,
mut button_input_events: EventReader<GamepadButtonInput>,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this would benefit from a comment explaining the difference between GamepadButtonInput and GamepadButtonChangedEvent

Copy link
Contributor

@nicopap nicopap left a comment

Choose a reason for hiding this comment

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

This is good IMO. Nice work!

Note to readers: I considered the case where you get several events in a single frame. With this setup it should work as expected.

@bravely-beep
Copy link
Contributor Author

bravely-beep commented Jul 1, 2023

Ideally Input::press/release would return a bool to prevent the duplicate check, but that'd be a breaking change, and would need some docs to explain how the returned value subtly differs from just_pressed/just_released.

Also, regarding what you mentioned about comments in the example: I think the core problem here is the existing nomenclature having some ambiguous or overloaded meanings ("input" / "button" / "axis"). I saw an old discussion on the discord about renaming Input to Pressable; not sure if it's too late to change that, but it could help disambiguate terminology surrounding the abstraction layer over GamepadButton. Probably worth dealing with in a separate PR though.

@nicopap
Copy link
Contributor

nicopap commented Jul 1, 2023

Supposedly the compiler is smart enough to eliminate the duplicate check.

Ideally Input::press/release would return a bool to prevent the duplicate check

Agree this should be part of a different PR.

I'm not sure how press/release works, but if they read the previous value, they should indeed return it (similarly to how HashMap::insert works in the std lib).

It's the kind of breaking change that shouldn't count as breaking change. Sure if someone ignored the clippy lint and did let () = input.press(x), they will get a compilation error. IMO, if they were doing that, it is specifically to have a compilation error (maybe so that they can account for the change and fix their code). So we should indulge them. We've been renaming things willy-nilly without much thought, so I doubt such a small breaking change would even be a consideration.

@@ -74,6 +74,7 @@ impl Plugin for InputPlugin {
// gamepad
.add_event::<GamepadConnectionEvent>()
.add_event::<GamepadButtonChangedEvent>()
.add_event::<GamepadButtonInput>()
Copy link
Member

Choose a reason for hiding this comment

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

I don't like it, but almost all other events have their name ending with Event.

Could you either rename that new event to GamepadButtonInputEvent, or renamed all the others?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

GamepadButtonInput was chosen to match the naming of KeyboardInput; I agree the inconsistent naming isn't great, so it might be good to rename both of them to end with Event.

@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Aug 20, 2023
@mockersf mockersf added this pull request to the merge queue Aug 20, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 20, 2023
@mockersf mockersf added this pull request to the merge queue Aug 20, 2023
Merged via the queue into bevyengine:main with commit 140d961 Aug 20, 2023
21 checks passed
@cart cart mentioned this pull request Oct 13, 2023
43 tasks
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-Enhancement A new feature 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
Development

Successfully merging this pull request may close these issues.

GamepadButtonInput event
5 participants