Skip to content

enable same state transitions #19363

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

mockersf
Copy link
Member

@mockersf mockersf commented May 25, 2025

Objective

  • Same state transitions have their uses but are not currently possible

Solution

  • Add a set_forced method on NextState that will trigger OnEnter and OnExit
  • Rerun state transitions when set_forced has been used
  • Rerun them is set is called after set_forced with the same state

@mockersf mockersf added the A-States App-level states machines label May 25, 2025
@benfrankel
Copy link
Contributor

How does this approach interact with computed and sub states?

@janhohenheim
Copy link
Member

janhohenheim commented May 25, 2025

@janhohenheim janhohenheim added the S-Needs-Review Needs reviewer attention (from anyone!) to move forward label May 25, 2025
@benfrankel
Copy link
Contributor

Additional previous work in #11160 and #11158 as well :)

@janhohenheim
Copy link
Member

janhohenheim commented May 25, 2025

Labeling as contentious given that all previous work in this regard was contested
Also related to usability of #19296

@janhohenheim janhohenheim added the X-Contentious There are nontrivial implications that should be thought through label May 25, 2025
@benfrankel
Copy link
Contributor

benfrankel commented May 25, 2025

We might want a working group for bevy_state at some point. There are some old footguns and missing features that users keep hitting again and again (like same-state transitions and the pre-PreStartup schedule), and it would take a deeper redesign to resolve them without introducing technical debt. I know how this can be done, but I don't believe it would be feasible for me to push anything through without a working group behind it.

My take on this particular feature though is that it's so basic and crucial that it's worth introducing technical debt to get it in even without a redesign (obviously since I've tried to do just that in 4 separate PRs). Just need to document how it interacts with computed and sub states.

@mockersf
Copy link
Member Author

How does this approach interact with computed and sub states?

I think it doesn't, it's only enabled when actually calling the method on NextState

This is scoped as small as I think is viable, the only extra thing in this PR is making sure that if next_state.set(State::SomeVariant) is called after next_state.set_forced(State::SomeVariant), it will still rerun the transition schedules

@mgi388
Copy link
Contributor

mgi388 commented May 26, 2025

next_state.set_forced(State::SomeVariant) reads to me like it's forcing the setting of that state now. This is because the context you're in here is "next state" so it makes you think "oh maybe the state will change now by forcing it".

But what set_forced is really doing is something to do with allowing same state transitions to trigger OnEnter and OnExit.

So whether this is just a naming thing, or a flaw in the design, I'm not sure, but set_forced doesn't seem right to me.

I also wondered whether you'd do this at the place you actually change the state (as in this PR), or if you do it in the definition of your states (if it's technically possible).

@mockersf
Copy link
Member Author

mockersf commented May 26, 2025

next_state.set_forced(State::SomeVariant) reads to me like it's forcing the setting of that state now. This is because the context you're in here is "next state" so it makes you think "oh maybe the state will change now by forcing it".

What do you think of set_with_forced_reload as a name? or set_and_enforce_reload?

I also wondered whether you'd do this at the place you actually change the state (as in this PR), or if you do it in the definition of your states (if it's technically possible).

I think it's better to do it like in this pr.

If we want to go for a breaking change, I would probably make this PR the default, and change the current set to set_if_not_already or something

@mgi388
Copy link
Contributor

mgi388 commented May 26, 2025

I think it's better to do it like in this pr.

What's your thinking? Is it because you specifically want per-call site control over this? The reason I mentioned it is that when coming up with a name and design for the per-call site version (this PR), it seems hard to come up with a concise and expressive design whereas when it's defined at the type level, it becomes easier to express. For example, maybe it looks something like this:

#[derive(States)]
enum GameState {
    #[default]
    MainMenu,
    SettingsMenu,
    InGame,
}

// Default behavior: schedules skipped for identical states.
impl StateTransitionBehavior for GameState {
    fn always_run_entry_exit_transitions() -> bool { false }
}

#[derive(States)]
enum ResettingState {
    #[default]
    Idle,
    Active,
}

// Always run transitions, even for identical states.
impl StateTransitionBehavior for ResettingState {
    fn always_run_entry_exit_transitions() -> bool { true }
}

fn set_game_state(mut next_game_state: ResMut<NextState<GameState>>) {
    // Standard behavior: schedules skipped if state is unchanged.
    next_game_state.set(GameState::InGame);
}

fn set_resetting_state(mut next_state: ResMut<NextState<ResettingState>>) {
    // Schedules always run, even if state is unchanged.
    next_state.set(ResettingState::Active);
}

That aside, if we're trying to do this in the NextState API, the most expressive approach could be some sort of builder-style pattern, e.g.,

next_game_state.set(GameState::InGame).with_entry_exit_transitions().apply()

But it raises questions of compatibility which we'd have to decide on.

If I had to pick something on the NextState API without being a breaking change, I think I'm inclined to go with this:

next_game_state.set_with_entry_exit(GameState::InGame); // or:
next_game_state.set_with_entry_exit_transitions(GameState::InGame); 

(noting that I picked entry_exit over enter_exit because the second one feels less idiomatic as a noun phrase)

Or, if you want to change set to do this (breaking change), and introduce a new one that turns it off, then call that one:

next_game_state.set_without_entry_exit(GameState::InGame); // or:
next_game_state.set_without_entry_exit_transitions(GameState::InGame);

If you wanted a breaking change and you want per-call site control, I think a builder pattern could work here and would be the most expressive.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-States App-level states machines S-Needs-Review Needs reviewer attention (from anyone!) to move forward X-Contentious There are nontrivial implications that should be thought through
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants