-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Nested Queries #21557
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
base: main
Are you sure you want to change the base?
Nested Queries #21557
Conversation
This should be possible, but the access checks are tricky.
…rom nested queries.
I'm not certain it's the best way of going about it but couldn't we implement this by storing the entity of the parent instead of the data and then resolving the data from the entity when it is needed? |
pub fn iter_mut(&mut self) -> QueryIter<'_, 's, D, F> { | ||
pub fn iter_mut(&mut self) -> QueryIter<'_, 's, D, F> | ||
where | ||
D: IterQueryData, |
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.
The PR looks good, but I don't fully understand IterQueryData
.
The trait says
A [`QueryData`] for which instances may be alive for different entities concurrently.
But when doing iter_mut
, we never have two 2 items concurrently right? since we would iterate through them one by one. So even if we have a nested query which accesses multiple entities, there would still not be a collision?
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.
But when doing
iter_mut
, we never have two 2 items concurrently right? since we would iterate through them one by one.
Nope, you can keep the old items around! The lifetime in fn next(&mut self)
isn't connected to the Item
type, so later calls don't invalidate earlier items. That's how things like collect()
work.
/// ``` | ||
#[track_caller] | ||
pub fn transmute_lens<NewD: QueryData>(&mut self) -> QueryLens<'_, NewD> { | ||
pub fn transmute_lens<NewD: SingleEntityQueryData>(&mut self) -> QueryLens<'_, NewD> { |
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.
How come we can only transmute to a SingleEntityQueryData
?
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.
Ah i see in the PR description that this could be relaxed in the future
I'm not sure what you mean. Like, instead of yielding a let mut items = query.iter_mut().collect::<Vec<_>>();
let mapped = items.iter_mut().map(|item| item.get_mut()).collect::<Vec<_>>(); |
Objective
Support queries that soundly access multiple entities.
This can be used to create queries that follow relations, as in #17647.
This can also be used to create queries that perform resource access. This has been supported since #16843, although that approach may become unsound if we do resources-as-components #19731, such as #21346.
Fixes #20315
Solution
Allow a
QueryData
that wants to access other entities to store aQueryState<D, F>
in itsWorldQuery::State
, so that it can create a nestedQuery<D, F>
during the outerfetch
.New
WorldQuery
methodsFor it to be sound to create the
Query
duringfetch
, we need to register theFilteredAccess
of the nested query and check for conflicts with other parameters. Create aWorldQuery::update_external_component_access
method for that purpose. ForQuery as SystemParam
, call this duringinit_access
so the access can be combined with the rest of the system access. For looseQueryState
s, call it duringQueryState::new
.In order to keep the query cache up-to-date, create a
WorldQuery::update_archetypes
method where it can callQueryState::update_archetypes_unsafe_world_cell
, and call it from there.New
QueryData
subtraitsSome operations would not be sound with nested queries! In particular, we want a
Parent<D>
query that reads data from the parent entity by following theChildOf
relation. But many entities may share a parent, so it's not sound to iterate aQuery<Parent<&mut C>>
.It is sound to
get_mut
, though, so we want the query type to exist, just not be iterable. And following the relation in the other direction for aQuery<Children<&mut C>>
is sound to iterate, since children are unique to a given parent.So, introduce two new
QueryData
subtraits:IterQueryData
- For anything it's sound to iterate. This is used to bounditer_mut
and related methods.SingleEntityQueryData
- For anything that only accesses data from one entity. This is used to boundEntityRef::get_components
(see Unsound to callEntityRef::get_components
with aQueryData
that performs resource access #20315). It's also used to boundtransmute
at the moment, although we may be able to relax that later (see below, under Future Work).Note that
SingleEntityQueryData: IterQueryData
, since single-entity queries never alias data across entities, andReadOnlyQueryData: IterQueryData
, since it's always sound to alias read-only data.Here is a summary of the traits implemented by some representative
QueryData
:&T
&mut T
Parent<&T>
Parent<&mut T>
(&mut T, Parent<&U>)
Children<&mut T>
Alternatives
We could avoid the need for the
IterQueryData
trait by making it a requirement for allQueryData
. That would reduce the number of traits required, at the cost of making it impossible to supportQuery<Parent<&mut C>>
.Showcase
Here is an implementation of a
Related<R, D, F>
query using this PR that almost works:That has a few flaws, notably that it fails with
because
QueryData
requires thatState = ReadOnly::State
, butQueryState<D, F> != QueryState<D::ReadOnly, F>
.It's also impossible to implement
get_state
, because constructing aQueryState
requires reading theDefaultQueryFilters
resource, butget_state
can be called fromtransmute
with no access.I believe it's possible to resolve those issues, but I don't think those solutions belong in this PR.
Future Work
There is more to do here, but this PR is already pretty big. Future work includes:
WorldQuery
types for working with relationships #17647AssetChanged
to use nested queries for resource access, and stop tracking resource access separately inAccess
SingleEntityQueryData
bound on transmutes and joins. This will require checking that the nested query access is also a subset of the original access. Although unless we also solve the problem ofget_state
being impossible to implement, transmuting to a query with nested queries won't work anyway.QueryIter
by offering afn fetch_next(&self) -> D::Item<'_>
method and relaxing theIterQueryData
bound onQuery::into_iter
andQuery::iter_mut
. This would work similar toiter_many_mut
anditer_many_inner
.IterQueryData
bound onQuery::single_inner
,Query::single_mut
, andSingle<D, F>
. This seems like it should be straightforward, because the method only returns a single item. But the way it checks that there is only one item is by fetching the second one!