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

EventReader iter can skip events if interrupted #1157

Closed
alice-i-cecile opened this issue Dec 28, 2020 · 6 comments
Closed

EventReader iter can skip events if interrupted #1157

alice-i-cecile opened this issue Dec 28, 2020 · 6 comments
Labels
A-Core Common functionality for all bevy apps C-Feature A new feature, making something new possible

Comments

@alice-i-cecile
Copy link
Member

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
Copy link
Member Author

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
Copy link
Member Author

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.

@Moxinilian Moxinilian added C-Feature A new feature, making something new possible A-Core Common functionality for all bevy apps labels Dec 28, 2020
@cart
Copy link
Member

cart commented Dec 28, 2020

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
Copy link
Member

cart commented Dec 28, 2020

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

@alice-i-cecile
Copy link
Member Author

alice-i-cecile commented Dec 28, 2020

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
Copy link
Member Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Core Common functionality for all bevy apps C-Feature A new feature, making something new possible
Projects
None yet
Development

No branches or pull requests

3 participants