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

Update Event send methods to return EventId #10551

Merged
merged 3 commits into from
Nov 16, 2023

Conversation

bushrat011899
Copy link
Contributor

@bushrat011899 bushrat011899 commented Nov 14, 2023

Objective

Solution

I've updated the various Event send methods to return the sent EventId(s). Since these methods previously returned nothing, and this information is cheap to copy, there should be minimal negative consequences to providing this additional information. In the case of send_batch, an iterator is returned built from Range and Map, which only consumes 16 bytes on the stack with no heap allocations for all batch sizes. As such, the cost of this information is negligible.

These changes are reflected for EventWriter and World. For World, the return types are optional to account for the possible lack of an Events resource. Again, these methods previously returned no information, so its inclusion should only be a benefit.

Usage

Now when sending events, the IDs of those events is available for immediate use:

// Example of a request-response system where the requester can track handled requests.

/// A system which can make and track requests
fn requester(
    mut requests: EventWriter<Request>,
    mut handled: EventReader<Handled>,
    mut pending: Local<HashSet<EventId<Request>>>,
) {
    // Check status of previous requests
    for Handled(id) in handled.read() {
        pending.remove(&id);
    }

    if !pending.is_empty() {
        error!("Not all my requests were handled on the previous frame!");
        pending.clear();
    }

    // Send a new request and remember its ID for later
    let request_id = requests.send(Request::MyRequest { /* ... */ });

    pending.insert(request_id);
}

/// A system which handles requests
fn responder(
    mut requests: EventReader<Request>,
    mut handled: EventWriter<Handled>,
) {
    for (request, id) in requests.read_with_id() {
        if handle(request).is_ok() {
            handled.send(Handled(id));
        }
    }
}

In the above example, a requester system can send request events, and keep track of which ones are currently pending by EventId. Then, a responder system can act on that event, providing the ID as a reference that the requester can use. Before this PR, it was not trivial for a system sending events to keep track of events by ID. This is unfortunate, since for a system reading events, it is trivial to access the ID of a event.


Changelog

  • Updated Events:
    • Added send_batch
    • Modified send to return the sent EventId
    • Modified send_default to return the sent EventId
  • Updated EventWriter
    • Modified send_batch to return all sent EventIds
    • Modified send to return the sent EventId
    • Modified send_default to return the sent EventId
  • Updated World
    • Modified send_event to return the sent EventId if sent, otherwise None.
    • Modified send_event_default to return the sent EventId if sent, otherwise None.
    • Modified send_event_batch to return all sent EventIds if sent, otherwise None.
  • Added unit test test_send_events_ids to ensure returned EventIds match the sent Events
  • Updated uses of modified methods.

Migration Guide

send / send_default / send_batch

For the following methods:

  • Events::send
  • Events::send_default
  • Events::send_batch
  • EventWriter::send
  • EventWriter::send_default
  • EventWriter::send_batch
  • World::send_event
  • World::send_event_default
  • World::send_event_batch

Ensure calls to these methods either handle the returned value, or suppress the result with ;.

// Now fails to compile due to mismatched return type
fn send_my_event(mut events: EventWriter<MyEvent>) {
    events.send_default()
}

// Fix
fn send_my_event(mut events: EventWriter<MyEvent>) {
    events.send_default();
}

This will most likely be noticed within match statements:

// Before
match is_pressed {
    true => events.send(PlayerAction::Fire),
//                 ^--^ No longer returns ()
    false => {}
}

// After
match is_pressed {
    true => {
        events.send(PlayerAction::Fire);
    },
    false => {}
}

- Updated `Events`:
  - Added `send_batch`
  - Modified `send` to return the sent `EventId`
  - Modified `send_default` to return the sent `EventId`
- Updated `EventWriter`
  - Modified `send_batch` to return all sent `EventId`s
  - Modified `send` to return the sent `EventId`
  - Modified `send_default` to return the sent `EventId`
- Updated `World`
  - Modified `send_event` to return the sent `EventId` if sent, otherwise `None`.
  - Modified `send_event_default` to return the sent `EventId` if sent, otherwise `None`.
  - Modified `send_event_batch` to return all sent `EventId`s if sent, otherwise `None`.
- Added unit test `test_send_events_ids` to ensure returned `EventId`s match the sent `Event`s
- Updated uses of modified methods.
@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 M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide labels Nov 14, 2023
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.

Good tests, a simple implementation, and a fantastic writeup. Clever work in the send_batch impl.

P.S. I suspect that the compiler actually fully optimizes out the return type when unused. I don't particularly care if it doesn't, but could be neat to look at the assembly.

@bushrat011899
Copy link
Contributor Author

Good tests, a simple implementation, and a fantastic writeup. Clever work in the send_batch impl.

Thanks!

P.S. I suspect that the compiler actually fully optimizes out the return type when unused. I don't particularly care if it doesn't, but could be neat to look at the assembly.

I suspect so too, but I'm not experienced enough to confirm or deny that for certain. In the worst case, 16 bytes of stack space will be wasted.

crates/bevy_ecs/src/event.rs Outdated Show resolved Hide resolved
@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Nov 16, 2023
Co-authored-by: Nicola Papale <nicopap@users.noreply.github.com>
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Nov 16, 2023
Merged via the queue into bevyengine:main with commit 3c689b9 Nov 16, 2023
21 checks passed
rdrpenguin04 pushed a commit to rdrpenguin04/bevy that referenced this pull request Jan 9, 2024
# Objective

- Fixes bevyengine#10532

## Solution

I've updated the various `Event` send methods to return the sent
`EventId`(s). Since these methods previously returned nothing, and this
information is cheap to copy, there should be minimal negative
consequences to providing this additional information. In the case of
`send_batch`, an iterator is returned built from `Range` and `Map`,
which only consumes 16 bytes on the stack with no heap allocations for
all batch sizes. As such, the cost of this information is negligible.

These changes are reflected for `EventWriter` and `World`. For `World`,
the return types are optional to account for the possible lack of an
`Events` resource. Again, these methods previously returned no
information, so its inclusion should only be a benefit.

## Usage

Now when sending events, the IDs of those events is available for
immediate use:

```rust
// Example of a request-response system where the requester can track handled requests.

/// A system which can make and track requests
fn requester(
    mut requests: EventWriter<Request>,
    mut handled: EventReader<Handled>,
    mut pending: Local<HashSet<EventId<Request>>>,
) {
    // Check status of previous requests
    for Handled(id) in handled.read() {
        pending.remove(&id);
    }

    if !pending.is_empty() {
        error!("Not all my requests were handled on the previous frame!");
        pending.clear();
    }

    // Send a new request and remember its ID for later
    let request_id = requests.send(Request::MyRequest { /* ... */ });

    pending.insert(request_id);
}

/// A system which handles requests
fn responder(
    mut requests: EventReader<Request>,
    mut handled: EventWriter<Handled>,
) {
    for (request, id) in requests.read_with_id() {
        if handle(request).is_ok() {
            handled.send(Handled(id));
        }
    }
}
```

In the above example, a `requester` system can send request events, and
keep track of which ones are currently pending by `EventId`. Then, a
`responder` system can act on that event, providing the ID as a
reference that the `requester` can use. Before this PR, it was not
trivial for a system sending events to keep track of events by ID. This
is unfortunate, since for a system reading events, it is trivial to
access the ID of a event.

---

## Changelog

- Updated `Events`:
  - Added `send_batch`
  - Modified `send` to return the sent `EventId`
  - Modified `send_default` to return the sent `EventId`
- Updated `EventWriter`
  - Modified `send_batch` to return all sent `EventId`s
  - Modified `send` to return the sent `EventId`
  - Modified `send_default` to return the sent `EventId`
- Updated `World`
- Modified `send_event` to return the sent `EventId` if sent, otherwise
`None`.
- Modified `send_event_default` to return the sent `EventId` if sent,
otherwise `None`.
- Modified `send_event_batch` to return all sent `EventId`s if sent,
otherwise `None`.
- Added unit test `test_send_events_ids` to ensure returned `EventId`s
match the sent `Event`s
- Updated uses of modified methods.

## Migration Guide

### `send` / `send_default` / `send_batch`

For the following methods:

- `Events::send`
- `Events::send_default`
- `Events::send_batch`
- `EventWriter::send`
- `EventWriter::send_default`
- `EventWriter::send_batch`
- `World::send_event`
- `World::send_event_default`
- `World::send_event_batch`

Ensure calls to these methods either handle the returned value, or
suppress the result with `;`.

```rust
// Now fails to compile due to mismatched return type
fn send_my_event(mut events: EventWriter<MyEvent>) {
    events.send_default()
}

// Fix
fn send_my_event(mut events: EventWriter<MyEvent>) {
    events.send_default();
}
```

This will most likely be noticed within `match` statements:

```rust
// Before
match is_pressed {
    true => events.send(PlayerAction::Fire),
//                 ^--^ No longer returns ()
    false => {}
}

// After
match is_pressed {
    true => {
        events.send(PlayerAction::Fire);
    },
    false => {}
}
```

---------

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
Co-authored-by: Nicola Papale <nicopap@users.noreply.github.com>
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 M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide 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.

Provide option to obtain EventId on EventWriter send()
3 participants