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

Use a finite State machine instead of a stack #5458

Closed
wants to merge 5 commits into from

Conversation

JoJoJet
Copy link
Member

@JoJoJet JoJoJet commented Jul 26, 2022

Objective

  • Stack-based state machines only make sense in specific circumstances -- finite state machines are more generally applicable.
  • Increase parity with schedule v3 ("stageless") #4391
  • Far less confusing.

Solution

  • Subtractive design -- most of the current API stays the same, we just yeet everything related to the old stack.
    • Should make it easier to migrate towards the full State overhaul in Stageless.

Changelog

  • Replaced the old stack-based state machine with a finite state machine.
  • State transition methods no longer return an error when trying to transition from to a duplicate state.

Migration Guide

All State methods relating to the previous stack-based implementation have been removed. The only operations are set and overwrite_set. The only associated run criteria are on_update, on_enter, and on_exit.

@JoJoJet
Copy link
Member Author

JoJoJet commented Jul 26, 2022

Kinda spooky that it didn't break CI at all...

@alice-i-cecile
Copy link
Member

There's a half-dozen issues this will close; check the stageless board for a list.

@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior A-ECS Entities, components, systems, and events C-Code-Quality A section of code that is hard to understand or change C-Usability A targeted quality-of-life change that makes Bevy easier to use X-Controversial There is active debate or serious implications around merging this PR labels Jul 26, 2022
@hymm
Copy link
Contributor

hymm commented Jul 26, 2022

I think we need combinable run criteria before merging this, so the behavior of having multiple states active can be retained.

@tim-blackbird
Copy link
Contributor

I think we need combinable run criteria before merging this, so the behavior of having multiple states active can be retained.

Would that not be as easy as checking two states in the same run criteria?
Or am I misunderstanding something c:

@JoJoJet
Copy link
Member Author

JoJoJet commented Jul 26, 2022

Would that not be as easy as checking two states in the same run criteria? Or am I misunderstanding something c:

It's probably not worth the effort to do that, honestly. We can just make run criteria loopless in a separate PR (after fixing issues with fixed timesteps), then come back to this one.

@wilk10
Copy link
Contributor

wilk10 commented Jul 27, 2022

I haven’t followed the stageless discussions very closely and i’m just starting to catch up.
Can someone please point me to where the replacement for push / pop operations have been discussed, if anyone knows it at the top of their head? Right now they’re very convenient to pause / unpause states without re-running the on_enter SystemSet of the paused state. Thanks so much!

With regards to this PR in particular, and not the design choice behind, in my opinion the Migration Guide here should include how to substitute push / pop operations, or at least point to where they’ve been discussed.

@alice-i-cecile
Copy link
Member

alice-i-cecile commented Jul 27, 2022

So, the "easy" solutions (from the engine-dev perspective) is to tell users to reimplement a state-stack model in 3rd party code if they want it.

However, that's far-and-away the clearest and most compelling use-case of the more advanced functionality that I've seen. Can you explain the pattern in depth, and perhaps link to your repo? Initial questions:

  1. Do you want to skip both on_enter and on_exit changes for the state?
  2. Would you be okay with a method that transitioned out without running on_exit + a method that transitioned in without running on_enter?
  3. What states were you using this for?
  4. Why did you want to avoid re-running on_enter?

@wilk10
Copy link
Contributor

wilk10 commented Jul 27, 2022

I have a colony sim game. Most of the time the game is in Play state, where units complete their tasks on the map, but when the player wants to place a building (and click the relevant button) the app enters PausedPlay state with a push operation. The Play state is then paused and units don’t move, animations don’t play etc, but nothing is despawned. In this state the player can still interact with the map while the systems that run only in PausedPlay state take care of checking if the building placement is valid, give feedback if it is, or it isn’t, etc. When the player confirms they want to place the building, the building is spawned, the PausedPlay state is popped and the app goes back to run the systems belonging to SystemSet::on_update(GameState::Play).
The same pattern can be implemented when the player manually signals they want to pause the game to inspect units, assign new tasks etc, like typical realtime with pause games.

As per the answers:

  1. Yes, definitely. On entering the Play state i spawn the map and initial units. On exiting the Play state (eg: when they quit to the main menu) i despawn them. I want to avoid to run these systems when going from Play to PausedPlay and back.
  2. I think so..? I’m stil unfamiliar with the stageless pattern, so my mental model with 0.7 is "run SystemSet::on_enter(GameState::Play) once" vs "run SystemSet::on_update(GameState::Play) continuously", while now i understand on_update is yeeted and there’s only on_enter / on_exit, is that correct? I’m not sure yet how to adapt this to stageless, hence my question.

(3 and 4 should be implied in the rest of the answer)

If you want to talk more in depth feel free to ping me on Discord (@elmo#4899). Thanks!

Edit: ah no i see on_update stays, so yeh my main question is to avoid on_enter / on_exit then, but I'm not sure yet if i have considered everything

@alice-i-cecile
Copy link
Member

Very helpful! Okay so three possible designs come to mind:

  1. Allow for explicit bypasses of the on_enter / on_exit system sets. This makes me nervous, because you need to remember to do this every time you swap states and it won't be easy to display or reason about.
  2. Add another group of system sets for on_transition(from: MyState, to: MyState) to trigger when a specific path is taken. This should allow you to define your map spawning / despawning systems only to trigger when transitioning to / from MainMenu from the appropriate states.
  3. In your specific case, move your spawning / despawning logic to on_enter / on_exit for MainMenu.

I think I'm opposed to 1 due to clarity and correctness concerns. 3 probably solves your particular problem, but I'm nervous that it's not strong enough.

2 seems like an elegant move towards the direction of #1597, but will add a bit of complexity back in.

@wilk10
Copy link
Contributor

wilk10 commented Jul 27, 2022

Thank you for looking into this! I really appreciate it.

I do agree that 2 sounds better, I think I could make it work.

From my point of view, the ideal scenario would be that a first party solution, whichever you land on, ships with the release that yeets the push / pop methods, and it's documented in the migration guide. That would be amazing.

@JoJoJet JoJoJet closed this Aug 23, 2022
@JoJoJet JoJoJet deleted the finite-state-machine branch December 21, 2022 15:49
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-Code-Quality A section of code that is hard to understand or change C-Usability A targeted quality-of-life change that makes Bevy easier to use X-Controversial There is active debate or serious implications around merging this PR
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants