Skip to content
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

Let FilteredEntity(Ref|Mut) receive access when nested. #18236

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

chescock
Copy link
Contributor

Objective

Let FilteredEntityRef and FilteredEntityMut receive access when nested inside tuples or #[derive(QueryData)] types. Make sure to exclude any access that would conflict with other subqueries!

Fixes #14349

Solution

Replace WorldQuery::set_access(state, access) with a new method, QueryData::provide_extra_access(state, access, available_access), that passes both the total available access and the currently used access. This is called after WorldQuery::update_component_access(), so any access used by ordinary subqueries will be known. FilteredEntityRef and FilteredEntityMut can use the combination to determine how much access they can safely take, while tuples can safely pass those parameters directly to their subqueries.

This requires a new Access::remove_conflicting_access() method that can be used to remove any access that would conflict with existing access. Implementing this method was easier by first factoring some common set manipulation code out of Access::extend. I can extract that refactoring to a separate PR if desired.

Have FilteredEntity(Ref|Mut) store Access instead of FilteredAccess because they do not need to keep track of the filter. This was necessary in an early draft but no longer is. I left it in because it's small and I'm touching that code anyway, but I can extract it to a separate PR if desired.

@alice-i-cecile alice-i-cecile added C-Feature A new feature, making something new possible A-ECS Entities, components, systems, and events D-Complex Quite challenging from either a design or technical perspective. Ask for help! X-Uncontroversial This work is generally agreed upon D-Unsafe Touches with unsafe code in some way S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Mar 10, 2025
@alice-i-cecile alice-i-cecile added the C-Bug An unexpected or incorrect behavior label Mar 11, 2025

let mut entity = new_query.single_mut(&mut world).unwrap();
let (_entity, mut entity) = new_query.single_mut(&mut world).unwrap();
Copy link
Member

Choose a reason for hiding this comment

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

This variable naming is very confusing.

access: &mut Access<ComponentId>,
available_access: &Access<ComponentId>,
) {
state.clone_from(available_access);
Copy link
Member

Choose a reason for hiding this comment

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

Comment here please: this is pretty tricky to understand when skimming out of context.

@@ -838,6 +818,40 @@ impl<T: SparseSetIndex> Access<T> {
}
}

/// Performs an in-place union of `other` into `self`, where either set may be inverted.
Copy link
Member

Choose a reason for hiding this comment

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

I'd like more docs here, explaining what the right logic is. The union terminology is very precise, but doesn't help people understand what that means in practice.

}
}

/// Performs an in-place set difference of `other` from `self`, where either set may be inverted.
Copy link
Member

Choose a reason for hiding this comment

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

Same request for more docs here.

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.

As far as I can tell this is correct, and mildly useful. I'd like to move forward with this, but this makes an already confusing bit of the code base more confusing. I've left suggestion for more docs, but I'd also like to see some unit tests for the invertable union / difference methods.

Those are pretty rough to mess up, and seem very testable!

@alice-i-cecile alice-i-cecile added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Mar 11, 2025
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 C-Feature A new feature, making something new possible D-Complex Quite challenging from either a design or technical perspective. Ask for help! D-Unsafe Touches with unsafe code in some way S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged X-Uncontroversial This work is generally agreed upon
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FilteredEntityRef/FilteredEntityMut's access is empty when used in composite queries
2 participants