From 0ca9d6968a646868e95f315e29177d860357109c Mon Sep 17 00:00:00 2001 From: Alice Cecile Date: Mon, 3 Feb 2025 17:13:42 -0500 Subject: [PATCH] Improve docs for `WorldQuery` (#17654) # 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 --- crates/bevy_ecs/src/query/fetch.rs | 12 ++++---- crates/bevy_ecs/src/query/filter.rs | 21 ++++++++------ crates/bevy_ecs/src/query/iter.rs | 2 +- crates/bevy_ecs/src/query/world_query.rs | 28 +++++++++++++------ crates/bevy_ecs/src/system/function_system.rs | 4 +-- 5 files changed, 41 insertions(+), 26 deletions(-) diff --git a/crates/bevy_ecs/src/query/fetch.rs b/crates/bevy_ecs/src/query/fetch.rs index afb56206454e4..31818bd44ae9a 100644 --- a/crates/bevy_ecs/src/query/fetch.rs +++ b/crates/bevy_ecs/src/query/fetch.rs @@ -1760,7 +1760,7 @@ unsafe impl WorldQuery for Option { 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, } @@ -1777,7 +1777,7 @@ unsafe impl WorldQuery for Option { ) { 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); } @@ -1788,7 +1788,7 @@ unsafe impl WorldQuery for Option { 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); } @@ -1803,7 +1803,7 @@ unsafe impl WorldQuery for Option { ) -> 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) }) } @@ -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),)*) } @@ -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); } } )* diff --git a/crates/bevy_ecs/src/query/filter.rs b/crates/bevy_ecs/src/query/filter.rs index 3a3d9a0162769..babc15c9d64dd 100644 --- a/crates/bevy_ecs/src/query/filter.rs +++ b/crates/bevy_ecs/src/query/filter.rs @@ -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. @@ -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 @@ -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, },)*) @@ -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); } } )* @@ -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); } } )* @@ -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) }))* } @@ -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) } } } @@ -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) })* } } @@ -805,7 +810,7 @@ unsafe impl QueryFilter for Added { 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) } } } @@ -1039,7 +1044,7 @@ unsafe impl QueryFilter for Changed { 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) } } } diff --git a/crates/bevy_ecs/src/query/iter.rs b/crates/bevy_ecs/src/query/iter.rs index 7e3fd4582bf21..39b30628912ab 100644 --- a/crates/bevy_ecs/src/query/iter.rs +++ b/crates/bevy_ecs/src/query/iter.rs @@ -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) }, } } diff --git a/crates/bevy_ecs/src/query/world_query.rs b/crates/bevy_ecs/src/query/world_query.rs index 88164e3398634..dbbe99b6c5e8a 100644 --- a/crates/bevy_ecs/src/query/world_query.rs +++ b/crates/bevy_ecs/src/query/world_query.rs @@ -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::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), @@ -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 @@ -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 @@ -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, @@ -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) },)*) } @@ -216,7 +226,7 @@ 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); })* } @@ -224,7 +234,7 @@ macro_rules! impl_tuple_world_query { 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); })* } @@ -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) },)*) } diff --git a/crates/bevy_ecs/src/system/function_system.rs b/crates/bevy_ecs/src/system/function_system.rs index 593d94e5a0917..0f3950d1d4ba6 100644 --- a/crates/bevy_ecs/src/system/function_system.rs +++ b/crates/bevy_ecs/src/system/function_system.rs @@ -630,7 +630,7 @@ impl SystemState { 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) } } @@ -644,7 +644,7 @@ impl SystemState { 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;