From a2adf4f382e3322536929d8d6eb1540019fb11b0 Mon Sep 17 00:00:00 2001 From: Alice Cecile Date: Tue, 14 Feb 2023 00:13:10 +0000 Subject: [PATCH] Remove .on_update method to improve API consistency and clarity (#7667) # Objective Fixes #7632. As discussed in #7634, it can be quite challenging for users to intuit the mental model of how states now work. ## Solution Rather than change the behavior of the `OnUpdate` system set, instead work on making sure it's easy to understand what's going on. Two things have been done: 1. Remove the `.on_update` method from our bevy of system building traits. This was special-cased and made states feel much more magical than they need to. 2. Improve the docs for the `OnUpdate` system set. --- crates/bevy_app/src/app.rs | 4 +-- crates/bevy_ecs/src/schedule/config.rs | 38 -------------------------- crates/bevy_ecs/src/schedule/state.rs | 7 +++-- examples/2d/texture_atlas.rs | 2 +- examples/ecs/generic_system.rs | 2 +- examples/ecs/state.rs | 4 +-- examples/games/alien_cake_addict.rs | 4 +-- examples/games/game_menu.rs | 12 ++++---- 8 files changed, 19 insertions(+), 54 deletions(-) diff --git a/crates/bevy_app/src/app.rs b/crates/bevy_app/src/app.rs index d364781109b07..fa019a009dd0d 100644 --- a/crates/bevy_app/src/app.rs +++ b/crates/bevy_app/src/app.rs @@ -317,7 +317,7 @@ impl App { /// initial state. /// /// This also adds an [`OnUpdate`] system set for each state variant, - /// which run during [`CoreSet::StateTransitions`] after the transitions are applied. + /// which run during [`CoreSet::Update`] after the transitions are applied. /// These systems sets only run if the [`State`] resource matches their label. /// /// If you would like to control how other systems run based on the current state, @@ -341,7 +341,7 @@ impl App { for variant in S::variants() { main_schedule.configure_set( OnUpdate(variant.clone()) - .in_base_set(CoreSet::StateTransitions) + .in_base_set(CoreSet::Update) .run_if(state_equals(variant)) .after(apply_state_transition::), ); diff --git a/crates/bevy_ecs/src/schedule/config.rs b/crates/bevy_ecs/src/schedule/config.rs index 0d27267b8d52d..e942d9af11c80 100644 --- a/crates/bevy_ecs/src/schedule/config.rs +++ b/crates/bevy_ecs/src/schedule/config.rs @@ -5,7 +5,6 @@ use crate::{ condition::{BoxedCondition, Condition}, graph_utils::{Ambiguity, Dependency, DependencyKind, GraphInfo}, set::{BoxedSystemSet, IntoSystemSet, SystemSet}, - state::{OnUpdate, States}, }, system::{BoxedSystem, IntoSystem, System}, }; @@ -102,8 +101,6 @@ pub trait IntoSystemSetConfig: sealed::IntoSystemSetConfig { /// The `Condition` will be evaluated at most once (per schedule run), /// the first time a system in this set prepares to run. fn run_if

(self, condition: impl Condition

) -> SystemSetConfig; - /// Add this set to the [`OnUpdate(state)`](OnUpdate) set. - fn on_update(self, state: impl States) -> SystemSetConfig; /// Suppress warnings and errors that would result from systems in this set having ambiguities /// (conflicting access but indeterminate order) with systems in `set`. fn ambiguous_with(self, set: impl IntoSystemSet) -> SystemSetConfig; @@ -146,10 +143,6 @@ where self.into_config().run_if(condition) } - fn on_update(self, state: impl States) -> SystemSetConfig { - self.into_config().on_update(state) - } - fn ambiguous_with(self, set: impl IntoSystemSet) -> SystemSetConfig { self.into_config().ambiguous_with(set) } @@ -190,10 +183,6 @@ impl IntoSystemSetConfig for BoxedSystemSet { self.into_config().run_if(condition) } - fn on_update(self, state: impl States) -> SystemSetConfig { - self.into_config().on_update(state) - } - fn ambiguous_with(self, set: impl IntoSystemSet) -> SystemSetConfig { self.into_config().ambiguous_with(set) } @@ -270,10 +259,6 @@ impl IntoSystemSetConfig for SystemSetConfig { self } - fn on_update(self, state: impl States) -> SystemSetConfig { - self.in_set(OnUpdate(state)) - } - fn ambiguous_with(mut self, set: impl IntoSystemSet) -> Self { ambiguous_with(&mut self.graph_info, Box::new(set.into_system_set())); self @@ -310,8 +295,6 @@ pub trait IntoSystemConfig: sealed::IntoSystemConfig { /// The `Condition` will be evaluated at most once (per schedule run), /// when the system prepares to run. fn run_if

(self, condition: impl Condition

) -> SystemConfig; - /// Add this system to the [`OnUpdate(state)`](OnUpdate) set. - fn on_update(self, state: impl States) -> SystemConfig; /// Suppress warnings and errors that would result from this system having ambiguities /// (conflicting access but indeterminate order) with systems in `set`. fn ambiguous_with(self, set: impl IntoSystemSet) -> SystemConfig; @@ -354,10 +337,6 @@ where self.into_config().run_if(condition) } - fn on_update(self, state: impl States) -> SystemConfig { - self.into_config().on_update(state) - } - fn ambiguous_with(self, set: impl IntoSystemSet) -> SystemConfig { self.into_config().ambiguous_with(set) } @@ -398,10 +377,6 @@ impl IntoSystemConfig<()> for BoxedSystem<(), ()> { self.into_config().run_if(condition) } - fn on_update(self, state: impl States) -> SystemConfig { - self.into_config().on_update(state) - } - fn ambiguous_with(self, set: impl IntoSystemSet) -> SystemConfig { self.into_config().ambiguous_with(set) } @@ -470,10 +445,6 @@ impl IntoSystemConfig<()> for SystemConfig { self } - fn on_update(self, state: impl States) -> Self { - self.in_set(OnUpdate(state)) - } - fn ambiguous_with(mut self, set: impl IntoSystemSet) -> Self { ambiguous_with(&mut self.graph_info, Box::new(set.into_system_set())); self @@ -549,11 +520,6 @@ where self.into_configs().after(set) } - /// Add this set to the [`OnUpdate(state)`](OnUpdate) set. - fn on_update(self, state: impl States) -> SystemConfigs { - self.into_configs().on_update(state) - } - /// Suppress warnings and errors that would result from these systems having ambiguities /// (conflicting access but indeterminate order) with systems in `set`. fn ambiguous_with(self, set: impl IntoSystemSet) -> SystemConfigs { @@ -654,10 +620,6 @@ impl IntoSystemConfigs<()> for SystemConfigs { self } - fn on_update(self, state: impl States) -> Self { - self.in_set(OnUpdate(state)) - } - fn chain(mut self) -> Self { self.chained = true; self diff --git a/crates/bevy_ecs/src/schedule/state.rs b/crates/bevy_ecs/src/schedule/state.rs index 4bc3ea9b09482..2ae15eb888e85 100644 --- a/crates/bevy_ecs/src/schedule/state.rs +++ b/crates/bevy_ecs/src/schedule/state.rs @@ -53,10 +53,11 @@ pub struct OnEnter(pub S); #[derive(ScheduleLabel, Clone, Debug, PartialEq, Eq, Hash)] pub struct OnExit(pub S); -/// A [`SystemSet`] that will run within `CoreSet::StateTransitions` when this state is active. +/// A [`SystemSet`] that will run within `CoreSet::Update` when this state is active. /// -/// This is provided for convenience. A more general [`state_equals`](crate::schedule::common_conditions::state_equals) -/// [condition](super::Condition) also exists for systems that need to run elsewhere. +/// This set, when created via `App::add_state`, is configured with both a base set and a run condition. +/// If all you want is the run condition, use the [`state_equals`](crate::schedule::common_conditions::state_equals) +/// [condition](super::Condition) directly. #[derive(SystemSet, Clone, Debug, PartialEq, Eq, Hash)] pub struct OnUpdate(pub S); diff --git a/examples/2d/texture_atlas.rs b/examples/2d/texture_atlas.rs index 4f4dd4800bc6a..fbb101426429d 100644 --- a/examples/2d/texture_atlas.rs +++ b/examples/2d/texture_atlas.rs @@ -9,7 +9,7 @@ fn main() { .add_plugins(DefaultPlugins.set(ImagePlugin::default_nearest())) // prevents blurry sprites .add_state::() .add_system_to_schedule(OnEnter(AppState::Setup), load_textures) - .add_system(check_textures.on_update(AppState::Setup)) + .add_system(check_textures.in_set(OnUpdate(AppState::Setup))) .add_system_to_schedule(OnEnter(AppState::Finished), setup) .run(); } diff --git a/examples/ecs/generic_system.rs b/examples/ecs/generic_system.rs index cdcb125202190..2427985b1a9e3 100644 --- a/examples/ecs/generic_system.rs +++ b/examples/ecs/generic_system.rs @@ -44,7 +44,7 @@ fn main() { .add_state::() .add_startup_system(setup_system) .add_system(print_text_system) - .add_system(transition_to_in_game_system.on_update(AppState::MainMenu)) + .add_system(transition_to_in_game_system.in_set(OnUpdate(AppState::MainMenu))) // add the cleanup systems .add_system_to_schedule( OnExit(AppState::MainMenu), diff --git a/examples/ecs/state.rs b/examples/ecs/state.rs index 4404e387294ea..1c38aac975552 100644 --- a/examples/ecs/state.rs +++ b/examples/ecs/state.rs @@ -18,10 +18,10 @@ fn main() { .add_system_to_schedule(OnEnter(AppState::Menu), setup_menu) // By contrast, on_update systems are stored in the main schedule, during CoreSet::Update, // and simply check the value of the `State` resource to see if they should run each frame. - .add_system(menu.on_update(AppState::Menu)) + .add_system(menu.in_set(OnUpdate(AppState::Menu))) .add_system_to_schedule(OnExit(AppState::Menu), cleanup_menu) .add_system_to_schedule(OnEnter(AppState::InGame), setup_game) - .add_systems((movement, change_color).on_update(AppState::InGame)) + .add_systems((movement, change_color).in_set(OnUpdate(AppState::InGame))) .run(); } diff --git a/examples/games/alien_cake_addict.rs b/examples/games/alien_cake_addict.rs index de8fc73c331d3..e3d20575aa89c 100644 --- a/examples/games/alien_cake_addict.rs +++ b/examples/games/alien_cake_addict.rs @@ -34,11 +34,11 @@ fn main() { scoreboard_system, spawn_bonus, ) - .on_update(GameState::Playing), + .in_set(OnUpdate(GameState::Playing)), ) .add_system_to_schedule(OnExit(GameState::Playing), teardown) .add_system_to_schedule(OnEnter(GameState::GameOver), display_score) - .add_system(gameover_keyboard.on_update(GameState::GameOver)) + .add_system(gameover_keyboard.in_set(OnUpdate(GameState::GameOver))) .add_system_to_schedule(OnExit(GameState::GameOver), teardown) .add_system(bevy::window::close_on_esc) .run(); diff --git a/examples/games/game_menu.rs b/examples/games/game_menu.rs index b7b4ae617ca57..c7ea05559e347 100644 --- a/examples/games/game_menu.rs +++ b/examples/games/game_menu.rs @@ -62,7 +62,7 @@ mod splash { // When entering the state, spawn everything needed for this screen .add_system_to_schedule(OnEnter(GameState::Splash), splash_setup) // While in this state, run the `countdown` system - .add_system(countdown.on_update(GameState::Splash)) + .add_system(countdown.in_set(OnUpdate(GameState::Splash))) // When exiting the state, despawn everything that was spawned for this screen .add_system_to_schedule( OnExit(GameState::Splash), @@ -134,7 +134,7 @@ mod game { impl Plugin for GamePlugin { fn build(&self, app: &mut App) { app.add_system_to_schedule(OnEnter(GameState::Game), game_setup) - .add_system(game.on_update(GameState::Game)) + .add_system(game.in_set(OnUpdate(GameState::Game))) .add_system_to_schedule(OnExit(GameState::Game), despawn_screen::); } } @@ -283,7 +283,9 @@ mod menu { OnEnter(MenuState::SettingsDisplay), display_settings_menu_setup, ) - .add_system(setting_button::.on_update(MenuState::SettingsDisplay)) + .add_system( + setting_button::.in_set(OnUpdate(MenuState::SettingsDisplay)), + ) .add_system_to_schedule( OnExit(MenuState::SettingsDisplay), despawn_screen::, @@ -293,13 +295,13 @@ mod menu { OnEnter(MenuState::SettingsSound), sound_settings_menu_setup, ) - .add_system(setting_button::.on_update(MenuState::SettingsSound)) + .add_system(setting_button::.in_set(OnUpdate(MenuState::SettingsSound))) .add_system_to_schedule( OnExit(MenuState::SettingsSound), despawn_screen::, ) // Common systems to all screens that handles buttons behaviour - .add_systems((menu_action, button_system).on_update(GameState::Menu)); + .add_systems((menu_action, button_system).in_set(OnUpdate(GameState::Menu))); } }