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

Stage::run::<StateTransitionStage>() causes the initial state to exit and enter again on first run #10

Closed
tjamaan opened this issue Apr 12, 2022 · 5 comments

Comments

@tjamaan
Copy link

tjamaan commented Apr 12, 2022

By setting NextState on first run in line 122 in state.rs the initial state will be switched to itself, thus triggering the exit and enter stages. If exit and enter are expensive (e.g. procedural generation) it will cause needless processing.

Link:

world.insert_resource(NextState(self.init_state.clone()));

@tjamaan
Copy link
Author

tjamaan commented Apr 12, 2022

Ways to fix this:

  1. Treat switching a state to itself as an edge-case and require users to set another resource (e.g. insert_resource(NextState(same_state)) + insert_resource(SameStateSwitch(true))) in order to execute exit and enter stages.
    1.1. Pros: Only need to check for resource SameStateSwitch(true) if current and new state are equal.
    1.2. Cons: Clunky if you need to switch state to itself intentionally. New users need to discover the need for SameStateSwitch resource.
  2. Turn NextState<T>(pub T) into NextState<T>(pub Option<T>).
    2.1. Pros: Simple to use.
    2.2. Cons: Matching an Option is not free. (It doesn't cost much though)

@thebluefish
Copy link

thebluefish commented Apr 12, 2022

If we're asking the user to insert a resource to transition, I would personally expect the existence of the NextState resource to indicate that a transition should occur. IMO we should simply not insert this resource by default, and remove it once the transition has processed.

@inodentry
Copy link
Contributor

That was the behavior in 0.1. I changed it, because "repeatedly inserting and removing a resource" (effectively "using a resource as an event") felt weird to me. But you are right, why not, it makes sense. In retrospect, if we are gonna use a resource to trigger a transition, that makes more sense. I guess I should bring it back.

@inodentry
Copy link
Contributor

OK, reverted back to the 0.1 states behavior, except now without the next != current check (transitioning to the same state as the current is allowed). The NextState behaves as you described: insert it to trigger a transition, and it gets auto-removed when the transition is performed.

I consider this bug important enough that I decided to release a 0.3.0 immediately, just for this.

@tjamaan
Copy link
Author

tjamaan commented Apr 12, 2022

I confirmed that the fix works. Only the initial state's enter stage is run at the start, and it only ran once.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants