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

Dynamic queries and builder API #9774

Merged
merged 57 commits into from
Jan 16, 2024

Conversation

james-j-obrien
Copy link
Contributor

@james-j-obrien james-j-obrien commented Sep 12, 2023

Objective

Expand the existing Query API to support more dynamic use cases i.e. scripting.

Prior Art

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:

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:

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:

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:

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:

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:

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:

fn function_that_takes_a_query(query: &Query<&A>) {
    // ...
}

fn system(query: Query<(&A, &B)>) {
    let lens = query.as_query_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, and only those you asked for:

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);

You can still use EntityMut or EntityRef in scenarios where you don't need to respect the restricted component access, the query will still respect filter terms.
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.

@github-actions
Copy link
Contributor

You added a new example but didn't add metadata for it. Please update the root Cargo.toml file.

@github-actions
Copy link
Contributor

You added a new example but didn't add metadata for it. Please update the root Cargo.toml file.

@james-j-obrien james-j-obrien marked this pull request as draft September 12, 2023 02:40
@alice-i-cecile alice-i-cecile added C-Feature A new feature, making something new possible A-ECS Entities, components, systems, and events X-Controversial There is active debate or serious implications around merging this PR labels Sep 12, 2023
@alice-i-cecile
Copy link
Member

With #9686 merged too, I wonder if there's a way we could automatically check or enforce that all of the methods and docs are kept in sync 🤔 A trait won't work, both because of ergonomics / discoverability, and because QueryState needs a World. We could use some horrible macro, but that hurts code readability. Maybe a CI check? I guess that's a question for another PR though.

Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

Very cool stuff! Some thoughts:

  1. The machinery is going to be a lot easier to review with even basic doc comments on each of the new types and traits :) I'm happy to help refine things when you're ready.
  2. We should try to merge a simple benchmark for these (that directly mirrors a static one) with this PR or shortly after.
  3. Adapting an existing example is not very informative for reviewers, since it doesn't showcase the power at all. Good to verify that things work as expected for now, but we'll need something better. Maybe two: one that demonstrates this feature in isolation, and another working with ComponentId?
  4. There's a lot of code, but it seems quite high quality: it's all about as direct as I can think to solve this particular problem. Good tests too!

@james-j-obrien
Copy link
Contributor Author

Thanks! Agree on all accounts. Going to take a pass at documentation tomorrow, I have mirrored the existing query benchmarks in my perf branch but I need to clean that up to integrate into the PR.

The example definitely isn't very illustrative, it was mostly a sanity check for me and I hadn't removed it yet. It doesn't prove much beyond what's shown in the tests which are more thorough. I want to create one just showing the query used statically and another showing some kind of scripting/dynamic use case.

@github-actions
Copy link
Contributor

You added a new example but didn't add metadata for it. Please update the root Cargo.toml file.

Copy link
Contributor

@hymm hymm left a comment

Choose a reason for hiding this comment

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

Made some suggested edits to some of the safety comments on FilteredEntityRef and FilteredEntityMut. The original comments felt a bit too vague. I'm not 100% on them yet though. Need to look more over the chain from UnsafeWorldCell -> UnsafeEntityCell -> FilteredEntityMut.

I think everything besides that can be safely punted to follow up prs.

crates/bevy_ecs/src/query/builder.rs Outdated Show resolved Hide resolved
/// Equivalent to [`Self::transmute_lens`] but also includes a [`WorldQueryFilter`] type.
///
/// Note that the lens will iterate the same tables and archetypes as the original query. This means that
/// additional archetypal query terms like [`With`](crate::query::With) and [`Without`](crate::query::Without)
Copy link
Contributor

Choose a reason for hiding this comment

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

feels weird that you could add With<A>, but some of the archetypes don't have an A or Without<A> and the archetype might have an A. Feels like we should be panicking if the user tries to do that.

Not really a safety issue, so I'd be fine with punting this into an issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I'm not a fan but as you say it's not a safety issue. As I'm sure you're aware the primary motivating use case is Changed and Added which aren't archetypal so won't continue to apply if not included, we could definitely panic on terms who's conditions no longer apply in a follow up.

Copy link
Member

Choose a reason for hiding this comment

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

As a follow-up, we should see if it would be worth adding a marker trait to forbid archetypal queries here.

crates/bevy_ecs/src/world/entity_ref.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/world/entity_ref.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/world/entity_ref.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/world/entity_ref.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/world/entity_ref.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@hymm hymm left a comment

Choose a reason for hiding this comment

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

Looks good to me.

Should open a couple issues once this is merged for followups for these comments.

  1. https://github.com/bevyengine/bevy/pull/9774/files#r1404760890
  2. https://github.com/bevyengine/bevy/pull/9774/files#r1424893221

Copy link
Member

@JoJoJet JoJoJet left a comment

Choose a reason for hiding this comment

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

Really happy with the state of this PR, and what it opens up 🌈. I'm hoping to approve this soon, though I wanna review it deeply a couple more times.

I'm not thrilled with the fact that we have yet another set of EnityRef and EntityMut types, but it's worth it to get dynamic queries. Hopefully we can improve this in the future. Maybe whatever solution we land on to improve performance of FilteredEntityRef will allow us to merge it back with EntityRef.

crates/bevy_ecs/src/query/access.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/world/entity_ref.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
crates/bevy_ecs/src/world/entity_ref.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/world/entity_ref.rs Show resolved Hide resolved
Copy link
Member

@JoJoJet JoJoJet left a comment

Choose a reason for hiding this comment

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

Thanks for all your work. I think this will be ready once the existing conversations are resolved.

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jan 16, 2024
Merged via the queue into bevyengine:main with commit ea42d14 Jan 16, 2024
26 checks passed
@james-j-obrien james-j-obrien deleted the dynamic-term-builder branch January 17, 2024 04:34
@Suficio
Copy link
Contributor

Suficio commented Feb 17, 2024

Missed the review process but just wanted to thank @james-j-obrien for the great work and getting this feature over the line!

Looks like it's time to revisit scripting for Bevy :)

kristoff3r added a commit to kristoff3r/bevy that referenced this pull request Feb 18, 2024
`update_archetype_component_access` was removed from queries in bevyengine#9774,
but some documentation still refered to it.
github-merge-queue bot pushed a commit that referenced this pull request Feb 18, 2024
# Objective

`update_archetype_component_access` was removed from queries in #9774,
but some documentation still refers to it.

## Solution

Update the documentation. Since a bunch of these were in SAFETY comments
it would be nice if someone who knows the details better could check
that the rest of those comments are still valid.
msvbg pushed a commit to msvbg/bevy that referenced this pull request Feb 26, 2024
# Objective

`update_archetype_component_access` was removed from queries in bevyengine#9774,
but some documentation still refers to it.

## Solution

Update the documentation. Since a bunch of these were in SAFETY comments
it would be nice if someone who knows the details better could check
that the rest of those comments are still valid.
msvbg pushed a commit to msvbg/bevy that referenced this pull request Feb 26, 2024
# Objective

`update_archetype_component_access` was removed from queries in bevyengine#9774,
but some documentation still refers to it.

## Solution

Update the documentation. Since a bunch of these were in SAFETY comments
it would be nice if someone who knows the details better could check
that the rest of those comments are still valid.
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 S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it X-Controversial There is active debate or serious implications around merging this PR
Projects
Status: Merged PR
Status: Done
Development

Successfully merging this pull request may close these issues.

7 participants