Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

It's surprising that using 'transitioning' to the same state triggers OnEnter #8191

Closed
deifactor opened this issue Mar 24, 2023 · 6 comments · Fixed by #8359
Closed

It's surprising that using 'transitioning' to the same state triggers OnEnter #8191

deifactor opened this issue Mar 24, 2023 · 6 comments · Fixed by #8359
Labels
A-ECS Entities, components, systems, and events C-Bug An unexpected or incorrect behavior C-Docs An addition or correction to our documentation
Milestone

Comments

@deifactor
Copy link

Bevy version

0.10

What you did

I was writing a system to transition to a "game over" state when the player's HP hit zero and one to show the UI on entering that state:

fn check_game_over(mut commands: Commands, query: Query<&HP, With<Player>>) {
    for hp in &query {
        if hp.0 <= 0 {
            commands.insert_resource(NextState(Some(GameState::GameOver)));
        }
    }
}

pub fn game_over_plugin(app: &mut App) {
    app.add_system(check_game_over)
        .add_system(show_game_over_screen.in_schedule(OnEnter(GameState::GameOver)));
}

What went wrong

Since NextState triggers OnEnter schedules unconditionally, this code constantly generates UI nodes. I assumed that since States: Eq, bevy would compare the next state to the current state, see that they're the same, and just do nothing (similar to DetectChangesMut::set_if_neq).

@deifactor deifactor added C-Bug An unexpected or incorrect behavior S-Needs-Triage This issue needs to be labelled labels Mar 24, 2023
@alice-i-cecile alice-i-cecile added C-Docs An addition or correction to our documentation A-ECS Entities, components, systems, and events and removed S-Needs-Triage This issue needs to be labelled labels Mar 24, 2023
@alice-i-cecile alice-i-cecile added this to the 0.11 milestone Mar 24, 2023
@alice-i-cecile
Copy link
Member

I'd like to see docs for this, and a set_if_neq method on NextState IMO

@alice-i-cecile alice-i-cecile added the D-Trivial Nice and easy! A great choice to get started with Bevy label Mar 24, 2023
@hymm
Copy link
Contributor

hymm commented Mar 24, 2023

a set_if_neq method on NextState IMO

Note that this would need to compare to the value in Res<State<T>>

@deifactor
Copy link
Author

deifactor commented Mar 24, 2023

a set_if_neq method on NextState IMO

Note that this would need to compare to the value in Res<State<T>>

Wouldn't it be enough to just change apply_state_transition? I didn't mean that NextState should have a method, I meant that the state transition logic should check this automatically.

@alice-i-cecile
Copy link
Member

I meant that the state transition logic should check this automatically.

Hmm. Maybe with the addition of the Transition schedules this is a safe default: users who want this behavior can add a self -> self schedule?

@alice-i-cecile alice-i-cecile removed the D-Trivial Nice and easy! A great choice to get started with Bevy label Mar 24, 2023
@deifactor
Copy link
Author

Maybe with the addition of the Transition schedules this is a safe default: users who want this behavior can add a self -> self schedule?

What are the use cases for wanting self -> self transitions to trigger things? I'm sure there are some, I just can't think of any. But I think it's reasonable to say "this is weird enough that you have to do something different".

@alice-i-cecile
Copy link
Member

To me this is the standard "power cycle" pattern, where you reinitialize everything to reset the state :)

alice-i-cecile pushed a commit that referenced this issue Apr 12, 2023
# Objective

Fix #8191.

Currently, a state transition will be triggered whenever the `NextState`
resource has a value, even if that "transition" is to the same state as
the previous one. This caused surprising/meaningless behavior, such as
the existence of an `OnTransition { from: A, to: A }` schedule.

## Solution

State transition schedules now only run if the new state is not equal to
the old state. Change detection works the same way, only being triggered
when the states compare not equal.

---

## Changelog

- State transition schedules are no longer run when transitioning to and
from the same state.

## Migration Guide

State transitions are now only triggered when the exited and entered
state differ. This means that if the world is currently in state `A`,
the `OnEnter(A)` schedule (or `OnExit`) will no longer be run if you
queue up a state transition to the same state `A`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events C-Bug An unexpected or incorrect behavior C-Docs An addition or correction to our documentation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants