-
-
Notifications
You must be signed in to change notification settings - Fork 4k
Make observers metadata / storage publicly visible #19608
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
Make observers metadata / storage publicly visible #19608
Conversation
} | ||
|
||
/// Collection of [`ObserverRunner`] for [`Observer`] registered to a particular trigger. | ||
impl CachedComponentObservers { | ||
/// Returns the observers listening for this trigger, regardless of target. |
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 changed trigger
to event
on a precedent line, but here trigger
is back, it's a bit confusing ; maybe doc linking to actual type may help, so documentation for what those terms refer to would be easily reached and consulted 🤔
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.
a minor comment on clarification needed regarding trigger vs events, but it's difficult to make better actionable feedbacks, I think this PR is useful and in a good enough state so I'm approving.
Objective
Our strategy for storing observers is made up of several moving parts, which are ultimately fairly simple nested HashMaps.
These types are currently
pub
, but lack any meaningful way to access this data.We have three options here:
pub
at all.Solution
I've opted for option 2, exposing read-only values. This is consistent with our existing approach to the ECS internals, allowing for easier debugging without risking wanton data corruption. If one day you would like to mutably access this data, please open an issue clearly explaining what you're trying to do.
This was a pretty mechanical change, exposing fields via getters. I've also opted to do my best to clarify some field names and documentation: please double-check those for correctness. It was hard to be fully confident, as the field names and documentation was not very clear ;)
Testing
I spent some time going through the code paths, making sure that users can trace all the way from
World
to the leaf nodes. Reviewers, please ensure the same!Notes for reviewers
This is part of a broader observer overhaul: I fully expect us to change up these internals and break these shiny new APIs. Probably even within the same cycle!
But clean up your work area first: this sort of read-only getter and improved docs will be important to replace as we work.