Make EventReader a SystemParam

[TheRawMeatball]

This commit makes EventReader a SystemParam to significantly cut down on boilerplate, and creates a simplified ManualEventReader to allow for use cases with direct access to Resources. As a necessary side effect, this commit also adds generic type support to #[derive(SystemParam)]. Before a merge, I'd also suggest cutting the EventReader API down to only iter() and iter_with_id(), since rust iterators provide more idiomatic solutions to the rest of the functions.

[cart]

Very cool. For most things I don't want to encourage special-cased SystemParams as they increase api surface, but events are "core" enough that I think its worth investing in ergonomics.

As a necessary side effect, this commit also adds generic type support to #[derive(SystemParam)]

Brilliant. Very glad to have that back. I punted it when adding lifetimes to SystemParams because I had no clue how to filter the generics. I'll add the pattern you used here to my repertoire 😄

Before a merge, I'd also suggest cutting the EventReader API down to only iter() and iter_with_id(), since rust iterators provide more idiomatic solutions to the rest of the functions.

I'm sold on that!

[TheRawMeatball]

Before merging, I'd suggest changing or extending the documentation. I'd do it, but I don't know what to add, I just have a feeling the docs emphasize the Events too much. Maybe not a large problem since the examples are changed, but still

[alice-i-cecile]

I've read over the changes, and like this PR a lot. The reduced API surface in particular is a good direction: I like being able to rely on core functionality more.

1. I discussed the "consuming event" pattern in EventReader iter can skip events if interrupted #1157: it looks like we should be able to do that with raw_events.iter().next() pretty elegantly, so this PR should close that issue.

2. IMO, a second event reader should be added to the docstring example, to clearly demonstrate that multiple event readers can read the same event.

3. We should have an example for how to manually handle your own events, possibly using the consuming event pattern. It's not obvious how to do so from the current examples / docs and you need to dig into how add_event works to figure it out.

[alice-i-cecile]

I would also like to see Events be able to be called in system parameters directly. Rather than mut my_events: ResMut<Events<MyEvent>>, use mut my_events: Events<MyEvent>. This allows us to expose a parallel-safe API for events (like our current Commands implementation) that only allows users to write to the end, which lets us run multiple systems that generate the same event in parallel.

In the rare cases where you want full read or write access (like when processing raw events), you can use Res<MyEvent> instead.

[tigregalis]

I love this change. The boilerplate was difficult to write from scratch from memory without looking at a reference implementation each time. This feels really intuitive (and cuts down on a lot of boilerplate).

[TheRawMeatball]

I would also like to see Events be able to be called in system parameters directly.

Hmm, Events is the backing resource, so I feel like using it directly would be a bad idea. Perhaps a EventWriter? If that is OK, I can implement it on this branch, but it'll have to wait until Ratysz s new scheduler lands. So, I'd recommend merging the reader first, and after the new scheduler is merged I'll open a PR for a writer as well

[tigregalis]

I would also like to see Events be able to be called in system parameters directly.

Hmm, Events is the backing resource, so I feel like using it directly would be a bad idea. Perhaps a EventWriter? If that is OK, I can implement it on this branch, but it'll have to wait until Ratysz s new scheduler lands. So, I'd recommend merging the reader first, and after the new scheduler is merged I'll open a PR for a writer as well

EventReader + EventWriter makes a lot of sense.

[TheRawMeatball]

The EventWriter is now implemented at https://github.com/TheRawMeatball/bevy/tree/event-writer! With possibly more things taking the same route Commands does, I might also simplify &mut Commands etc to just be Commands

[TheRawMeatball]

I changed it back since it was a minor change, though I don't think its necessary since we're only checking if an event occured, so its probably fine to use either. I originally chose next since it made simpler code.

[cart]

Changing next() to latest() (or next_back()) isn't a frivolous no-op. Calling next() in the context above results in:

1. returning the oldest event
2. updating the index to ignore all newer events in the buffers

This would result in dropping newer events if we queue up multiple in the same frame. If someone called events.send(A) and then events.send(B), in this case B should clearly be the final state we end up in. Would you not consider ending in state A a bug?

[TheRawMeatball]

Hmm, I thought it wouldn't matter since all events are erased at the end of the frame, do they last longer? Also, since we aren't looking at the state in the events, wouldn't A and B be identical

[cart]

Bawhaha you are absolutely right. I totally missed that we are just calling is_some() (and reading the "latest" value from the window directly).

Thanks for keeping me honest 😄

[cart]

(it really is a frivolous no-op)