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

Allow configuration of WorldQuery with runtime values #6390

Closed

Conversation

jakobhellermann
Copy link
Contributor

@jakobhellermann jakobhellermann commented Oct 27, 2022

Objective

Sometimes it is useful to query for types where the list of types is not known at runtime, e.g. for scripting. This can be done either by implementing all of the query machinery from scratch (see bevy_ecs_dynamic), or by extending bevy_ecs to handle these use-cases. This PR is an attempt to to the latter, my allowing arbitrary configuration of world query types.
This solution is an alternative to #6240, attempting to provide a safe to use and less error prone API.

Solution

  • introduce a Config associated type to WorldQuery
    • this is () for current query types &T, &mut T, Q::Config for Option<Q> and (A::Config, B::Config, ..) for (A, B, ..)
  • Make QueryState::new require Q::Config: Default and F::Config: Default. This propagates quite far, to world.query, Query: SystemParam and ExtractComponentPlugin
  • implement WorldQuery for Ptr and PtrMut with a Config = ComponentId, yielding Ptr and MutUntyped
  • implement WorldQuery for Vec<Q> with <Vec<Q> as WorldQuery>::Config = Vec<Q::Config>

This allows the following:

let component_id_1 = world.init_component::<TestComponent>();
let component_id_2 = world.init_component::<TestComponent2>();

world.spawn((TestComponent(num_1), TestComponent2(num_2)));

let mut query_state = QueryState::<(Entity, Vec<Ptr<'_>>), ()>::new_with_config(
    &mut world,
    ((), vec![component_id_1, component_id_2]), // entity configuration () and vec configuration
    (), // no filter config
);

let results = query_state.iter(&world); // returns one element of (entity, vec![ptr_1, ptr_2])

TODOS:

  • add untyped pendants to filter types
  • decide how to handle #[derive(WorldQuery)]
    • current impl: it generates Default::default so only works with "classical" world queries
    • solution proposal 1: use a config type (Config_Field1, Config_Field)
    • solution proposal 2: generate a mirror type with the same fields but of the respective config type
  • decide if the added : Default bounds are too annoying for general usage or if they regress compilation errors
    • if we were to use WorldQuery<Config = ()> instead things would be better, but this would not allow us to reuse (A, B, C) with configuration
    • maybe associated type bounds would help, but they are unstable (WorldQuery<Config: Default>)
  • implement WorldQuery for &dyn Reflect and &mut dyn Reflect with Config = TypeId yielding &dyn Reflect and Mut<dyn Reflect>
  • implement WorldQuery for smallvec and possibly other aggregation types

Changelog

To be written

@jakobhellermann jakobhellermann added C-Feature A new feature, making something new possible A-ECS Entities, components, systems, and events labels Oct 27, 2022
@alice-i-cecile alice-i-cecile added the C-Usability A targeted quality-of-life change that makes Bevy easier to use label Oct 27, 2022
@BoxyUwU BoxyUwU self-requested a review October 27, 2022 21:26
@james7132 james7132 self-requested a review October 28, 2022 08:03
Comment on lines +1694 to +1556
const IS_DENSE: bool = false;
const IS_ARCHETYPAL: bool = true;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know exactly how these work, but they are just hints to the query iteration code that all of the items in the query are dense, right? Is setting this to false correct here?

IS_ARCHETYPE is unconditionally true for &T and &mut T as well.

Copy link
Contributor

@Suficio Suficio Oct 28, 2022

Choose a reason for hiding this comment

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

I considered adding a specialized PtrDense type, didn't go very far with it though.

storage_type: StorageType,
component_size: usize,
// storage_type = TableStorage
table_components: Option<Ptr<'w>>,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For &[mut] T we store a Option<ThinSlicePtr<'w, UnsafeCell<()>>> here, gotten from column.get_data_slice.
Instead I call column.get_data_ptr and offset with .byte_add(fetch.component_size * index). I'm pretty sure this is okay, because the BlobVec data_ptr has provenance for the entire vec, and it is not like &vec[0] as *ptr.

@Suficio
Copy link
Contributor

Suficio commented Oct 28, 2022

One test case that I would like to see implemented here is QueryState<(Entity, Vec<Ptr>), _>.

I like this PR in that it allows each WorldQuery to rely on the output of init_state to be returned.

I do believe that it results in a situation where you don't know whether init_state config has correct input or whether it needs to be inferred otherwise, but this is certainly an added layer of safety.

The added type complexity and additional considerations that need to be made all over the codebase appear as large drawbacks of this approach since this is a rather niche feature.

@Suficio
Copy link
Contributor

Suficio commented Oct 28, 2022

Since this PR also implements dynamic world queries, I'll leave the alternative world query implementations for #6240 here: Suficio/bevy_js_prototype#20

@jakobhellermann
Copy link
Contributor Author

One test case that I would like to see implemented here is QueryState<(Entity, Vec), _>.

I did that in 6281526

The added type complexity and additional considerations that need to be made all over the codebase appear as large drawbacks of this approach since this is a rather niche feature.

Yeah, that's a bit unfortunate, but at least I think these bounds will mostly appear in engine code, not user code.

@jakobhellermann
Copy link
Contributor Author

jakobhellermann commented Oct 28, 2022

No idea why CI is read, it complains about things like method `archetype_fetch` is not a member of trait `WorldQuery` which seem to be obviously wrong.
EDIT: seems like this was changed in the latest commit #4800, but I didn't merge or rebase that yet.
EDIT 2: rebased

@alice-i-cecile alice-i-cecile added the D-Complex Quite challenging from either a design or technical perspective. Ask for help! label Nov 17, 2022
@nicopap nicopap self-requested a review March 14, 2023 19:59
@anlumo
Copy link

anlumo commented Mar 14, 2023

I want to implement scripting with hot reload support into my bevy application, so this PR would be great to have merged! What is missing to get this approved?

@nicopap nicopap mentioned this pull request Jun 11, 2023
github-merge-queue bot pushed a commit that referenced this pull request Jan 16, 2024
# 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>
@alice-i-cecile
Copy link
Member

Resolved by #9774.

@jakobhellermann jakobhellermann deleted the world-query-config branch January 18, 2024 12:59
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-Feature A new feature, making something new possible 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!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants