Configurable Events<T> storage

[TheRawMeatball]

Solves #2071

[NathanSWard]

Is it worth loosening this requirement from just DoubleEndedIterators?
If we're now allowing custom storage types for Events, we should maybe support more than just these types.
For example, if I don't want any duplicate events, I may want to make a Storage type which uses a HashSet internally, but hash_set::Iter does not implement DoubleEndedIter.

[TheRawMeatball]

This trait is a mostly internal trait, which isn't meant to be implemented. HashSet would also fail because it doesn't match the Deref<[_]> criteria.

[TheRawMeatball]

Blocked on #2074

[alice-i-cecile]

This is great functionality, and the code quality looks good. It could just use slightly more than 0 documentation about how to use it <3

[alice-i-cecile]

This needs a comment explaining what it does and why you might want to use it ;)

[mockersf]

too bad events would need to #[derive(Event)] 😞

[alice-i-cecile]

too bad events would need to #[derive(Event)] 😞

On the plus side, this will make me feel much more confident in excluding "Event" from all of my event types :p It should also allow us to extend the API further / increase clarity further elsewhere.

[cart]

Like the derive(Component) proposal, this prevents the ability to use foreign types as events without putting them in a wrapper. Thats one of the bigger downsides imo. Ex: if winit ever sorts out its lifetime weirdness, we want to export raw winit events. This would also affect the ability to integrate things like "external physics library events".

This also feels a bit "inside out". When storing thing X in a hashset or vec, we don't define:

#[derive(Storage)]
#[storage(Vec)]
struct X {
}

You just choose to insert X into a specific storage. We should at least discuss alternatives like:

struct Events<T, S: Storage<S> = Vec<S>> {
}

app.add_event::<KeyCode>()
app.add_event_with_storage::<MyEvent, HashSet<MyEvent>>()

The downside of course is that systems would then need to include the storage type like Res<Events<MyEvent, HashSet<MyEvent>>>.

But we could alias that to something like HashSetEvents<MyEvent>.

[TheRawMeatball]

This is also an alternative, but it isn't one that's particularly appealing IMO, as this means all code must know about the underlying storage type of events, even though for most code these are irrelevant impl details. For integrating with third party types, I don't see an issue with continuing to use custom wrapper types - this is already possible and what we're doing, and it's a decent solution.

[cart]

I don't think store(3) is descriptive enough of what is happening here. We "store" all events, not just 3 of them. I think #[smallvec(3)] makes more sense. This has the added benefit of setting the pattern for future storage derives (ex: #[vecdeque].

[cart]

Although im a little worried that we aren't properly scoping this attribute to events. Theres a world where another derive uses attribute names like store or smallvec.

[alice-i-cecile]

I love #[smallvec(3)] here for both of those reasons.

[mockersf]

#[smallvec(3)] is very hard to read without something linking the attribute to its derive

#[event(storage = smallvec(3))]?

[alice-i-cecile]

as this means all code must know about the underlying storage type of events, even though for most code these are irrelevant impl details

I agree, and think it's really important that users can effortlessly change the storage types. Much like component storage types, this is going to be a common tuning lever. If we can tweak it in a single place and it just works (or even better, automatically explore this vs. benchmarks!) that's an incredible win for usability.

[cart]

This is also an alternative, but it isn't one that's particularly appealing IMO, as this means all code must know about the underlying storage type of events, even though for most code these are irrelevant impl details

Yeah this aspect doesn't bother me as much. Especially in the context of bevy_ecs, whose primary purpose is "inversion of control".

For integrating with third party types, I don't see an issue with continuing to use custom wrapper types - this is already possible and what we're doing, and it's a decent solution.

We currently "mirror" types, which increases maintenance burden and implementation size. It also kills interop with things that use the original type. This was intentional for things like bevy_window because we want it to be a backend-agnostic abstraction, but that won't be true for all event types. Crates providing bevy "middleware" for libraries like physics and sound might be forced to create "wrapper types", which requires the dreaded event.0.field or abusing Deref traits.

[alice-i-cecile]

I wanted to make a collection of arbitrary events today: the Event trait would have been nice to have.

[james7132]

Closing in favor of #7957.