From 656fdd3e0a137801d9d2f0aa7decb8fd4f0b7b39 Mon Sep 17 00:00:00 2001 From: james7132 Date: Sat, 9 Mar 2024 19:52:59 -0800 Subject: [PATCH 1/6] Implement QueryData for &Archetype and EntityLocation --- crates/bevy_ecs/src/query/fetch.rs | 148 ++++++++++++++++++++++++++++- 1 file changed, 147 insertions(+), 1 deletion(-) diff --git a/crates/bevy_ecs/src/query/fetch.rs b/crates/bevy_ecs/src/query/fetch.rs index 7f249f4a36660..52505d58de91a 100644 --- a/crates/bevy_ecs/src/query/fetch.rs +++ b/crates/bevy_ecs/src/query/fetch.rs @@ -2,7 +2,7 @@ use crate::{ archetype::Archetype, change_detection::{Ticks, TicksMut}, component::{Component, ComponentId, StorageType, Tick}, - entity::Entity, + entity::{Entities, Entity, EntityLocation}, query::{Access, DebugCheckedUnwrap, FilteredAccess, WorldQuery}, storage::{ComponentSparseSet, Table, TableRow}, world::{ @@ -27,6 +27,12 @@ use std::{cell::UnsafeCell, marker::PhantomData}; /// but nesting of tuples allows infinite `WorldQuery`s. /// - **[`Entity`].** /// Gets the identifier of the queried entity. +/// - **[`EntityLocation`].** +/// Gets the location metadata of the queried entity. +/// - **[`EntityRef`].** +/// Read-only access to arbitrary components on the queried entity. +/// - **[`EntityMut`].** +/// Mutable access to arbitrary components on the queried entity. /// - **[`Option`].** /// By default, a world query only tests entities that have the matching component types. /// Wrapping it into an `Option` will increase the query search space, and it will return `None` if an entity doesn't satisfy the `WorldQuery`. @@ -345,6 +351,75 @@ unsafe impl QueryData for Entity { /// SAFETY: access is read only unsafe impl ReadOnlyQueryData for Entity {} +/// SAFETY: +/// `update_component_access` and `update_archetype_component_access` do nothing. +/// This is sound because `fetch` does not access components. +unsafe impl WorldQuery for EntityLocation { + type Item<'w> = EntityLocation; + type Fetch<'w> = &'w Entities; + type State = (); + + fn shrink<'wlong: 'wshort, 'wshort>(item: Self::Item<'wlong>) -> Self::Item<'wshort> { + item + } + + unsafe fn init_fetch<'w>( + world: UnsafeWorldCell<'w>, + _state: &Self::State, + _last_run: Tick, + _this_run: Tick, + ) -> Self::Fetch<'w> { + world.entities() + } + + const IS_DENSE: bool = true; + + #[inline] + unsafe fn set_archetype<'w>( + _fetch: &mut Self::Fetch<'w>, + _state: &Self::State, + _archetype: &'w Archetype, + _table: &Table, + ) { + } + + #[inline] + unsafe fn set_table<'w>(_fetch: &mut Self::Fetch<'w>, _state: &Self::State, _table: &'w Table) { + } + + #[inline(always)] + unsafe fn fetch<'w>( + fetch: &mut Self::Fetch<'w>, + entity: Entity, + _table_row: TableRow, + ) -> Self::Item<'w> { + fetch.get(entity).debug_checked_unwrap() + } + + fn update_component_access(_state: &Self::State, _access: &mut FilteredAccess) {} + + fn init_state(_world: &mut World) {} + + fn get_state(_world: &World) -> Option<()> { + Some(()) + } + + fn matches_component_set( + _state: &Self::State, + _set_contains_id: &impl Fn(ComponentId) -> bool, + ) -> bool { + true + } +} + +/// SAFETY: `Self` is the same as `Self::ReadOnly` +unsafe impl QueryData for EntityLocation { + type ReadOnly = Self; +} + +/// SAFETY: access is read only +unsafe impl ReadOnlyQueryData for EntityLocation {} + /// SAFETY: /// `fetch` accesses all components in a readonly way. /// This is sound because `update_component_access` and `update_archetype_component_access` set read access for all components and panic when appropriate. @@ -709,6 +784,77 @@ unsafe impl<'a> QueryData for FilteredEntityMut<'a> { type ReadOnly = FilteredEntityRef<'a>; } +/// SAFETY: +/// `update_component_access` and `update_archetype_component_access` do nothing. +/// This is sound because `fetch` does not access components. +unsafe impl WorldQuery for &Archetype { + type Item<'w> = &'w Archetype; + type Fetch<'w> = Option<&'w Archetype>; + type State = (); + + fn shrink<'wlong: 'wshort, 'wshort>(item: Self::Item<'wlong>) -> Self::Item<'wshort> { + item + } + + unsafe fn init_fetch<'w>( + _world: UnsafeWorldCell<'w>, + _state: &Self::State, + _last_run: Tick, + _this_run: Tick, + ) -> Self::Fetch<'w> { + None + } + + const IS_DENSE: bool = false; + + #[inline] + unsafe fn set_archetype<'w>( + fetch: &mut Self::Fetch<'w>, + _state: &Self::State, + archetype: &'w Archetype, + _table: &Table, + ) { + fetch.replace(archetype); + } + + #[inline] + unsafe fn set_table<'w>(_fetch: &mut Self::Fetch<'w>, _state: &Self::State, _table: &'w Table) { + unreachable!(); + } + + #[inline(always)] + unsafe fn fetch<'w>( + fetch: &mut Self::Fetch<'w>, + _entity: Entity, + _table_row: TableRow, + ) -> Self::Item<'w> { + fetch.debug_checked_unwrap() + } + + fn update_component_access(_state: &Self::State, _access: &mut FilteredAccess) {} + + fn init_state(_world: &mut World) {} + + fn get_state(_world: &World) -> Option<()> { + Some(()) + } + + fn matches_component_set( + _state: &Self::State, + _set_contains_id: &impl Fn(ComponentId) -> bool, + ) -> bool { + true + } +} + +/// SAFETY: `Self` is the same as `Self::ReadOnly` +unsafe impl QueryData for &Archetype { + type ReadOnly = Self; +} + +/// SAFETY: access is read only +unsafe impl ReadOnlyQueryData for &Archetype {} + #[doc(hidden)] pub struct ReadFetch<'w, T> { // T::STORAGE_TYPE = StorageType::Table From 4538f5035cb1dca102a9a710dd6ee4f478d0cf2d Mon Sep 17 00:00:00 2001 From: james7132 Date: Sun, 10 Mar 2024 13:44:19 -0700 Subject: [PATCH 2/6] Make &Archetype a dense WorldQuery --- crates/bevy_ecs/src/query/fetch.rs | 29 +++++++++++++++++------------ 1 file changed, 17 insertions(+), 12 deletions(-) diff --git a/crates/bevy_ecs/src/query/fetch.rs b/crates/bevy_ecs/src/query/fetch.rs index 52505d58de91a..c74dc1b78f36e 100644 --- a/crates/bevy_ecs/src/query/fetch.rs +++ b/crates/bevy_ecs/src/query/fetch.rs @@ -1,5 +1,5 @@ use crate::{ - archetype::Archetype, + archetype::{Archetype, Archetypes}, change_detection::{Ticks, TicksMut}, component::{Component, ComponentId, StorageType, Tick}, entity::{Entities, Entity, EntityLocation}, @@ -393,7 +393,8 @@ unsafe impl WorldQuery for EntityLocation { entity: Entity, _table_row: TableRow, ) -> Self::Item<'w> { - fetch.get(entity).debug_checked_unwrap() + // SAFETY: `fetch` must be called with an entity that exists in the world + unsafe { fetch.get(entity).debug_checked_unwrap() } } fn update_component_access(_state: &Self::State, _access: &mut FilteredAccess) {} @@ -789,7 +790,7 @@ unsafe impl<'a> QueryData for FilteredEntityMut<'a> { /// This is sound because `fetch` does not access components. unsafe impl WorldQuery for &Archetype { type Item<'w> = &'w Archetype; - type Fetch<'w> = Option<&'w Archetype>; + type Fetch<'w> = (&'w Entities, &'w Archetypes); type State = (); fn shrink<'wlong: 'wshort, 'wshort>(item: Self::Item<'wlong>) -> Self::Item<'wshort> { @@ -797,38 +798,42 @@ unsafe impl WorldQuery for &Archetype { } unsafe fn init_fetch<'w>( - _world: UnsafeWorldCell<'w>, + world: UnsafeWorldCell<'w>, _state: &Self::State, _last_run: Tick, _this_run: Tick, ) -> Self::Fetch<'w> { - None + (world.entities(), world.archetypes()) } - const IS_DENSE: bool = false; + // This could probably be a non-dense query and just get set a Option<&Archetype> fetch value in + // set_archetypes, but forcing archetypal iteration is likely to be slower in any compound query. + const IS_DENSE: bool = true; #[inline] unsafe fn set_archetype<'w>( - fetch: &mut Self::Fetch<'w>, + _fetch: &mut Self::Fetch<'w>, _state: &Self::State, - archetype: &'w Archetype, + _archetype: &'w Archetype, _table: &Table, ) { - fetch.replace(archetype); } #[inline] unsafe fn set_table<'w>(_fetch: &mut Self::Fetch<'w>, _state: &Self::State, _table: &'w Table) { - unreachable!(); } #[inline(always)] unsafe fn fetch<'w>( fetch: &mut Self::Fetch<'w>, - _entity: Entity, + entity: Entity, _table_row: TableRow, ) -> Self::Item<'w> { - fetch.debug_checked_unwrap() + let (entities, archetypes) = *fetch; + // SAFETY: `fetch` must be called with an entity that exists in the world + let location = unsafe { entities.get(entity).debug_checked_unwrap() }; + // SAFETY: The assigned archetype for a living entity must always be valid. + unsafe { archetypes.get(location.archetype_id).debug_checked_unwrap() } } fn update_component_access(_state: &Self::State, _access: &mut FilteredAccess) {} From 19bda6c21a82c06f99f35dfcc1074fd58cac7738 Mon Sep 17 00:00:00 2001 From: james7132 Date: Sun, 10 Mar 2024 13:51:41 -0700 Subject: [PATCH 3/6] Address documentation comments --- crates/bevy_ecs/src/query/fetch.rs | 4 +++- crates/bevy_ecs/src/system/query.rs | 8 ++++++-- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/crates/bevy_ecs/src/query/fetch.rs b/crates/bevy_ecs/src/query/fetch.rs index c74dc1b78f36e..072aa43ee6ef1 100644 --- a/crates/bevy_ecs/src/query/fetch.rs +++ b/crates/bevy_ecs/src/query/fetch.rs @@ -18,7 +18,7 @@ use std::{cell::UnsafeCell, marker::PhantomData}; /// /// There are many types that natively implement this trait: /// -/// - **Component references.** +/// - **Component references. (&T and &mut T)** /// Fetches a component by reference (immutably or mutably). /// - **`QueryData` tuples.** /// If every element of a tuple implements `QueryData`, then the tuple itself also implements the same trait. @@ -33,6 +33,8 @@ use std::{cell::UnsafeCell, marker::PhantomData}; /// Read-only access to arbitrary components on the queried entity. /// - **[`EntityMut`].** /// Mutable access to arbitrary components on the queried entity. +/// - **[`&Archetype`].** +/// Read-only access to the archetype-level metadata of the queried entity. /// - **[`Option`].** /// By default, a world query only tests entities that have the matching component types. /// Wrapping it into an `Option` will increase the query search space, and it will return `None` if an entity doesn't satisfy the `WorldQuery`. diff --git a/crates/bevy_ecs/src/system/query.rs b/crates/bevy_ecs/src/system/query.rs index 773a0c0e3aff4..cb6e7b134a0a9 100644 --- a/crates/bevy_ecs/src/system/query.rs +++ b/crates/bevy_ecs/src/system/query.rs @@ -1292,12 +1292,16 @@ impl<'w, 's, D: QueryData, F: QueryFilter> Query<'w, 's, D, F> { /// Besides removing parameters from the query, you can also /// make limited changes to the types of parameters. /// - /// * Can always add/remove `Entity` + /// * Can always add/remove [`Entity`] + /// * Can always add/remove [`EntityLocation`] + /// * Can always add/remove [`&Archetype`] /// * `Ref` <-> `&T` /// * `&mut T` -> `&T` /// * `&mut T` -> `Ref` /// * [`EntityMut`](crate::world::EntityMut) -> [`EntityRef`](crate::world::EntityRef) - /// + /// + /// [`EntityLocation`]: crate::entity::EntityLocation + /// [`&Archetype`]: crate::archetype::Archetype pub fn transmute_lens(&mut self) -> QueryLens<'_, NewD> { self.transmute_lens_filtered::() } From eec32fa0df7e1d7b18de879c2fa7f91b41baa361 Mon Sep 17 00:00:00 2001 From: james7132 Date: Mon, 11 Mar 2024 01:29:44 -0700 Subject: [PATCH 4/6] Fix comment --- crates/bevy_ecs/src/query/fetch.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/bevy_ecs/src/query/fetch.rs b/crates/bevy_ecs/src/query/fetch.rs index 072aa43ee6ef1..8b464cee8345c 100644 --- a/crates/bevy_ecs/src/query/fetch.rs +++ b/crates/bevy_ecs/src/query/fetch.rs @@ -808,7 +808,7 @@ unsafe impl WorldQuery for &Archetype { (world.entities(), world.archetypes()) } - // This could probably be a non-dense query and just get set a Option<&Archetype> fetch value in + // This could probably be a non-dense query and just set a Option<&Archetype> fetch value in // set_archetypes, but forcing archetypal iteration is likely to be slower in any compound query. const IS_DENSE: bool = true; From 08abd2accd2beb6b4deac4842e444f4b49d7f5e3 Mon Sep 17 00:00:00 2001 From: James Liu Date: Sun, 17 Mar 2024 12:23:51 -0700 Subject: [PATCH 5/6] Update crates/bevy_ecs/src/query/fetch.rs Co-authored-by: Alice Cecile --- crates/bevy_ecs/src/query/fetch.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/bevy_ecs/src/query/fetch.rs b/crates/bevy_ecs/src/query/fetch.rs index 8b464cee8345c..a87160809ee1d 100644 --- a/crates/bevy_ecs/src/query/fetch.rs +++ b/crates/bevy_ecs/src/query/fetch.rs @@ -33,7 +33,7 @@ use std::{cell::UnsafeCell, marker::PhantomData}; /// Read-only access to arbitrary components on the queried entity. /// - **[`EntityMut`].** /// Mutable access to arbitrary components on the queried entity. -/// - **[`&Archetype`].** +/// - **[`&Archetype`](Archetype).** /// Read-only access to the archetype-level metadata of the queried entity. /// - **[`Option`].** /// By default, a world query only tests entities that have the matching component types. From 3587cfb056bd0a10ee9fd68c1ac2a2aaff177620 Mon Sep 17 00:00:00 2001 From: james7132 Date: Fri, 22 Mar 2024 19:37:45 -0700 Subject: [PATCH 6/6] Add a comment about why EntityLocation is dense --- crates/bevy_ecs/src/query/fetch.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/crates/bevy_ecs/src/query/fetch.rs b/crates/bevy_ecs/src/query/fetch.rs index a87160809ee1d..682d789d7cc0e 100644 --- a/crates/bevy_ecs/src/query/fetch.rs +++ b/crates/bevy_ecs/src/query/fetch.rs @@ -374,6 +374,8 @@ unsafe impl WorldQuery for EntityLocation { world.entities() } + // This is set to true to avoid forcing archetypal iteration in compound queries, is likely to be slower + // in most practical use case. const IS_DENSE: bool = true; #[inline]