-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
Add PreviousState Resource accessible in OnEnter #21995
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 PreviousState Resource accessible in OnEnter #21995
Conversation
|
Welcome, new contributor! Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your first contribution, and welcome! (Coming from a fellow new-ish contributor)
It looks good to me logic wise, but I defer to someone else to make doubly sure since I’m relatively inexperienced with the state code.
I wonder if this change is worth 1) a release note and 2) an update to the states.rs example to show how one can now read the PreviousState as a resource. The example update could be a candidate for a follow up if desired?
| .chain(), | ||
| ); | ||
| schedule.add_systems( | ||
| ApplyDeferred |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic is good, good work and thanks for the change. We probably could remove that ApplyDeferred though
alice-i-cecile
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ApplyDeferred needs to be removed, then this LGTM :)
8fd45f1 to
572ce0e
Compare
|
Alright, so I've removed the |
|
Yes please; handling merge conflicts is done by authors around here :) |
572ce0e to
d7c12e9
Compare
d7c12e9 to
eecfc4c
Compare
|
Should it be possible to disable this? It's keeping an extra clone in memory of the state when not everyone needs it |
|
@mockersf I wouldn't bother - unless you have many states using tens of megabytes each (at which point, you already need to do some optimizing), it's not going to use much extra memory. That said, if you can find a way to let people opt-in to this on a per-state basis without much extra work, I wouldn't complain. |
|
We put no limit on the size of the state, and it's not |
|
@mockersf Yet the hit (in terms of CPU usage and memory usage) is unlikely to be noticed by the end user, unless their state is quite large and complicated, or they are running quite slow hardware. That said, if this "previous state" stuff could be moved to a wrapper that allows opting in on a per-case basis, then I wouldn't complain. |
I think you can do just that by letting the user insert the resource (would need to change how this pr is now) if they want to use it or by feature flagging (would need to change a little of this pr, probably). |
Objective
PreviousStateresource to track the last active state, making it accessible during state transitions (e.g., inOnEnterschedules).Solution
PreviousState<S: States>resource.internal_apply_state_transitionto manage the lifecycle ofPreviousState:PreviousStateto the exited state.PreviousStateis removed.PreviousStateis updated to preserve the last known state.PreviousStateinAppExtStatesfor reflection and general access.PreviousStateis available forOnEntersystems by usingApplyDeferredordering insetup_state_transitions_in_world.Testing
PreviousStateis created, updated, and persisted correctly across multiple state transitions.Showcase
You can now access the state you just transitioned from directly in your systems!
This is my first contribution, would highly appreciate some feedback!