From 1f2fd3d29dfd854f25ae917ebe1c63d727a16340 Mon Sep 17 00:00:00 2001 From: mgi388 <135186256+mgi388@users.noreply.github.com> Date: Tue, 17 Jun 2025 07:34:22 +1000 Subject: [PATCH] Fix SubStates with multiple source states not reacting to all source changes (#19595) # Objective - Fix issue where `SubStates` depending on multiple source states would only react when _all_ source states changed simultaneously. - SubStates should be created/destroyed whenever _any_ of their source states transitions, not only when all change together. # Solution - Changed the "did parent change" detection logic from AND to OR. We need to check if _any_ of the event readers changed, not if _all_ of them changed. - See https://github.com/bevyengine/bevy/actions/runs/15610159742/job/43968937544?pr=19595 for failing test proof before I pushed the fix. - The generated code we want needs `||`s not `&&`s like this: ```rust fn register_sub_state_systems_in_schedule>(schedule: &mut Schedule) { let apply_state_transition = |(mut ereader0, mut ereader1, mut ereader2): ( EventReader>, EventReader>, EventReader>, ), event: EventWriter>, commands: Commands, current_state_res: Option>>, next_state_res: Option>>, (s0, s1, s2): ( Option>>, Option>>, Option>>, )| { // With `||` we can correctly count parent changed if any of the sources changed. let parent_changed = (ereader0.read().last().is_some() || ereader1.read().last().is_some() || ereader2.read().last().is_some()); let next_state = take_next_state(next_state_res); if !parent_changed && next_state.is_none() { return; } // ... } } ``` # Testing - Add new test. - Check the fix worked in my game. --- crates/bevy_state/src/state/mod.rs | 197 +++++++++++++++++++++++ crates/bevy_state/src/state/state_set.rs | 2 +- 2 files changed, 198 insertions(+), 1 deletion(-) diff --git a/crates/bevy_state/src/state/mod.rs b/crates/bevy_state/src/state/mod.rs index 9267478281..61ee0627a3 100644 --- a/crates/bevy_state/src/state/mod.rs +++ b/crates/bevy_state/src/state/mod.rs @@ -642,6 +642,203 @@ mod tests { } } + #[derive(PartialEq, Eq, Debug, Hash, Clone)] + enum MultiSourceComputedState { + FromSimpleBTrue, + FromSimple2B2, + FromBoth, + } + + impl ComputedStates for MultiSourceComputedState { + type SourceStates = (SimpleState, SimpleState2); + + fn compute((simple_state, simple_state2): (SimpleState, SimpleState2)) -> Option { + match (simple_state, simple_state2) { + // If both are in their special states, prioritize the "both" variant. + (SimpleState::B(true), SimpleState2::B2) => Some(Self::FromBoth), + // If only SimpleState is B(true). + (SimpleState::B(true), _) => Some(Self::FromSimpleBTrue), + // If only SimpleState2 is B2. + (_, SimpleState2::B2) => Some(Self::FromSimple2B2), + // Otherwise, no computed state. + _ => None, + } + } + } + + /// This test ensures that [`ComputedStates`] with multiple source states + /// react when any source changes. + #[test] + fn computed_state_with_multiple_sources_should_react_to_any_source_change() { + let mut world = World::new(); + EventRegistry::register_event::>(&mut world); + EventRegistry::register_event::>(&mut world); + EventRegistry::register_event::>(&mut world); + + world.init_resource::>(); + world.init_resource::>(); + + let mut schedules = Schedules::new(); + let mut apply_changes = Schedule::new(StateTransition); + SimpleState::register_state(&mut apply_changes); + SimpleState2::register_state(&mut apply_changes); + MultiSourceComputedState::register_computed_state_systems(&mut apply_changes); + schedules.insert(apply_changes); + + world.insert_resource(schedules); + setup_state_transitions_in_world(&mut world); + + // Initial state: SimpleState::A, SimpleState2::A1 and + // MultiSourceComputedState should not exist yet. + world.run_schedule(StateTransition); + assert_eq!(world.resource::>().0, SimpleState::A); + assert_eq!(world.resource::>().0, SimpleState2::A1); + assert!(!world.contains_resource::>()); + + // Change only SimpleState to B(true) - this should trigger + // MultiSourceComputedState. + world.insert_resource(NextState::Pending(SimpleState::B(true))); + world.run_schedule(StateTransition); + assert_eq!( + world.resource::>().0, + SimpleState::B(true) + ); + assert_eq!(world.resource::>().0, SimpleState2::A1); + // The computed state should exist because SimpleState changed to + // B(true). + assert!(world.contains_resource::>()); + assert_eq!( + world.resource::>().0, + MultiSourceComputedState::FromSimpleBTrue + ); + + // Reset SimpleState to A - computed state should be removed. + world.insert_resource(NextState::Pending(SimpleState::A)); + world.run_schedule(StateTransition); + assert!(!world.contains_resource::>()); + + // Now change only SimpleState2 to B2 - this should also trigger + // MultiSourceComputedState. + world.insert_resource(NextState::Pending(SimpleState2::B2)); + world.run_schedule(StateTransition); + assert_eq!(world.resource::>().0, SimpleState::A); + assert_eq!(world.resource::>().0, SimpleState2::B2); + // The computed state should exist because SimpleState2 changed to B2. + assert!(world.contains_resource::>()); + assert_eq!( + world.resource::>().0, + MultiSourceComputedState::FromSimple2B2 + ); + + // Test that changes to both states work. + world.insert_resource(NextState::Pending(SimpleState::B(true))); + world.insert_resource(NextState::Pending(SimpleState2::A1)); + world.run_schedule(StateTransition); + assert_eq!( + world.resource::>().0, + MultiSourceComputedState::FromSimpleBTrue + ); + } + + // Test SubState that depends on multiple source states. + #[derive(PartialEq, Eq, Debug, Default, Hash, Clone)] + enum MultiSourceSubState { + #[default] + Active, + } + + impl SubStates for MultiSourceSubState { + type SourceStates = (SimpleState, SimpleState2); + + fn should_exist( + (simple_state, simple_state2): (SimpleState, SimpleState2), + ) -> Option { + // SubState should exist when: + // - SimpleState is B(true), OR + // - SimpleState2 is B2 + match (simple_state, simple_state2) { + (SimpleState::B(true), _) | (_, SimpleState2::B2) => Some(Self::Active), + _ => None, + } + } + } + + impl States for MultiSourceSubState { + const DEPENDENCY_DEPTH: usize = ::SourceStates::SET_DEPENDENCY_DEPTH + 1; + } + + impl FreelyMutableState for MultiSourceSubState {} + + /// This test ensures that [`SubStates`] with multiple source states react + /// when any source changes. + #[test] + fn sub_state_with_multiple_sources_should_react_to_any_source_change() { + let mut world = World::new(); + EventRegistry::register_event::>(&mut world); + EventRegistry::register_event::>(&mut world); + EventRegistry::register_event::>(&mut world); + + world.init_resource::>(); + world.init_resource::>(); + + let mut schedules = Schedules::new(); + let mut apply_changes = Schedule::new(StateTransition); + SimpleState::register_state(&mut apply_changes); + SimpleState2::register_state(&mut apply_changes); + MultiSourceSubState::register_sub_state_systems(&mut apply_changes); + schedules.insert(apply_changes); + + world.insert_resource(schedules); + setup_state_transitions_in_world(&mut world); + + // Initial state: SimpleState::A, SimpleState2::A1 and + // MultiSourceSubState should not exist yet. + world.run_schedule(StateTransition); + assert_eq!(world.resource::>().0, SimpleState::A); + assert_eq!(world.resource::>().0, SimpleState2::A1); + assert!(!world.contains_resource::>()); + + // Change only SimpleState to B(true) - this should trigger + // MultiSourceSubState. + world.insert_resource(NextState::Pending(SimpleState::B(true))); + world.run_schedule(StateTransition); + assert_eq!( + world.resource::>().0, + SimpleState::B(true) + ); + assert_eq!(world.resource::>().0, SimpleState2::A1); + // The sub state should exist because SimpleState changed to B(true). + assert!(world.contains_resource::>()); + + // Reset to initial state. + world.insert_resource(NextState::Pending(SimpleState::A)); + world.run_schedule(StateTransition); + assert!(!world.contains_resource::>()); + + // Now change only SimpleState2 to B2 - this should also trigger + // MultiSourceSubState creation. + world.insert_resource(NextState::Pending(SimpleState2::B2)); + world.run_schedule(StateTransition); + assert_eq!(world.resource::>().0, SimpleState::A); + assert_eq!(world.resource::>().0, SimpleState2::B2); + // The sub state should exist because SimpleState2 changed to B2. + assert!(world.contains_resource::>()); + + // Finally, test that it works when both change simultaneously. + world.insert_resource(NextState::Pending(SimpleState::B(false))); + world.insert_resource(NextState::Pending(SimpleState2::A1)); + world.run_schedule(StateTransition); + // After this transition, the state should not exist since SimpleState + // is B(false). + assert!(!world.contains_resource::>()); + + // Change both at the same time. + world.insert_resource(NextState::Pending(SimpleState::B(true))); + world.insert_resource(NextState::Pending(SimpleState2::B2)); + world.run_schedule(StateTransition); + assert!(world.contains_resource::>()); + } + #[test] fn check_transition_orders() { let mut world = World::new(); diff --git a/crates/bevy_state/src/state/state_set.rs b/crates/bevy_state/src/state/state_set.rs index 69a6c41b3d..3cf1e1d260 100644 --- a/crates/bevy_state/src/state/state_set.rs +++ b/crates/bevy_state/src/state/state_set.rs @@ -293,7 +293,7 @@ macro_rules! impl_state_set_sealed_tuples { current_state_res: Option>>, next_state_res: Option>>, ($($val),*,): ($(Option>>),*,)| { - let parent_changed = ($($evt.read().last().is_some())&&*); + let parent_changed = ($($evt.read().last().is_some())||*); let next_state = take_next_state(next_state_res); if !parent_changed && next_state.is_none() {