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

Add RefreshState resource and support same-state transitions #13361

Closed
wants to merge 9 commits into from

Conversation

benfrankel
Copy link
Contributor

@benfrankel benfrankel commented May 14, 2024

Objective

Solution

Allow same-state transitions for freely mutable states via next_state.set(current_state). Support same-state transitions for computed states via a new RefreshState<S> resource that explicitly flags a specific computed state to transition regardless of whether its value changes.

RefreshState can be understood as a limited NextState for computed states that can only trigger same-state transitions, necessary because same-state transitions for computed states cannot be implicitly assumed.

Testing

  • Manually tested the restart feature added to cargo run --example alien_cake_addict.
  • Have not yet tested refreshing computed states with RefreshState. This will require a new unit test and example.

Changelog

  • Removed the check preventing state transitions from the current state to itself.
  • Added a resource RefreshState<S> to flag a computed or sub state to transition regardless of whether its value changes.

Migration Guide

NextState<S> can now trigger state transitions from the current state to itself. To preserve the old behavior, check the current state in State<S> before setting the next state.

@alice-i-cecile alice-i-cecile added C-Enhancement A new feature A-ECS Entities, components, systems, and events X-Contentious There are nontrivial implications that should be thought through labels May 15, 2024
@lee-orr
Copy link
Contributor

lee-orr commented May 16, 2024

I personally dislike this approach, for two reasons:

  • I don't think a computed state should be manually refreshable - if we add something there, it should be something that can be computed.
  • A refresh should not be able to happen by accident - so it should be part of "set", but rather it's own thing. Otherwise, if you make a change to a state that doesn't affect a computed child of it, it might think it should still refresh - for example, triggering a level reload when you were un-pausing

I think other approaches can be found for this - and it was a useful exploration - but ultimately I do not think it's a good choice here.

@benfrankel
Copy link
Contributor Author

benfrankel commented May 16, 2024

The first concern is understandable. Personally I think the benefits of this PR are worth the tradeoff of very slightly relaxing the purity of computed states:

  • The actual value of the state is still strictly computed.
  • The change-state transitions are still strictly computed.
  • Specifically only the same-state transitions, which weren't possible at all before, are manually triggered.

Every approach will have tradeoffs. If the only downside of this PR is giving up a tiny amount of purity in an opt-in escape hatch, I think that's a win. Especially in the context of a game engine.

A refresh should not be able to happen by accident - so it should be part of "set", but rather it's own thing. Otherwise, if you make a change to a state that doesn't affect a computed child of it, it might think it should still refresh - for example, triggering a level reload when you were un-pausing

Refreshes can't happen by accident with this approach. They only happen if you explicitly call next_state.set(current_state) for freely mutable states or refresh_state.refresh() for computed states.

@benfrankel
Copy link
Contributor Author

benfrankel commented May 20, 2024

Closing this PR in favor of the 3rd-party solution I published: https://github.com/benfrankel/pyri_state. In pyri_state there is a flush: bool flag inside NextState that is analogous to the RefreshState in this PR.

Hopefully pyri_state will be upstreamed in the future. If not, it will remain as an alternative to bevy_state.

@benfrankel benfrankel closed this May 20, 2024
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-Enhancement A new feature X-Contentious There are nontrivial implications that should be thought through
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Need the OnEnter() and OnExit() to be run even if when transitioning to and from the same state.
3 participants