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

Remove StateTransition schedule in favor of using observers #15133

Open
alice-i-cecile opened this issue Sep 10, 2024 · 5 comments
Open

Remove StateTransition schedule in favor of using observers #15133

alice-i-cecile opened this issue Sep 10, 2024 · 5 comments
Labels
A-States App-level states machines C-Feature A new feature, making something new possible D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Ready-For-Implementation This issue is ready for an implementation PR. Go for it! X-Controversial There is active debate or serious implications around merging this PR

Comments

@alice-i-cecile
Copy link
Member

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

Updating the app state is a very flexible and expressive operation, which is fundamentally deferred. States typically changes only once per frame, after PostUpdate.

This can lead to unwanted delays when handling multiple dependent transitions at once.

What solution would you like?

As discussed in #15127, it would be nice to remove this meta-schedule, and instead run the various OnEnter / OnExit / OnTransition schedules on demand, leveraging the new observers. This is a good fit for "highly complex rarely exercised logic", and resolves the delay problem mentioned above while simplifying the mental model for consumers.

#15127 should be tackled in concert with this, removing NextState completely, and relying entirely on commands to handle transitions.

What alternative(s) have you considered?

Users can manually add more state transition systems to their schedule, including in their own schedules that run inside of Main. This is somewhat messy, nonstandard and hard to discover.

Additional context

Discussed briefly on Discord with @MiniaczQ here.

This design was not originally taken (including in the substates refactor) because observers didn't exist at the time!

@alice-i-cecile alice-i-cecile added C-Feature A new feature, making something new possible M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Ready-For-Implementation This issue is ready for an implementation PR. Go for it! X-Contentious There are nontrivial implications that should be thought through D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes A-States App-level states machines labels Sep 10, 2024
@benfrankel
Copy link
Contributor

benfrankel commented Sep 10, 2024

If this is meant to replace the fixed once-per-frame state transitions schedule instead of providing an additional option, I'm strongly opposed. If not, I'd like to see an example use case that isn't already covered by world.run_schedule or a custom command that calls that (why don't we have a commands.run_schedule?).

@alice-i-cecile
Copy link
Member Author

Can you explain why you prefer a fixed once-per-frame state transition?

And yeah, we can just add commands.run_schedule.

@MiniaczQ
Copy link
Contributor

I'm actually not super sold on the importance of "run state transition at any point in time", I think we should have it as a niche feature and keep synchronized transitions as the main feature.

The biggest use for state transitions is spawning in entities, which may be processed in multiple schedules.
If entities are spawned at arbitrary times within the schedule, it's hard to guarantee all of them are processed correctly every time.

@alice-i-cecile alice-i-cecile added X-Controversial There is active debate or serious implications around merging this PR and removed X-Contentious There are nontrivial implications that should be thought through labels Sep 10, 2024
@benfrankel
Copy link
Contributor

for example if you have a Level(usize) state, and you generate / load a new level on enter
you don't want arbitrarily many Level transitions in the same frame, at arbitrary points in the schedule
probably
there may be some use cases where you do want that that i'm not aware of

From Pyrious on Discord

or if you're in the middle of the frame and trigger a state change, then the last half of the frame you're running systems in the new state
which likely isn't intended

From doot on Discord

@benfrankel
Copy link
Contributor

benfrankel commented Sep 10, 2024

I'd like to see an example use case that isn't already covered by world.run_schedule or a custom command that calls that (why don't we have a commands.run_schedule?).

Actually, world.run_schedule alone requires you to run the transition hooks (systems) for all state types via the StateTransitions schedule. You don't get the flexibility of only running the transition hooks for one state type and its descendant (computed / sub) states.

There is actually a proposal in the hackmd called "Flush hooks as observers" where the StateTransitions schedule remains, but instead of running a bunch of regular systems with ordering, it triggers an event for each state type, and observers will respond to the event by running state transition hooks. This enables triggering the event for a particular state type outside of the StateTransitions schedule, wherever you want, while keeping the once-per-frame flush point intact.

But again, whether this proposal should be implemented depends on the existence of an example use case, which I don't have but I also haven't thought much about.

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 C-Feature A new feature, making something new possible D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Ready-For-Implementation This issue is ready for an implementation PR. Go for it! X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
Development

No branches or pull requests

3 participants