Convert queries to a more general form #10037

hymm · 2023-10-06T21:58:56Z

Objective

Allow transforming a query into a more generalized form, but matches the same data. i.e. turn a Query<(&A, &B)> into a Query<&A>. This allows passing the narrower query into a function that takes the more general query.

Solution

Create a transmute_fetch method that transforms an existing QueryState into a new one by creating new filter_state and fetch_state, but keeping the matched archetypes and other info.
Don't allow going from a fetch that gets optional data to one that is more narrow. i.e. Option<&T> or EntityMut into &T.
Add a transmute method on Query that returns QueryLens that stores the new fetch_state and filter_state.
Don't allow generalize to be run with non archetypal filters. This could maybe be relaxed in the future, but removing a Changed filter or switching from Changed to Added felt very footgunny. As you would naively expect this not to match new data, but it actually could. As a byproduct, this ends up disallowing Change and Added in the fetch.
Remove any filters from the type. As these are all archetypal, and archetypes have already been filtered. This allows the generalized query to be used in more places.

fn function_that_takes_a_query(query: &Query<&A>) {
    assert_eq!(query.single().0, 10);
}

fn system_1(query: Query<&A>) {
    function_that_takes_a_query(&query);
}

fn system_2(query: Query<(&A, &B)>) {
    let query_lens = query.transmute_fetch::<&A>();
    let q = query_lens.query();
    function_that_takes_a_query(&q);
}

Changelog

add a transmute_fetch method to transform queries to a similar types.

Future Work

add a transmute_fetch_and_filter that would allow Changed and Added filters
similar approach could be used to join to queries and take the intersection of their matched archetypes.

To Do

consider adding transmute_fetch_and_filter in this PR

JoJoJet

This would be useful for APIs like .iter_ancestors() or .iter_descendants(), which can only be called on Query<&Parent> and Query<&Children>.

crates/bevy_ecs/src/query/fetch.rs

crates/bevy_ecs/src/query/state.rs

crates/bevy_ecs/src/system/query.rs

Co-authored-by: Joseph <21144246+JoJoJet@users.noreply.github.com>

s-puig · 2023-10-07T22:37:58Z

Maybe projection would be a more fitting name than generalize?

- some other cleanup.

A-Walrus · 2023-10-08T14:23:02Z

Would it make sense to implement Query::get_component (and friends), through this? As in turn a Query<(&A,&B)> to a Query<&A> and then use Query::get on that?
Relevant discussions: #9910 #9904

hymm · 2023-10-08T15:24:01Z

Would it make sense to implement Query::get_component (and friends), through this? As in turn a Query<(&A,&B)> to a Query<&A> and then use Query::get on that?
Relevant discussions: #9910 #9904

I'm not sure I follow. Those discussions feel very orthogonal to what this is trying to accomplish.

hymm · 2023-10-08T15:27:30Z

I may rename this to transmute_query as I think it'll work to change a Option -> EntityRef. And also interact correctly with a bunch of the other WorldQuerys.

A-Walrus · 2023-10-08T15:48:16Z

Would it make sense to implement Query::get_component (and friends), through this? As in turn a Query<(&A,&B)> to a Query<&A> and then use Query::get on that?
Relevant discussions: #9910 #9904

I'm not sure I follow. Those discussions feel very orthogonal to what this is trying to accomplish.

Just an idea for some future work that this PR opens up.
As I understand from those discussions Query::get_component has had many implementation problems namely a lot of unsafe code with some soundness issues. I believe it could be implemented in a fairly straightforward using the query transformations that this PR adds. The purpose of get_component is to get some component through a query. This could be achieved by first transforming a query from one like Query<(&A, &B)> to something like Query<&A> and then using a regular Query::get on the new query. (I understand that this would only work if the query only has archetypal filters)

crates/bevy_ecs/src/query/fetch.rs

crates/bevy_ecs/src/query/state.rs

A-Walrus

This looks like a really useful PR! Couple of nits

hymm · 2023-10-08T16:02:47Z

This could be achieved by first transforming a query from one like Query<(&A, &B)> to something like Query<&A> and then using a regular Query::get on the new query. (I understand that this would only work if the query only has archetypal filters)

This wouldn't be a good method of creating convenience methods for that, as this is relatively expensive compared to a get or get_mut.

hymm · 2023-10-08T20:39:23Z

I think Option<&T> to &T is allowed right now and would lead to UB. Need to figure out how to disallow it. Might use another associated const.

edit:

let query_state = world.query::<Option<&A>>();
let mut new_query_state = query_state.restrict_fetch::<&A>(world.components());
let x = new_query_state.single(&world);

This panics on the fetch and hits what's supposed to be unreachable. So at least it's not UB. I'll try to find a way to make this panic earlier.

edit2: actually the panic is a runtime panic only, so this is probably UB.

alice-i-cecile · 2023-11-15T14:04:03Z

Superseded by #9774.

# Objective Expand the existing `Query` API to support more dynamic use cases i.e. scripting. ## Prior Art - #6390 - #8308 - #10037 ## Solution - Create a `QueryBuilder` with runtime methods to define the set of component accesses for a built query. - Create new `WorldQueryData` implementations `FilteredEntityMut` and `FilteredEntityRef` as variants of `EntityMut` and `EntityRef` that provide run time checked access to the components included in a given query. - Add new methods to `Query` to create "query lens" with a subset of the access of the initial query. ### Query Builder The `QueryBuilder` API allows you to define a query at runtime. At it's most basic use it will simply create a query with the corresponding type signature: ```rust let query = QueryBuilder::<Entity, With<A>>::new(&mut world).build(); // is equivalent to let query = QueryState::<Entity, With<A>>::new(&mut world); ``` Before calling `.build()` you also have the opportunity to add additional accesses and filters. Here is a simple example where we add additional filter terms: ```rust let entity_a = world.spawn((A(0), B(0))).id(); let entity_b = world.spawn((A(0), C(0))).id(); let mut query_a = QueryBuilder::<Entity>::new(&mut world) .with::<A>() .without::<C>() .build(); assert_eq!(entity_a, query_a.single(&world)); ``` This alone is useful in that allows you to decide which archetypes your query will match at runtime. However it is also very limited, consider a case like the following: ```rust let query_a = QueryBuilder::<&A>::new(&mut world) // Add an additional access .data::<&B>() .build(); ``` This will grant the query an additional read access to component B however we have no way of accessing the data while iterating as the type signature still only includes &A. For an even more concrete example of this consider dynamic components: ```rust let query_a = QueryBuilder::<Entity>::new(&mut world) // Adding a filter is easy since it doesn't need be read later .with_id(component_id_a) // How do I access the data of this component? .ref_id(component_id_b) .build(); ``` With this in mind the `QueryBuilder` API seems somewhat incomplete by itself, we need some way method of accessing the components dynamically. So here's one: ### Query Transmutation If the problem is not having the component in the type signature why not just add it? This PR also adds transmute methods to `QueryBuilder` and `QueryState`. Here's a simple example: ```rust world.spawn(A(0)); world.spawn((A(1), B(0))); let mut query = QueryBuilder::<()>::new(&mut world) .with::<B>() .transmute::<&A>() .build(); query.iter(&world).for_each(|a| assert_eq!(a.0, 1)); ``` The `QueryState` and `QueryBuilder` transmute methods look quite similar but are different in one respect. Transmuting a builder will always succeed as it will just add the additional accesses needed for the new terms if they weren't already included. Transmuting a `QueryState` will panic in the case that the new type signature would give it access it didn't already have, for example: ```rust let query = QueryState::<&A, Option<&B>>::new(&mut world); /// This is fine, the access for Option<&A> is less restrictive than &A query.transmute::<Option<&A>>(&world); /// Oh no, this would allow access to &B on entities that might not have it, so it panics query.transmute::<&B>(&world); /// This is right out query.transmute::<&C>(&world); ``` This is quite an appealing API to also have available on `Query` however it does pose one additional wrinkle: In order to to change the iterator we need to create a new `QueryState` to back it. `Query` doesn't own it's own state though, it just borrows it, so we need a place to borrow it from. This is why `QueryLens` exists, it is a place to store the new state so it can be borrowed when you call `.query()` leaving you with an API like this: ```rust fn function_that_takes_a_query(query: &Query<&A>) { // ... } fn system(query: Query<(&A, &B)>) { let lens = query.transmute_lens::<&A>(); let q = lens.query(); function_that_takes_a_query(&q); } ``` Now you may be thinking: Hey, wait a second, you introduced the problem with dynamic components and then described a solution that only works for static components! Ok, you got me, I guess we need a bit more: ### Filtered Entity References Currently the only way you can access dynamic components on entities through a query is with either `EntityMut` or `EntityRef`, however these can access all components and so conflict with all other accesses. This PR introduces `FilteredEntityMut` and `FilteredEntityRef` as alternatives that have additional runtime checking to prevent accessing components that you shouldn't. This way you can build a query with a `QueryBuilder` and actually access the components you asked for: ```rust let mut query = QueryBuilder::<FilteredEntityRef>::new(&mut world) .ref_id(component_id_a) .with(component_id_b) .build(); let entity_ref = query.single(&world); // Returns Some(Ptr) as we have that component and are allowed to read it let a = entity_ref.get_by_id(component_id_a); // Will return None even though the entity does have the component, as we are not allowed to read it let b = entity_ref.get_by_id(component_id_b); ``` For the most part these new structs have the exact same methods as their non-filtered equivalents. Putting all of this together we can do some truly dynamic ECS queries, check out the `dynamic` example to see it in action: ``` Commands: comp, c Create new components spawn, s Spawn entities query, q Query for entities Enter a command with no parameters for usage. > c A, B, C, Data 4 Component A created with id: 0 Component B created with id: 1 Component C created with id: 2 Component Data created with id: 3 > s A, B, Data 1 Entity spawned with id: 0v0 > s A, C, Data 0 Entity spawned with id: 1v0 > q &Data 0v0: Data: [1, 0, 0, 0] 1v0: Data: [0, 0, 0, 0] > q B, &mut Data 0v0: Data: [2, 1, 1, 1] > q B || C, &Data 0v0: Data: [2, 1, 1, 1] 1v0: Data: [0, 0, 0, 0] ``` ## Changelog - Add new `transmute_lens` methods to `Query`. - Add new types `QueryBuilder`, `FilteredEntityMut`, `FilteredEntityRef` and `QueryLens` - `update_archetype_component_access` has been removed, archetype component accesses are now determined by the accesses set in `update_component_access` - Added method `set_access` to `WorldQuery`, this is called before `update_component_access` for queries that have a restricted set of accesses, such as those built by `QueryBuilder` or `QueryLens`. This is primarily used by the `FilteredEntity*` variants and has an empty trait implementation. - Added method `get_state` to `WorldQuery` as a fallible version of `init_state` when you don't have `&mut World` access. ## Future Work Improve performance of `FilteredEntityMut` and `FilteredEntityRef`, currently they have to determine the accesses a query has in a given archetype during iteration which is far from ideal, especially since we already did the work when matching the archetype in the first place. To avoid making more internal API changes I have left it out of this PR. --------- Co-authored-by: Mike Hsu <mike.hsu@gmail.com>

hymm added 3 commits October 6, 2023 13:25

generalize query state

3445370

add generalize method to query

75be6b2

cargo fmt

6fcf755

hymm force-pushed the generalize-query branch from df48816 to 6fcf755 Compare October 6, 2023 22:00

ci fixes

7561765

alice-i-cecile added A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Complex Quite challenging from either a design or technical perspective. Ask for help! labels Oct 6, 2023

hymm marked this pull request as draft October 7, 2023 01:34

james7132 requested review from james7132 and JoJoJet October 7, 2023 06:57

JoJoJet reviewed Oct 7, 2023

View reviewed changes

crates/bevy_ecs/src/query/fetch.rs Outdated Show resolved Hide resolved

crates/bevy_ecs/src/query/state.rs Outdated Show resolved Hide resolved

crates/bevy_ecs/src/query/state.rs Outdated Show resolved Hide resolved

crates/bevy_ecs/src/system/query.rs Outdated Show resolved Hide resolved

Switch to safe methods.

851028d

Co-authored-by: Joseph <21144246+JoJoJet@users.noreply.github.com>

hymm force-pushed the generalize-query branch from b8ccfe4 to 851028d Compare October 7, 2023 17:30

hymm added 3 commits October 7, 2023 10:35

revert mistaken change

d50c654

rename some stuff and improve doc

cb74ccc

change new_state to return Option<State>

689772f

hymm added 3 commits October 7, 2023 15:45

more renaming

1d174db

change restrict_fetch to take &Components make it pub crate

e69dcec

- some other cleanup.

allowing getting 2 queries from a &QueryLens is unsafe

60751fb

hymm force-pushed the generalize-query branch from ad0dc60 to 60751fb Compare October 8, 2023 04:58

clippy

3a4184e

hymm force-pushed the generalize-query branch from ec08d5f to 3a4184e Compare October 8, 2023 05:35

hymm marked this pull request as ready for review October 8, 2023 05:52

A-Walrus reviewed Oct 8, 2023

View reviewed changes

crates/bevy_ecs/src/query/fetch.rs Outdated Show resolved Hide resolved

A-Walrus reviewed Oct 8, 2023

View reviewed changes

crates/bevy_ecs/src/query/state.rs Outdated Show resolved Hide resolved

A-Walrus reviewed Oct 8, 2023

View reviewed changes

crates/bevy_ecs/src/query/state.rs Outdated Show resolved Hide resolved

A-Walrus reviewed Oct 8, 2023

View reviewed changes

hymm added 7 commits October 8, 2023 22:40

don't allow Option<&T> transmutes

90d878d

switch to using a fixed bit set for optional checking

ec28e85

add some more tests

f4e455c

remove IS_EXACT

216d90e

add some tests and fix some bugs

6f2e9c0

document new access methods

227dc00

restrict_fetch -> transmute_fetch

6228491

hymm force-pushed the generalize-query branch from 4b0923c to 6228491 Compare October 10, 2023 02:54

hymm added 7 commits October 9, 2023 20:28

use existing method

b1b2f6c

add compile fail tests

e7271c8

clippy

e4b408b

add a failing option test

47ce677

get new test passing

9d3e526

add track caller

4814f4d

rename new_state -> get_state

8e0cf64

hymm force-pushed the generalize-query branch from 183a501 to 826b7c5 Compare October 24, 2023 04:33

clean up docs.

f90bc25

hymm force-pushed the generalize-query branch from 826b7c5 to f90bc25 Compare October 24, 2023 04:45

alice-i-cecile changed the title ~~Generalize query~~ Convert queries to a more general form Nov 4, 2023

james-j-obrien mentioned this pull request Nov 14, 2023

Dynamic queries and builder API #9774

Merged

alice-i-cecile closed this Nov 15, 2023

hymm deleted the generalize-query branch August 12, 2024 03:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert queries to a more general form #10037

Convert queries to a more general form #10037

hymm commented Oct 6, 2023 •

edited

Loading

JoJoJet left a comment

s-puig commented Oct 7, 2023

A-Walrus commented Oct 8, 2023 •

edited

Loading

hymm commented Oct 8, 2023

hymm commented Oct 8, 2023

A-Walrus commented Oct 8, 2023

A-Walrus left a comment

hymm commented Oct 8, 2023

hymm commented Oct 8, 2023 •

edited

Loading

alice-i-cecile commented Nov 15, 2023

Convert queries to a more general form #10037

Convert queries to a more general form #10037

Conversation

hymm commented Oct 6, 2023 • edited Loading

Objective

Solution

Changelog

Future Work

To Do

JoJoJet left a comment

Choose a reason for hiding this comment

s-puig commented Oct 7, 2023

A-Walrus commented Oct 8, 2023 • edited Loading

hymm commented Oct 8, 2023

hymm commented Oct 8, 2023

A-Walrus commented Oct 8, 2023

A-Walrus left a comment

Choose a reason for hiding this comment

hymm commented Oct 8, 2023

hymm commented Oct 8, 2023 • edited Loading

alice-i-cecile commented Nov 15, 2023

hymm commented Oct 6, 2023 •

edited

Loading

A-Walrus commented Oct 8, 2023 •

edited

Loading

hymm commented Oct 8, 2023 •

edited

Loading