Observer trigger refactor #19935
Observer trigger refactor #19935
Conversation
alice-i-cecile
added
A-ECS
|
What is the motivation for this change? The hackMD linked on the previous PR is kinda big and the few search terms did not hit the part that mentions the motivation for this |
The usage of ComponentId is quite confusing: events are not components. By newtyping this, we can prevent stupid mistakes, avoid leaking internal details and make the code clearer for users and engine devs reading it. |
hukasu
added
S-Ready-For-Final-Review
| resource::Resource, | ||
| world::World, | ||
| }; | ||
|
|
||
| #[doc(hidden)] | ||
| struct RegisteredEvent { | ||
| component_id: ComponentId, | ||
| event_key: EventKey, | ||
| // Required to flush the secondary buffer and drop events even if left unchanged. | ||
| previously_updated: bool, | ||
| // SAFETY: The component ID and the function must be used to fetch the Events<T> resource |
There was a problem hiding this comment.
Maybe the comment would be clearer if it said "The EventKey's component ID" ?
| /// of the event passed into the observer system. | ||
| pub unsafe fn with_event(mut self, event: ComponentId) -> Self { | ||
| pub unsafe fn with_event(mut self, event: EventKey) -> Self { |
There was a problem hiding this comment.
In Observers::get_observers_mut, we have event_key: EventKey, and here we have event: EventKey. As a reader of the code, I'm wondering if they're different for a reason. Or are they just two ways of spelling the same thing?
Also, there are distinct structs Event and EventKey. In other parts of the PR, "the type of the event" was previously named event_type and changed to event_key. So maybe this arg should be event_key ?
| @@ -182,7 +182,7 @@ pub struct ObserverTrigger { | |||
| /// The [`Entity`] of the observer handling the trigger. | |||
| pub observer: Entity, | |||
| /// The [`Event`] the trigger targeted. | |||
| pub event_type: ComponentId, | |||
| pub event_key: EventKey, | |||
There was a problem hiding this comment.
If I understand right, this isn't the Event as such, it's the EventKey that represents the type of the event. So the comment could be made more precise.
| @@ -749,7 +749,7 @@ impl<'w> DeferredWorld<'w> { | |||
| #[inline] | |||
| pub(crate) unsafe fn trigger_observers( | |||
| &mut self, | |||
| event: ComponentId, | |||
| event: EventKey, | |||
There was a problem hiding this comment.
would it be clearer to name this event_key, as the type of the argument is EventKey not Event ?
| @@ -773,7 +773,7 @@ impl<'w> DeferredWorld<'w> { | |||
| #[inline] | |||
| pub(crate) unsafe fn trigger_observers_with_data<E, T>( | |||
| &mut self, | |||
| event: ComponentId, | |||
| event: EventKey, | |||
There was a problem hiding this comment.
would it be clearer to name this event_key, as the type of the argument is EventKey not Event ?
|
@SteveAlexander the PR already went in, could you take your suggestions and open a new PR? |
|
Thanks @hukasu, I responded to a request to review on Discord, and didn't check the timestamp :-) Yes, I'll open a new PR. |
Objective
ComponentId#19755