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

Separate gamepad state code from gamepad event code and other customizations #700

Merged
merged 3 commits into from
Oct 21, 2020

Conversation

simpuid
Copy link
Contributor

@simpuid simpuid commented Oct 18, 2020

  • Earlier implementation of bevy_gilrs controls both the state and event part of gamepad input. In this PR the gamepad state part is moved to bevy_input. Now bevy_gilrs generates raw events in form of GamepadEvent in FIRST stage and then bevy_input reads those events in EVENT_UPDATE stage and update the gamepad states stored as Input<GamepadButton>, Axis<GamepadButton> and Axis<GamepadAxis>.

  • Gilrs uses same events(the events exposed in the API) to maintain gamepad state, so there is no way to query the state of connected gamepad during the connection event. This makes querying for default states of buttons and axis on connection event redundant. So this PR simplifies the connection event code.

  • Internally, gilrs' buttons can be classified into 2 types:

    1. which takes continuous values between 0.0 and 1.0 (both inclusive) from platform code.
    2. which takes discrete pressed/released event from platform code.

    Both buttons can generate ButtonPressed, ButtonReleased and ButtonChange event in gilrs.
    For buttons of first type, values from this function is used to generate ButtonPressed and ButtonRelease event internally in gilrs.
    For buttons of second type, (pressed, released) is mapped to (1.0, 0.0) which generates ButtonChange event internally in gilrs.
    So this PR emulates the button press/release from only ButtonChange event in bevy_input crate (ButtonPressed and ButtonReleased becomes redundant)

  • Resource GamepadProperty holds the data required for this button press/release emulation. The data is stored as ButtonProperty struct and can be changed anytime, which was not possible in earlier implementation as GilrsBuilder::set_axis_to_btn can be called only during initialization. Moreover, user can set default struct as well as specific struct for each GamepadButton as required. Default value of gilrs is used as default.

  • AxisProperty is used to store the deadzones and threshold for GamepadAxis in the same way as ButtonProperty. Arbitrary default value is used for deadzone and default value of gilrs is used for threshold.

  • ButtonAxisProperty is used to store the deadzones and threshold for GamepadButton that behave as axis in the same way as ButtonProperty. Arbitrary default value is used for deadzone and default value of gilrs is used for threshold.

  • gamepad_input example is updated to print axis values only on change.

  • gamepad_input_event example is added which uses the raw events (ButtonChange and AxisChange)

  • Parameter of Axis<T>::get and Axis<T>::remove is changed from &T to T to make is more analogous to Input<T> (separate commit, can be reverted)

@simpuid simpuid force-pushed the gilrs_extra branch 2 times, most recently from 96b0aa5 to aaaa15f Compare October 18, 2020 20:10
@simpuid simpuid changed the title Separate gamepad state code from gamepad event code Separate gamepad state code from gamepad event code and other goodies Oct 18, 2020
@simpuid simpuid changed the title Separate gamepad state code from gamepad event code and other goodies Separate gamepad state code from gamepad event code and other customizations Oct 18, 2020
@memoryruins memoryruins added C-Code-Quality A section of code that is hard to understand or change A-Input Player input via keyboard, mouse, gamepad, and more labels Oct 20, 2020
stage::EVENT_UPDATE,
gilrs_update_system.thread_local_system(),
);
.add_startup_system_to_stage(
Copy link
Member

Choose a reason for hiding this comment

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

nit: this is equivalent to add_startup_system(...), which is the approach we use everywhere else for "adding to the default stage".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

}

#[derive(Debug, Clone)]
pub struct ButtonProperty {
Copy link
Member

Choose a reason for hiding this comment

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

"Property" already has meaning in Bevy (ex: the bevy_property crate). Can we rename everything that is XProperty to XSettings?

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 to *Setting

event: Res<Events<GamepadEvent>>,
properties: Res<GamepadProperty>,
) {
button.update();
Copy link
Member

Choose a reason for hiding this comment

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

nit: lets use a slightly more descriptive name here like button_input

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

mut button: ResMut<Input<GamepadButton>>,
mut axis: ResMut<Axis<GamepadAxis>>,
mut button_axis: ResMut<Axis<GamepadButton>>,
event: Res<Events<GamepadEvent>>,
Copy link
Member

Choose a reason for hiding this comment

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

nit: lets make this events because its a collection

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

gamepad_event_system.system(),
)
.add_system_to_stage(bevy_app::stage::EVENT_UPDATE, gamepad_event_system.system())
.init_resource::<Axis<GamepadButton>>()
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 redundant. We already init this on line 56

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed

println!("Pressed {:?}", GamepadButton(*gamepad, *button_type));
let gamepad_button = GamepadButton(*gamepad, *button_type);
if inputs.just_pressed(gamepad_button) {
println!("{:?} Pressed", gamepad_button);
} else if inputs.just_released(GamepadButton(*gamepad, *button_type)) {
Copy link
Member

Choose a reason for hiding this comment

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

you could use gamepad_button here too

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 to button_inputs

@@ -19,17 +20,24 @@ struct GamepadLobby {
gamepad_event_reader: EventReader<GamepadEvent>,
}

#[derive(Default)]
Copy link
Member

Choose a reason for hiding this comment

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

I think this example is a good way to test that the implementation works correctly, but I don't think its the best illustration of how people should actually use the api. I'd prefer it if we trimmed it down to be more like the other input examples. The GamepadLobby should probably stay as-is, but can we remove GamepadData (and all "previous value" testing), print pressed, released, and the Axis<GamepadButton> axis for a single button (instead of an array), and print a single Axis value? (printing each frame is fine)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now it prints press/release events for South button, value of button RightTrigger2, value for LeftStickX all in the same system gamepad_system

@simpuid
Copy link
Contributor Author

simpuid commented Oct 21, 2020

I shall merge all to a single commit before merge

@cart
Copy link
Member

cart commented Oct 21, 2020

Looks good to me! I prefer XSettings over XSetting because in each case, its a "collection of configurable things", but I think I'll just merge this now and then follow up with a pr.

@cart cart merged commit d01ba9e into bevyengine:master Oct 21, 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-Code-Quality A section of code that is hard to understand or change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants