Skip to content

Commit

Permalink
Improve docs for WorldQuery (#17654)
Browse files Browse the repository at this point in the history 
# Objective

While working on #17649, I found the docs for `WorldQuery` and the
related traits frustratingly vague.

## Solution

Clarify them and add some more tangible advice.

Also fix a copy-pasted typo in related comments.

---------

Co-authored-by: James O'Brien <[email protected]>
  • Loading branch information
@alice-i-cecile @james-j-obrien
alice-i-cecile and james-j-obrien authored Feb 3, 2025
1 parent 29d0ef6 commit 0ca9d69
Show file tree
Hide file tree
Showing 5 changed files with 41 additions and 26 deletions.
12 changes: 6 additions & 6 deletions crates/bevy_ecs/src/query/fetch.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1760,7 +1760,7 @@ unsafe impl<T: WorldQuery> WorldQuery for Option<T> {
this_run: Tick,
) -> OptionFetch<'w, T> {
OptionFetch {
// SAFETY: The invariants are uphold by the caller.
// SAFETY: The invariants are upheld by the caller.
fetch: unsafe { T::init_fetch(world, state, last_run, this_run) },
matches: false,
}
Expand All @@ -1777,7 +1777,7 @@ unsafe impl<T: WorldQuery> WorldQuery for Option<T> {
) {
fetch.matches = T::matches_component_set(state, &|id| archetype.contains(id));
if fetch.matches {
// SAFETY: The invariants are uphold by the caller.
// SAFETY: The invariants are upheld by the caller.
unsafe {
T::set_archetype(&mut fetch.fetch, state, archetype, table);
}
Expand All @@ -1788,7 +1788,7 @@ unsafe impl<T: WorldQuery> WorldQuery for Option<T> {
unsafe fn set_table<'w>(fetch: &mut OptionFetch<'w, T>, state: &T::State, table: &'w Table) {
fetch.matches = T::matches_component_set(state, &|id| table.has_column(id));
if fetch.matches {
// SAFETY: The invariants are uphold by the caller.
// SAFETY: The invariants are upheld by the caller.
unsafe {
T::set_table(&mut fetch.fetch, state, table);
}
Expand All @@ -1803,7 +1803,7 @@ unsafe impl<T: WorldQuery> WorldQuery for Option<T> {
) -> Self::Item<'w> {
fetch
.matches
// SAFETY: The invariants are uphold by the caller.
// SAFETY: The invariants are upheld by the caller.
.then(|| unsafe { T::fetch(&mut fetch.fetch, entity, table_row) })
}

Expand Down Expand Up @@ -2073,7 +2073,7 @@ macro_rules! impl_anytuple_fetch {
#[inline]
unsafe fn init_fetch<'w>(_world: UnsafeWorldCell<'w>, state: &Self::State, _last_run: Tick, _this_run: Tick) -> Self::Fetch<'w> {
let ($($name,)*) = state;
// SAFETY: The invariants are uphold by the caller.
// SAFETY: The invariants are upheld by the caller.
($(( unsafe { $name::init_fetch(_world, $name, _last_run, _this_run) }, false),)*)
}

Expand All @@ -2091,7 +2091,7 @@ macro_rules! impl_anytuple_fetch {
$(
$name.1 = $name::matches_component_set($state, &|id| _archetype.contains(id));
if $name.1 {
// SAFETY: The invariants are uphold by the caller.
// SAFETY: The invariants are upheld by the caller.
unsafe { $name::set_archetype(&mut $name.0, $state, _archetype, _table); }
}
)*
Expand Down
21 changes: 13 additions & 8 deletions crates/bevy_ecs/src/query/filter.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,8 @@ pub unsafe trait QueryFilter: WorldQuery {
///
/// This enables optimizations for [`crate::query::QueryIter`] that rely on knowing exactly how
/// many elements are being iterated (such as `Iterator::collect()`).
///
/// If this is `true`, then [`QueryFilter::filter_fetch`] must always return true.
const IS_ARCHETYPAL: bool;

/// Returns true if the provided [`Entity`] and [`TableRow`] should be included in the query results.
Expand All @@ -94,6 +96,9 @@ pub unsafe trait QueryFilter: WorldQuery {
/// Note that this is called after already restricting the matched [`Table`]s and [`Archetype`]s to the
/// ones that are compatible with the Filter's access.
///
/// Implementors of this method will generally either have a trivial `true` body (required for archetypal filters),
/// or call [`WorldQuery::fetch`] to access the raw data needed to make the final decision on filter inclusion.
///
/// # Safety
///
/// Must always be called _after_ [`WorldQuery::set_table`] or [`WorldQuery::set_archetype`]. `entity` and
Expand Down Expand Up @@ -426,7 +431,7 @@ macro_rules! impl_or_query_filter {
unsafe fn init_fetch<'w>(world: UnsafeWorldCell<'w>, state: &Self::State, last_run: Tick, this_run: Tick) -> Self::Fetch<'w> {
let ($($filter,)*) = state;
($(OrFetch {
// SAFETY: The invariants are uphold by the caller.
// SAFETY: The invariants are upheld by the caller.
fetch: unsafe { $filter::init_fetch(world, $filter, last_run, this_run) },
matches: false,
},)*)
Expand All @@ -439,7 +444,7 @@ macro_rules! impl_or_query_filter {
$(
$filter.matches = $filter::matches_component_set($state, &|id| table.has_column(id));
if $filter.matches {
// SAFETY: The invariants are uphold by the caller.
// SAFETY: The invariants are upheld by the caller.
unsafe { $filter::set_table(&mut $filter.fetch, $state, table); }
}
)*
Expand All @@ -457,7 +462,7 @@ macro_rules! impl_or_query_filter {
$(
$filter.matches = $filter::matches_component_set($state, &|id| archetype.contains(id));
if $filter.matches {
// SAFETY: The invariants are uphold by the caller.
// SAFETY: The invariants are upheld by the caller.
unsafe { $filter::set_archetype(&mut $filter.fetch, $state, archetype, table); }
}
)*
Expand All @@ -470,7 +475,7 @@ macro_rules! impl_or_query_filter {
table_row: TableRow
) -> Self::Item<'w> {
let ($($filter,)*) = fetch;
// SAFETY: The invariants are uphold by the caller.
// SAFETY: The invariants are upheld by the caller.
false $(|| ($filter.matches && unsafe { $filter::filter_fetch(&mut $filter.fetch, entity, table_row) }))*
}

Expand Down Expand Up @@ -521,7 +526,7 @@ macro_rules! impl_or_query_filter {
entity: Entity,
table_row: TableRow
) -> bool {
// SAFETY: The invariants are uphold by the caller.
// SAFETY: The invariants are upheld by the caller.
unsafe { Self::fetch(fetch, entity, table_row) }
}
}
Expand Down Expand Up @@ -554,7 +559,7 @@ macro_rules! impl_tuple_query_filter {
table_row: TableRow
) -> bool {
let ($($name,)*) = fetch;
// SAFETY: The invariants are uphold by the caller.
// SAFETY: The invariants are upheld by the caller.
true $(&& unsafe { $name::filter_fetch($name, entity, table_row) })*
}
}
Expand Down Expand Up @@ -805,7 +810,7 @@ unsafe impl<T: Component> QueryFilter for Added<T> {
entity: Entity,
table_row: TableRow,
) -> bool {
// SAFETY: The invariants are uphold by the caller.
// SAFETY: The invariants are upheld by the caller.
unsafe { Self::fetch(fetch, entity, table_row) }
}
}
Expand Down Expand Up @@ -1039,7 +1044,7 @@ unsafe impl<T: Component> QueryFilter for Changed<T> {
entity: Entity,
table_row: TableRow,
) -> bool {
// SAFETY: The invariants are uphold by the caller.
// SAFETY: The invariants are upheld by the caller.
unsafe { Self::fetch(fetch, entity, table_row) }
}
}
Expand Down
2 changes: 1 addition & 1 deletion crates/bevy_ecs/src/query/iter.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ impl<'w, 's, D: QueryData, F: QueryFilter> QueryIter<'w, 's, D, F> {
// SAFETY: We only access table data that has been registered in `query_state`.
tables: unsafe { &world.storages().tables },
archetypes: world.archetypes(),
// SAFETY: The invariants are uphold by the caller.
// SAFETY: The invariants are upheld by the caller.
cursor: unsafe { QueryIterationCursor::init(world, query_state, last_run, this_run) },
}
}
Expand Down
28 changes: 19 additions & 9 deletions crates/bevy_ecs/src/query/world_query.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,12 +40,18 @@ use variadics_please::all_tuples;
/// [`QueryFilter`]: crate::query::QueryFilter
pub unsafe trait WorldQuery {
/// The item returned by this [`WorldQuery`]
/// For `QueryData` this will be the item returned by the query.
/// For `QueryData` this will be the data retrieved by the query,
/// and is visible to the end user when calling e.g. `Query<Self>::get`.
///
/// For `QueryFilter` this will be either `()`, or a `bool` indicating whether the entity should be included
/// or a tuple of such things.
/// Archetypal query filters (like `With`) set this to `()`,
/// as the filtering is done by selecting the archetypes to iterate over via [`WorldQuery::matches_component_set`],
/// while non-archetypal query filters (like `Changed`) set this to a `bool` and evaluate the filter for each entity,
/// after the set of possible archetypes has been narrowed down.
type Item<'a>;

/// Per archetype/table state used by this [`WorldQuery`] to fetch [`Self::Item`](WorldQuery::Item)
/// Per archetype/table state retrieved by this [`WorldQuery`] to compute [`Self::Item`](WorldQuery::Item) for each entity.
type Fetch<'a>: Clone;

/// State used to construct a [`Self::Fetch`](WorldQuery::Fetch). This will be cached inside [`QueryState`](crate::query::QueryState),
Expand All @@ -59,7 +65,8 @@ pub unsafe trait WorldQuery {
/// This function manually implements subtyping for the query fetches.
fn shrink_fetch<'wlong: 'wshort, 'wshort>(fetch: Self::Fetch<'wlong>) -> Self::Fetch<'wshort>;

/// Creates a new instance of this fetch.
/// Creates a new instance of [`Self::Fetch`](WorldQuery::Fetch),
/// by combining data from the [`World`] with the cached [`Self::State`](WorldQuery::State).
/// Readonly accesses resources registered in [`WorldQuery::update_component_access`].
///
/// # Safety
Expand All @@ -76,7 +83,9 @@ pub unsafe trait WorldQuery {
) -> Self::Fetch<'w>;

/// Returns true if (and only if) every table of every archetype matched by this fetch contains
/// all of the matched components. This is used to select a more efficient "table iterator"
/// all of the matched components.
///
/// This is used to select a more efficient "table iterator"
/// for "dense" queries. If this returns true, [`WorldQuery::set_table`] must be used before
/// [`WorldQuery::fetch`] can be called for iterators. If this returns false,
/// [`WorldQuery::set_archetype`] must be used before [`WorldQuery::fetch`] can be called for
Expand Down Expand Up @@ -147,7 +156,8 @@ pub unsafe trait WorldQuery {
/// Returns `true` if this query matches a set of components. Otherwise, returns `false`.
///
/// Used to check which [`Archetype`]s can be skipped by the query
/// (if none of the [`Component`](crate::component::Component)s match)
/// (if none of the [`Component`](crate::component::Component)s match).
/// This is how archetypal query filters like `With` work.
fn matches_component_set(
state: &Self::State,
set_contains_id: &impl Fn(ComponentId) -> bool,
Expand Down Expand Up @@ -201,7 +211,7 @@ macro_rules! impl_tuple_world_query {
#[inline]
unsafe fn init_fetch<'w>(world: UnsafeWorldCell<'w>, state: &Self::State, last_run: Tick, this_run: Tick) -> Self::Fetch<'w> {
let ($($name,)*) = state;
// SAFETY: The invariants are uphold by the caller.
// SAFETY: The invariants are upheld by the caller.
($(unsafe { $name::init_fetch(world, $name, last_run, this_run) },)*)
}

Expand All @@ -216,15 +226,15 @@ macro_rules! impl_tuple_world_query {
) {
let ($($name,)*) = fetch;
let ($($state,)*) = state;
// SAFETY: The invariants are uphold by the caller.
// SAFETY: The invariants are upheld by the caller.
$(unsafe { $name::set_archetype($name, $state, archetype, table); })*
}

#[inline]
unsafe fn set_table<'w>(fetch: &mut Self::Fetch<'w>, state: &Self::State, table: &'w Table) {
let ($($name,)*) = fetch;
let ($($state,)*) = state;
// SAFETY: The invariants are uphold by the caller.
// SAFETY: The invariants are upheld by the caller.
$(unsafe { $name::set_table($name, $state, table); })*
}

Expand All @@ -235,7 +245,7 @@ macro_rules! impl_tuple_world_query {
table_row: TableRow
) -> Self::Item<'w> {
let ($($name,)*) = fetch;
// SAFETY: The invariants are uphold by the caller.
// SAFETY: The invariants are upheld by the caller.
($(unsafe { $name::fetch($name, entity, table_row) },)*)
}

Expand Down
4 changes: 2 additions & 2 deletions crates/bevy_ecs/src/system/function_system.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -630,7 +630,7 @@ impl<Param: SystemParam> SystemState<Param> {
world: UnsafeWorldCell<'w>,
) -> SystemParamItem<'w, 's, Param> {
let change_tick = world.increment_change_tick();
// SAFETY: The invariants are uphold by the caller.
// SAFETY: The invariants are upheld by the caller.
unsafe { self.fetch(world, change_tick) }
}

Expand All @@ -644,7 +644,7 @@ impl<Param: SystemParam> SystemState<Param> {
world: UnsafeWorldCell<'w>,
change_tick: Tick,
) -> SystemParamItem<'w, 's, Param> {
// SAFETY: The invariants are uphold by the caller.
// SAFETY: The invariants are upheld by the caller.
let param =
unsafe { Param::get_param(&mut self.param_state, &self.meta, world, change_tick) };
self.meta.last_run = change_tick;
Expand Down

0 comments on commit 0ca9d69

Please sign in to comment.