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<T: SubStates<SourceStates = Self>>(schedule: &mut Schedule) {
  let apply_state_transition = |(mut ereader0, mut ereader1, mut ereader2): (
      EventReader<StateTransitionEvent<S0::RawState>>,
      EventReader<StateTransitionEvent<S1::RawState>>,
      EventReader<StateTransitionEvent<S2::RawState>>,
  ),
      event: EventWriter<StateTransitionEvent<T>>,
      commands: Commands,
      current_state_res: Option<ResMut<State<T>>>,
      next_state_res: Option<ResMut<NextState<T>>>,
      (s0, s1, s2): (
          Option<Res<State<S0::RawState>>>,
          Option<Res<State<S1::RawState>>>,
          Option<Res<State<S2::RawState>>>,
  )| {
    // 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.
This commit is contained in:
mgi388 2025-06-17 07:34:22 +10:00 committed by GitHub
parent 8661e914a5
commit 1f2fd3d29d
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
2 changed files with 198 additions and 1 deletions

View File

@ -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<Self> {
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::<StateTransitionEvent<SimpleState>>(&mut world);
EventRegistry::register_event::<StateTransitionEvent<SimpleState2>>(&mut world);
EventRegistry::register_event::<StateTransitionEvent<MultiSourceComputedState>>(&mut world);
world.init_resource::<State<SimpleState>>();
world.init_resource::<State<SimpleState2>>();
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::<State<SimpleState>>().0, SimpleState::A);
assert_eq!(world.resource::<State<SimpleState2>>().0, SimpleState2::A1);
assert!(!world.contains_resource::<State<MultiSourceComputedState>>());
// 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::<State<SimpleState>>().0,
SimpleState::B(true)
);
assert_eq!(world.resource::<State<SimpleState2>>().0, SimpleState2::A1);
// The computed state should exist because SimpleState changed to
// B(true).
assert!(world.contains_resource::<State<MultiSourceComputedState>>());
assert_eq!(
world.resource::<State<MultiSourceComputedState>>().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::<State<MultiSourceComputedState>>());
// 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::<State<SimpleState>>().0, SimpleState::A);
assert_eq!(world.resource::<State<SimpleState2>>().0, SimpleState2::B2);
// The computed state should exist because SimpleState2 changed to B2.
assert!(world.contains_resource::<State<MultiSourceComputedState>>());
assert_eq!(
world.resource::<State<MultiSourceComputedState>>().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::<State<MultiSourceComputedState>>().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<Self> {
// 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 = <Self as SubStates>::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::<StateTransitionEvent<SimpleState>>(&mut world);
EventRegistry::register_event::<StateTransitionEvent<SimpleState2>>(&mut world);
EventRegistry::register_event::<StateTransitionEvent<MultiSourceSubState>>(&mut world);
world.init_resource::<State<SimpleState>>();
world.init_resource::<State<SimpleState2>>();
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::<State<SimpleState>>().0, SimpleState::A);
assert_eq!(world.resource::<State<SimpleState2>>().0, SimpleState2::A1);
assert!(!world.contains_resource::<State<MultiSourceSubState>>());
// 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::<State<SimpleState>>().0,
SimpleState::B(true)
);
assert_eq!(world.resource::<State<SimpleState2>>().0, SimpleState2::A1);
// The sub state should exist because SimpleState changed to B(true).
assert!(world.contains_resource::<State<MultiSourceSubState>>());
// Reset to initial state.
world.insert_resource(NextState::Pending(SimpleState::A));
world.run_schedule(StateTransition);
assert!(!world.contains_resource::<State<MultiSourceSubState>>());
// 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::<State<SimpleState>>().0, SimpleState::A);
assert_eq!(world.resource::<State<SimpleState2>>().0, SimpleState2::B2);
// The sub state should exist because SimpleState2 changed to B2.
assert!(world.contains_resource::<State<MultiSourceSubState>>());
// 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::<State<MultiSourceSubState>>());
// 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::<State<MultiSourceSubState>>());
}
#[test]
fn check_transition_orders() {
let mut world = World::new();

View File

@ -293,7 +293,7 @@ macro_rules! impl_state_set_sealed_tuples {
current_state_res: Option<ResMut<State<T>>>,
next_state_res: Option<ResMut<NextState<T>>>,
($($val),*,): ($(Option<Res<State<$param::RawState>>>),*,)| {
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() {