From a513ad0597a17aaef96eb1b4fe3bcc7a001ef51f Mon Sep 17 00:00:00 2001 From: Periwink Date: Mon, 10 Jun 2024 11:05:25 -0400 Subject: [PATCH] Handle `just_pressed`/`just_released` in the `FixedUpdate` schedule (#522) * both tests pass * simplify implementation * clippy * update benchmarks * add consume tests * fmt * clean up tests * fmt * remove disable/enable logic * update * update timing test * fix tests for timing feature * lint * add release notes * lint --------- Co-authored-by: Charles Bournhonesque --- RELEASES.md | 3 + benches/action_state.rs | 2 + src/action_state.rs | 28 +++++ src/plugin.rs | 38 +++++- src/systems.rs | 32 +++++ tests/fixed_update.rs | 266 ++++++++++++++++++++++++++++++++++++++++ 6 files changed, 367 insertions(+), 2 deletions(-) create mode 100644 tests/fixed_update.rs diff --git a/RELEASES.md b/RELEASES.md index 5b3abc9a..aef93d96 100644 --- a/RELEASES.md +++ b/RELEASES.md @@ -21,6 +21,9 @@ ### Enhancements +- Inputs are now handled correctly in the `FixedUpdate` schedule! Previously, the `ActionState`s were only updated in the `PreUpdate` schedule, so you could have situations where an action was + marked as `just_pressed` multiple times in a row (if the `FixedUpdate` schedule ran multiple times in a frame) or was missed entirely (if the `FixedUpdate` schedule ran 0 times in a frame). + #### Input Processors Input processors allow you to create custom logic for axis-like input manipulation. diff --git a/benches/action_state.rs b/benches/action_state.rs index 36f305931..6531333d 100644 --- a/benches/action_state.rs +++ b/benches/action_state.rs @@ -64,6 +64,8 @@ fn criterion_benchmark(c: &mut Criterion) { action, ActionData { state: ButtonState::JustPressed, + update_state: ButtonState::Released, + fixed_update_state: ButtonState::Released, value: 0.0, axis_pair: None, #[cfg(feature = "timing")] diff --git a/src/action_state.rs b/src/action_state.rs index 2a1990da..d56a00fe 100644 --- a/src/action_state.rs +++ b/src/action_state.rs @@ -21,6 +21,10 @@ use serde::{Deserialize, Serialize}; pub struct ActionData { /// Is the action pressed or released? pub state: ButtonState, + /// The `state` of the action in the `Main` schedule + pub update_state: ButtonState, + /// The `state` of the action in the `FixedMain` schedule + pub fixed_update_state: ButtonState, /// The "value" of the binding that triggered the action. /// /// See [`ActionState::value`] for more details. @@ -136,6 +140,30 @@ impl Default for ActionState { } impl ActionState { + /// We are about to enter the `Main` schedule, so we: + /// - save all the changes applied to `state` into the `fixed_update_state` + /// - switch to loading the `update_state` + pub(crate) fn swap_to_update_state(&mut self) { + for (_action, action_datum) in self.action_data.iter_mut() { + // save the changes applied to `state` into `fixed_update_state` + action_datum.fixed_update_state = action_datum.state; + // switch to loading the `update_state` into `state` + action_datum.state = action_datum.update_state; + } + } + + /// We are about to enter the `FixedMain` schedule, so we: + /// - save all the changes applied to `state` into the `update_state` + /// - switch to loading the `fixed_update_state` + pub(crate) fn swap_to_fixed_update_state(&mut self) { + for (_action, action_datum) in self.action_data.iter_mut() { + // save the changes applied to `state` into `update_state` + action_datum.update_state = action_datum.state; + // switch to loading the `fixed_update_state` into `state` + action_datum.state = action_datum.fixed_update_state; + } + } + /// Updates the [`ActionState`] based on a vector of [`ActionData`], ordered by [`Actionlike::id`](Actionlike). /// /// The `action_data` is typically constructed from [`InputMap::which_pressed`](crate::input_map::InputMap), diff --git a/src/plugin.rs b/src/plugin.rs index 542eea7f..904b1dca 100644 --- a/src/plugin.rs +++ b/src/plugin.rs @@ -17,11 +17,12 @@ use core::hash::Hash; use core::marker::PhantomData; use std::fmt::Debug; -use bevy::app::{App, Plugin}; +use bevy::app::{App, Plugin, RunFixedMainLoop}; use bevy::ecs::prelude::*; use bevy::input::{ButtonState, InputSystem}; -use bevy::prelude::{PostUpdate, PreUpdate}; +use bevy::prelude::{FixedPostUpdate, PostUpdate, PreUpdate}; use bevy::reflect::TypePath; +use bevy::time::run_fixed_main_schedule; #[cfg(feature = "ui")] use bevy::ui::UiSystem; @@ -100,6 +101,7 @@ impl Plugin match self.machine { Machine::Client => { + // Main schedule app.add_systems( PreUpdate, tick_action_state:: @@ -142,6 +144,38 @@ impl Plugin update_action_state_from_interaction:: .in_set(InputManagerSystem::ManualControl), ); + + // FixedMain schedule + app.add_systems( + RunFixedMainLoop, + ( + swap_to_fixed_update::, + // we want to update the ActionState only once, even if the FixedMain schedule runs multiple times + update_action_state::, + ) + .chain() + .before(run_fixed_main_schedule), + ); + + #[cfg(feature = "ui")] + app.configure_sets(bevy::app::FixedPreUpdate, InputManagerSystem::ManualControl); + #[cfg(feature = "ui")] + app.add_systems( + bevy::app::FixedPreUpdate, + update_action_state_from_interaction:: + .in_set(InputManagerSystem::ManualControl), + ); + app.add_systems(FixedPostUpdate, release_on_input_map_removed::); + app.add_systems( + FixedPostUpdate, + tick_action_state:: + .in_set(InputManagerSystem::Tick) + .before(InputManagerSystem::Update), + ); + app.add_systems( + RunFixedMainLoop, + swap_to_update::.after(run_fixed_main_schedule), + ); } Machine::Server => { app.add_systems( diff --git a/src/systems.rs b/src/systems.rs index f88ab088..181b9325 100644 --- a/src/systems.rs +++ b/src/systems.rs @@ -28,6 +28,38 @@ use bevy::ui::Interaction; #[cfg(feature = "egui")] use bevy_egui::EguiContext; +/// We are about to enter the `Main` schedule, so we: +/// - save all the changes applied to `state` into the `fixed_update_state` +/// - switch to loading the `update_state` +pub fn swap_to_update( + mut query: Query<&mut ActionState>, + action_state: Option>>, +) { + if let Some(mut action_state) = action_state { + action_state.swap_to_update_state(); + } + + for mut action_state in query.iter_mut() { + action_state.swap_to_update_state(); + } +} + +/// We are about to enter the `FixedMain` schedule, so we: +/// - save all the changes applied to `state` into the `update_state` +/// - switch to loading the `fixed_update_state` +pub fn swap_to_fixed_update( + mut query: Query<&mut ActionState>, + action_state: Option>>, +) { + if let Some(mut action_state) = action_state { + action_state.swap_to_fixed_update_state(); + } + + for mut action_state in query.iter_mut() { + action_state.swap_to_fixed_update_state(); + } +} + /// Advances actions timer. /// /// Clears the just-pressed and just-released values of all [`ActionState`]s. diff --git a/tests/fixed_update.rs b/tests/fixed_update.rs new file mode 100644 index 00000000..71223dea --- /dev/null +++ b/tests/fixed_update.rs @@ -0,0 +1,266 @@ +use bevy::input::InputPlugin; +use bevy::prelude::*; +use bevy::time::TimeUpdateStrategy; +use bevy::MinimalPlugins; +use leafwing_input_manager::action_state::ActionState; +use leafwing_input_manager::input_map::InputMap; +use leafwing_input_manager::input_mocking::MockInput; +use leafwing_input_manager::plugin::{InputManagerPlugin, InputManagerSystem}; +use leafwing_input_manager_macros::Actionlike; +use std::time::Duration; + +#[derive(Actionlike, Clone, Copy, Debug, Reflect, PartialEq, Eq, Hash)] +enum TestAction { + Up, +} + +#[derive(Resource, Default)] +struct UpdateCounter { + just_pressed: usize, +} + +#[derive(Resource, Default)] +struct FixedUpdateCounter { + /// how many times did the FixedUpdate schedule run? + run: usize, + /// how many times did the Up button get just_pressed? + just_pressed: usize, +} + +fn build_app(fixed_timestep: Duration, frame_timestep: Duration) -> App { + let mut app = App::new(); + app.add_plugins(MinimalPlugins) + .add_plugins(InputPlugin) + .add_plugins(InputManagerPlugin::::default()) + .init_resource::() + .init_resource::() + .init_resource::>() + .insert_resource(InputMap::::new([( + TestAction::Up, + KeyCode::ArrowUp, + )])) + .insert_resource(Time::::from_duration(fixed_timestep)) + .insert_resource(TimeUpdateStrategy::ManualDuration(frame_timestep)); + + app.add_systems(Update, update_counter); + app.add_systems(FixedUpdate, fixed_update_counter); + + // we have to set an initial time for TimeUpdateStrategy::ManualDuration to work properly + let startup = app.world().resource::>().startup(); + app.world_mut() + .resource_mut::>() + .update_with_instant(startup); + app +} + +/// Keep track of some events that happen in the FixedUpdate schedule +/// - did the FixedUpdate schedule run during the frame? +/// - was the button marked as `just_pressed` in the FixedUpdate schedule? +fn fixed_update_counter( + mut counter: ResMut, + action: Res>, +) { + if action.just_pressed(&TestAction::Up) { + counter.just_pressed += 1; + } + counter.run += 1; +} + +/// Keep track of some events that happen in the Update schedule +/// - was the button marked as `just_pressed` in the Update schedule? +fn update_counter(mut counter: ResMut, action: Res>) { + if action.just_pressed(&TestAction::Up) { + counter.just_pressed += 1; + } +} + +/// Reset the counters at the end of the frame +fn reset_counters(app: &mut App) { + let mut fixed_update_counters = app.world_mut().resource_mut::(); + fixed_update_counters.run = 0; + fixed_update_counters.just_pressed = 0; + let mut update_counters = app.world_mut().resource_mut::(); + update_counters.just_pressed = 0; +} + +/// Assert that the FixedUpdate schedule run the expected number of times +fn check_fixed_update_run_count(app: &mut App, expected: usize) { + assert_eq!( + app.world() + .get_resource::() + .unwrap() + .run, + expected, + "FixedUpdate schedule should have run {} times", + expected + ); +} + +/// Assert that the button was just_pressed the expected number of times during the FixedUpdate schedule +fn check_fixed_update_just_pressed_count(app: &mut App, expected: usize) { + assert_eq!( + app.world() + .get_resource::() + .unwrap() + .just_pressed, + expected, + "Button should have been just_pressed {} times during the FixedUpdate schedule", + expected + ); +} + +/// Assert that the button was just_pressed the expected number of times during the Update schedule +fn check_update_just_pressed_count(app: &mut App, expected: usize) { + assert_eq!( + app.world() + .get_resource::() + .unwrap() + .just_pressed, + expected, + "Button should have been just_pressed {} times during the Update schedule", + expected + ); +} + +/// We have 2 frames without a FixedUpdate schedule in between (F1 - F2 - FU - F3) +/// +/// A button pressed in F1 should still be marked as `just_pressed` in FU +#[test] +fn frame_without_fixed_timestep() { + let mut app = build_app(Duration::from_millis(10), Duration::from_millis(9)); + + app.press_input(KeyCode::ArrowUp); + + // Frame 1: button is just_pressed and the FixedUpdate schedule does not run + app.update(); + check_update_just_pressed_count(&mut app, 1); + check_fixed_update_run_count(&mut app, 0); + reset_counters(&mut app); + + // Frame 2: the FixedUpdate schedule should run once, the button is `just_pressed` for FixedUpdate + // because we tick independently in FixedUpdate and Update. + // Button is not `just_pressed` anymore in the Update schedule. + app.update(); + check_update_just_pressed_count(&mut app, 0); + check_fixed_update_run_count(&mut app, 1); + check_fixed_update_just_pressed_count(&mut app, 1); + reset_counters(&mut app); + + // Frame 3: the FixedUpdate schedule should run once, the button should now be `pressed` in both schedules + app.update(); + check_update_just_pressed_count(&mut app, 0); + check_fixed_update_run_count(&mut app, 1); + check_fixed_update_just_pressed_count(&mut app, 0); + reset_counters(&mut app); + + // make sure that the timings didn't get updated twice (once in Update and once in FixedUpdate) + // (the `tick` function has been called twice, but it uses `Time` to update the time, + // which is only updated in `PreUpdate`, which is what we want) + #[cfg(feature = "timing")] + assert_eq!( + app.world() + .get_resource::>() + .unwrap() + .current_duration(&TestAction::Up), + Duration::from_millis(18) + ); +} + +/// We have a frame with two FixedUpdate schedule executions in between (F1 - FU1 - FU2 - F2) +/// +/// A button pressed in F1 should still be marked as `just_pressed` in FU1, and should just be +/// `pressed` in FU2 +#[test] +fn frame_with_two_fixed_timestep() { + let mut app = build_app(Duration::from_millis(4), Duration::from_millis(9)); + + app.press_input(KeyCode::ArrowUp); + + // Frame 1: the FixedUpdate schedule should run twice, but the button should be just_pressed only once + // in FixedUpdate + app.update(); + check_update_just_pressed_count(&mut app, 1); + check_fixed_update_run_count(&mut app, 2); + check_fixed_update_just_pressed_count(&mut app, 1); + reset_counters(&mut app); + + // Frame 2: the FixedUpdate schedule should run twice, but the buttton is not just_pressed anymore + app.update(); + check_update_just_pressed_count(&mut app, 0); + check_fixed_update_run_count(&mut app, 2); + check_fixed_update_just_pressed_count(&mut app, 0); + reset_counters(&mut app); + + // make sure that the timings didn't get updated twice (once in Update and once in FixedUpdate) + // (the `tick` function has been called twice, but it uses `Time` to update the time, + // which is only updated in `PreUpdate`, which is what we want) + #[cfg(feature = "timing")] + assert_eq!( + app.world() + .get_resource::>() + .unwrap() + .current_duration(&TestAction::Up), + Duration::from_millis(18) + ); +} + +/// Check that if the action is consumed in FU1, it will still be consumed in F2. +/// (i.e. consuming is shared between the `FixedMain` and `Main` schedules) +#[test] +fn test_consume_in_fixed_update() { + let mut app = build_app(Duration::from_millis(5), Duration::from_millis(5)); + + app.add_systems( + FixedPostUpdate, + |mut action: ResMut>| { + action.consume(&TestAction::Up); + }, + ); + + app.press_input(KeyCode::ArrowUp); + + // Frame 1: the FixedUpdate schedule should run once and the button should be just_pressed only once + app.update(); + check_update_just_pressed_count(&mut app, 1); + check_fixed_update_run_count(&mut app, 1); + check_fixed_update_just_pressed_count(&mut app, 1); + reset_counters(&mut app); + + // the button should still be consumed, even after we exit the FixedUpdate schedule + assert!( + app.world() + .get_resource::>() + .unwrap() + .action_data(&TestAction::Up) + .unwrap() + .consumed, + ); +} + +/// Check that if the action is consumed in F1, it will still be consumed in FU1. +/// (i.e. consuming is shared between the `FixedMain` and `Main` schedules) +#[test] +fn test_consume_in_update() { + let mut app = build_app(Duration::from_millis(5), Duration::from_millis(5)); + + app.press_input(KeyCode::ArrowUp); + fn consume_action(mut action: ResMut>) { + action.consume(&TestAction::Up); + } + + app.add_systems( + PreUpdate, + consume_action.in_set(InputManagerSystem::ManualControl), + ); + + app.add_systems(FixedUpdate, |action: Res>| { + // check that the action is still consumed in the FixedMain schedule + assert!( + action.consumed(&TestAction::Up), + "Action should still be consumed in FixedUpdate" + ); + }); + + // the FixedUpdate schedule should run once + app.update(); +}