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

[Merged by Bors] - exact sized event iterators #3863

Closed
wants to merge 3 commits into from

Conversation

mockersf
Copy link
Member

@mockersf mockersf commented Feb 4, 2022

Objective

  • Remove Resource binding on events, introduce a new Event trait
  • Ensure event iterators are ExactSizeIterator

Solution

Changelog

  • Events, EventWriter, EventReader and so on now require that the underlying type is Event, rather than Resource. Both of these are trivial supertraits of Send + Sync + 'static with universal blanket implementations: this change is currently purely cosmetic.
  • Event reader iterators now implement ExactSizeIterator

@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Feb 4, 2022
@mockersf mockersf force-pushed the exact-sized-events branch 2 times, most recently from 86540de to ddd900c Compare February 4, 2022 11:45
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.

Yes please to an event trait. IMO we may want to remove the blanket impl later, but that can be done when we need it.

@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events C-Code-Quality A section of code that is hard to understand or change C-Usability A targeted quality-of-life change that makes Bevy easier to use and removed S-Needs-Triage This issue needs to be labelled labels Feb 4, 2022
@alice-i-cecile
Copy link
Member

Related to #2073, which also introduces and uses an Event trait.

@alice-i-cecile alice-i-cecile added the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Feb 4, 2022
@mockersf
Copy link
Member Author

mockersf commented Feb 4, 2022

@alice-i-cecile why the C-Breaking-Change? I don't think there's anything breaking here?

@alice-i-cecile
Copy link
Member

The Resource bound is changing to Event. This will break external code that abstracts over events. The fix is completely trivial though.

@mockersf
Copy link
Member Author

mockersf commented Feb 4, 2022

Nop, everything that impl Resource also impl Event, the two traits are equivalent (at least for now). I copied the declaration and the blanket impl...

@alice-i-cecile
Copy link
Member

alice-i-cecile commented Feb 4, 2022

So, consider this extension method on World from #3839, which I stole from my private code base:

    pub fn send_event<E: Resource>(&mut self, event: E) {
        let mut events: Mut<Events<E>> = self.get_resource_mut()
        .expect("The specified event resource was not found in the world. Did you forget to call `app.add_event::<E>()`?");

        events.send(event);
    }

This can be written entirely in end user code, and will cease to compile after this change. Every possible use of it will still be valid, but the type signature itself is now wrong because of the bound here:

impl<T: Event> Events<T> {
.

I 100% think this is the right call, but the change is technically breaking 🤷🏽

EDIT: no, this will still compile, because Rust can infer that all Resource will fulfill Event. You were right.

@alice-i-cecile alice-i-cecile removed the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Feb 4, 2022
@alice-i-cecile
Copy link
Member

@mockersf can you write a changelog for this when you get a moment? Stealing from #4685 should cover most of it.

@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 May 7, 2022
@alice-i-cecile
Copy link
Member

bors r+

bors bot pushed a commit that referenced this pull request May 9, 2022
# Objective

- Remove `Resource` binding on events, introduce a new `Event` trait
- Ensure event iterators are `ExactSizeIterator`

## Solution

- Builds on #2382 and #2969

## Changelog

- Events<T>, EventWriter<T>, EventReader<T> and so on now require that the underlying type is Event, rather than Resource. Both of these are trivial supertraits of Send + Sync + 'static with universal blanket implementations: this change is currently purely cosmetic.
- Event reader iterators now implement ExactSizeIterator
@bors bors bot changed the title exact sized event iterators [Merged by Bors] - exact sized event iterators May 9, 2022
@bors bors bot closed this May 9, 2022
robtfm pushed a commit to robtfm/bevy that referenced this pull request May 10, 2022
# Objective

- Remove `Resource` binding on events, introduce a new `Event` trait
- Ensure event iterators are `ExactSizeIterator`

## Solution

- Builds on bevyengine#2382 and bevyengine#2969

## Changelog

- Events<T>, EventWriter<T>, EventReader<T> and so on now require that the underlying type is Event, rather than Resource. Both of these are trivial supertraits of Send + Sync + 'static with universal blanket implementations: this change is currently purely cosmetic.
- Event reader iterators now implement ExactSizeIterator
exjam pushed a commit to exjam/bevy that referenced this pull request May 22, 2022
# Objective

- Remove `Resource` binding on events, introduce a new `Event` trait
- Ensure event iterators are `ExactSizeIterator`

## Solution

- Builds on bevyengine#2382 and bevyengine#2969

## Changelog

- Events<T>, EventWriter<T>, EventReader<T> and so on now require that the underlying type is Event, rather than Resource. Both of these are trivial supertraits of Send + Sync + 'static with universal blanket implementations: this change is currently purely cosmetic.
- Event reader iterators now implement ExactSizeIterator
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
# Objective

- Remove `Resource` binding on events, introduce a new `Event` trait
- Ensure event iterators are `ExactSizeIterator`

## Solution

- Builds on bevyengine#2382 and bevyengine#2969

## Changelog

- Events<T>, EventWriter<T>, EventReader<T> and so on now require that the underlying type is Event, rather than Resource. Both of these are trivial supertraits of Send + Sync + 'static with universal blanket implementations: this change is currently purely cosmetic.
- Event reader iterators now implement ExactSizeIterator
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-Code-Quality A section of code that is hard to understand or change C-Usability A targeted quality-of-life change that makes Bevy easier to use 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.

3 participants