docs: Documentation and clean up of `bevy_input` · Pull Request #3692 · bevyengine/bevy

ghost · 2022-01-16T07:48:39Z

Part of #3492.

Objective

Add and update the bevy_input documentation to achieve a 100% documentation coverage.
Add the #![warn(missing_docs)] lint to keep the documentation coverage for the future.

Solution

Added and updated the documentation and tests.
Added #[warn(missing_docs)] to the lib.rs file.
Ran the cargo +nightly doc -p bevy_input --all-features --no-deps command with the RUSTDOCFLAGS="-Z unstable-options -- show-coverage" environment variable which returned a doc coverage of 100%.

Additional work

I did some additional work while documenting the crate. I'm happy to change/discuss any of these.

Tuple structs

I changed GamepadAxis, GamepadButton, Gamepad, GamepadEvent, and GamepadEventRaw to be normal structs instead of tuple structs. Main reason being is that tuple structs are not descriptive and extending them is harder.

Constructors

I added new() functions to GamepadAxis, GamepadButton, GamepadEvent, GamepadEventRaw, Gamepad, KeyboardInput, MouseButtonInput, MouseMotion, MouseWheel, and TouchInput. The main reason being that it made changing the tuple structs to normal structs easier. The Events also received a new() function that isn't used inside of Bevy, but I thought it would be a good idea to add them for consistency and also for users of Bevy that might use these to simulate inputs (for tests or even game logic).

Structure change

I split up the keyboard.rs, mouse.rs, gamepad.rs, and touch.rs files into smaller chunks, because some of these files were quite huge and hard to navigate (especially gamepad.rs).

Logic change

I changed the gamepad_connection_system to run after the InputSystem instead of in parallel, because this system checks the GamepadEvents which get send inside of the gamepad_event_system. This means that the gamepad_connection_system could respond to the events one frame later, instead of instantly resulting in one frame lag.

Old possible case:

gamepad_connection_system (reacts to the GamepadEvents too early)
gamepad_event_system (sends the GamepadEvents)

New:

gamepad_event_system (sends the GamepadEvents)
gamepad_connection_system (reacts to the GamepadEvents)

Removed constants

I removed the Axis::MIN and Axis::Max constants, because they were only used inside of one spot. I also removed the ALL_BUTTON_TYPES and ALL_AXIS_TYPES constants, because I added strum instead to use the EnumIter instead (as suggested by @alice-i-cecile).

Renamed `ElementState`

I renamed the ElementState to ButtonState as the old name was too generic and if something can be pressed it is automatically button-like (thanks to @alice-i-cecile for bringing it up).

Breaking Changes

GamepadAxis, GamepadButton, Gamepad, GamepadEvent, and GamepadEventRaw are no longer tuple structs. To create instances of them you would have to use the new() function or a struct literal.
The Axis::MIN, Axis::Max, ALL_BUTTON_TYPES and ALL_AXIS_TYPES constants have been removed. The minimum axis value is -1.0 and the maximum axis value is 1.0. To iterate through the GamepadButtonTypes and GamepadAxisTypes you can now use the iter() method provided by strum (GamepadButtonType::iter() or GamepadAxisType::iter()).
The ElementState received a rename and is now called ButtonState.

Continued updating bevy_input. Small fixes.

alice-i-cecile · 2022-01-18T02:31:00Z

crates/bevy_gilrs/src/gilrs_system.rs

 pub fn gilrs_event_startup_system(gilrs: NonSend<Gilrs>, mut events: EventWriter<GamepadEventRaw>) {
    for (id, _) in gilrs.gamepads() {
-        events.send(GamepadEventRaw(
+        events.send(GamepadEventRaw::new(


On review, I think this is nicer than the tuple structs.

Not sure if I like it better than a named struct though.

In my eyes tuple structs should be avoided in most cases, because they are a pain to work with. I'm fine with both the named struct and the new() constructor. Would probably be a good idea to have a place where we note things like this so we can keep things consistent.

Yeah, we have an engine style guide: https://github.com/bevyengine/bevy/blob/main/.github/contributing/engine_style_guide.md

@cart, do you have strong feelings?

alice-i-cecile

Excellent work. The tests are great, the docs are clear and comprehensive, the cleanup is solid and uncontroversial. #3419 should be rebased on top of this IMO.

I'll do another pass once these comments are addressed.

crates/bevy_input/src/axis.rs

crates/bevy_input/src/gamepad/axis.rs

crates/bevy_input/src/gamepad/button.rs

crates/bevy_input/src/gamepad/event.rs

crates/bevy_input/src/gamepad/gamepads.rs

crates/bevy_input/src/plugin.rs

crates/bevy_input/src/state.rs

crates/bevy_input/src/system.rs

alice-i-cecile

Alright, this looks good to me now :) Can you please compile a condensed list of breaking changes and stick it in the PR description?

ghost · 2022-01-20T17:35:22Z

@alice-i-cecile Done.

There is still an open conversation about the exit_on_esc_system if you missed it. I'm considering if we might just remove it all together.

alice-i-cecile · 2022-01-20T19:29:57Z

@KDecay WRT exist_on_esc_system, my feeling is that we need to take a holistic look at how we want to handle these "helper systems". There's a few of them scattered about the code base IIRC, and I don't like them in here either. My feeling is that they should probably be either in a separate crate, or in-lined into our examples.

Regardless, I think that's out of scope for this PR :)

ghost · 2022-01-20T19:43:01Z

I agree. It would be a great idea to collect them all and discuss what we want to do with them. I resolved the conversation for now.

Currently the only open thing in here is which type of struct creation we prefer:

Tuple structs
Struct literals
new() functions.

alice-i-cecile · 2022-01-20T19:45:00Z

Currently the only open thing in here is which type of struct creation we prefer:

Tuple structs

Struct literals

new() functions.

@cart, when you get a chance could you weigh in on this? Then we'll add the decision to the style guide.

alice-i-cecile · 2022-02-04T19:26:26Z

This PR should be merged before #3419.

mockersf · 2022-02-15T20:46:44Z

the merge commit may have gone wrong

ghost · 2022-02-15T21:11:01Z

Did it? Looking at the Files changed I can't see any obvious thing that went wrong. The merge conflict was caused because of the gamepad.rs file that got modified in #2934. I added a missing semicolon that was added in there, but ignored the .iter() removal, because we now use strum to iterate through the enums.

mockersf · 2022-02-15T21:23:49Z

My bad, somehow GitHub UI is not loading for me when I try to view the changes. Loading it when in another browser works fine 🤷

alice-i-cecile · 2022-02-24T23:07:08Z

@bevyengine/docs-team Can I get a couple more eyes on this? IMO this should be merged ASAP, to unblock more improvements to bevy_input.

alice-i-cecile · 2022-02-25T00:34:55Z

@BoxyUwU isn't thrilled with the addition of the strum dependency, and doesn't like the fact that their macro uses .iter as the method name. This is reasonable.

I wrote my own replacement for it here: https://github.com/Leafwing-Studios/leafwing_input_manager/blob/main/macros/src/actionlike.rs. IMO we should remove that change from this PR, and then remind me to make a PR to port that macro over.

ghost · 2022-02-25T22:21:14Z

Done. You could create an issue for it so we don't forget and can worry about fixing it another time.

# Objective - Part of the splitting process of #3692. ## Solution - Remove / change the tuple structs inside of `gamepad.rs` of `bevy_input` to normal structs. ## Reasons - It made the `gamepad_connection_system` cleaner. - It made the `gamepad_input_events.rs` example cleaner (which is probably the most notable change for the user facing API). - Tuple structs are not descriptive (`.0`, `.1`). - Using tuple structs for more than 1 field is a bad idea (This means that the `Gamepad` type might be fine as a tuple struct, but I still prefer normal structs over tuple structs). Feel free to discuss this change as this is more or less just a matter of taste. ## Changelog ### Changed - The `Gamepad`, `GamepadButton`, `GamepadAxis`, `GamepadEvent` and `GamepadEventRaw` types are now normal structs instead of tuple structs and have a `new()` function. ## Migration Guide - The `Gamepad`, `GamepadButton`, `GamepadAxis`, `GamepadEvent` and `GamepadEventRaw` types are now normal structs instead of tuple structs and have a `new()` function. To migrate change every instantiation to use the `new()` function instead and use the appropriate field names instead of `.0` and `.1`.

# Objective - Part of the splitting process of #3692. ## Solution - Document `keyboard.rs` inside of `bevy_input`. Co-authored-by: KDecay <KDecayMusic@protonmail.com>

# Objective - Part of the splitting process of bevyengine#3692. ## Solution - Document `keyboard.rs` inside of `bevy_input`. Co-authored-by: KDecay <KDecayMusic@protonmail.com>

# Objective - Part of the splitting process of bevyengine#3692. ## Solution - Document `system.rs` inside of `bevy_input`.

@alice-i-cecile

# Objective - Part of the splitting process of bevyengine#3692. ## Solution - Rename `ElementState` to `ButtonState` ## Reasons - The old name was too generic. - If something can be pressed it is automatically button-like (thanks to @alice-i-cecile for bringing it up in bevyengine#3692). - The reason it is called `ElementState` is because that's how `winit` calls it. - It is used to define if a keyboard or mouse **button** is pressed or not. - Discussion in bevyengine#3692. ## Changelog ### Changed - The `ElementState` type received a rename and is now called `ButtonState`. ## Migration Guide - The `ElementState` type received a rename and is now called `ButtonState`. To migrate you just have to change every occurrence of `ElementState` to `ButtonState`. Co-authored-by: KDecay <KDecayMusic@protonmail.com>

# Objective - Part of the splitting process of bevyengine#3692. ## Solution - Document `input.rs` inside of `bevy_input`.

# Objective - Part of the splitting process of bevyengine#3692. ## Solution - Add more tests to `input.rs` inside of `bevy_input`. ## Note - The tests would now catch a change like bevyengine#4410 and fail accordingly.

# Objective - Part of the splitting process of bevyengine#3692. ## Solution - Document `mouse.rs` inside of `bevy_input`. Co-authored-by: KDecay <KDecayMusic@protonmail.com>

# Objective - Part of the splitting process of bevyengine#3692. ## Solution - Remove / change the tuple structs inside of `gamepad.rs` of `bevy_input` to normal structs. ## Reasons - It made the `gamepad_connection_system` cleaner. - It made the `gamepad_input_events.rs` example cleaner (which is probably the most notable change for the user facing API). - Tuple structs are not descriptive (`.0`, `.1`). - Using tuple structs for more than 1 field is a bad idea (This means that the `Gamepad` type might be fine as a tuple struct, but I still prefer normal structs over tuple structs). Feel free to discuss this change as this is more or less just a matter of taste. ## Changelog ### Changed - The `Gamepad`, `GamepadButton`, `GamepadAxis`, `GamepadEvent` and `GamepadEventRaw` types are now normal structs instead of tuple structs and have a `new()` function. ## Migration Guide - The `Gamepad`, `GamepadButton`, `GamepadAxis`, `GamepadEvent` and `GamepadEventRaw` types are now normal structs instead of tuple structs and have a `new()` function. To migrate change every instantiation to use the `new()` function instead and use the appropriate field names instead of `.0` and `.1`.

# Objective - Part of the splitting process of bevyengine#3692. ## Solution - Document `keyboard.rs` inside of `bevy_input`. Co-authored-by: KDecay <KDecayMusic@protonmail.com>

# Objective - Part of the splitting process of bevyengine#3692. ## Solution - Change the `gamepad_connection_system` to run after the `InputSystem` label. ## Reasons I changed the `gamepad_connection_system` to run after the `InputSystem` instead of in parallel, because this system checks the `GamepadEvent`s which get send inside of the `gamepad_event_system`. This means that the `gamepad_connection_system` could respond to the events one frame later, instead of instantly resulting in one frame lag. Old possible case: 1. `gamepad_connection_system` (reacts to the `GamepadEvent`s too early) 2. `gamepad_event_system` (sends the `GamepadEvent`s) New fixed ordering: 1. `gamepad_event_system` (sends the `GamepadEvent`s) 2. `gamepad_connection_system` (reacts to the `GamepadEvent`s)

# Objective - Fixes #5544 - Part of the splitting process of #3692. ## Solution - Document everything in the `gamepad.rs` file. - Add a doc example for mocking gamepad input. --- ## Changelog - Added and updated the documentation inside of the `gamepad.rs` file.

# Objective - Fixes bevyengine#5544 - Part of the splitting process of bevyengine#3692. ## Solution - Document everything in the `gamepad.rs` file. - Add a doc example for mocking gamepad input. --- ## Changelog - Added and updated the documentation inside of the `gamepad.rs` file.

# Objective - Part of the splitting process of bevyengine#3692. ## Solution - Change the `gamepad_connection_system` to run after the `InputSystem` label. ## Reasons I changed the `gamepad_connection_system` to run after the `InputSystem` instead of in parallel, because this system checks the `GamepadEvent`s which get send inside of the `gamepad_event_system`. This means that the `gamepad_connection_system` could respond to the events one frame later, instead of instantly resulting in one frame lag. Old possible case: 1. `gamepad_connection_system` (reacts to the `GamepadEvent`s too early) 2. `gamepad_event_system` (sends the `GamepadEvent`s) New fixed ordering: 1. `gamepad_event_system` (sends the `GamepadEvent`s) 2. `gamepad_connection_system` (reacts to the `GamepadEvent`s)

# Objective - Part of the splitting process of bevyengine#3692. ## Solution - Document `touch.rs` inside of `bevy_input`.

# Objective - Part of the splitting process of bevyengine#3692. ## Solution - Document `axis.rs` inside of `bevy_input`.

# Objective - Part of the splitting process of bevyengine#3692. ## Solution - Document `system.rs` inside of `bevy_input`.

@alice-i-cecile

# Objective - Part of the splitting process of bevyengine#3692. ## Solution - Rename `ElementState` to `ButtonState` ## Reasons - The old name was too generic. - If something can be pressed it is automatically button-like (thanks to @alice-i-cecile for bringing it up in bevyengine#3692). - The reason it is called `ElementState` is because that's how `winit` calls it. - It is used to define if a keyboard or mouse **button** is pressed or not. - Discussion in bevyengine#3692. ## Changelog ### Changed - The `ElementState` type received a rename and is now called `ButtonState`. ## Migration Guide - The `ElementState` type received a rename and is now called `ButtonState`. To migrate you just have to change every occurrence of `ElementState` to `ButtonState`. Co-authored-by: KDecay <KDecayMusic@protonmail.com>

# Objective - Part of the splitting process of bevyengine#3692. ## Solution - Document `input.rs` inside of `bevy_input`.

# Objective - Part of the splitting process of bevyengine#3692. ## Solution - Add more tests to `input.rs` inside of `bevy_input`. ## Note - The tests would now catch a change like bevyengine#4410 and fail accordingly.

# Objective - Part of the splitting process of bevyengine#3692. ## Solution - Document `mouse.rs` inside of `bevy_input`. Co-authored-by: KDecay <KDecayMusic@protonmail.com>

# Objective - Part of the splitting process of bevyengine#3692. ## Solution - Remove / change the tuple structs inside of `gamepad.rs` of `bevy_input` to normal structs. ## Reasons - It made the `gamepad_connection_system` cleaner. - It made the `gamepad_input_events.rs` example cleaner (which is probably the most notable change for the user facing API). - Tuple structs are not descriptive (`.0`, `.1`). - Using tuple structs for more than 1 field is a bad idea (This means that the `Gamepad` type might be fine as a tuple struct, but I still prefer normal structs over tuple structs). Feel free to discuss this change as this is more or less just a matter of taste. ## Changelog ### Changed - The `Gamepad`, `GamepadButton`, `GamepadAxis`, `GamepadEvent` and `GamepadEventRaw` types are now normal structs instead of tuple structs and have a `new()` function. ## Migration Guide - The `Gamepad`, `GamepadButton`, `GamepadAxis`, `GamepadEvent` and `GamepadEventRaw` types are now normal structs instead of tuple structs and have a `new()` function. To migrate change every instantiation to use the `new()` function instead and use the appropriate field names instead of `.0` and `.1`.

# Objective - Part of the splitting process of bevyengine#3692. ## Solution - Document `keyboard.rs` inside of `bevy_input`. Co-authored-by: KDecay <KDecayMusic@protonmail.com>

kdecay added 2 commits January 16, 2022 08:16

Added docs to and refactor of bevy_input.

bedb48b

Continued updating bevy_input. Small fixes.

Merge branch 'main' into update_bevy_input_docs

96e7393

github-actions bot added the S-Needs-Triage This issue needs to be labelled label Jan 16, 2022

james7132 added A-Input Player input via keyboard, mouse, gamepad, and more C-Docs An addition or correction to our documentation and removed S-Needs-Triage This issue needs to be labelled labels Jan 16, 2022

ghost mentioned this pull request Jan 17, 2022

Document how to configure gamepads #3246

Open

alice-i-cecile reviewed Jan 18, 2022

View reviewed changes

alice-i-cecile requested changes Jan 18, 2022

View reviewed changes

kdecay added 2 commits January 19, 2022 20:46

Small changes, added strum and ElementState rename

13b16d2

Fixed doc links.

4072fa7

alice-i-cecile added the C-Code-Quality A section of code that is hard to understand or change label Jan 19, 2022

alice-i-cecile approved these changes Jan 19, 2022

View reviewed changes

alice-i-cecile added the M-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Jan 19, 2022

KDecay added 2 commits February 4, 2022 20:05

Merge branch 'main' into update_bevy_input_docs

83b959f

ButtonState instead of ElementState

57028b5

Merge branch 'main' into update_bevy_input_docs

f58ae88

Merge branch 'main' into update_bevy_input_docs

0ee8a80

Remove EnumIter and strum dependency

20c21a6

bors bot pushed a commit that referenced this pull request May 17, 2022

Update keyboard.rs docs in bevy_input (#4517)

85dd291

# Objective - Part of the splitting process of #3692. ## Solution - Document `keyboard.rs` inside of `bevy_input`. Co-authored-by: KDecay <KDecayMusic@protonmail.com>

exjam pushed a commit to exjam/bevy that referenced this pull request May 22, 2022

Update system.rs docs in bevy_input (bevyengine#4524)

382c3d0

# Objective - Part of the splitting process of bevyengine#3692. ## Solution - Document `system.rs` inside of `bevy_input`.

exjam pushed a commit to exjam/bevy that referenced this pull request May 22, 2022

Update input.rs docs in bevy_input (bevyengine#4521)

ef68c8c

# Objective - Part of the splitting process of bevyengine#3692. ## Solution - Document `input.rs` inside of `bevy_input`.

This was referenced Aug 2, 2022

GamepadEventsRaw vs GamepadEvents is confusing and undocumented #5544

Closed

[Merged by Bors] - Document gamepad.rs #5548

Closed

ghost deleted the update_bevy_input_docs branch August 7, 2022 08:27

ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023

Update touch.rs docs (bevyengine#4523)

cd2be74

# Objective - Part of the splitting process of bevyengine#3692. ## Solution - Document `touch.rs` inside of `bevy_input`.

ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023

Update axis.rs docs in bevy_input (bevyengine#4525)

763d40a

# Objective - Part of the splitting process of bevyengine#3692. ## Solution - Document `axis.rs` inside of `bevy_input`.

ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023

Update system.rs docs in bevy_input (bevyengine#4524)

f1b3560

# Objective - Part of the splitting process of bevyengine#3692. ## Solution - Document `system.rs` inside of `bevy_input`.

ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023

Update input.rs docs in bevy_input (bevyengine#4521)

0b9fd01

# Objective - Part of the splitting process of bevyengine#3692. ## Solution - Document `input.rs` inside of `bevy_input`.

This pull request was closed.

Uh oh!

Conversation

ghost commented Jan 16, 2022 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Objective

Solution

Additional work

Tuple structs

Constructors

Structure change

Logic change

Removed constants

Renamed ElementState

Breaking Changes

Uh oh!

alice-i-cecile Jan 18, 2022

Choose a reason for hiding this comment

Uh oh!

alice-i-cecile Jan 18, 2022

Choose a reason for hiding this comment

Uh oh!

ghost Jan 19, 2022

Choose a reason for hiding this comment

Uh oh!

alice-i-cecile Jan 19, 2022

Choose a reason for hiding this comment

Uh oh!

alice-i-cecile left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alice-i-cecile left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented Jan 20, 2022

Uh oh!

alice-i-cecile commented Jan 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Jan 20, 2022

Uh oh!

alice-i-cecile commented Jan 20, 2022

Uh oh!

alice-i-cecile commented Feb 4, 2022

Uh oh!

mockersf commented Feb 15, 2022

Uh oh!

ghost commented Feb 15, 2022 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mockersf commented Feb 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alice-i-cecile commented Feb 24, 2022

Uh oh!

alice-i-cecile commented Feb 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Feb 25, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ghost commented Jan 16, 2022 •

edited by ghost

Loading

Renamed `ElementState`

alice-i-cecile left a comment •

edited

Loading

alice-i-cecile commented Jan 20, 2022 •

edited

Loading

ghost commented Feb 15, 2022 •

edited by ghost

Loading

mockersf commented Feb 15, 2022 •

edited

Loading

alice-i-cecile commented Feb 25, 2022 •

edited

Loading