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.
This commit is contained in:
Alice Cecile 2023-02-14 00:13:10 +00:00
parent 17ed41d707
commit 0a9c469d19
8 changed files with 19 additions and 54 deletions

View file

@ -317,7 +317,7 @@ impl App {
/// initial state. /// initial state.
/// ///
/// This also adds an [`OnUpdate`] system set for each state variant, /// 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<S>`] resource matches their label. /// These systems sets only run if the [`State<S>`] resource matches their label.
/// ///
/// If you would like to control how other systems run based on the current state, /// 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() { for variant in S::variants() {
main_schedule.configure_set( main_schedule.configure_set(
OnUpdate(variant.clone()) OnUpdate(variant.clone())
.in_base_set(CoreSet::StateTransitions) .in_base_set(CoreSet::Update)
.run_if(state_equals(variant)) .run_if(state_equals(variant))
.after(apply_state_transition::<S>), .after(apply_state_transition::<S>),
); );

View file

@ -5,7 +5,6 @@ use crate::{
condition::{BoxedCondition, Condition}, condition::{BoxedCondition, Condition},
graph_utils::{Ambiguity, Dependency, DependencyKind, GraphInfo}, graph_utils::{Ambiguity, Dependency, DependencyKind, GraphInfo},
set::{BoxedSystemSet, IntoSystemSet, SystemSet}, set::{BoxedSystemSet, IntoSystemSet, SystemSet},
state::{OnUpdate, States},
}, },
system::{BoxedSystem, IntoSystem, System}, 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 `Condition` will be evaluated at most once (per schedule run),
/// the first time a system in this set prepares to run. /// the first time a system in this set prepares to run.
fn run_if<P>(self, condition: impl Condition<P>) -> SystemSetConfig; fn run_if<P>(self, condition: impl Condition<P>) -> 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 /// Suppress warnings and errors that would result from systems in this set having ambiguities
/// (conflicting access but indeterminate order) with systems in `set`. /// (conflicting access but indeterminate order) with systems in `set`.
fn ambiguous_with<M>(self, set: impl IntoSystemSet<M>) -> SystemSetConfig; fn ambiguous_with<M>(self, set: impl IntoSystemSet<M>) -> SystemSetConfig;
@ -146,10 +143,6 @@ where
self.into_config().run_if(condition) self.into_config().run_if(condition)
} }
fn on_update(self, state: impl States) -> SystemSetConfig {
self.into_config().on_update(state)
}
fn ambiguous_with<M>(self, set: impl IntoSystemSet<M>) -> SystemSetConfig { fn ambiguous_with<M>(self, set: impl IntoSystemSet<M>) -> SystemSetConfig {
self.into_config().ambiguous_with(set) self.into_config().ambiguous_with(set)
} }
@ -190,10 +183,6 @@ impl IntoSystemSetConfig for BoxedSystemSet {
self.into_config().run_if(condition) self.into_config().run_if(condition)
} }
fn on_update(self, state: impl States) -> SystemSetConfig {
self.into_config().on_update(state)
}
fn ambiguous_with<M>(self, set: impl IntoSystemSet<M>) -> SystemSetConfig { fn ambiguous_with<M>(self, set: impl IntoSystemSet<M>) -> SystemSetConfig {
self.into_config().ambiguous_with(set) self.into_config().ambiguous_with(set)
} }
@ -270,10 +259,6 @@ impl IntoSystemSetConfig for SystemSetConfig {
self self
} }
fn on_update(self, state: impl States) -> SystemSetConfig {
self.in_set(OnUpdate(state))
}
fn ambiguous_with<M>(mut self, set: impl IntoSystemSet<M>) -> Self { fn ambiguous_with<M>(mut self, set: impl IntoSystemSet<M>) -> Self {
ambiguous_with(&mut self.graph_info, Box::new(set.into_system_set())); ambiguous_with(&mut self.graph_info, Box::new(set.into_system_set()));
self self
@ -310,8 +295,6 @@ pub trait IntoSystemConfig<Params>: sealed::IntoSystemConfig<Params> {
/// The `Condition` will be evaluated at most once (per schedule run), /// The `Condition` will be evaluated at most once (per schedule run),
/// when the system prepares to run. /// when the system prepares to run.
fn run_if<P>(self, condition: impl Condition<P>) -> SystemConfig; fn run_if<P>(self, condition: impl Condition<P>) -> 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 /// Suppress warnings and errors that would result from this system having ambiguities
/// (conflicting access but indeterminate order) with systems in `set`. /// (conflicting access but indeterminate order) with systems in `set`.
fn ambiguous_with<M>(self, set: impl IntoSystemSet<M>) -> SystemConfig; fn ambiguous_with<M>(self, set: impl IntoSystemSet<M>) -> SystemConfig;
@ -354,10 +337,6 @@ where
self.into_config().run_if(condition) self.into_config().run_if(condition)
} }
fn on_update(self, state: impl States) -> SystemConfig {
self.into_config().on_update(state)
}
fn ambiguous_with<M>(self, set: impl IntoSystemSet<M>) -> SystemConfig { fn ambiguous_with<M>(self, set: impl IntoSystemSet<M>) -> SystemConfig {
self.into_config().ambiguous_with(set) self.into_config().ambiguous_with(set)
} }
@ -398,10 +377,6 @@ impl IntoSystemConfig<()> for BoxedSystem<(), ()> {
self.into_config().run_if(condition) self.into_config().run_if(condition)
} }
fn on_update(self, state: impl States) -> SystemConfig {
self.into_config().on_update(state)
}
fn ambiguous_with<M>(self, set: impl IntoSystemSet<M>) -> SystemConfig { fn ambiguous_with<M>(self, set: impl IntoSystemSet<M>) -> SystemConfig {
self.into_config().ambiguous_with(set) self.into_config().ambiguous_with(set)
} }
@ -470,10 +445,6 @@ impl IntoSystemConfig<()> for SystemConfig {
self self
} }
fn on_update(self, state: impl States) -> Self {
self.in_set(OnUpdate(state))
}
fn ambiguous_with<M>(mut self, set: impl IntoSystemSet<M>) -> Self { fn ambiguous_with<M>(mut self, set: impl IntoSystemSet<M>) -> Self {
ambiguous_with(&mut self.graph_info, Box::new(set.into_system_set())); ambiguous_with(&mut self.graph_info, Box::new(set.into_system_set()));
self self
@ -549,11 +520,6 @@ where
self.into_configs().after(set) 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 /// Suppress warnings and errors that would result from these systems having ambiguities
/// (conflicting access but indeterminate order) with systems in `set`. /// (conflicting access but indeterminate order) with systems in `set`.
fn ambiguous_with<M>(self, set: impl IntoSystemSet<M>) -> SystemConfigs { fn ambiguous_with<M>(self, set: impl IntoSystemSet<M>) -> SystemConfigs {
@ -654,10 +620,6 @@ impl IntoSystemConfigs<()> for SystemConfigs {
self self
} }
fn on_update(self, state: impl States) -> Self {
self.in_set(OnUpdate(state))
}
fn chain(mut self) -> Self { fn chain(mut self) -> Self {
self.chained = true; self.chained = true;
self self

View file

@ -53,10 +53,11 @@ pub struct OnEnter<S: States>(pub S);
#[derive(ScheduleLabel, Clone, Debug, PartialEq, Eq, Hash)] #[derive(ScheduleLabel, Clone, Debug, PartialEq, Eq, Hash)]
pub struct OnExit<S: States>(pub S); pub struct OnExit<S: States>(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) /// This set, when created via `App::add_state`, is configured with both a base set and a run condition.
/// [condition](super::Condition) also exists for systems that need to run elsewhere. /// 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)] #[derive(SystemSet, Clone, Debug, PartialEq, Eq, Hash)]
pub struct OnUpdate<S: States>(pub S); pub struct OnUpdate<S: States>(pub S);

View file

@ -9,7 +9,7 @@ fn main() {
.add_plugins(DefaultPlugins.set(ImagePlugin::default_nearest())) // prevents blurry sprites .add_plugins(DefaultPlugins.set(ImagePlugin::default_nearest())) // prevents blurry sprites
.add_state::<AppState>() .add_state::<AppState>()
.add_system_to_schedule(OnEnter(AppState::Setup), load_textures) .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) .add_system_to_schedule(OnEnter(AppState::Finished), setup)
.run(); .run();
} }

View file

@ -44,7 +44,7 @@ fn main() {
.add_state::<AppState>() .add_state::<AppState>()
.add_startup_system(setup_system) .add_startup_system(setup_system)
.add_system(print_text_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 the cleanup systems
.add_system_to_schedule( .add_system_to_schedule(
OnExit(AppState::MainMenu), OnExit(AppState::MainMenu),

View file

@ -18,10 +18,10 @@ fn main() {
.add_system_to_schedule(OnEnter(AppState::Menu), setup_menu) .add_system_to_schedule(OnEnter(AppState::Menu), setup_menu)
// By contrast, on_update systems are stored in the main schedule, during CoreSet::Update, // By contrast, on_update systems are stored in the main schedule, during CoreSet::Update,
// and simply check the value of the `State<T>` resource to see if they should run each frame. // and simply check the value of the `State<T>` 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(OnExit(AppState::Menu), cleanup_menu)
.add_system_to_schedule(OnEnter(AppState::InGame), setup_game) .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(); .run();
} }

View file

@ -34,11 +34,11 @@ fn main() {
scoreboard_system, scoreboard_system,
spawn_bonus, spawn_bonus,
) )
.on_update(GameState::Playing), .in_set(OnUpdate(GameState::Playing)),
) )
.add_system_to_schedule(OnExit(GameState::Playing), teardown) .add_system_to_schedule(OnExit(GameState::Playing), teardown)
.add_system_to_schedule(OnEnter(GameState::GameOver), display_score) .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_to_schedule(OnExit(GameState::GameOver), teardown)
.add_system(bevy::window::close_on_esc) .add_system(bevy::window::close_on_esc)
.run(); .run();

View file

@ -62,7 +62,7 @@ mod splash {
// When entering the state, spawn everything needed for this screen // When entering the state, spawn everything needed for this screen
.add_system_to_schedule(OnEnter(GameState::Splash), splash_setup) .add_system_to_schedule(OnEnter(GameState::Splash), splash_setup)
// While in this state, run the `countdown` system // 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 // When exiting the state, despawn everything that was spawned for this screen
.add_system_to_schedule( .add_system_to_schedule(
OnExit(GameState::Splash), OnExit(GameState::Splash),
@ -134,7 +134,7 @@ mod game {
impl Plugin for GamePlugin { impl Plugin for GamePlugin {
fn build(&self, app: &mut App) { fn build(&self, app: &mut App) {
app.add_system_to_schedule(OnEnter(GameState::Game), game_setup) 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::<OnGameScreen>); .add_system_to_schedule(OnExit(GameState::Game), despawn_screen::<OnGameScreen>);
} }
} }
@ -283,7 +283,9 @@ mod menu {
OnEnter(MenuState::SettingsDisplay), OnEnter(MenuState::SettingsDisplay),
display_settings_menu_setup, display_settings_menu_setup,
) )
.add_system(setting_button::<DisplayQuality>.on_update(MenuState::SettingsDisplay)) .add_system(
setting_button::<DisplayQuality>.in_set(OnUpdate(MenuState::SettingsDisplay)),
)
.add_system_to_schedule( .add_system_to_schedule(
OnExit(MenuState::SettingsDisplay), OnExit(MenuState::SettingsDisplay),
despawn_screen::<OnDisplaySettingsMenuScreen>, despawn_screen::<OnDisplaySettingsMenuScreen>,
@ -293,13 +295,13 @@ mod menu {
OnEnter(MenuState::SettingsSound), OnEnter(MenuState::SettingsSound),
sound_settings_menu_setup, sound_settings_menu_setup,
) )
.add_system(setting_button::<Volume>.on_update(MenuState::SettingsSound)) .add_system(setting_button::<Volume>.in_set(OnUpdate(MenuState::SettingsSound)))
.add_system_to_schedule( .add_system_to_schedule(
OnExit(MenuState::SettingsSound), OnExit(MenuState::SettingsSound),
despawn_screen::<OnSoundSettingsMenuScreen>, despawn_screen::<OnSoundSettingsMenuScreen>,
) )
// Common systems to all screens that handles buttons behaviour // 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)));
} }
} }