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 state scoped events #15085

Merged
merged 3 commits into from
Sep 9, 2024
Merged

Add state scoped events #15085

merged 3 commits into from
Sep 9, 2024

Conversation

UkoeHB
Copy link
Contributor

@UkoeHB UkoeHB commented Sep 7, 2024

Objective

Solution

  • Allow registering state scoped events that will be automatically cleared when exiting a state. This is most of the time not obviously useful, but enables users to write correct code that will avoid/reduce edge conditions (such as systems that aren't state scoped polling for a state scoped event and having unintended side effects outside a specific state instance).

Testing

Did not test.


Showcase

Added state scoped events that will be automatically cleared when exiting a state. Useful when you want to guarantee clean state transitions.

Normal way to add an event:

fn setup(app: &mut App) {
    app.add_event::<MyGameEvent>();
}

Add a state-scoped event (NEW):

fn setup(app: &mut App) {
    app.add_state_scoped_event::<MyGameEvent>(GameState::Play);
}

@TrialDragon TrialDragon added C-Feature A new feature, making something new possible D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes S-Needs-Review Needs reviewer attention (from anyone!) to move forward A-States App-level states machines labels Sep 7, 2024
@TrialDragon TrialDragon added the A-App Bevy apps and plugins label Sep 7, 2024
@MiniaczQ
Copy link
Contributor

MiniaczQ commented Sep 8, 2024

Nice seeing more people jump on the state boat :)
State scoped events was something I wanted to add, but wasn't quite sure how, I like this approach.
I like the implementation, but we could use an example for discoverability.

For the future, we should consider that events E can be generate outside of state S and figure out how to deal with that:

  1. Also cleanup on entry (ever growing queue if we never enter S)
  2. Lock the event queue, no-op write attempts
  3. Remove the event queue resource and add falliable methods to writers/readers

@alice-i-cecile alice-i-cecile added the M-Needs-Release-Note Work that should be called out in the blog due to impact label Sep 8, 2024
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I'm on board. This is a consistent, simple extension with predictable behavior.

@UkoeHB
Copy link
Contributor Author

UkoeHB commented Sep 8, 2024

Also cleanup on entry (ever growing queue if we never enter S)

@MiniaczQ there are are a couple things to consider: events that might be sent into a state, and events sent during OnEnter. The later can be solved by only clearing before OnEnter runs, and the former can maybe be solved by making it opt-in (although that would introduce API complexity). In general I expect state-scoped events to be generated and consumed within that state (since that's their scope - i.e. the only region where the events actually make any sense).

@MiniaczQ
Copy link
Contributor

MiniaczQ commented Sep 9, 2024

Also cleanup on entry (ever growing queue if we never enter S)

@MiniaczQ there are are a couple things to consider: events that might be sent into a state, and events sent during OnEnter. The later can be solved by only clearing before OnEnter runs, and the former can maybe be solved by making it opt-in (although that would introduce API complexity). In general I expect state-scoped events to be generated and consumed within that state (since that's their scope - i.e. the only region where the events actually make any sense).

Yeah, lots to consider, we can start by merging this :)

@alice-i-cecile alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Sep 9, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Sep 9, 2024
Merged via the queue into bevyengine:main with commit adc2cf7 Sep 9, 2024
29 checks passed
@UkoeHB UkoeHB deleted the state_events branch September 9, 2024 17:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-App Bevy apps and plugins 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-Release-Note Work that should be called out in the blog due to impact S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants