Skip to content

Support declaring resource access in Queries.#16843

Merged
alice-i-cecile merged 2 commits intobevyengine:mainfrom
chescock:resource-access-from-queries
Dec 17, 2024
Merged

Support declaring resource access in Queries.#16843
alice-i-cecile merged 2 commits intobevyengine:mainfrom
chescock:resource-access-from-queries

Conversation

@chescock
Copy link
Contributor

@chescock chescock commented Dec 16, 2024

Objective

Allow resources to be accessed soundly by QueryData and QueryFilter implementations.

This mostly works today, and is used in bevy-trait-query and will be used by #16810. The problem is that the access is not made visible to the executor, so it would be possible for a system with resource access in a query to run concurrently with a system that accesses the resource with ResMut, resulting in Undefined Behavior.

Solution

Define calling add_resource_read or add_resource_write in WorldQuery::update_component_access to be a supported way to declare resource access in a query.
Modify QueryState::new_with_access to check for resource access and report it in archetype_component_acccess.
Modify FilteredAccess::is_compatible to consider resource access conflicting even on queries with disjoint filters.

chescock
chescock commented Dec 16, 2024
View reviewed changes
crates/bevy_ecs/src/query/state.rs
}
}

if state.component_access.access().has_write_all_resources() {
Copy link
Contributor Author

@chescock chescock Dec 16, 2024

Choose a reason for hiding this comment

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

This will also support mutable resource access in queries, mainly because it was straightforward to do so, and because allowing access.add_resource_write to compile but do nothing seemed dangerous. But it's hard to do anything useful with mutable resource access. Multiple query items may be alive at once, so you can't use a mutable borrow in the fetch item.

Would it make more sense to simply prohibit mutable access to resources, and panic here if there is any?

Copy link
Member

@alice-i-cecile alice-i-cecile Dec 16, 2024

Choose a reason for hiding this comment

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

Let's leave it in for now: it may be useful when writing generic code.

@chescock chescock mentioned this pull request Dec 16, 2024
Merged
@mnmaita mnmaita added C-Feature A new feature, making something new possible A-ECS Entities, components, systems, and events S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Dec 16, 2024
@hymm hymm self-requested a review December 16, 2024 19:49
alice-i-cecile
alice-i-cecile approved these changes Dec 16, 2024
View reviewed changes
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.

Straightforward. I'm excited to use this for indexes.

@alice-i-cecile alice-i-cecile added D-Straightforward Simple bug fixes and API improvements, docs, test and examples D-Unsafe Touches with unsafe code in some way labels Dec 16, 2024
@alice-i-cecile alice-i-cecile added the P-Unsound A bug that results in undefined compiler behavior label Dec 16, 2024
bushrat011899
bushrat011899 approved these changes Dec 16, 2024
View reviewed changes
Copy link
Contributor

@bushrat011899 bushrat011899 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!

@alice-i-cecile alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Dec 16, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Dec 17, 2024
Merged via the queue into bevyengine:main with commit 5f4b5a3 Dec 17, 2024
@chescock chescock mentioned this pull request Dec 31, 2024
Open
@vil-mo vil-mo mentioned this pull request Jan 3, 2025
Merged
github-merge-queue bot pushed a commit that referenced this pull request Jan 6, 2025
@vil-mo
Disallow requesting write resource access in Queries (#17116)
b30ee2d 
Related to #16843

Since `WorldQuery::Fetch` is `Clone`, it can't store mutable references
to resources, so it doesn't make sense to mutably access resources. In
that sense, it is hard to find usecases of mutably accessing resources
and to clearly define, what mutably accessing resources would mean, so
it's been decided to disallow write resource access.
Also changed documentation of safety requirements of
`WorldQuery::init_fetch` and `WorldQuery::fetch` to clearly state to the
caller, what safety invariants they need to uphold.
ecoskey pushed a commit to ecoskey/bevy that referenced this pull request Jan 6, 2025
@chescock @ecoskey
Support declaring resource access in Queries. (bevyengine#16843)
7116be5 
# Objective

Allow resources to be accessed soundly by `QueryData` and `QueryFilter`
implementations.

This mostly works today, and is used in `bevy-trait-query` and will be
used by bevyengine#16810. The problem is that the access is not made visible to
the executor, so it would be possible for a system with resource access
in a query to run concurrently with a system that accesses the resource
with `ResMut`, resulting in Undefined Behavior.

## Solution

Define calling `add_resource_read` or `add_resource_write` in
`WorldQuery::update_component_access` to be a supported way to declare
resource access in a query.
Modify `QueryState::new_with_access` to check for resource access and
report it in `archetype_component_acccess`.
Modify `FilteredAccess::is_compatible` to consider resource access
conflicting even on queries with disjoint filters.
mrchantey pushed a commit to mrchantey/bevy that referenced this pull request Feb 4, 2025
@chescock @mrchantey
Support declaring resource access in Queries. (bevyengine#16843)
f84062d 
# Objective

Allow resources to be accessed soundly by `QueryData` and `QueryFilter`
implementations.

This mostly works today, and is used in `bevy-trait-query` and will be
used by bevyengine#16810. The problem is that the access is not made visible to
the executor, so it would be possible for a system with resource access
in a query to run concurrently with a system that accesses the resource
with `ResMut`, resulting in Undefined Behavior.

## Solution

Define calling `add_resource_read` or `add_resource_write` in
`WorldQuery::update_component_access` to be a supported way to declare
resource access in a query.
Modify `QueryState::new_with_access` to check for resource access and
report it in `archetype_component_acccess`.
Modify `FilteredAccess::is_compatible` to consider resource access
conflicting even on queries with disjoint filters.
mrchantey pushed a commit to mrchantey/bevy that referenced this pull request Feb 4, 2025
@vil-mo @mrchantey
Disallow requesting write resource access in Queries (bevyengine#17116)
a027b57 
Related to bevyengine#16843

Since `WorldQuery::Fetch` is `Clone`, it can't store mutable references
to resources, so it doesn't make sense to mutably access resources. In
that sense, it is hard to find usecases of mutably accessing resources
and to clearly define, what mutably accessing resources would mean, so
it's been decided to disallow write resource access.
Also changed documentation of safety requirements of
`WorldQuery::init_fetch` and `WorldQuery::fetch` to clearly state to the
caller, what safety invariants they need to uphold.
@Victoronz Victoronz mentioned this pull request Mar 20, 2025
Closed
@chescock chescock mentioned this pull request Jul 28, 2025
Closed
@chescock chescock mentioned this pull request Oct 15, 2025
Merged
@chescock chescock mentioned this pull request Dec 3, 2025
Merged
github-merge-queue bot pushed a commit that referenced this pull request Feb 23, 2026
@chescock
Nested Queries (#21557)
fc69a76 
# 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 a
`QueryState<D, F>` in its `WorldQuery::State`, so that it can create a
nested `Query<D, F>` during the outer `fetch`.

### `NestedQuery` type

Introduce a `NestedQuery` type that implements `QueryData` by yielding a
`Query`. It is intended to be used inside other implementations of
`QueryData`, either for manual implementations or
`#[derive(QueryData)]`. It is not normally useful to query directly,
since it's equivalent to adding another `Query` parameter to a system.

In theory, we could directly `impl QueryData for Query`, but this would
be too easy to do accidentally. Having to explicitly import and write
`NestedQuery` will make it clear that it's something unusual, and also
allows us to remove the need for passing `'static` for the `'w` and `'s`
lifetimes.

### New `WorldQuery` methods

For it to be sound to create the `Query` during `fetch`, we need to
register the `FilteredAccess` of the nested query and check for
conflicts with other parameters. Create a
`WorldQuery::update_external_component_access` method for that purpose.
For `Query as SystemParam`, call this during `init_access` so the access
can be combined with the rest of the system access. For loose
`QueryState`s, call it during `QueryState::new`.

In order to keep the query cache up-to-date, create a
`WorldQuery::update_archetypes` method where it can call
`QueryState::update_archetypes_unsafe_world_cell`, and call it *from*
there.

### New `QueryData` subtraits

Some 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 the `ChildOf` relation. But many entities may share a parent,
so it's not sound to iterate a `Query<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 a `Query<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
bound `iter_mut` and related methods.
* `SingleEntityQueryData` - For anything that only accesses data from
one entity. This is used to bound `EntityRef::get_components` (see
#20315). It's also used to bound `transmute` 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, and `ReadOnlyQueryData:
IterQueryData`, since it's always sound to alias read-only data.

Here is a summary of the traits implemented by some representative
`QueryData`:

| Data | Iter | ReadOnly | SingleEntity |
| -- | -- | -- | -- |
| `&T` | ✓ | ✓ | ✓ |
| `&mut T` | ✓ | x | ✓ |
| `Parent<&T>` | ✓ | ✓ | x |
| `Parent<&mut T>` | x | x | x |
| `(&mut T, Parent<&U>)` | ✓ | x | x |
| `Children<&mut T>` | ✓ | x | x |

## Alternatives

We could avoid the need for the `IterQueryData` trait by making it a
requirement for *all* `QueryData`. That would reduce the number of
traits required, at the cost of making it impossible to support
`Query<Parent<&mut C>>`.

## Showcase

Here is an implementation of a `Related<R, D, F>` query using this PR: 

<details>

```rust
pub struct Related<R: Relationship, D: QueryData + 'static, F: QueryFilter + 'static = ()>(
    RelatedInner<R, D, F>,
);

type RelatedInner<R, D, F> = (
    &'static R,
    NestedQuery<D, (F, With<<R as Relationship>::RelationshipTarget>)>,
);

unsafe impl<R: Relationship, D: QueryData + 'static, F: QueryFilter + 'static> WorldQuery
    for Related<R, D, F>
{
    type Fetch<'w> = <RelatedInner<R, D, F> as WorldQuery>::Fetch<'w>;
    type State = <RelatedInner<R, D, F> as WorldQuery>::State;

    fn shrink_fetch<'wlong: 'wshort, 'wshort>(fetch: Self::Fetch<'wlong>) -> Self::Fetch<'wshort> {
        <RelatedInner<R, D, F> as WorldQuery>::shrink_fetch(fetch)
    }

    unsafe fn init_fetch<'w, 's>(
        world: UnsafeWorldCell<'w>,
        state: &'s Self::State,
        last_run: Tick,
        this_run: Tick,
    ) -> Self::Fetch<'w> {
        unsafe {
            <RelatedInner<R, D, F> as WorldQuery>::init_fetch(world, state, last_run, this_run)
        }
    }

    const IS_DENSE: bool = <RelatedInner<R, D, F> as WorldQuery>::IS_DENSE;

    unsafe fn set_archetype<'w, 's>(
        fetch: &mut Self::Fetch<'w>,
        state: &'s Self::State,
        archetype: &'w Archetype,
        table: &'w Table,
    ) {
        unsafe {
            <RelatedInner<R, D, F> as WorldQuery>::set_archetype(fetch, state, archetype, table)
        };
    }

    unsafe fn set_table<'w, 's>(
        fetch: &mut Self::Fetch<'w>,
        state: &'s Self::State,
        table: &'w Table,
    ) {
        unsafe { <RelatedInner<R, D, F> as WorldQuery>::set_table(fetch, state, table) };
    }

    fn update_component_access(state: &Self::State, access: &mut FilteredAccess) {
        <RelatedInner<R, D, F> as WorldQuery>::update_component_access(state, access);
    }

    fn init_nested_access(
        state: &Self::State,
        system_name: Option<&str>,
        component_access_set: &mut FilteredAccessSet,
        world: UnsafeWorldCell,
    ) {
        <RelatedInner<R, D, F> as WorldQuery>::init_nested_access(state, system_name, component_access_set, world);
    }

    fn init_state(world: &mut World) -> Self::State {
        <RelatedInner<R, D, F> as WorldQuery>::init_state(world)
    }

    fn get_state(components: &Components) -> Option<Self::State> {
        <RelatedInner<R, D, F> as WorldQuery>::get_state(components)
    }

    fn matches_component_set(
        state: &Self::State,
        set_contains_id: &impl Fn(ComponentId) -> bool,
    ) -> bool {
        <RelatedInner<R, D, F> as WorldQuery>::matches_component_set(state, set_contains_id)
    }

    fn update_archetypes(state: &mut Self::State, world: UnsafeWorldCell) {
        <RelatedInner<R, D, F> as WorldQuery>::update_archetypes(state, world);
    }
}

unsafe impl<R: Relationship, D: QueryData + 'static, F: QueryFilter + 'static> QueryData
    for Related<R, D, F>
{
    const IS_READ_ONLY: bool = D::IS_READ_ONLY;
    type ReadOnly = Related<R, D::ReadOnly, F>;
    type Item<'w, 's> = Option<D::Item<'w, 's>>;

    fn shrink<'wlong: 'wshort, 'wshort, 's>(
        item: Self::Item<'wlong, 's>,
    ) -> Self::Item<'wshort, 's> {
        item.map(D::shrink)
    }

    unsafe fn fetch<'w, 's>(
        state: &'s Self::State,
        fetch: &mut Self::Fetch<'w>,
        entity: Entity,
        table_row: TableRow,
    ) -> Self::Item<'w, 's> {
        let (relationship, query) =
            unsafe { <RelatedInner<R, D, F> as QueryData>::fetch(state, fetch, entity, table_row) };
        query.get_inner(relationship.get()).ok()
    }
}

unsafe impl<R: Relationship, D: ReadOnlyQueryData + 'static, F: QueryFilter + 'static> ReadOnlyQueryData for Related<R, D, F> { }

// Note that we require `D: ReadOnlyQueryData` for `Related: IterQueryData`
unsafe impl<R: Relationship, D: ReadOnlyQueryData + 'static, F: QueryFilter + 'static> IterQueryData for Related<R, D, F> { }
```

</details>

I'd like to leave that to a follow-up PR to allow bikeshedding the API,
and to take advantage of #21581 to remove the `Option`, but I think it
works!

## Future Work

There is more to do here, but this PR is already pretty big. Future work
includes:

* #17647
* Following #21346, update `AssetChanged` to use nested queries for
resource access, and stop tracking resource access separately in
`Access`
* Implement `get_state` for `NestedQuery`. This is difficult because
constructing a `QueryState` requires reading the `DefaultQueryFilters`
resource, but `get_state` can be called from `transmute` with no access.
* Relax the `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 of
implementing `get_state`, transmuting to a query with nested queries
won't work anyway.
* Support streaming iteration for `QueryIter` by offering a `fn
fetch_next(&self) -> D::Item<'_>` method and relaxing the
`IterQueryData` bound on `Query::into_iter` and `Query::iter_mut`. This
would work similar to `iter_many_mut` and `iter_many_inner`.
* Relax the `IterQueryData` bound on `Query::single_inner`,
`Query::single_mut`, and `Single<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!
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alice-i-cecile alice-i-cecile alice-i-cecile approved these changes

@hymm hymm Awaiting requested review from hymm

@joseph-gio joseph-gio Awaiting requested review from joseph-gio

+1 more reviewer

@bushrat011899 bushrat011899 bushrat011899 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

A-ECS Entities, components, systems, and events C-Feature A new feature, making something new possible D-Straightforward Simple bug fixes and API improvements, docs, test and examples D-Unsafe Touches with unsafe code in some way P-Unsound A bug that results in undefined compiler behavior S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@chescock @bushrat011899 @alice-i-cecile @mnmaita