Let FilteredEntity(Ref|Mut) receive access when nested.
#18236
Conversation
alice-i-cecile
added
C-Feature
There was a problem hiding this comment.
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
added
S-Waiting-on-Author
|
Added some more comments and tests! While I'm adding docs to the set manipulation methods: A way to make this even more clear would be to create a new type that wraps a Would it be worth doing that, either before, after, or as part of this PR? |
|
I'd prefer to do that separately, but I do think it's a good idea. As long as we're not getting perf regressions I think some clarity there would be really helpful. |
chescock
added
S-Needs-Review
| state.clear_writes(); | ||
| // Prevent any access that would conflict with other accesses in the current query | ||
| state.remove_conflicting_access(access); | ||
| // Finally, add the resulting access to the query access |
There was a problem hiding this comment.
This is interesting; because of how the macro is written, each term of a tuple inside a query like Query<(&A, &B)> sequentially updates the access by comparing with the access computed from the first n previous elements of the tuple?
There was a problem hiding this comment.
I think so? It does two passes. We first call update_component_access to determine the static access for things like &A and &mut B, and then call provide_extra_access to offer any leftover access. So you can do either (&mut A, FilteredEntityMut) or (FilteredEntityMut, &mut A), and the &mut A will claim the A access before the FilteredEntityMut sees it.
But within provide_extra_access, it offers access to each term in order and looks at the access that previous terms took. That causes the behavior you asked about below where (FilteredEntityRef, FilteredEntityMut) can be different from (FilteredEntityMut, FilteredEntityRef).
| @@ -422,6 +423,89 @@ mod tests { | |||
| } | |||
| } | |||
|
|
|||
| #[test] | |||
| fn builder_provide_access() { | |||
| let query = | ||
| QueryState::<(Entity, &A, &B)>::new(&mut world).transmute::<FilteredEntityRef>(&world); | ||
| let query = QueryState::<(Entity, &A, &B)>::new(&mut world) | ||
| .transmute::<(Entity, FilteredEntityRef)>(&world); |
There was a problem hiding this comment.
How come the tests needs an extra 'Entity' in the signature?
There was a problem hiding this comment.
That was just to make the tests start failing before I made the other changes :).
| .data::<&B>() | ||
| .build(); | ||
|
|
||
| // The `FilteredEntityRef` only has read access, so the `FilteredEntityMut` can have read access without conflicts |
There was a problem hiding this comment.
The access is provided from left to right, so if the tuple had been (FilteredEntityMut, FilteredEntityRef), the Ref wouldn't have read access to the mutable component?
It is not super obvious that the order matters, maybe this should be documented slightly more in the FilteredEntityRef/Mut docstrings?
There was a problem hiding this comment.
The access is provided from left to right, so if the tuple had been (FilteredEntityMut, FilteredEntityRef), the Ref wouldn't have read access to the mutable component?
Yes, that's right!
It is not super obvious that the order matters, maybe this should be documented slightly more in the FilteredEntityRef/Mut docstrings?
I wanted to ensure that it was sound to use more than one FilteredEntity thing in a query, but it's not really useful, and I don't expect anyone to actually want to do it. So I'd be inclined to leave the exact semantics out of the documentation so as not to confuse normal users with the details of unimportant edge cases.
alice-i-cecile
added
S-Ready-For-Final-Review
Objective
Let
FilteredEntityRefandFilteredEntityMutreceive 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 afterWorldQuery::update_component_access(), so any access used by ordinary subqueries will be known.FilteredEntityRefandFilteredEntityMutcan 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 ofAccess::extend. I can extract that refactoring to a separate PR if desired.Have
FilteredEntity(Ref|Mut)storeAccessinstead ofFilteredAccessbecause 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.