Skip to content

Don't ignore default query filters for EntityRef or EntityMut #20163

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 2 commits into from
Jul 17, 2025

Conversation

chescock
Copy link
Contributor

@chescock chescock commented Jul 16, 2025

Objective

Don't ignore default query filters for EntityRef or EntityMut.

Currently, Query<EntityRef> will include entities with a Disabled component, even though queries like Query<()> or Query<Entity> would not. This was noticed in #19711 (comment).

Solution

Change Access::contains to completely ignore read access and just look at filters and archetypal access. Filters covers With, Without, &, and &mut, while archetypal covers Has and Allows.

Note that Option<&Disabled> will no longer count as a use of Disabled, though.

But do ignore them for `EntityRefExcept` or `EntityMutExcept` that mention the filtered component.
@tim-blackbird tim-blackbird added C-Bug An unexpected or incorrect behavior A-ECS Entities, components, systems, and events S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jul 16, 2025
@tim-blackbird tim-blackbird added this to the 0.17 milestone Jul 16, 2025
Copy link
Contributor

@Trashtalk217 Trashtalk217 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor nit

@@ -272,6 +272,13 @@ impl<T: SparseSetIndex> Access<T> {
.contains(index.sparse_set_index())
}

/// Returns `true` if this either has bounded access including this component
/// or unbounded access not including this component.
pub(crate) fn has_component_read_exception(&self, index: T) -> bool {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't it be something like has_component_read_or_exception, now it sounds like the component has something called a read_exception, which sounds like a Java error.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe an has_component_exception would be even cleaner, but probably more tricky because this part of the code is perf sensitive

/// Returns `true` if this either has bounded access including this component
/// or unbounded access not including this component.
pub(crate) fn has_component_read_exception(&self, index: T) -> bool {
self.component_read_and_writes
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A comment here on why this works would be helpful: this is quite mysterious.

Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

has_component_read_exception is very unclear to me, but this is simple and seems to work (yay tests). If you can find a better name and improve the docs / comments to clarify why this works I'll approve this :)

@chescock
Copy link
Contributor Author

I'm having trouble wording things well, but maybe I can simplify it instead. I realized EntityRefExcept was a red herring here: It's surprising that EntityRefExcept<Disabled> doesn't turn off the filter, but nobody is actually going to write that.

It might work more cleanly if we completely ignore read access and just look at filters and archetypal access. Filters covers With, Without, &, and &mut, while archetypal covers Has and Allows.

That won't count Option<&Disabled> as a use, though. Is that an important case to handle? My impression is that query filters will be for marker components where you'll want to use Has<Disabled> anyway. And it's consistent with edge cases like (EntityRef, Option<&Disabled>). But it is a bit surprising!

@alice-i-cecile
Copy link
Member

That won't count Option<&Disabled> as a use, though. Is that an important case to handle? My impression is that query filters will be for marker components where you'll want to use Has anyway

Yes, Has is much more important than Option here :)

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jul 17, 2025
@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 Jul 17, 2025
Merged via the queue into bevyengine:main with commit 25cb339 Jul 17, 2025
34 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-Bug An unexpected or incorrect behavior S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants