From 07aa9e564188fc0f39a69ce3e19199ea43076cf1 Mon Sep 17 00:00:00 2001 From: MiniaczQ Date: Sun, 2 Jun 2024 15:36:44 +0200 Subject: [PATCH] Combine transition systems of `Substates` (#13626) # Objective Prerequisite to #13579. Combine separate `Substates` transition systems to centralize transition logic and exert more control over it. ## Solution Originally the transition happened in 2 stages: - `apply_state_transition` in `ManualTransitions` handled `NextState`, - closure system in `DependentTransitions` handled parent-related changes, insertion and deletion of the substate. Now: - Both transitions are processed in a single closure system during `DependentTransitions`. - Since `Substates` no longer use `ManualTransitions`, it's been renamed to `RootTransitions`. Only root states use it. - When `Substates` state comes into existence, it will try to initialize from `NextState` and fallback to `should_exist` result. - Remove `apply_state_transition` from public API. Consequentially, removed the possibility of multiple `StateTransitionEvent`s when both transition systems fire in a single frame. ## Changelog - Renamed `ManualTransitions` to `RootTransitions`. - `Substates` will initialize their value with `NextState` if available and fallback to `should_exist` result. ## Migration Guide - `apply_state_transition` is no longer publicly available, run the `StateTransition` schedule instead. --------- Co-authored-by: Alice Cecile --- crates/bevy_state/macros/src/states.rs | 6 +- crates/bevy_state/src/lib.rs | 4 +- .../src/state/freely_mutable_state.rs | 30 ++- crates/bevy_state/src/state/resources.rs | 19 +- crates/bevy_state/src/state/state_set.rs | 172 ++++++++++-------- crates/bevy_state/src/state/sub_states.rs | 23 ++- crates/bevy_state/src/state/transitions.rs | 53 +----- 7 files changed, 161 insertions(+), 146 deletions(-) diff --git a/crates/bevy_state/macros/src/states.rs b/crates/bevy_state/macros/src/states.rs index 76a6cbcddf..7a3872fdd1 100644 --- a/crates/bevy_state/macros/src/states.rs +++ b/crates/bevy_state/macros/src/states.rs @@ -118,11 +118,7 @@ pub fn derive_substates(input: TokenStream) -> TokenStream { type SourceStates = #source_state_type; fn should_exist(sources: #source_state_type) -> Option { - if matches!(sources, #source_state_value) { - Some(Self::default()) - } else { - None - } + matches!(sources, #source_state_value).then_some(Self::default()) } } diff --git a/crates/bevy_state/src/lib.rs b/crates/bevy_state/src/lib.rs index 2104dcedcd..8555564e84 100644 --- a/crates/bevy_state/src/lib.rs +++ b/crates/bevy_state/src/lib.rs @@ -38,7 +38,7 @@ pub mod prelude { pub use crate::condition::*; #[doc(hidden)] pub use crate::state::{ - apply_state_transition, ComputedStates, NextState, OnEnter, OnExit, OnTransition, State, - StateSet, StateTransition, StateTransitionEvent, States, SubStates, + ComputedStates, NextState, OnEnter, OnExit, OnTransition, State, StateSet, StateTransition, + StateTransitionEvent, States, SubStates, }; } diff --git a/crates/bevy_state/src/state/freely_mutable_state.rs b/crates/bevy_state/src/state/freely_mutable_state.rs index c1e4e72e5c..5073b6b910 100644 --- a/crates/bevy_state/src/state/freely_mutable_state.rs +++ b/crates/bevy_state/src/state/freely_mutable_state.rs @@ -1,9 +1,13 @@ -use bevy_ecs::prelude::Schedule; use bevy_ecs::schedule::{IntoSystemConfigs, IntoSystemSetConfigs}; use bevy_ecs::system::IntoSystem; +use bevy_ecs::{ + event::EventWriter, + prelude::Schedule, + system::{Commands, ResMut}, +}; -use super::states::States; -use super::transitions::*; +use super::{states::States, NextState, State}; +use super::{take_next_state, transitions::*}; /// This trait allows a state to be mutated directly using the [`NextState`](crate::state::NextState) resource. /// @@ -32,8 +36,24 @@ pub trait FreelyMutableState: States { .in_set(StateTransitionSteps::TransitionSchedules), ) .configure_sets( - ApplyStateTransition::::apply() - .in_set(StateTransitionSteps::ManualTransitions), + ApplyStateTransition::::apply().in_set(StateTransitionSteps::RootTransitions), ); } } + +fn apply_state_transition( + event: EventWriter>, + commands: Commands, + current_state: Option>>, + next_state: Option>>, +) { + let Some(next_state) = take_next_state(next_state) else { + return; + }; + let Some(current_state) = current_state else { + return; + }; + if next_state != *current_state.get() { + internal_apply_state_transition(event, commands, Some(current_state), Some(next_state)); + } +} diff --git a/crates/bevy_state/src/state/resources.rs b/crates/bevy_state/src/state/resources.rs index 279ea8589c..66aa42bf14 100644 --- a/crates/bevy_state/src/state/resources.rs +++ b/crates/bevy_state/src/state/resources.rs @@ -1,7 +1,8 @@ use std::ops::Deref; use bevy_ecs::{ - system::Resource, + change_detection::DetectChangesMut, + system::{ResMut, Resource}, world::{FromWorld, World}, }; @@ -107,7 +108,7 @@ impl Deref for State { /// next_game_state.set(GameState::InGame); /// } /// ``` -#[derive(Resource, Debug, Default)] +#[derive(Resource, Debug, Default, Clone)] #[cfg_attr( feature = "bevy_reflect", derive(bevy_reflect::Reflect), @@ -132,3 +133,17 @@ impl NextState { *self = Self::Unchanged; } } + +pub(crate) fn take_next_state( + next_state: Option>>, +) -> Option { + let mut next_state = next_state?; + + match std::mem::take(next_state.bypass_change_detection()) { + NextState::Pending(x) => { + next_state.set_changed(); + Some(x) + } + NextState::Unchanged => None, + } +} diff --git a/crates/bevy_state/src/state/state_set.rs b/crates/bevy_state/src/state/state_set.rs index feb9f6e7ac..59c5efda7e 100644 --- a/crates/bevy_state/src/state/state_set.rs +++ b/crates/bevy_state/src/state/state_set.rs @@ -8,9 +8,9 @@ use bevy_utils::all_tuples; use self::sealed::StateSetSealed; use super::{ - apply_state_transition, computed_states::ComputedStates, internal_apply_state_transition, - last_transition, run_enter, run_exit, run_transition, sub_states::SubStates, - ApplyStateTransition, State, StateTransitionEvent, StateTransitionSteps, States, + computed_states::ComputedStates, internal_apply_state_transition, last_transition, run_enter, + run_exit, run_transition, sub_states::SubStates, take_next_state, ApplyStateTransition, + NextState, State, StateTransitionEvent, StateTransitionSteps, States, }; mod sealed { @@ -93,28 +93,29 @@ impl StateSet for S { fn register_computed_state_systems_in_schedule>( schedule: &mut Schedule, ) { - let system = |mut parent_changed: EventReader>, - event: EventWriter>, - commands: Commands, - current_state: Option>>, - state_set: Option>>| { - if parent_changed.is_empty() { - return; - } - parent_changed.clear(); + let apply_state_transition = + |mut parent_changed: EventReader>, + event: EventWriter>, + commands: Commands, + current_state: Option>>, + state_set: Option>>| { + if parent_changed.is_empty() { + return; + } + parent_changed.clear(); - let new_state = - if let Some(state_set) = S::convert_to_usable_state(state_set.as_deref()) { - T::compute(state_set) - } else { - None - }; + let new_state = + if let Some(state_set) = S::convert_to_usable_state(state_set.as_deref()) { + T::compute(state_set) + } else { + None + }; - internal_apply_state_transition(event, commands, current_state, new_state); - }; + internal_apply_state_transition(event, commands, current_state, new_state); + }; schedule - .add_systems(system.in_set(ApplyStateTransition::::apply())) + .add_systems(apply_state_transition.in_set(ApplyStateTransition::::apply())) .add_systems( last_transition:: .pipe(run_enter::) @@ -140,45 +141,55 @@ impl StateSet for S { fn register_sub_state_systems_in_schedule>( schedule: &mut Schedule, ) { - let system = |mut parent_changed: EventReader>, - event: EventWriter>, - commands: Commands, - current_state: Option>>, - state_set: Option>>| { - if parent_changed.is_empty() { - return; - } - parent_changed.clear(); + // | parent changed | next state | already exists | should exist | what happens | + // | -------------- | ---------- | -------------- | ------------ | -------------------------------- | + // | false | false | false | - | - | + // | false | false | true | - | - | + // | false | true | false | false | - | + // | true | false | false | false | - | + // | true | true | false | false | - | + // | true | false | true | false | Some(current) -> None | + // | true | true | true | false | Some(current) -> None | + // | true | false | false | true | None -> Some(default) | + // | true | true | false | true | None -> Some(next) | + // | true | true | true | true | Some(current) -> Some(next) | + // | false | true | true | true | Some(current) -> Some(next) | + // | true | false | true | true | Some(current) -> Some(current) | - let new_state = - if let Some(state_set) = S::convert_to_usable_state(state_set.as_deref()) { - T::should_exist(state_set) - } else { - None + let apply_state_transition = + |mut parent_changed: EventReader>, + event: EventWriter>, + commands: Commands, + current_state_res: Option>>, + next_state_res: Option>>, + state_set: Option>>| { + let parent_changed = parent_changed.read().last().is_some(); + let next_state = take_next_state(next_state_res); + + if !parent_changed && next_state.is_none() { + return; + } + + let current_state = current_state_res.as_ref().map(|s| s.get()).cloned(); + + let should_exist = match parent_changed { + true => { + if let Some(state_set) = S::convert_to_usable_state(state_set.as_deref()) { + T::should_exist(state_set) + } else { + None + } + } + false => current_state.clone(), }; - match new_state { - Some(value) => { - if current_state.is_none() { - internal_apply_state_transition( - event, - commands, - current_state, - Some(value), - ); - } - } - None => { - internal_apply_state_transition(event, commands, current_state, None); - } + let new_state = should_exist + .map(|initial_state| next_state.or(current_state).unwrap_or(initial_state)); + internal_apply_state_transition(event, commands, current_state_res, new_state); }; - }; schedule - .add_systems(system.in_set(ApplyStateTransition::::apply())) - .add_systems( - apply_state_transition::.in_set(StateTransitionSteps::ManualTransitions), - ) + .add_systems(apply_state_transition.in_set(ApplyStateTransition::::apply())) .add_systems( last_transition:: .pipe(run_enter::) @@ -214,7 +225,7 @@ macro_rules! impl_state_set_sealed_tuples { fn register_computed_state_systems_in_schedule>( schedule: &mut Schedule, ) { - let system = |($(mut $evt),*,): ($(EventReader>),*,), event: EventWriter>, commands: Commands, current_state: Option>>, ($($val),*,): ($(Option>>),*,)| { + let apply_state_transition = |($(mut $evt),*,): ($(EventReader>),*,), event: EventWriter>, commands: Commands, current_state: Option>>, ($($val),*,): ($(Option>>),*,)| { if ($($evt.is_empty())&&*) { return; } @@ -230,7 +241,7 @@ macro_rules! impl_state_set_sealed_tuples { }; schedule - .add_systems(system.in_set(ApplyStateTransition::::apply())) + .add_systems(apply_state_transition.in_set(ApplyStateTransition::::apply())) .add_systems(last_transition::.pipe(run_enter::).in_set(StateTransitionSteps::EnterSchedules)) .add_systems(last_transition::.pipe(run_exit::).in_set(StateTransitionSteps::ExitSchedules)) .add_systems(last_transition::.pipe(run_transition::).in_set(StateTransitionSteps::TransitionSchedules)) @@ -244,32 +255,49 @@ macro_rules! impl_state_set_sealed_tuples { fn register_sub_state_systems_in_schedule>( schedule: &mut Schedule, ) { - let system = |($(mut $evt),*,): ($(EventReader>),*,), event: EventWriter>, commands: Commands, current_state: Option>>, ($($val),*,): ($(Option>>),*,)| { - if ($($evt.is_empty())&&*) { + let apply_state_transition = |($(mut $evt),*,): ($(EventReader>),*,), event: EventWriter>, commands: Commands, current_state_res: Option>>, next_state_res: Option>>, ($($val),*,): ($(Option>>),*,)| { + let parent_changed = ($($evt.read().last().is_some())&&*); + let next_state = take_next_state(next_state_res); + + if !parent_changed && next_state.is_none() { return; } - $($evt.clear();)* - let new_state = if let ($(Some($val)),*,) = ($($param::convert_to_usable_state($val.as_deref())),*,) { - T::should_exist(($($val),*, )) - } else { - None - }; - match new_state { - Some(value) => { - if current_state.is_none() { - internal_apply_state_transition(event, commands, current_state, Some(value)); + let current_state = current_state_res.as_ref().map(|s| s.get()).cloned(); + + let should_exist = match parent_changed { + true => { + if let ($(Some($val)),*,) = ($($param::convert_to_usable_state($val.as_deref())),*,) { + T::should_exist(($($val),*, )) + } else { + None } } + false => current_state.clone(), + }; + + match should_exist { + Some(initial_state) => { + let new_state = match (current_state, next_state) { + (_, Some(next_state)) => next_state, + (Some(current_state), None) => current_state, + (None, None) => initial_state, + }; + internal_apply_state_transition( + event, + commands, + current_state_res, + Some(new_state), + ); + } None => { - internal_apply_state_transition(event, commands, current_state, None); - }, + internal_apply_state_transition(event, commands, current_state_res, None); + } }; }; schedule - .add_systems(system.in_set(ApplyStateTransition::::apply())) - .add_systems(apply_state_transition::.in_set(StateTransitionSteps::ManualTransitions)) + .add_systems(apply_state_transition.in_set(ApplyStateTransition::::apply())) .add_systems(last_transition::.pipe(run_enter::).in_set(StateTransitionSteps::EnterSchedules)) .add_systems(last_transition::.pipe(run_exit::).in_set(StateTransitionSteps::ExitSchedules)) .add_systems(last_transition::.pipe(run_transition::).in_set(StateTransitionSteps::TransitionSchedules)) diff --git a/crates/bevy_state/src/state/sub_states.rs b/crates/bevy_state/src/state/sub_states.rs index 8046a059b9..91ef1325b3 100644 --- a/crates/bevy_state/src/state/sub_states.rs +++ b/crates/bevy_state/src/state/sub_states.rs @@ -96,13 +96,11 @@ pub use bevy_state_macros::SubStates; /// ``` /// /// However, you can also manually implement them. If you do so, you'll also need to manually implement the `States` & `FreelyMutableState` traits. -/// Unlike the derive, this does not require an implementation of [`Default`], since you are providing the `exists` function -/// directly. /// /// ``` /// # use bevy_ecs::prelude::*; /// # use bevy_state::prelude::*; -/// # use bevy_state::state::FreelyMutableState; +/// # use bevy_state::state::{FreelyMutableState, NextState}; /// /// /// Computed States require some state to derive from /// #[derive(States, Clone, PartialEq, Eq, Hash, Debug, Default)] @@ -127,11 +125,10 @@ pub use bevy_state_macros::SubStates; /// /// We then define the compute function, which takes in the [`Self::SourceStates`] /// fn should_exist(sources: Option) -> Option { /// match sources { -/// /// When we are in game, so we want a GamePhase state to exist, and the default is -/// /// GamePhase::Setup -/// Some(AppState::InGame { .. }) => Some(GamePhase::Setup), -/// /// Otherwise, we don't want the `State` resource to exist, -/// /// so we return None. +/// /// When we are in game, we want a GamePhase state to exist. +/// /// We can set the initial value here or overwrite it through [`NextState`]. +/// Some(AppState::InGame { .. }) => Some(Self::Setup), +/// /// If we don't want the `State` resource to exist we return [`None`]. /// _ => None /// } /// } @@ -154,8 +151,14 @@ pub trait SubStates: States + FreelyMutableState { /// This function gets called whenever one of the [`SourceStates`](Self::SourceStates) changes. /// The result is used to determine the existence of [`State`](crate::state::State). /// - /// If the result is [`None`], the [`State`](crate::state::State) resource will be removed from the world, otherwise - /// if the [`State`](crate::state::State) resource doesn't exist - it will be created with the [`Some`] value. + /// If the result is [`None`], the [`State`](crate::state::State) resource will be removed from the world, + /// otherwise if the [`State`](crate::state::State) resource doesn't exist + /// it will be created from the returned [`Some`] as the initial state. + /// + /// Value within [`Some`] is ignored if the state already exists in the world + /// and only symbolises that the state should still exist. + /// + /// Initial value can also be overwritten by [`NextState`](crate::state::NextState). fn should_exist(sources: Self::SourceStates) -> Option; /// This function sets up systems that compute the state whenever one of the [`SourceStates`](Self::SourceStates) diff --git a/crates/bevy_state/src/state/transitions.rs b/crates/bevy_state/src/state/transitions.rs index bb3480765b..b816b4ad17 100644 --- a/crates/bevy_state/src/state/transitions.rs +++ b/crates/bevy_state/src/state/transitions.rs @@ -9,11 +9,7 @@ use bevy_ecs::{ world::World, }; -use super::{ - freely_mutable_state::FreelyMutableState, - resources::{NextState, State}, - states::States, -}; +use super::{resources::State, states::States}; /// The label of a [`Schedule`] that runs whenever [`State`] /// enters this state. @@ -57,7 +53,7 @@ pub struct StateTransitionEvent { /// These system sets are run sequentially, in the order of the enum variants. #[derive(SystemSet, Clone, Debug, PartialEq, Eq, Hash)] pub(crate) enum StateTransitionSteps { - ManualTransitions, + RootTransitions, DependentTransitions, ExitSchedules, TransitionSchedules, @@ -142,7 +138,7 @@ pub fn setup_state_transitions_in_world( let mut schedule = Schedule::new(StateTransition); schedule.configure_sets( ( - StateTransitionSteps::ManualTransitions, + StateTransitionSteps::RootTransitions, StateTransitionSteps::DependentTransitions, StateTransitionSteps::ExitSchedules, StateTransitionSteps::TransitionSchedules, @@ -159,49 +155,6 @@ pub fn setup_state_transitions_in_world( } } -/// If a new state is queued in [`NextState`], this system -/// takes the new state value from [`NextState`] and updates [`State`], as well as -/// sending the relevant [`StateTransitionEvent`]. -/// -/// If the [`State`] resource does not exist, it does nothing. Removing or adding states -/// should be done at App creation or at your own risk. -/// -/// For [`SubStates`](crate::state::SubStates) - it only applies the state if the `SubState` currently exists. Otherwise, it is wiped. -/// When a `SubState` is re-created, it will use the result of it's `should_exist` method. -pub fn apply_state_transition( - event: EventWriter>, - commands: Commands, - current_state: Option>>, - next_state: Option>>, -) { - // We want to check if the State and NextState resources exist - let Some(mut next_state_resource) = next_state else { - return; - }; - - match next_state_resource.as_ref() { - NextState::Pending(new_state) => { - if let Some(current_state) = current_state { - if new_state != current_state.get() { - let new_state = new_state.clone(); - internal_apply_state_transition( - event, - commands, - Some(current_state), - Some(new_state), - ); - } - } - } - NextState::Unchanged => { - // This is the default value, so we don't need to re-insert the resource - return; - } - } - - *next_state_resource.as_mut() = NextState::::Unchanged; -} - /// Returns the latest state transition event of type `S`, if any are available. pub fn last_transition( mut reader: EventReader>,