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

Commands::send_event #14933

Merged
merged 5 commits into from
Aug 27, 2024
Merged

Commands::send_event #14933

merged 5 commits into from
Aug 27, 2024

Conversation

robtfm
Copy link
Contributor

@robtfm robtfm commented Aug 27, 2024

Objective

sending events tends to be low-frequency so ergonomics can be prioritized over efficiency.
add Commands::send_event to send any type of event without needing a writer in hand.

i don't know how we feel about these kind of ergonomic things, i add this to all my projects and find it useful. adding mut this_particular_event_writer: EventWriter<ThisParticularEvent> every time i want to send something is unnecessarily cumbersome.
it also simplifies the "send and receive in the same system" pattern significantly.

basic example before:

fn my_func(
    q: Query<(Entity, &State)>,
    mut damage_event_writer: EventWriter<DamageEvent>,
    mut heal_event_writer: EventWriter<HealEvent>,
) {
    for (entity, state) in q.iter() {
        if let Some(damage) = state.get_damage() {
            damage_event_writer.send(DamageEvent { entity, damage });
        }

        if let Some(heal) = state.get_heal() {
            heal_event_writer.send(HealEvent { entity, heal });
        }
    }
}

basic example after:

import bevy::ecs::event::SendEventEx;

fn my_func(
    mut commands: Commands,
    q: Query<(Entity, &State)>,
) {
    for (entity, state) in q.iter() {
        if let Some(damage) = state.get_damage() {
            commands.send_event(DamageEvent { entity, damage });
        }

        if let Some(heal) = state.get_heal() {
            commands.send_event(HealEvent { entity, heal });
        }
    }
}

send/receive in the same system before:

fn send_and_receive_param_set(
    mut param_set: ParamSet<(EventReader<DebugEvent>, EventWriter<DebugEvent>)>,
) {
    // We must collect the events to resend, because we can't access the writer while we're iterating over the reader.
    let mut events_to_resend = Vec::new();

    // This is p0, as the first parameter in the `ParamSet` is the reader.
    for event in param_set.p0().read() {
        if event.resend_from_param_set {
            events_to_resend.push(event.clone());
        }
    }

    // This is p1, as the second parameter in the `ParamSet` is the writer.
    for mut event in events_to_resend {
        event.times_sent += 1;
        param_set.p1().send(event);
    }
}

after:

use bevy::ecs::event::SendEventEx;

fn send_via_commands_and_receive(
    mut reader: EventReader<DebugEvent>,
    mut commands: Commands,
) {
    for event in reader.read() {
        if event.resend_via_commands {
            commands.send_event(DebugEvent {
                times_sent: event.times_sent + 1,
                ..event.clone()
            });
        }
    }
}

@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Aug 27, 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.

Since this is in bevy_ecs itself, we shouldn't use an extension trait.

I'm in favor of adding this though, provided we have a bit of docs laying out the drawbacks: I've seen this helper show up repeatedly in various projects I've reviewed.

The type-erased nature is genuinely useful for more complex patterns, and this is consistent with observers.

@alice-i-cecile alice-i-cecile added the S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged label Aug 27, 2024
@alice-i-cecile
Copy link
Member

FYI, #13818 adds another little helper for sending and receiving events in the same system.

@NiseVoid
Copy link
Contributor

This pattern is indeed extremely common. Here's a few cursed variations from my game:

impl Command for MyEvent {
    fn apply(self, world: &mut World) {
        world.send_event(self);
    }
}
commands.add(|world: &mut World| world.send_event(event));

One possible footgun here could be that DeferredWorld has send_event but would also get commands().send_event, which would be less efficient.

@robtfm
Copy link
Contributor Author

robtfm commented Aug 27, 2024

FYI, #13818 adds another little helper for sending and receiving events in the same system.

sorry, i don't see it?

One possible footgun here could be that DeferredWorld has send_event but would also get commands().send_event, which would be less efficient.

i can rename - i generally call it fire_event but i was trying to be a bit more vanilla with the naming..

@NiseVoid
Copy link
Contributor

i can rename - i generally call it fire_event but i was trying to be a bit more vanilla with the naming..

I think send_event is fine, it does match better with the other ones, we can either assume people will know commands are slower, or have the docs tell people to use the one on World or DeferredWorld instead if they have access to it.

@ThomasAlban
Copy link
Contributor

If this is merged, would it be the default way to send events or would there be 2 ways of doing the same thing? If so, which one would be preferred?

@alice-i-cecile
Copy link
Member

The EventWriter approach is preferred: it has much lower overhead. The commands approach is good for lazy prototyping, cold loop code where you prefer the ergonomics, or working with heterogeneous event types.

@atornity
Copy link
Contributor

thoughts on automatically registering the event type as well? So that people who use this pattern won't have to call App::add_event.

@alice-i-cecile
Copy link
Member

We definitely shouldn't do that: add_event configures a specific kind of event clearing that not every event wants.

@robtfm
Copy link
Contributor Author

robtfm commented Aug 27, 2024

We definitely shouldn't do that: add_event configures a specific kind of event clearing that not every event wants.

we could add it only if Events<E> isn't found; i believe the way to configure events manually is by adding the resource so it shouldn't interfere?

Copy link
Member

@janhohenheim janhohenheim left a comment

Choose a reason for hiding this comment

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

Definitely wanted to use this pattern in the past for code running in unit tests! Thanks for the PR :)
No thoughts on the automatic registration of the passed event.
Got some minor suggestions before approval.

crates/bevy_ecs/src/event/send_event.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/event/send_event.rs Show resolved Hide resolved
crates/bevy_ecs/src/event/send_event.rs Show resolved Hide resolved
crates/bevy_ecs/src/system/commands/mod.rs Outdated Show resolved Hide resolved
@janhohenheim janhohenheim added the D-Straightforward Simple bug fixes and API improvements, docs, test and examples label Aug 27, 2024
Co-authored-by: Jan Hohenheim <jan@hohenheim.ch>
@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-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Aug 27, 2024
@alice-i-cecile
Copy link
Member

Merging once CI is green: suggestions probably broke formatting.

auto-merge was automatically disabled August 27, 2024 23:34

Head branch was pushed to by a user without write access

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Aug 27, 2024
Merged via the queue into bevyengine:main with commit 45281e6 Aug 27, 2024
26 checks passed
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-Usability A targeted quality-of-life change that makes Bevy easier to use D-Straightforward Simple bug fixes and API improvements, docs, test and examples 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.

6 participants