-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Observers overhaul: statically-typed multi-event-listening and safer dynamic-event-listening #14674
base: main
Are you sure you want to change the base?
Conversation
Don't know if this helps, but here's an example of a macro that does tuple expansion: https://github.com/viridia/quill/blob/main/crates/bevy_mod_stylebuilder/src/lib.rs#L80 |
…g some of the unsafe blocks. also added a safe method for adding events to an observer when it's all untyped
…t the event type matches the event set before casting
…ke it more obvious that it does the same thing
It looks like your PR is a breaking change, but you didn't provide a migration guide. Could you add some context on what users should update when this change get released in a new version of Bevy? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I only looked at the tests)
I like the idea, and it seems to be a big amount of work, kudos!
I wonder if there would be a way to resolve some of the ergonomics:
- introducing
SemiDynamicEvent
andDynamicEvent
is a bit confusing - the user needing to use
Or2
,Or3
, etc. - using
Err
as a match branch for dynamic events
world.observe(|t: Trigger<(Foo, Bar)>, mut res: ResMut<R>| { | ||
res.0 += 1; | ||
match t.event() { | ||
Or2::A(Foo(v)) => assert_eq!(5, *v), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like what the PR enables, but I think there are some ergonomic papercuts that should be resolved. It's not great that the user has to remember to use Or2
, Or3
, etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, unfortunately I don't think we're getting anything like anonymous enums anytime soon. Suggestions welcome on how to improve it!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
t.event::<Foo>()
returning an Option
would be more ergonomic IMO and would also work for dynamic events by trying to downcast to Foo
.
Probably wouldn't be as cheap as an enum for static events though (although I haven't looked at the code). If that's important, I suppose you could have the user define a named enum with a #[derive]
to make it work in a Trigger
. Verbose but easy to understand and use.
Observer::new(|_: Trigger<OnAdd, A>, mut res: ResMut<R>| res.0 += 1) | ||
.with_event(on_remove) | ||
}, | ||
Observer::new(|_: Trigger<DynamicEvent, A>, mut res: ResMut<R>| res.0 += 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm looking at the tests to understand the new things you added; and I noticed that this test doesn't access the dynamic event? I think it would be nice for to showcase how to access the event from the observer
|trigger: Trigger<SemiDynamicEvent<OnAdd>, A>, mut res: ResMut<R>| { | ||
match trigger.event() { | ||
Ok(_onadd) => res.assert_order(0), | ||
Err(_ptr) => res.assert_order(1), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Err
is used to match on the dynamic event? That seems a bit unintuitive, since I would associate it with an error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I viewed it as "the statically known types failed to match", so returning as a pointer is a kind of failure condition. Would you recommend swapping it to Or2, instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO I would use a custom enum here, with Static
and Dynamic
arms.
Removed the auto-registration feature of |
world.flush(); | ||
assert_eq!(3, world.resource::<R>().0); | ||
world.trigger_targets(OtherPropagating, child); | ||
world.flush(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can also remove the last two flushes and enclose the result in a new context. See:
let grandparent = {};
let parent = {};
let child = {
world
.spawn(Parent(parent))
.observe(
|_: Trigger<(EventPropagating, OtherPropagating)>, mut res: ResMut<R>| {
res.0 += 1
},
)
.id()
};
// TODO: ideally this flush is not necessary, but right now observe() returns WorldEntityMut
// and therefore does not automatically flush.
world.flush();
world.trigger_targets(EventPropagating, child);
assert_eq!(3, world.resource::<R>().0);
world.trigger_targets(OtherPropagating, child);
assert_eq!(6, world.resource::<R>().0);
As a note about the concerns for EDIT: I didn't actually realise quite how close |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry: I didn't see this at all. I like the general motivation here, but I'm not fully sold on the ergonomics and API. My gut feeling was that this should be handled in a way akin to AnyOf
, where the user gets a tuple of options back, defining which events were triggered, but I suppose that doesn't quite make sense here: you're not checking for all of the events at once 🤔 I'm very tempted to wait for min_exhaustive_patterns
and see if it improves things.
More importantly, I'd like to see this work unified with #15287 and the conjunction behavior in #15327. #15325 is also related. That doesn't have to be this PR, but I'd like to see a plan for how this all fits together, and ideally get it all out in a single cycle.
The benchmarks are great, but docs are missing, making this a lot harder for users and reviewers to understand.
In summary:
- Yes, I want to do something like this.
- I want to see how this fits into broader reworks for hooks, observers and conjunction behavior and ship that all in a single cycle.
- Needs docs!
cmds.entity(e).trigger((Damage(5), Poison)) Which would make |
Yep, I think the |
Objective
Closes #14649.
Solution
The following presented solution is:
Trigger<Foo>
toTrigger<(Foo, Bar)>
.0.14
will continue to function as expected without any code changes.Event Sets
We introduce a new trait named
EventSet
that performs a role similar toWorldQuery
, although with a much smaller API surface. AnEventSet
can be / is implemented by:Event
types.Event
types (up to arity =15). Supports nesting tuples inside tuples.DynamicEvent
type that allows raw-pointer-style observation.SemiDynamicEvent
type that allows you to first match on statically-known event types, and if none match operates like aDynamicEvent
.The most notable operations that an
EventSet
provides are:unchecked_cast
: Casts the raw pointer into the type of the output item.matches
: Returns true if the event set contains the provided event component ID.init_components
: Initializes all required event types to have component IDs.Statically-known multiple-event observers
Your observers can now be upgraded from single-event observers to multiple-event observers with the simple change to a tuple for your
Trigger
generic parameter. If a single event type is specified in the trigger, then the returned item type remains a&Event
or&mut Event
. If a tuple of event types are specified in the trigger, then an enum with the same arity is the returned item type, for example:enum Or2<EventA, EventB> { A(EventA), B(EventB) }
.Dynamically-known event observers
The
DynamicEvent
type does not perform any pointer casting or type matching logic, and instead returnstrue
when matching on ANY event type. This is most useful when you want to listen for event types that are not known at compile time. The returned items accessible withTrigger::event()
andTrigger::event_mut()
arePtr
andPtrMut
, respectively, which are type-erased but lifetime'd pointers.Semi-dynamically-known event observers
For complex scenarios, we can also combine both statically-known and runtime-known event types in a single observer, using
SemiDynamicEvent<Static>
. This means all event types that don't match the specified statically-known event types will be returned as aPtr
/PtrMut
. The returned item type forSemiDynamicEvent
isResult<StaticallyKnownItems, PtrMut>
.TODO
EventSet
types.Testing
observer_multiple_events_static
: Tests functionality of reading event data from two different statically-known event types.observer_multiple_events_dynamic
: TestsDynamicEvent
's behavior of passing pointers as-is.observer_multiple_events_semidynamic
: TestsSemiDynamicEvent
's behavior of first trying to match on statically-known event types, and falling back toDynamicEvent
if none match.observer_propagating_multi_event_between
: Tests that event sets do not interfere with current propagation code.observer_propagating_multi_event_all
: Same as aboveobserve_multievent
: Tests the matching speed of tuple event sets from size 1 to 15 (No observed slowdowns).(A,)
(size=1) tuple implementation simply forwards toA
, which is equivalent to current observer code.observe_dynamic
: Tests the matching speed of runtime known event types withDynamicEvent
(No observed slowdowns).observe_semidynamic
: Tests the matching speed of combined statically-known and runtime-known event types withSemiDynamicEvent
(No observed slowdowns).Migration Guide
Observer::with_event
was renamed toObserver::with_event_unchecked
. If you were using this function, consider using a new one that has been added under the old name, which isn't markedunsafe
but requires yourTrigger<E>
'sE
type to be eitherDynamicEvent
orSemiDynamicEvent
.