Refactor state scoped events to match entities.

[AlephCubed]

This adds support for clearing events when entering a state (instead of just when exiting) and updates the names to match DespawnOnExitState.

Before:

app.add_state_scoped_event::<MyGameEvent>(GameState::Play);

After:

app
  .add_event::<MyGameEvent>()
  .clear_events_on_exit_state::<MyGameEvent>(GameState::Play);

[alice-i-cecile]

@mgi388 @benfrankel want to review this one? :)

[ElliottjPierce]

LGTM; nothing blocking approval.

A couple potential ux issues with how the event itself is added, but that's not a huge deal IMO.

[ElliottjPierce]

Not for this pr, but it's a little strange that a clear on transition event might not be cleared if the state is immediately changed again quickly.

[mgi388]

Also happy to be convinced otherwise, but as far as naming goes, and matching the rename to despawn_entities_on_exit_state, I feel like a better name would be clear_events_on_exit_state. I think that's what it does right? It clears (any pending) events (when you) exit (the) state.

By the way, the add_ prefix tripped me up hence why I wanted to suggest an alternative. add_event_cleared_on_exit_state reads like you're adding an event rather than registering a behavior.

[AlephCubed]

Both the previous version, add_state_scoped_event, and the current version do add the event as well as configure it.
I did previously have a version which didn't add the event automatically, so you would need to replace add_state_scoped_event with add_event + clear_events_on_exit_state. This was changed because of this conversation.

I guess we could do something in-between. Since adding an event multiple times doesn't do anything, we could revert the name back to clear_events_on_exit_state, but also automatically add the event.

[mgi388]

Thanks for the linked context. Yeah tricky. So if both of these are the same:

#[derive(Event, Debug)]
struct StateScopedEvent;

app.add_event::<StateScopedEvent>();
app.clear_events_on_exit_state::<StateScopedEvent>(TestState::A);

#[derive(Event, Debug)]
struct StateScopedEvent;

app.clear_events_on_exit_state::<StateScopedEvent>(TestState::A);

Then readers of the second won't know it actually also registers the event. So maybe there are some options.

1. Verbose name.

#[derive(Event, Debug)]
struct StateScopedEvent;

app.add_event_and_clear_events_on_exit_state::<StateScopedEvent>(TestState::A);

2. Do nothing except the rename, i.e., just use clear_events_on_exit_state and keep it also registering the event inside.

3. Don't register the event, i.e., go back to what you had before that conversation.

4. A magical world where we had builders.

#[derive(Event, Debug)]
struct StateScopedEvent;

app
  .add_event::<StateScopedEvent>()
  .clear_events_on_enter_state(TestState::A)
  .clear_events_on_exit_state(TestState::A);

I have no idea if 4 is feasible even though that's my preference at a glance. Certainly I would guess not for this PR. So if I had to pick myself, I would pick option 1. for this PR.

Feel free to see what others think before you rely on my sole opinion.

[alice-i-cecile]

4 should be totally doable: all of these app builder methods should return &mut App. In any case, I really don't want to be mixing event adding and behavioral config here. The fact that this was initially the case was a mistake IMO.

[AlephCubed]

I have reverted back to clear_events_on_exit_state, but unlike with 4 it requires specifying the event type each time.

app
  .add_event::<StateScopedEvent>()
  .clear_events_on_enter_state::<StateScopedEvent>(TestState::A)
  .clear_events_on_exit_state::<StateScopedEvent>(TestState::A);

[mgi388]

Yeah that LGTM.

FWIW, for 4. I was meaning something more like this, where I've put an add_system on there as well to show that it is returning an app only after building/registering the event.

app.add_event(
    Event::new::<StateScopedEvent1>()
        .clear_on_enter_state(TestState::A)
        .clear_on_exit_state(TestState::B),
)
.add_systems(Update, || {})
.add_event(
    Event::new::<StateScopedEvent1>()
        .clear_on_enter_state(TestState::B),
)
.add_systems(Update, || {});

(It's sort of just pseudocode, I didn't check how easily this would actually be to do).

To me it's similar to how in bevy_asset_loader you call one thing on app to configure your loading state, e.g.,

app.add_loading_state(
    LoadingState::new(BootState::Loading)
        .continue_to_state(BootState::Loaded)
        .on_failure_continue_to_state(BootState::Error)
        .with_dynamic_assets_file::<StandardDynamicAssetCollection>("cursor.assets.ron")
        .load_collection::<BootAssetCollection>()
        .load_collection::<LoadingAssetCollection>()
        .load_collection::<GrayscaleCursorAssetCollection>()
        .load_collection::<ColoredCursorAssetCollection>()
        .load_collection::<SoundEffectPacketAssetCollection>()
        .load_collection::<MagicItemsAssetCollection>()
        .load_collection::<RegimentDisplayNamesAssetCollection>()
        .init_resource::<CursorRegistry>(),
);

I just wanted to share how I could see something like this being an (arguably, and IMO) more usable API to encapsulate the "configuration" you want to apply to the event you're working with, but my comment here doesn't block the PR.

[alice-i-cecile]

I have reverted back to clear_events_on_exit_state, but unlike with 4 it requires specifying the event type each time.

Quick question: if you use an impl Trait argument, rather than a generic, can we get type inference working properly for these methods?

[AlephCubed]

Quick question: if you use an impl Trait argument, rather than a generic, can we get type inference working properly for these methods?

Not 100% sure, but I don't think so. We need to call world.get_resource_mut::<Events<E>>() which, as far as I know, is not possible with a impl Trait.

[mgi388]

LGTM