Skip to content

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

Merged
merged 9 commits into from
Jun 16, 2025

Conversation

alice-i-cecile
Copy link
Member

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:

  1. Make these internals not pub at all.
  2. Make the data read-only accessible.
  3. Make the data mutably accessible.

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.

@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use X-Contentious There are nontrivial implications that should be thought through D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jun 13, 2025
@alice-i-cecile alice-i-cecile added this to the 0.17 milestone Jun 13, 2025
@alice-i-cecile alice-i-cecile requested a review from Vrixyz June 13, 2025 02:04
}

/// Collection of [`ObserverRunner`] for [`Observer`] registered to a particular trigger.
impl CachedComponentObservers {
/// Returns the observers listening for this trigger, regardless of target.
Copy link
Member

@Vrixyz Vrixyz Jun 13, 2025

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 🤔

Copy link
Member

@Vrixyz Vrixyz left a 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.

@alice-i-cecile alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jun 13, 2025
@alice-i-cecile alice-i-cecile enabled auto-merge June 16, 2025 21:42
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jun 16, 2025
Merged via the queue into bevyengine:main with commit 7237c21 Jun 16, 2025
32 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it X-Contentious There are nontrivial implications that should be thought through
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

3 participants