bevyengine / bevy

A refreshingly simple data-driven game engine built in Rust
https://bevyengine.org
Apache License 2.0
36.26k stars 3.58k forks source link

Support same-state transitions out of the box via new schedules #15478

Open benfrankel opened 1 month ago

benfrankel commented 1 month ago

What problem does this solve or what need does it fill?

Support same-state transitions out of the box for things like "restart game" or "reset UI". See https://github.com/bevyengine/bevy/issues/14447 for more information.

What solution would you like?

Provide the following schedules:

Schedule names are bikesheddable.

What alternative(s) have you considered?

Notice that each schedule actually encodes 2 separate concepts:

  1. Ordering (when the systems run): Do the systems run on the exit side, enter side, or in between? This is necessary so that teardown systems run before setup systems.
  2. Condition (whether the systems run): Which state transitions cause the systems to run? Is it a transition that exits a specific state S? Or a transition from S to S? Etc.

This can be split into two orthogonal axes using system sets and run conditions respectively. This allows you to schedule a system to run on the enter side of a S -> _ transition for example, or even use state pattern-matching to match on any class of transitions you're interested in.

This approach has a proof of concept in pyri_state that maintains comparable ergonomics, but it would require a bigger internal / breaking change to implement in bevy_state.

MiniaczQ commented 1 month ago

I'd rather keep the name of OnExit (here OnChangeExit) and OnEnter (here OnChangeEnter) and instead use OnAnyExit(here OnExit) and OnAnyEnter(hereOnEnter). It's prevent the breaking change and I still think the currentOnExit` is the best default behavior, so the name should be shorter.

shanecelis commented 1 month ago

I'm against putting this in main. I also haven't seen a compelling motivation for why one would want to handle identity transitions. Show me the states and conditions where this is warranted. It takes a no-op in the current implementation and makes it an "op".

It adds to the API surface in a way that is going to be difficult to explain in the documentation why you'd choose one schedule over another without having to explain "state transitions" also means "state identity transitions", which aren't really transitions.

If people truly want to adopt that complexity, let them. It can be implemented by a third-party crate and has an implementation in the examples showing the way.

benfrankel commented 1 month ago

I could provide many examples where this is desirable. The most obvious example is restarting a game or level, or refreshing some UI that sets itself up upon entering a state. Another example is selecting a level to enter in the pause menu of a puzzle game, which may happen to be the level you're already in. "Exit the current state and then enter it again" is very natural, and not supporting it creates a weird edge case.

shanecelis commented 1 month ago

Thank you for providing examples. The level selection is a good example. Let's work with that. Assuming you're using a state for the level, changing it to the same level currently is a no-op, which is what motivates an identity state transition. But I believe there's another verb here, which is you also want to reset the level, which can happen to and in any level at any time, which is not a state; it's an event.

Now you may object that resetting a level is tearing it down and recreating it. But for some games, like sokoban for instance, resetting a level can just be resetting a few transforms to their original positions. Resetting can be much cheaper than tearing down and setting back up. Here's my pseudo-code to facilitate the discussion.

#[derive(States)]
pub enum GameState {
    Level(pub u8),
}
// ...
        .add_systems(OnExit(GameState::Level), teardown_level)
        .add_systems(OnEnter(GameState::Level), setup_level)

// Let's say there are two ways to deal with level resets.
// 1. Brute force approach. (Expensive)
        .add_systems(Update, (teardown_level, setup_level).chain().run_if(on_event(ResetLevel)))

// 2. Minimal dynamic state. (Cheap)
        .add_systems(Update, reset_player_and_elements.run_if(on_event(ResetLevel)))

For me this reinforces my point. A state machine is a good fundamental building block, but it is in its own way opinionated about what constitutes a state: You can only be in one state, and you can only transition to another state. Often times trying to use a state machine forces you to recon with latent or implicit states; this is good! It helps your design become more explicit.

Perhaps we should consider creating an example that shows this kind of level selection pattern using states and a reset event. I notice there aren't any examples with states that have additional data to them like Level(pub u8).

benfrankel commented 1 month ago

I don't consider performance to be a legitimate issue in the level selector example. If the "level 1 -> 2" transition logic is considered sufficiently performant, then reusing that logic for "level 1 -> 1" would be as well. If somehow that's not the case, this proposal provides the tools to micro-optimize by isolating same-state transitions anyways.

A same-state transition is still a state transition, it's not a totally different concept. Mathematically it's a self-edge. In practice it means exiting the current state and then entering the current state instead of a different state. It's natural and convenient, and treating it like a special case is an unnecessary papercut.

shanecelis commented 1 month ago

Well, I've been subjected to enough single-player games where dying was penalized mainly by waiting for a game to reload its level that I consider it a valid performance issue.

benfrankel commented 1 month ago

Sure, you'd want to restrict a system that loads the next level to only run when it differs from the previous level. That would go in OnChangeEnter(S), which is why a schedule with that behavior should still be provided. Personally I'd argue for even more flexibility via run conditions instead of schedules, because I absolutely agree that these tools are needed and the user should have the power to decide what makes the most sense on a case by case basis.

shanecelis commented 1 month ago

I relent that self-edges are permissible and prevalent in finite state machines. But they don't do anything mathematically. We have to choose leave them as no-ops or make them do something.

Let's look at your proposal given the level loading example and the cheap and expensive level reset.

// Let's say there are two ways to deal with level resets.
// 1. Brute force approach. (Expensive)
    .add_systems(OnExit(GameState::Level), teardown_level)
    .add_systems(OnEnter(GameState::Level), setup_level)

// 2. Minimal dynamic state. (Cheap)
    .add_systems(OnChangeExit(GameState::Level), teardown_level)
    .add_systems(OnChangeEnter(GameState::Level), setup_level)
    .add_systems(OnEnter(GameState::Level), reset_player_and_elements)

Now, I don't mind the look of either, but in the second cheap way (preferred), there is a issue we have to contend with. What order do the Enters run? Any, Change, Re. Can they be ordered in the general case? I fear they can't be. Here reset_player_and_elements must come after setup_level so we can by fiat declare that the ordering will be Change, Any, Re, but will that work for everyone, all the time?

benfrankel commented 1 month ago

Yeah, regarding ordering you're right, and a user will have the same issue if they follow the custom_transitions example themselves. This is a good reason for the engine to provide a proper solution out of the box (not necessarily this particular proposal I guess, I'd just hoped it would be more palatable because it already exists in an upstream example).

The system set / run condition approach doesn't have the ordering issue OTOH because putting a run condition on a system doesn't force it into a different system set, whereas in present bevy_state the two concepts are awkwardly merged along a single axis.