Store QueryState in Query as a Cow and remove distinct QueryLens type
#15848
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ItsDoot
added
A-ECS
ItsDoot
added
the
S-Waiting-on-Author
It's probably worth moving this change to a separate pr. Makes this pr a bit hard to review. |
|
Going to take a different approach to merging |
github-merge-queue bot
pushed a commit
that referenced
this pull request
Feb 5, 2025
# Objective
Simplify and expand the API for `QueryState`.
`QueryState` has a lot of methods that mirror those on `Query`. These
are then multiplied by variants that take `&World`, `&mut World`, and
`UnsafeWorldCell`. In addition, many of them have `_manual` variants
that take `&QueryState` and avoid calling `update_archetypes()`. Not all
of the combinations exist, however, so some operations are not possible.
## Solution
Introduce methods to get a `Query` from a `QueryState`. That will reduce
duplication between the types, and ensure that the full `Query` API is
always available for `QueryState`.
Introduce methods on `Query` that consume the query to return types with
the full `'w` lifetime. This avoids issues with borrowing where things
like `query_state.query(&world).get(entity)` don't work because they
borrow from the temporary `Query`.
Finally, implement `Copy` for read-only `Query`s. `get_inner` and
`iter_inner` currently take `&self`, so changing them to consume `self`
would be a breaking change. By making `Query: Copy`, they can consume a
copy of `self` and continue to work.
The consuming methods also let us simplify the implementation of methods
on `Query`, by doing `fn foo(&self) { self.as_readonly().foo_inner() }`
and `fn foo_mut(&mut self) { self.reborrow().foo_inner() }`. That
structure makes it more difficult to accidentally extend lifetimes,
since the safe `as_readonly()` and `reborrow()` methods shrink them
appropriately. The optimizer is able to see that they are both identity
functions and inline them, so there should be no performance cost.
Note that this change would conflict with #15848. If `QueryState` is
stored as a `Cow`, then the consuming methods cannot be implemented, and
`Copy` cannot be implemented.
## Future Work
The next step is to mark the methods on `QueryState` as `#[deprecated]`,
and move the implementations into `Query`.
## Migration Guide
`Query::to_readonly` has been renamed to `Query::as_readonly`.
mrchantey
pushed a commit
to mrchantey/bevy
that referenced
this pull request
Feb 17, 2025
# Objective
Simplify and expand the API for `QueryState`.
`QueryState` has a lot of methods that mirror those on `Query`. These
are then multiplied by variants that take `&World`, `&mut World`, and
`UnsafeWorldCell`. In addition, many of them have `_manual` variants
that take `&QueryState` and avoid calling `update_archetypes()`. Not all
of the combinations exist, however, so some operations are not possible.
## Solution
Introduce methods to get a `Query` from a `QueryState`. That will reduce
duplication between the types, and ensure that the full `Query` API is
always available for `QueryState`.
Introduce methods on `Query` that consume the query to return types with
the full `'w` lifetime. This avoids issues with borrowing where things
like `query_state.query(&world).get(entity)` don't work because they
borrow from the temporary `Query`.
Finally, implement `Copy` for read-only `Query`s. `get_inner` and
`iter_inner` currently take `&self`, so changing them to consume `self`
would be a breaking change. By making `Query: Copy`, they can consume a
copy of `self` and continue to work.
The consuming methods also let us simplify the implementation of methods
on `Query`, by doing `fn foo(&self) { self.as_readonly().foo_inner() }`
and `fn foo_mut(&mut self) { self.reborrow().foo_inner() }`. That
structure makes it more difficult to accidentally extend lifetimes,
since the safe `as_readonly()` and `reborrow()` methods shrink them
appropriately. The optimizer is able to see that they are both identity
functions and inline them, so there should be no performance cost.
Note that this change would conflict with bevyengine#15848. If `QueryState` is
stored as a `Cow`, then the consuming methods cannot be implemented, and
`Copy` cannot be implemented.
## Future Work
The next step is to mark the methods on `QueryState` as `#[deprecated]`,
and move the implementations into `Query`.
## Migration Guide
`Query::to_readonly` has been renamed to `Query::as_readonly`.
|
I made an attempt at doing this with type parameters: #18162 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Objective
The
QueryLenstype is basically aQuery, except it stores theQueryStateitself rather than a reference to it. We can unifyQueryandQueryLensby allowingQueryto hold either a reference or a value, whichCowprovides. As a byproduct, we can also now also returnQuerys fromWorld::query, rather thanQueryStates.Solution
The following changes have been made:
QueryStatecan now beCloned (required byCow)WorldQuery::Statetypes are now required to beClone.Queryreturning'shave been replaced with'_.World::query/World::query_filteredfunctions have been renamed toWorld::query_state/World::query_state_filtered.World::query/World::query_filteredfunctions have been added that returnQueryinstead ofQueryState.Query<D, F>::into_state(self) -> QueryState<D, F>(clones the state if it was borrowed)QueryState<D, F>::as_query(&mut self, &mut World) -> Query<D, F>QueryState<D, F>::as_readonly_query(&self, &World) -> Query<D::ReadOnly, F>QueryState<D, F>::into_query(self, &mut World) -> Query<D, F>QueryState<D, F>::into_readonly_query(self, &World) -> Query<D::ReadOnly, F>QueryState<D, F>::into_readonly(self) -> QueryState<D::ReadOnly, F>QueryState::as_readonly, but by value instead of by reference.QueryLenstype has been removed.Querycan function exactly like it.Query::transmute_lens_filtered(&mut self) -> QueryLenschanged toQuery::transmute_filtered(&mut self) -> QueryQuery::transmute_lens(&mut self) -> QueryLenschanged toQuery::transmute(&mut self) -> QueryQuery::join(&mut self, other: &mut Query) -> QueryLenschanged toQuery::join(&mut self, other: &mut Query) -> QueryQuery::join_filtered(&mut self, other: &mut Query) -> QueryLenschanged toQuery::join_filtered(&mut self, other: &mut Query) -> QueryQueryData/QueryFilterderives nowimpl Clone.Testing
Showcase
TODO?
Migration Guide
QueryLensusages should be replaced withQuery.Query::transmute_lens_filtered(&mut self) -> QueryLenschanged toQuery::transmute_filtered(&mut self) -> QueryQuery::transmute_lens(&mut self) -> QueryLenschanged toQuery::transmute(&mut self) -> QueryQuery::join(&mut self, other: &mut Query) -> QueryLenschanged toQuery::join(&mut self, other: &mut Query) -> QueryQuery::join_filtered(&mut self, other: &mut Query) -> QueryLenschanged toQuery::join_filtered(&mut self, other: &mut Query) -> QueryWorldQuery::Statetypes are now required to implementClone.World::queryandWorld::query_filteredfunctions were renamed toWorld::query_stateandWorld::query_state_filtered, respectively.Queryfunctions returning data with a'snow return'_, i.e. the query state lifetime is now bound by theQueryinstance rather than the containedQueryState.