Add EventConsumer #2145
Add EventConsumer #2145
Conversation
Moves impl blocks directly after types, organizes like-types together, presents shared functionality first
Also added handy drain_with_id method
alice-i-cecile
added
A-ECS
Event buffer's state should not clash with State concept
crates/bevy_ecs/src/event.rs
Outdated
| }; | ||
|
|
||
| event_instances.map(|ei| { | ||
| trace!("Events::drain_with_id -> {}", ei.event_id); |
There was a problem hiding this comment.
In the case we call drain() it may be a little confusing if our trace! message is "Events::drain_with_id", however I'm not sure if there is an obvious work around for this.... 🤔
There was a problem hiding this comment.
Yeah. I'm not super concerned about that; if you're using this trace functionality you're a relatively advanced user IMO.
alice-i-cecile
force-pushed
the
event-consumer
branch
from
8f47946
to
51b6195
Compare
Consistency with event_consumer and improves simplicity
Fails on line 467, after we try again
Required to ensure Event readers continue functioning correctly
|
@NathanSWard this has all the functionality I want now and works correctly! Caught a serious existing bug in our |
Co-authored-by: Nathan Ward <[email protected]>
Co-authored-by: Nathan Ward <[email protected]>
| // We can't use .add_event or our events will be cleaned up too soon | ||
| .init_resource::<Events<MyEvent>>() |
There was a problem hiding this comment.
I feel like this is a little nuisanced and could be easily missed by a user.
Is there a way to group an EventConsumer with add_event or warn if they are both added?
There was a problem hiding this comment.
Hmm. I don't think there's a sensible way to group it. The latter feels useful, but I'm not quite sure how we'd manage that.
Basically we'd need fairly global analysis to get that right; we need to see a) does any system use an EventConsumer and b) was add_event::T ever called.
There was a problem hiding this comment.
Maybe the answer is to rename add_event to be more explicit? I'm not sure what name you'd want there though :/
We can also ramp up the docs on both EventConsumer and add_event some more?
There was a problem hiding this comment.
Actually: what if we added a cleanup field to add_event that explicitly controlled the behavior? I think that would help make things much more explicit and demistify the default behavior.
|
FYI, I'm still a bit iffy about the single consumer nature of this. It makes me very worried about maintainability of systems using this code. That said, this is much simpler than a proper multiconsumer solution like #1776 + bevyengine/rfcs#17 and builds on the existing architecture without any breaking changes. |
That's fair. Since this is a concern and we may not move forward with this, @alice-i-cecile is it alright if I rebase #2149 on main instead of this branch? |
Absolutely: that's an easy fix. FYI, the improvements I made to the drain methods are also strict improvements over the existing implementation, and solve a serious bug. |
Taken from #2145 On draining, `Events` did not reset it's internal event count members.
Taken from #2145 On draining and clearing, dangling `EventReaders` would not read into the correct event offset.
|
Do we have any further thoughts on this PR? |
|
I'm unhappy with both the current double buffering solution and the event consumer pattern here due to the likelihood of introducing unexpected and hard to mitigate bugs. Seperate schedules may solve the fixed time step problem though, which would make me feel okay about double buffering again. Let's wait and see what comes of the pipelining work IMO. |
|
Closing this out; it's not a great solution. |
Taken from bevyengine#2145 On draining and clearing, dangling `EventReaders` would not read into the correct event offset.
Add an EventConsumer type, which consumes events rather than reading them. Good for simple automatic consumption, which is particularly likely to occur with the per-entity-events pattern found in #2116.