bevyengine / bevy

A refreshingly simple data-driven game engine built in Rust
https://bevyengine.org
Apache License 2.0
36.11k stars 3.56k forks source link

EventReader iter can skip events if interrupted #1157

Closed alice-i-cecile closed 3 years ago

alice-i-cecile commented 3 years ago

Bevy version

0.4

Operating system & version

Ex: Windows 10, Ubuntu 18.04, iOS 14.

What you did

Ran the example here: https://github.com/alice-i-cecile/understanding-bevy/blob/afe2d7dff009037f2f272c0dff18fab14af64aa2/src/ecs/systems-queries_code/examples/concurrency.rs

What you expected to happen

Each event should be processed in order.

What actually happened

Most events are skipped, because the system runs out of time to process them and EventReader.iter() updates its internal counter all the way to the end of the current queue. This is documented but is surprising behavior that is likely generally unhelpful.

Additional information

Exposing the current length of the EventReader would also be helpful for similar tasks :)

alice-i-cecile commented 3 years ago

EventReader.earliest and EventReader.latest have the same behavior, with even stranger consequences for most use cases, and so don't get around this issue.

alice-i-cecile commented 3 years ago

For the use pattern outlined in my example, I'm fine manually updating the Events, rather than using the double buffer from Events::update. But in order to do so, I would want a nice way to pop off one event at a time (you should also be able to remove one particular event), rather than having to do something terrible with drain or clear and then reconstructing the events at the end of the stage.

cart commented 3 years ago

The "bug" in your code is that you fail to process events on the frame (or the frame after) they were produced.

I think the crux of this issue is that you would prefer event queues with unbounded allocations. The "double buffered" approach we use by default ensures that if events are not processed within a given frame (or the frame after), they will be freed / not take up space in the queue. I think this is a desirable default for most events because users won't process most events by default. It keeps the total allocations and the cost of readers low. From an efficiency perspective I think the current approach is basically optimal for games. And the "you must handle events each frame" constraint is pretty reasonable imo. Every other major engine has the same constraints, they just have a different interface (ex: register an event handler for a specific event type instead of using a system).

There will be some cases where unbounded event queues are desirable, but its the sort of thing you need to specifically code around, otherwise you risk allocating memory ad-infinitum.

cart commented 3 years ago

But adding an option to "pop off" a single event (for a given reader) sounds like a reasonable add.

alice-i-cecile commented 3 years ago

Yeah, I did a bunch more digging into this after filing an issue. I was trying to use Bevy's events as if they were consuming, whereas they by default work on a observing / pub-sub / double buffer paradigm.

Adding a couple of methods to Event for pop from end and pop specific event would let us comfortably support the rarer, consuming form with a bit of leg work on the part of the user (you can just avoid calling Event::update() for your event type) but leave the rest of the design untouched :)

alice-i-cecile commented 3 years ago

This can be resolved with ManualEventReader, and the proposed pattern is better handled by the planned async systems. Closing this.