From 46ff91e8c0459f799693cdbfd1a9465b3f384c05 Mon Sep 17 00:00:00 2001 From: notmd Date: Wed, 16 Apr 2025 17:32:24 +0700 Subject: [PATCH 01/16] wip --- crates/bevy_ecs/macros/src/lib.rs | 4 +- crates/bevy_ecs/src/archetype.rs | 51 +++++- crates/bevy_ecs/src/bundle.rs | 46 +++-- crates/bevy_ecs/src/query/state.rs | 54 +++++- crates/bevy_ecs/src/system/builder.rs | 14 +- crates/bevy_ecs/src/system/commands/mod.rs | 6 +- crates/bevy_ecs/src/system/function_system.rs | 13 +- crates/bevy_ecs/src/system/system_param.rs | 169 ++++++++++++------ crates/bevy_ecs/src/world/entity_ref.rs | 12 +- 9 files changed, 272 insertions(+), 97 deletions(-) diff --git a/crates/bevy_ecs/macros/src/lib.rs b/crates/bevy_ecs/macros/src/lib.rs index a657765ac23f9..b6339e94c2a91 100644 --- a/crates/bevy_ecs/macros/src/lib.rs +++ b/crates/bevy_ecs/macros/src/lib.rs @@ -431,9 +431,9 @@ pub fn derive_system_param(input: TokenStream) -> TokenStream { } } - unsafe fn new_archetype(state: &mut Self::State, archetype: &#path::archetype::Archetype, system_meta: &mut #path::system::SystemMeta) { + unsafe fn new_archetype(state: &Self::State, archetype: &#path::archetype::Archetype, system_meta: &mut #path::system::SystemMeta, world: &#path::world::World) { // SAFETY: The caller ensures that `archetype` is from the World the state was initialized from in `init_state`. - unsafe { <#fields_alias::<'_, '_, #punctuated_generic_idents> as #path::system::SystemParam>::new_archetype(&mut state.state, archetype, system_meta) } + unsafe { <#fields_alias::<'_, '_, #punctuated_generic_idents> as #path::system::SystemParam>::new_archetype(&state.state, archetype, system_meta, world) } } fn apply(state: &mut Self::State, system_meta: &#path::system::SystemMeta, world: &mut #path::world::World) { diff --git a/crates/bevy_ecs/src/archetype.rs b/crates/bevy_ecs/src/archetype.rs index f55acf6d35feb..46b449d62bdd3 100644 --- a/crates/bevy_ecs/src/archetype.rs +++ b/crates/bevy_ecs/src/archetype.rs @@ -25,8 +25,10 @@ use crate::{ entity::{Entity, EntityLocation}, observer::Observers, storage::{ImmutableSparseSet, SparseArray, SparseSet, SparseSetIndex, TableId, TableRow}, + world::{unsafe_world_cell::UnsafeWorldCell, DeferredWorld, World}, }; use alloc::{boxed::Box, vec::Vec}; +use bevy_ecs_macros::Event; use bevy_platform_support::collections::HashMap; use core::{ hash::Hash, @@ -110,6 +112,9 @@ impl ArchetypeId { } } +#[derive(Event)] +pub(crate) struct ArchetypeCreated(pub ArchetypeId); + /// Used in [`ArchetypeAfterBundleInsert`] to track whether components in the bundle are newly /// added or already existed in the entity's archetype. #[derive(Copy, Clone, Eq, PartialEq)] @@ -812,6 +817,36 @@ pub struct ArchetypeRecord { pub(crate) column: Option, } +/// Metadata about the [`ArchetypeId`] and whether it is newly create or not. +#[derive(Clone, Copy)] +pub(crate) struct ArchetypeIdState { + id: ArchetypeId, + is_new: bool, +} + +impl ArchetypeIdState { + /// Creates a new [`ArchetypeIdState`] with the given ID and marks it as old. + #[inline] + pub(crate) fn old(id: ArchetypeId) -> Self { + Self { id, is_new: false } + } + + /// Get the [`ArchetypeId`]. + #[inline] + pub(crate) fn id(&self) -> ArchetypeId { + self.id + } + + #[inline] + pub(crate) fn trigger_if_new(&self, world: &mut DeferredWorld) { + std::dbg!(self.is_new); + if self.is_new { + std::println!("triggering archetype created {:?}", self.id); + world.trigger(ArchetypeCreated(self.id)); + } + } +} + impl Archetypes { pub(crate) fn new() -> Self { let mut archetypes = Archetypes { @@ -933,7 +968,7 @@ impl Archetypes { table_id: TableId, table_components: Vec, sparse_set_components: Vec, - ) -> ArchetypeId { + ) -> ArchetypeIdState { let archetype_identity = ArchetypeComponents { sparse_set_components: sparse_set_components.into_boxed_slice(), table_components: table_components.into_boxed_slice(), @@ -942,10 +977,14 @@ impl Archetypes { let archetypes = &mut self.archetypes; let archetype_component_count = &mut self.archetype_component_count; let component_index = &mut self.by_component; - *self + let mut is_new = false; + let is_new_ref = &mut is_new; + let archetype_id = *self .by_components .entry(archetype_identity) .or_insert_with_key(move |identity| { + // std::dbg!("creating new archetype"); + *is_new_ref = true; let ArchetypeComponents { table_components, sparse_set_components, @@ -975,7 +1014,13 @@ impl Archetypes { .zip(sparse_set_archetype_components), )); id - }) + }); + + // std::println!("After get_id_or_insert archetype {is_new}"); + ArchetypeIdState { + id: archetype_id, + is_new, + } } /// Returns the number of components that are stored in archetypes. diff --git a/crates/bevy_ecs/src/bundle.rs b/crates/bevy_ecs/src/bundle.rs index d87ef517f15d2..c0e92f7827e27 100644 --- a/crates/bevy_ecs/src/bundle.rs +++ b/crates/bevy_ecs/src/bundle.rs @@ -6,8 +6,8 @@ pub use bevy_ecs_macros::Bundle; use crate::{ archetype::{ - Archetype, ArchetypeAfterBundleInsert, ArchetypeId, Archetypes, BundleComponentStatus, - ComponentStatus, SpawnBundleStatus, + Archetype, ArchetypeAfterBundleInsert, ArchetypeId, ArchetypeIdState, Archetypes, + BundleComponentStatus, ComponentStatus, SpawnBundleStatus, }, change_detection::MaybeLocation, component::{ @@ -734,12 +734,12 @@ impl BundleInfo { components: &Components, observers: &Observers, archetype_id: ArchetypeId, - ) -> ArchetypeId { + ) -> ArchetypeIdState { if let Some(archetype_after_insert_id) = archetypes[archetype_id] .edges() .get_archetype_after_bundle_insert(self.id) { - return archetype_after_insert_id; + return ArchetypeIdState::old(archetype_after_insert_id); } let mut new_table_components = Vec::new(); let mut new_sparse_set_components = Vec::new(); @@ -793,7 +793,7 @@ impl BundleInfo { added, existing, ); - archetype_id + ArchetypeIdState::old(archetype_id) } else { let table_id; let table_components; @@ -841,7 +841,7 @@ impl BundleInfo { .edges_mut() .cache_archetype_after_bundle_insert( self.id, - new_archetype_id, + new_archetype_id.id(), bundle_status, added_required_components, added, @@ -874,7 +874,7 @@ impl BundleInfo { observers: &Observers, archetype_id: ArchetypeId, intersection: bool, - ) -> Option { + ) -> Option { // Check the archetype graph to see if the bundle has been // removed from this archetype in the past. let archetype_after_remove_result = { @@ -887,7 +887,7 @@ impl BundleInfo { }; let result = if let Some(result) = archetype_after_remove_result { // This bundle removal result is cached. Just return that! - result + result.map(|id| ArchetypeIdState::old(id)) } else { let mut next_table_components; let mut next_sparse_set_components; @@ -954,11 +954,11 @@ impl BundleInfo { if intersection { current_archetype .edges_mut() - .cache_archetype_after_bundle_remove(self.id(), result); + .cache_archetype_after_bundle_remove(self.id(), result.map(|id| id.id())); } else { current_archetype .edges_mut() - .cache_archetype_after_bundle_take(self.id(), result); + .cache_archetype_after_bundle_take(self.id(), result.map(|id| id.id())); } result } @@ -1023,14 +1023,15 @@ impl<'w> BundleInserter<'w> { // SAFETY: We will not make any accesses to the command queue, component or resource data of this world let bundle_info = world.bundles.get_unchecked(bundle_id); let bundle_id = bundle_info.id(); - let new_archetype_id = bundle_info.insert_bundle_into_archetype( + let archetype_id_state = bundle_info.insert_bundle_into_archetype( &mut world.archetypes, &mut world.storages, &world.components, &world.observers, archetype_id, ); - if new_archetype_id == archetype_id { + + let inserter = if archetype_id_state.id() == archetype_id { let archetype = &mut world.archetypes[archetype_id]; // SAFETY: The edge is assured to be initialized when we called insert_bundle_into_archetype let archetype_after_insert = unsafe { @@ -1051,8 +1052,9 @@ impl<'w> BundleInserter<'w> { world: world.as_unsafe_world_cell(), } } else { - let (archetype, new_archetype) = - world.archetypes.get_2_mut(archetype_id, new_archetype_id); + let (archetype, new_archetype) = world + .archetypes + .get_2_mut(archetype_id, archetype_id_state.id()); // SAFETY: The edge is assured to be initialized when we called insert_bundle_into_archetype let archetype_after_insert = unsafe { archetype @@ -1090,7 +1092,11 @@ impl<'w> BundleInserter<'w> { world: world.as_unsafe_world_cell(), } } - } + }; + + archetype_id_state.trigger_if_new(&mut inserter.world.into_deferred()); + + inserter } /// # Safety @@ -1394,22 +1400,24 @@ impl<'w> BundleSpawner<'w> { change_tick: Tick, ) -> Self { let bundle_info = world.bundles.get_unchecked(bundle_id); - let new_archetype_id = bundle_info.insert_bundle_into_archetype( + let archetype_id_state = bundle_info.insert_bundle_into_archetype( &mut world.archetypes, &mut world.storages, &world.components, &world.observers, ArchetypeId::EMPTY, ); - let archetype = &mut world.archetypes[new_archetype_id]; + let archetype = &mut world.archetypes[archetype_id_state.id()]; let table = &mut world.storages.tables[archetype.table_id()]; - Self { + let spawner = Self { bundle_info: bundle_info.into(), table: table.into(), archetype: archetype.into(), change_tick, world: world.as_unsafe_world_cell(), - } + }; + archetype_id_state.trigger_if_new(&mut spawner.world.into_deferred()); + spawner } #[inline] diff --git a/crates/bevy_ecs/src/query/state.rs b/crates/bevy_ecs/src/query/state.rs index e9a00f4646e1f..7e529e144187b 100644 --- a/crates/bevy_ecs/src/query/state.rs +++ b/crates/bevy_ecs/src/query/state.rs @@ -1,13 +1,16 @@ use crate::{ - archetype::{Archetype, ArchetypeComponentId, ArchetypeGeneration, ArchetypeId}, - component::{ComponentId, Tick}, + archetype::{ + Archetype, ArchetypeComponentId, ArchetypeCreated, ArchetypeGeneration, ArchetypeId, + }, + component::{ComponentHooks, ComponentId, Mutable, StorageType, Tick}, entity::{Entity, EntityEquivalent, EntitySet, UniqueEntityArray}, entity_disabling::DefaultQueryFilters, - prelude::FromWorld, + error::Result, + prelude::{Component, FromWorld, Observer, Trigger}, query::{Access, FilteredAccess, QueryCombinationIter, QueryIter, QueryParIter, WorldQuery}, storage::{SparseSetIndex, TableId}, system::Query, - world::{unsafe_world_cell::UnsafeWorldCell, World, WorldId}, + world::{unsafe_world_cell::UnsafeWorldCell, DeferredWorld, World, WorldId}, }; #[cfg(all(not(target_arch = "wasm32"), feature = "multi_threaded"))] @@ -17,6 +20,7 @@ use alloc::vec::Vec; use core::{fmt, ptr}; use fixedbitset::FixedBitSet; use log::warn; +use std::boxed::Box; #[cfg(feature = "trace")] use tracing::Span; @@ -87,6 +91,42 @@ pub struct QueryState { par_iter_span: Span, } +impl Component for QueryState { + const STORAGE_TYPE: StorageType = StorageType::SparseSet; + type Mutability = Mutable; + + fn register_component_hooks(hooks: &mut ComponentHooks) { + hooks.on_add(|mut world, ctx| { + let entity = ctx.entity; + std::dbg!("QueryState::on_add"); + let observer = Observer::new( + move |trigger: Trigger, mut world: DeferredWorld| -> Result { + std::dbg!("QueryState::on_add::observer"); + let archetype_id: ArchetypeId = trigger.event().0; + let archetype_ptr: *const Archetype = world + .archetypes() + .get(archetype_id) + .expect("Invalid ArchetypeId"); + let Ok(mut entity) = world.get_entity_mut(entity) else { + // TODO query is not exists anymore, despawn this observer + return Ok(()); + }; + let mut state = entity + .get_mut::>() + .expect("QueryState not found"); + unsafe { + state.new_archetype_internal(&*archetype_ptr); + } + + Ok(()) + }, + ); + // observer.watch_entity(ctx.entity); + world.commands().spawn(observer); + }); + } +} + impl fmt::Debug for QueryState { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { f.debug_struct("QueryState") @@ -533,7 +573,6 @@ impl QueryState { let archetypes = world.archetypes(); let old_generation = core::mem::replace(&mut self.archetype_generation, archetypes.generation()); - for archetype in &archetypes[old_generation..] { // SAFETY: The validate_world call ensures that the world is the same the QueryState // was initialized from. @@ -679,10 +718,13 @@ impl QueryState { /// # Safety /// `archetype` must be from the `World` this state was initialized from. pub unsafe fn update_archetype_component_access( - &mut self, + &self, archetype: &Archetype, access: &mut Access, ) { + if !self.matched_archetypes.contains(archetype.id().index()) { + return; + } // As a fast path, we can iterate directly over the components involved // if the `access` is finite. if let Ok(iter) = self.component_access.access.try_iter_component_access() { diff --git a/crates/bevy_ecs/src/system/builder.rs b/crates/bevy_ecs/src/system/builder.rs index 6261b9e35587e..60c451f78a6c0 100644 --- a/crates/bevy_ecs/src/system/builder.rs +++ b/crates/bevy_ecs/src/system/builder.rs @@ -3,6 +3,7 @@ use bevy_utils::synccell::SyncCell; use variadics_please::all_tuples; use crate::{ + entity::Entity, prelude::QueryBuilder, query::{QueryData, QueryFilter, QueryState}, resource::Resource, @@ -211,10 +212,9 @@ impl ParamBuilder { unsafe impl<'w, 's, D: QueryData + 'static, F: QueryFilter + 'static> SystemParamBuilder> for QueryState { - fn build(self, world: &mut World, system_meta: &mut SystemMeta) -> QueryState { + fn build(self, world: &mut World, system_meta: &mut SystemMeta) -> Entity { self.validate_world(world.id()); - init_query_param(world, system_meta, &self); - self + init_query_param(world, system_meta, Some(self)) } } @@ -290,12 +290,12 @@ unsafe impl< T: FnOnce(&mut QueryBuilder), > SystemParamBuilder> for QueryParamBuilder { - fn build(self, world: &mut World, system_meta: &mut SystemMeta) -> QueryState { + fn build(self, world: &mut World, system_meta: &mut SystemMeta) -> Entity { let mut builder = QueryBuilder::new(world); (self.0)(&mut builder); let state = builder.build(); - init_query_param(world, system_meta, &state); - state + + init_query_param(world, system_meta, Some(state)) } } @@ -943,7 +943,7 @@ mod tests { #[derive(SystemParam)] #[system_param(builder)] struct CustomParam<'w, 's> { - query: Query<'w, 's, ()>, + query: Query<'w, 'w, ()>, local: Local<'s, usize>, } diff --git a/crates/bevy_ecs/src/system/commands/mod.rs b/crates/bevy_ecs/src/system/commands/mod.rs index 4cb6d61bc0e9a..0a1b3ef981c82 100644 --- a/crates/bevy_ecs/src/system/commands/mod.rs +++ b/crates/bevy_ecs/src/system/commands/mod.rs @@ -134,16 +134,18 @@ const _: () = { } unsafe fn new_archetype( - state: &mut Self::State, + state: &Self::State, archetype: &bevy_ecs::archetype::Archetype, system_meta: &mut bevy_ecs::system::SystemMeta, + world: &World, ) { // SAFETY: Caller guarantees the archetype is from the world used in `init_state` unsafe { <__StructFieldsAlias<'_, '_> as bevy_ecs::system::SystemParam>::new_archetype( - &mut state.state, + &state.state, archetype, system_meta, + world, ); }; } diff --git a/crates/bevy_ecs/src/system/function_system.rs b/crates/bevy_ecs/src/system/function_system.rs index 4e05ef4dae360..fe0d3d92b8a56 100644 --- a/crates/bevy_ecs/src/system/function_system.rs +++ b/crates/bevy_ecs/src/system/function_system.rs @@ -479,7 +479,9 @@ impl SystemState { for archetype in &archetypes[old_generation..] { // SAFETY: The assertion above ensures that the param_state was initialized from `world`. - unsafe { Param::new_archetype(&mut self.param_state, archetype, &mut self.meta) }; + unsafe { + Param::new_archetype(&self.param_state, archetype, &mut self.meta, world.world()) + }; } } @@ -790,7 +792,14 @@ where for archetype in &archetypes[old_generation..] { // SAFETY: The assertion above ensures that the param_state was initialized from `world`. - unsafe { F::Param::new_archetype(&mut state.param, archetype, &mut self.system_meta) }; + unsafe { + F::Param::new_archetype( + &state.param, + archetype, + &mut self.system_meta, + world.world(), + ) + }; } } diff --git a/crates/bevy_ecs/src/system/system_param.rs b/crates/bevy_ecs/src/system/system_param.rs index 99d4c72df66e2..e17eb95f4b108 100644 --- a/crates/bevy_ecs/src/system/system_param.rs +++ b/crates/bevy_ecs/src/system/system_param.rs @@ -4,7 +4,7 @@ use crate::{ bundle::Bundles, change_detection::{MaybeLocation, Ticks, TicksMut}, component::{ComponentId, ComponentTicks, Components, Tick}, - entity::Entities, + entity::{Entities, Entity}, query::{ Access, FilteredAccess, FilteredAccessSet, QueryData, QueryFilter, QuerySingleError, QueryState, ReadOnlyQueryData, @@ -234,9 +234,10 @@ pub unsafe trait SystemParam: Sized { reason = "The parameters here are intentionally unused by the default implementation; however, putting underscores here will result in the underscores being copied by rust-analyzer's tab completion." )] unsafe fn new_archetype( - state: &mut Self::State, + state: &Self::State, archetype: &Archetype, system_meta: &mut SystemMeta, + world: &World, ) { } @@ -339,21 +340,26 @@ unsafe impl<'w, 's, D: ReadOnlyQueryData + 'static, F: QueryFilter + 'static> Re // SAFETY: Relevant query ComponentId and ArchetypeComponentId access is applied to SystemMeta. If // this Query conflicts with any prior access, a panic will occur. unsafe impl SystemParam for Query<'_, '_, D, F> { - type State = QueryState; - type Item<'w, 's> = Query<'w, 's, D, F>; + type State = Entity; + type Item<'w, 's> = Query<'w, 'w, D, F>; fn init_state(world: &mut World, system_meta: &mut SystemMeta) -> Self::State { - let state = QueryState::new_with_access(world, &mut system_meta.archetype_component_access); - init_query_param(world, system_meta, &state); - state + init_query_param::(world, system_meta, None) } unsafe fn new_archetype( - state: &mut Self::State, + state: &Self::State, archetype: &Archetype, system_meta: &mut SystemMeta, + world: &World, ) { - state.new_archetype(archetype, &mut system_meta.archetype_component_access); + let state = world.entity(*state).get::>().unwrap(); + state.update_archetype_component_access( + archetype, + &mut system_meta.archetype_component_access, + ); + // std::dbg!(archetype.id()); + // state.new_archetype(archetype, &mut system_meta.archetype_component_access); } #[inline] @@ -363,6 +369,7 @@ unsafe impl SystemParam for Qu world: UnsafeWorldCell<'w>, change_tick: Tick, ) -> Self::Item<'w, 's> { + let state = get_query_state(*state, world).unwrap(); // SAFETY: We have registered all of the query's world accesses, // so the caller ensures that `world` has permission to access any // world data that the query needs. @@ -374,19 +381,42 @@ unsafe impl SystemParam for Qu pub(crate) fn init_query_param( world: &mut World, system_meta: &mut SystemMeta, - state: &QueryState, -) { - assert_component_access_compatibility( - &system_meta.name, - core::any::type_name::(), - core::any::type_name::(), - &system_meta.component_access_set, - &state.component_access, - world, - ); - system_meta - .component_access_set - .add(state.component_access.clone()); + state: Option>, +) -> Entity { + let mut query_state = world.query::<(Entity, &QueryState)>(); + if let Some((entity, query_state)) = query_state.iter(world).next() { + assert_component_access_compatibility( + &system_meta.name, + core::any::type_name::(), + core::any::type_name::(), + &system_meta.component_access_set, + &query_state.component_access, + world, + ); + + system_meta + .component_access_set + .add(query_state.component_access.clone()); + entity + } else { + let state: QueryState = state.unwrap_or_else(|| { + QueryState::new_with_access(world, &mut system_meta.archetype_component_access) + }); + + assert_component_access_compatibility( + &system_meta.name, + core::any::type_name::(), + core::any::type_name::(), + &system_meta.component_access_set, + &state.component_access, + world, + ); + + system_meta + .component_access_set + .add(state.component_access.clone()); + world.spawn(state).id() + } } fn assert_component_access_compatibility( @@ -409,23 +439,38 @@ fn assert_component_access_compatibility( panic!("error[B0001]: Query<{}, {}> in system {system_name} accesses component(s) {accesses}in a way that conflicts with a previous system parameter. Consider using `Without` to create disjoint Queries or merging conflicting Queries into a `ParamSet`. See: https://bevyengine.org/learn/errors/b0001", ShortName(query_type), ShortName(filter_type)); } +/// SAFETY: Caller must ensure no other mutable access to QueryState +unsafe fn get_query_state<'w, D: QueryData + 'static, F: QueryFilter + 'static>( + entity: Entity, + world: UnsafeWorldCell<'w>, +) -> Option<&'w QueryState> { + let state = world + .get_entity(entity) + .unwrap() + .get::>() + .unwrap(); + + Some(state) +} + // SAFETY: Relevant query ComponentId and ArchetypeComponentId access is applied to SystemMeta. If // this Query conflicts with any prior access, a panic will occur. unsafe impl<'a, D: QueryData + 'static, F: QueryFilter + 'static> SystemParam for Single<'a, D, F> { - type State = QueryState; + type State = Entity; type Item<'w, 's> = Single<'w, D, F>; fn init_state(world: &mut World, system_meta: &mut SystemMeta) -> Self::State { - Query::init_state(world, system_meta) + Query::::init_state(world, system_meta) } unsafe fn new_archetype( - state: &mut Self::State, + state: &Self::State, archetype: &Archetype, system_meta: &mut SystemMeta, + world: &World, ) { // SAFETY: Delegate to existing `SystemParam` implementations. - unsafe { Query::new_archetype(state, archetype, system_meta) }; + unsafe { Query::::new_archetype(state, archetype, system_meta, world) }; } #[inline] @@ -435,6 +480,7 @@ unsafe impl<'a, D: QueryData + 'static, F: QueryFilter + 'static> SystemParam fo world: UnsafeWorldCell<'w>, change_tick: Tick, ) -> Self::Item<'w, 's> { + let state = get_query_state::(*state, world).unwrap(); // SAFETY: State ensures that the components it accesses are not accessible somewhere elsewhere. // The caller ensures the world matches the one used in init_state. let query = unsafe { @@ -455,6 +501,7 @@ unsafe impl<'a, D: QueryData + 'static, F: QueryFilter + 'static> SystemParam fo system_meta: &SystemMeta, world: UnsafeWorldCell, ) -> Result<(), SystemParamValidationError> { + let state = get_query_state::(*state, world).unwrap(); // SAFETY: State ensures that the components it accesses are not mutably accessible elsewhere // and the query is read only. // The caller ensures the world matches the one used in init_state. @@ -482,20 +529,21 @@ unsafe impl<'a, D: QueryData + 'static, F: QueryFilter + 'static> SystemParam fo unsafe impl<'a, D: QueryData + 'static, F: QueryFilter + 'static> SystemParam for Option> { - type State = QueryState; + type State = Entity; type Item<'w, 's> = Option>; fn init_state(world: &mut World, system_meta: &mut SystemMeta) -> Self::State { - Single::init_state(world, system_meta) + Single::::init_state(world, system_meta) } unsafe fn new_archetype( - state: &mut Self::State, + state: &Self::State, archetype: &Archetype, system_meta: &mut SystemMeta, + world: &World, ) { // SAFETY: Delegate to existing `SystemParam` implementations. - unsafe { Single::new_archetype(state, archetype, system_meta) }; + unsafe { Single::::new_archetype(state, archetype, system_meta, &world) }; } #[inline] @@ -505,6 +553,7 @@ unsafe impl<'a, D: QueryData + 'static, F: QueryFilter + 'static> SystemParam world: UnsafeWorldCell<'w>, change_tick: Tick, ) -> Self::Item<'w, 's> { + let state = get_query_state::(*state, world).unwrap(); state.validate_world(world.id()); // SAFETY: State ensures that the components it accesses are not accessible elsewhere. // The caller ensures the world matches the one used in init_state. @@ -527,6 +576,7 @@ unsafe impl<'a, D: QueryData + 'static, F: QueryFilter + 'static> SystemParam system_meta: &SystemMeta, world: UnsafeWorldCell, ) -> Result<(), SystemParamValidationError> { + let state = get_query_state::(*state, world).unwrap(); // SAFETY: State ensures that the components it accesses are not mutably accessible elsewhere // and the query is read only. // The caller ensures the world matches the one used in init_state. @@ -563,20 +613,21 @@ unsafe impl<'a, D: ReadOnlyQueryData + 'static, F: QueryFilter + 'static> ReadOn unsafe impl SystemParam for Populated<'_, '_, D, F> { - type State = QueryState; - type Item<'w, 's> = Populated<'w, 's, D, F>; + type State = Entity; + type Item<'w, 's> = Populated<'w, 'w, D, F>; fn init_state(world: &mut World, system_meta: &mut SystemMeta) -> Self::State { - Query::init_state(world, system_meta) + Query::::init_state(world, system_meta) } unsafe fn new_archetype( - state: &mut Self::State, + state: &Self::State, archetype: &Archetype, system_meta: &mut SystemMeta, + world: &World, ) { // SAFETY: Delegate to existing `SystemParam` implementations. - unsafe { Query::new_archetype(state, archetype, system_meta) }; + unsafe { Query::::new_archetype(state, archetype, system_meta, world) }; } #[inline] @@ -597,6 +648,7 @@ unsafe impl SystemParam system_meta: &SystemMeta, world: UnsafeWorldCell, ) -> Result<(), SystemParamValidationError> { + let state = get_query_state::(*state, world).unwrap(); // SAFETY: // - We have read-only access to the components accessed by query. // - The caller ensures the world matches the one used in init_state. @@ -790,9 +842,9 @@ macro_rules! impl_param_set { ($($param,)*) } - unsafe fn new_archetype(state: &mut Self::State, archetype: &Archetype, system_meta: &mut SystemMeta) { + unsafe fn new_archetype(state: &Self::State, archetype: &Archetype, system_meta: &mut SystemMeta, world: &World) { // SAFETY: The caller ensures that `archetype` is from the World the state was initialized from in `init_state`. - unsafe { <($($param,)*) as SystemParam>::new_archetype(state, archetype, system_meta); } + unsafe { <($($param,)*) as SystemParam>::new_archetype(state, archetype, system_meta, world); } } fn apply(state: &mut Self::State, system_meta: &SystemMeta, world: &mut World) { @@ -1957,13 +2009,14 @@ unsafe impl SystemParam for Vec { } unsafe fn new_archetype( - state: &mut Self::State, + state: &Self::State, archetype: &Archetype, system_meta: &mut SystemMeta, + world: &World, ) { for state in state { // SAFETY: The caller ensures that `archetype` is from the World the state was initialized from in `init_state`. - unsafe { T::new_archetype(state, archetype, system_meta) }; + unsafe { T::new_archetype(state, archetype, system_meta, &world) }; } } @@ -2008,13 +2061,14 @@ unsafe impl SystemParam for ParamSet<'_, '_, Vec> { } unsafe fn new_archetype( - state: &mut Self::State, + state: &Self::State, archetype: &Archetype, system_meta: &mut SystemMeta, + world: &World, ) { for state in state { // SAFETY: The caller ensures that `archetype` is from the World the state was initialized from in `init_state`. - unsafe { T::new_archetype(state, archetype, system_meta) } + unsafe { T::new_archetype(state, archetype, system_meta, world) } } } @@ -2093,13 +2147,13 @@ macro_rules! impl_system_param_tuple { } #[inline] - unsafe fn new_archetype(($($param,)*): &mut Self::State, archetype: &Archetype, system_meta: &mut SystemMeta) { + unsafe fn new_archetype(($($param,)*): &Self::State, archetype: &Archetype, system_meta: &mut SystemMeta, world: &World) { #[allow( unused_unsafe, reason = "Zero-length tuples will not run anything in the unsafe block." )] // SAFETY: The caller ensures that `archetype` is from the World the state was initialized from in `init_state`. - unsafe { $($param::new_archetype($param, archetype, system_meta);)* } + unsafe { $($param::new_archetype($param, archetype, system_meta, world);)* } } #[inline] @@ -2272,12 +2326,13 @@ unsafe impl SystemParam for StaticSystemParam<'_, '_, } unsafe fn new_archetype( - state: &mut Self::State, + state: &Self::State, archetype: &Archetype, system_meta: &mut SystemMeta, + world: &World, ) { // SAFETY: The caller guarantees that the provided `archetype` matches the World used to initialize `state`. - unsafe { P::new_archetype(state, archetype, system_meta) }; + unsafe { P::new_archetype(state, archetype, system_meta, world) }; } fn apply(state: &mut Self::State, system_meta: &SystemMeta, world: &mut World) { @@ -2524,7 +2579,12 @@ trait DynParamState: Sync + Send { /// /// # Safety /// `archetype` must be from the [`World`] used to initialize `state` in [`SystemParam::init_state`]. - unsafe fn new_archetype(&mut self, archetype: &Archetype, system_meta: &mut SystemMeta); + unsafe fn new_archetype( + &self, + archetype: &Archetype, + system_meta: &mut SystemMeta, + world: &World, + ); /// Applies any deferred mutations stored in this [`SystemParam`]'s state. /// This is used to apply [`Commands`] during [`ApplyDeferred`](crate::prelude::ApplyDeferred). @@ -2554,9 +2614,14 @@ impl DynParamState for ParamState { self } - unsafe fn new_archetype(&mut self, archetype: &Archetype, system_meta: &mut SystemMeta) { + unsafe fn new_archetype( + &self, + archetype: &Archetype, + system_meta: &mut SystemMeta, + world: &World, + ) { // SAFETY: The caller ensures that `archetype` is from the World the state was initialized from in `init_state`. - unsafe { T::new_archetype(&mut self.0, archetype, system_meta) }; + unsafe { T::new_archetype(&self.0, archetype, system_meta, world) }; } fn apply(&mut self, system_meta: &SystemMeta, world: &mut World) { @@ -2618,12 +2683,13 @@ unsafe impl SystemParam for DynSystemParam<'_, '_> { } unsafe fn new_archetype( - state: &mut Self::State, + state: &Self::State, archetype: &Archetype, system_meta: &mut SystemMeta, + world: &World, ) { // SAFETY: The caller ensures that `archetype` is from the World the state was initialized from in `init_state`. - unsafe { state.0.new_archetype(archetype, system_meta) }; + unsafe { state.0.new_archetype(archetype, system_meta, world) }; } fn apply(state: &mut Self::State, system_meta: &SystemMeta, world: &mut World) { @@ -2773,11 +2839,10 @@ mod tests { #[derive(SystemParam)] pub struct SpecialQuery< 'w, - 's, D: QueryData + Send + Sync + 'static, F: QueryFilter + Send + Sync + 'static = (), > { - _query: Query<'w, 's, D, F>, + _query: Query<'w, 'w, D, F>, } fn my_system(_: SpecialQuery<(), ()>) {} diff --git a/crates/bevy_ecs/src/world/entity_ref.rs b/crates/bevy_ecs/src/world/entity_ref.rs index 22383e86b36e9..8a7966cf00362 100644 --- a/crates/bevy_ecs/src/world/entity_ref.rs +++ b/crates/bevy_ecs/src/world/entity_ref.rs @@ -2005,7 +2005,7 @@ impl<'w> EntityWorldMut<'w> { )? }; - if new_archetype_id == old_location.archetype_id { + if new_archetype_id.id() == old_location.archetype_id { return None; } @@ -2021,6 +2021,8 @@ impl<'w> EntityWorldMut<'w> { ) }; + new_archetype_id.trigger_if_new(&mut deferred_world); + // SAFETY: all bundle components exist in World unsafe { trigger_on_replace_and_on_remove_hooks_and_observers( @@ -2074,7 +2076,7 @@ impl<'w> EntityWorldMut<'w> { entities, archetypes, storages, - new_archetype_id, + new_archetype_id.id(), ); } self.world.flush(); @@ -2191,7 +2193,7 @@ impl<'w> EntityWorldMut<'w> { ) .expect("intersections should always return a result"); - if new_archetype_id == location.archetype_id { + if new_archetype_id.id() == location.archetype_id { return location; } @@ -2206,6 +2208,8 @@ impl<'w> EntityWorldMut<'w> { ) }; + new_archetype_id.trigger_if_new(&mut deferred_world); + // SAFETY: all bundle components exist in World unsafe { trigger_on_replace_and_on_remove_hooks_and_observers( @@ -2247,7 +2251,7 @@ impl<'w> EntityWorldMut<'w> { &mut world.entities, &mut world.archetypes, &mut world.storages, - new_archetype_id, + new_archetype_id.id(), ); new_location From 25a045e371a478eafd0d24fac05b9a25c78b5072 Mon Sep 17 00:00:00 2001 From: notmd Date: Thu, 17 Apr 2025 01:38:46 +0700 Subject: [PATCH 02/16] cleanup and fix test --- crates/bevy_ecs/src/archetype.rs | 5 +- crates/bevy_ecs/src/query/state.rs | 20 ++++--- crates/bevy_ecs/src/system/system_param.rs | 61 ++++++++-------------- crates/bevy_ecs/src/world/entity_ref.rs | 10 +++- 4 files changed, 46 insertions(+), 50 deletions(-) diff --git a/crates/bevy_ecs/src/archetype.rs b/crates/bevy_ecs/src/archetype.rs index 46b449d62bdd3..f78fab24468e4 100644 --- a/crates/bevy_ecs/src/archetype.rs +++ b/crates/bevy_ecs/src/archetype.rs @@ -25,7 +25,7 @@ use crate::{ entity::{Entity, EntityLocation}, observer::Observers, storage::{ImmutableSparseSet, SparseArray, SparseSet, SparseSetIndex, TableId, TableRow}, - world::{unsafe_world_cell::UnsafeWorldCell, DeferredWorld, World}, + world::DeferredWorld, }; use alloc::{boxed::Box, vec::Vec}; use bevy_ecs_macros::Event; @@ -839,9 +839,7 @@ impl ArchetypeIdState { #[inline] pub(crate) fn trigger_if_new(&self, world: &mut DeferredWorld) { - std::dbg!(self.is_new); if self.is_new { - std::println!("triggering archetype created {:?}", self.id); world.trigger(ArchetypeCreated(self.id)); } } @@ -983,7 +981,6 @@ impl Archetypes { .by_components .entry(archetype_identity) .or_insert_with_key(move |identity| { - // std::dbg!("creating new archetype"); *is_new_ref = true; let ArchetypeComponents { table_components, diff --git a/crates/bevy_ecs/src/query/state.rs b/crates/bevy_ecs/src/query/state.rs index 7e529e144187b..c2b48eeecf9f4 100644 --- a/crates/bevy_ecs/src/query/state.rs +++ b/crates/bevy_ecs/src/query/state.rs @@ -20,7 +20,6 @@ use alloc::vec::Vec; use core::{fmt, ptr}; use fixedbitset::FixedBitSet; use log::warn; -use std::boxed::Box; #[cfg(feature = "trace")] use tracing::Span; @@ -87,6 +86,7 @@ pub struct QueryState { pub(super) is_dense: bool, pub(crate) fetch_state: D::State, pub(crate) filter_state: F::State, + sync_by_observer: bool, #[cfg(feature = "trace")] par_iter_span: Span, } @@ -98,10 +98,10 @@ impl Component for QueryState< fn register_component_hooks(hooks: &mut ComponentHooks) { hooks.on_add(|mut world, ctx| { let entity = ctx.entity; - std::dbg!("QueryState::on_add"); let observer = Observer::new( move |trigger: Trigger, mut world: DeferredWorld| -> Result { - std::dbg!("QueryState::on_add::observer"); + std::println!("sync archetype for {}", core::any::type_name::()); + let archetype_id: ArchetypeId = trigger.event().0; let archetype_ptr: *const Archetype = world .archetypes() @@ -114,6 +114,7 @@ impl Component for QueryState< let mut state = entity .get_mut::>() .expect("QueryState not found"); + state.sync_by_observer = true; unsafe { state.new_archetype_internal(&*archetype_ptr); } @@ -121,7 +122,6 @@ impl Component for QueryState< Ok(()) }, ); - // observer.watch_entity(ctx.entity); world.commands().spawn(observer); }); } @@ -314,6 +314,7 @@ impl QueryState { component_access, matched_tables: Default::default(), matched_archetypes: Default::default(), + sync_by_observer: false, #[cfg(feature = "trace")] par_iter_span: tracing::info_span!( "par_for_each", @@ -349,6 +350,7 @@ impl QueryState { component_access, matched_tables: Default::default(), matched_archetypes: Default::default(), + sync_by_observer: false, #[cfg(feature = "trace")] par_iter_span: tracing::info_span!( "par_for_each", @@ -364,7 +366,9 @@ impl QueryState { /// /// This will create read-only queries, see [`Self::query_mut`] for mutable queries. pub fn query<'w, 's>(&'s mut self, world: &'w World) -> Query<'w, 's, D::ReadOnly, F> { - self.update_archetypes(world); + if !self.sync_by_observer { + self.update_archetypes(world); + } self.query_manual(world) } @@ -453,7 +457,9 @@ impl QueryState { last_run: Tick, this_run: Tick, ) -> Query<'w, 's, D, F> { - self.update_archetypes_unsafe_world_cell(world); + if !self.sync_by_observer { + self.update_archetypes_unsafe_world_cell(world); + } // SAFETY: // - The caller ensured we have the correct access to the world. // - We called `update_archetypes_unsafe_world_cell`, which calls `validate_world`. @@ -832,6 +838,7 @@ impl QueryState { component_access: self.component_access.clone(), matched_tables: self.matched_tables.clone(), matched_archetypes: self.matched_archetypes.clone(), + sync_by_observer: false, #[cfg(feature = "trace")] par_iter_span: tracing::info_span!( "par_for_each", @@ -975,6 +982,7 @@ impl QueryState { component_access: joined_component_access, matched_tables, matched_archetypes, + sync_by_observer: false, #[cfg(feature = "trace")] par_iter_span: tracing::info_span!( "par_for_each", diff --git a/crates/bevy_ecs/src/system/system_param.rs b/crates/bevy_ecs/src/system/system_param.rs index e17eb95f4b108..81edd11b46635 100644 --- a/crates/bevy_ecs/src/system/system_param.rs +++ b/crates/bevy_ecs/src/system/system_param.rs @@ -65,7 +65,7 @@ use variadics_please::{all_tuples, all_tuples_enumerated}; /// # #[derive(SystemParam)] /// # struct ParamsExample<'w, 's> { /// # query: -/// Query<'w, 's, Entity>, +/// Query<'w, 'w, Entity>, /// # res: /// Res<'w, SomeResource>, /// # res_mut: @@ -171,7 +171,7 @@ use variadics_please::{all_tuples, all_tuples_enumerated}; /// #[derive(SystemParam)] /// #[system_param(builder)] /// pub struct CustomParam<'w, 's> { -/// query: Query<'w, 's, ()>, +/// query: Query<'w, 'w, ()>, /// local: Local<'s, usize>, /// } /// @@ -358,8 +358,6 @@ unsafe impl SystemParam for Qu archetype, &mut system_meta.archetype_component_access, ); - // std::dbg!(archetype.id()); - // state.new_archetype(archetype, &mut system_meta.archetype_component_access); } #[inline] @@ -383,40 +381,27 @@ pub(crate) fn init_query_param system_meta: &mut SystemMeta, state: Option>, ) -> Entity { - let mut query_state = world.query::<(Entity, &QueryState)>(); - if let Some((entity, query_state)) = query_state.iter(world).next() { - assert_component_access_compatibility( - &system_meta.name, - core::any::type_name::(), - core::any::type_name::(), - &system_meta.component_access_set, - &query_state.component_access, - world, - ); - - system_meta - .component_access_set - .add(query_state.component_access.clone()); - entity - } else { - let state: QueryState = state.unwrap_or_else(|| { - QueryState::new_with_access(world, &mut system_meta.archetype_component_access) - }); - - assert_component_access_compatibility( - &system_meta.name, - core::any::type_name::(), - core::any::type_name::(), - &system_meta.component_access_set, - &state.component_access, - world, + let state: QueryState = state.unwrap_or_else(|| { + std::println!( + "Creating new query state {}", + std::any::type_name::>() ); + QueryState::new_with_access(world, &mut system_meta.archetype_component_access) + }); + + assert_component_access_compatibility( + &system_meta.name, + core::any::type_name::(), + core::any::type_name::(), + &system_meta.component_access_set, + &state.component_access, + world, + ); - system_meta - .component_access_set - .add(state.component_access.clone()); - world.spawn(state).id() - } + system_meta + .component_access_set + .add(state.component_access.clone()); + world.spawn(state).id() } fn assert_component_access_compatibility( @@ -2971,11 +2956,11 @@ mod tests { #[test] fn system_param_where_clause() { #[derive(SystemParam)] - pub struct WhereParam<'w, 's, D> + pub struct WhereParam<'w, D> where D: 'static + QueryData, { - _q: Query<'w, 's, D, ()>, + _q: Query<'w, 'w, D, ()>, } fn my_system(_: WhereParam<()>) {} diff --git a/crates/bevy_ecs/src/world/entity_ref.rs b/crates/bevy_ecs/src/world/entity_ref.rs index 8a7966cf00362..58e242b62a6ee 100644 --- a/crates/bevy_ecs/src/world/entity_ref.rs +++ b/crates/bevy_ecs/src/world/entity_ref.rs @@ -5365,7 +5365,10 @@ mod tests { world.run_system_once(system).unwrap(); - fn system(_: Query<&mut TestComponent>, query: Query>) { + fn system( + _: Query<&mut TestComponent>, + query: Query, With>, + ) { for entity_ref in query.iter() { assert!(matches!( entity_ref.get::(), @@ -5447,7 +5450,10 @@ mod tests { world.run_system_once(system).unwrap(); - fn system(_: Query<&mut TestComponent>, mut query: Query>) { + fn system( + _: Query<&mut TestComponent>, + mut query: Query, With>, + ) { for mut entity_mut in query.iter_mut() { assert!(entity_mut .get_mut::() From 537ad1ce1be2fb452c8bd32df645e0b54ab13f3e Mon Sep 17 00:00:00 2001 From: notmd Date: Fri, 18 Apr 2025 14:37:16 +0700 Subject: [PATCH 03/16] Use wrapper type for `QueryState` --- crates/bevy_ecs/src/archetype.rs | 27 ++++- crates/bevy_ecs/src/bundle.rs | 10 +- crates/bevy_ecs/src/query/state.rs | 113 ++++++++++++++------- crates/bevy_ecs/src/system/system_param.rs | 44 ++++---- crates/bevy_ecs/src/world/entity_ref.rs | 4 +- 5 files changed, 129 insertions(+), 69 deletions(-) diff --git a/crates/bevy_ecs/src/archetype.rs b/crates/bevy_ecs/src/archetype.rs index f78fab24468e4..c2d07c3295ddc 100644 --- a/crates/bevy_ecs/src/archetype.rs +++ b/crates/bevy_ecs/src/archetype.rs @@ -818,7 +818,6 @@ pub struct ArchetypeRecord { } /// Metadata about the [`ArchetypeId`] and whether it is newly create or not. -#[derive(Clone, Copy)] pub(crate) struct ArchetypeIdState { id: ArchetypeId, is_new: bool, @@ -838,9 +837,24 @@ impl ArchetypeIdState { } #[inline] - pub(crate) fn trigger_if_new(&self, world: &mut DeferredWorld) { + pub(crate) fn trigger_if_new(&mut self, world: &mut DeferredWorld) { if self.is_new { world.trigger(ArchetypeCreated(self.id)); + // let id = self.id; + // world.commands().queue(move |world: &mut World| { + // world.trigger(ArchetypeCreated(id)); + // std::println!("Trigger {:?}", id); + // }); + self.is_new = false; + } + } +} + +#[cfg(debug_assertions)] +impl Drop for ArchetypeIdState { + fn drop(&mut self) { + if self.is_new { + panic!("New {:?} was not triggered before being dropped", self.id); } } } @@ -855,13 +869,16 @@ impl Archetypes { }; // SAFETY: Empty archetype has no components unsafe { - archetypes.get_id_or_insert( + let mut archetype_id_state = archetypes.get_id_or_insert( &Components::default(), &Observers::default(), TableId::empty(), Vec::new(), Vec::new(), ); + // No observers that are listening to the `ArchetypeCreated` event at this + // point so we can safely ignore it + archetype_id_state.is_new = false; } archetypes } @@ -957,8 +974,10 @@ impl Archetypes { /// `table_components` and `sparse_set_components` must be sorted /// /// # Safety - /// [`TableId`] must exist in tables + /// - [`TableId`] must exist in tables /// `table_components` and `sparse_set_components` must exist in `components` + /// - Caller must call `ArchetypeIdState::trigger_if_new` on the returned `ArchetypeIdState` + #[must_use] pub(crate) unsafe fn get_id_or_insert( &mut self, components: &Components, diff --git a/crates/bevy_ecs/src/bundle.rs b/crates/bevy_ecs/src/bundle.rs index c0e92f7827e27..2404aadc80e53 100644 --- a/crates/bevy_ecs/src/bundle.rs +++ b/crates/bevy_ecs/src/bundle.rs @@ -866,6 +866,7 @@ impl BundleInfo { /// /// # Safety /// `archetype_id` must exist and components in `bundle_info` must exist + /// Caller must call `ArchetypeIdState::trigger_if_new` on the returned value pub(crate) unsafe fn remove_bundle_from_archetype( &self, archetypes: &mut Archetypes, @@ -950,15 +951,16 @@ impl BundleInfo { Some(new_archetype_id) }; let current_archetype = &mut archetypes[archetype_id]; + let archetype_id_result = result.as_ref().map(|id| id.id()); // Cache the result in an edge. if intersection { current_archetype .edges_mut() - .cache_archetype_after_bundle_remove(self.id(), result.map(|id| id.id())); + .cache_archetype_after_bundle_remove(self.id(), archetype_id_result); } else { current_archetype .edges_mut() - .cache_archetype_after_bundle_take(self.id(), result.map(|id| id.id())); + .cache_archetype_after_bundle_take(self.id(), archetype_id_result); } result } @@ -1023,7 +1025,7 @@ impl<'w> BundleInserter<'w> { // SAFETY: We will not make any accesses to the command queue, component or resource data of this world let bundle_info = world.bundles.get_unchecked(bundle_id); let bundle_id = bundle_info.id(); - let archetype_id_state = bundle_info.insert_bundle_into_archetype( + let mut archetype_id_state = bundle_info.insert_bundle_into_archetype( &mut world.archetypes, &mut world.storages, &world.components, @@ -1400,7 +1402,7 @@ impl<'w> BundleSpawner<'w> { change_tick: Tick, ) -> Self { let bundle_info = world.bundles.get_unchecked(bundle_id); - let archetype_id_state = bundle_info.insert_bundle_into_archetype( + let mut archetype_id_state = bundle_info.insert_bundle_into_archetype( &mut world.archetypes, &mut world.storages, &world.components, diff --git a/crates/bevy_ecs/src/query/state.rs b/crates/bevy_ecs/src/query/state.rs index c2b48eeecf9f4..70d8287c36d1d 100644 --- a/crates/bevy_ecs/src/query/state.rs +++ b/crates/bevy_ecs/src/query/state.rs @@ -2,11 +2,11 @@ use crate::{ archetype::{ Archetype, ArchetypeComponentId, ArchetypeCreated, ArchetypeGeneration, ArchetypeId, }, - component::{ComponentHooks, ComponentId, Mutable, StorageType, Tick}, + component::{ComponentId, HookContext, Tick}, entity::{Entity, EntityEquivalent, EntitySet, UniqueEntityArray}, entity_disabling::DefaultQueryFilters, error::Result, - prelude::{Component, FromWorld, Observer, Trigger}, + prelude::{FromWorld, Observer, Trigger}, query::{Access, FilteredAccess, QueryCombinationIter, QueryIter, QueryParIter, WorldQuery}, storage::{SparseSetIndex, TableId}, system::Query, @@ -17,6 +17,7 @@ use crate::{ use crate::entity::UniqueEntityEquivalentSlice; use alloc::vec::Vec; +use bevy_ecs_macros::Component; use core::{fmt, ptr}; use fixedbitset::FixedBitSet; use log::warn; @@ -62,10 +63,10 @@ pub(super) union StorageId { /// [`State`]: crate::query::world_query::WorldQuery::State /// [`Fetch`]: crate::query::world_query::WorldQuery::Fetch /// [`Table`]: crate::storage::Table -#[repr(C)] // SAFETY NOTE: // Do not add any new fields that use the `D` or `F` generic parameters as this may // make `QueryState::as_transmuted_state` unsound if not done with care. +#[repr(C)] pub struct QueryState { world_id: WorldId, pub(crate) archetype_generation: ArchetypeGeneration, @@ -91,42 +92,6 @@ pub struct QueryState { par_iter_span: Span, } -impl Component for QueryState { - const STORAGE_TYPE: StorageType = StorageType::SparseSet; - type Mutability = Mutable; - - fn register_component_hooks(hooks: &mut ComponentHooks) { - hooks.on_add(|mut world, ctx| { - let entity = ctx.entity; - let observer = Observer::new( - move |trigger: Trigger, mut world: DeferredWorld| -> Result { - std::println!("sync archetype for {}", core::any::type_name::()); - - let archetype_id: ArchetypeId = trigger.event().0; - let archetype_ptr: *const Archetype = world - .archetypes() - .get(archetype_id) - .expect("Invalid ArchetypeId"); - let Ok(mut entity) = world.get_entity_mut(entity) else { - // TODO query is not exists anymore, despawn this observer - return Ok(()); - }; - let mut state = entity - .get_mut::>() - .expect("QueryState not found"); - state.sync_by_observer = true; - unsafe { - state.new_archetype_internal(&*archetype_ptr); - } - - Ok(()) - }, - ); - world.commands().spawn(observer); - }); - } -} - impl fmt::Debug for QueryState { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { f.debug_struct("QueryState") @@ -1927,6 +1892,76 @@ impl From> for QueryState, immutable)] +pub(crate) struct QueryStateWrapper(QueryState); + +impl QueryStateWrapper { + pub(crate) fn new(query_state: QueryState) -> Self { + Self(query_state) + } + + pub(crate) fn inner(&self) -> &QueryState { + &self.0 + } +} + +fn on_add_query_state( + mut world: DeferredWorld, + ctx: HookContext, +) { + let entity = ctx.entity; + + let observer = Observer::new( + move |trigger: Trigger, mut world: DeferredWorld| -> Result { + let archetype_id: ArchetypeId = trigger.event().0; + let archetype_ptr: *const Archetype = world + .archetypes() + .get(archetype_id) + .expect("Invalid ArchetypeId"); + let Some(mut state) = world.get_entity_mut(entity).ok().and_then(|entity| unsafe { + // SAFETY: This in intended mutation + entity.into_mut_assume_mutable::>() + }) else { + world.commands().entity(trigger.observer()).despawn(); + return Ok(()); + }; + + // SAFETY: The archetype and state is from the same world + unsafe { + state.0.new_archetype_internal(&*archetype_ptr); + } + + Ok(()) + }, + ); + + world.commands().queue(move |world: &mut World| { + world.spawn(observer); + let world = world.as_unsafe_world_cell(); + let entity_mut = world.get_entity(entity).unwrap(); + // SAFETY: This is intended mutation + let mut state = unsafe { + entity_mut + .get_mut_assume_mutable::>() + .unwrap() + }; + // Its still possible archetype created after the state is created and before getting sync by + // the observer. The archetype [Observer, ObserverState] and [QueryState] for example. + // SAFETY: We keep a mutable reference but this method doesn't read that reference. + state.0.update_archetypes_unsafe_world_cell(world); + state.0.sync_by_observer = true; + + std::println!( + "Spawned observer for {}", + std::any::type_name::>() + ); + }); +} + #[cfg(test)] mod tests { use crate::{ diff --git a/crates/bevy_ecs/src/system/system_param.rs b/crates/bevy_ecs/src/system/system_param.rs index 81edd11b46635..853081f970de2 100644 --- a/crates/bevy_ecs/src/system/system_param.rs +++ b/crates/bevy_ecs/src/system/system_param.rs @@ -7,7 +7,7 @@ use crate::{ entity::{Entities, Entity}, query::{ Access, FilteredAccess, FilteredAccessSet, QueryData, QueryFilter, QuerySingleError, - QueryState, ReadOnlyQueryData, + QueryState, QueryStateWrapper, ReadOnlyQueryData, }, resource::Resource, storage::ResourceData, @@ -353,8 +353,11 @@ unsafe impl SystemParam for Qu system_meta: &mut SystemMeta, world: &World, ) { - let state = world.entity(*state).get::>().unwrap(); - state.update_archetype_component_access( + let state = world + .entity(*state) + .get::>() + .unwrap(); + state.inner().update_archetype_component_access( archetype, &mut system_meta.archetype_component_access, ); @@ -381,27 +384,27 @@ pub(crate) fn init_query_param system_meta: &mut SystemMeta, state: Option>, ) -> Entity { + let component_id = world.register_component::>(); let state: QueryState = state.unwrap_or_else(|| { - std::println!( - "Creating new query state {}", - std::any::type_name::>() - ); QueryState::new_with_access(world, &mut system_meta.archetype_component_access) }); + let mut component_access = state.component_access.clone(); + // The Query need access to its own state. Just to be consistent, this will not + // cause a conflict since QueryStateWrapper is immutable + component_access.add_component_read(component_id); assert_component_access_compatibility( &system_meta.name, core::any::type_name::(), core::any::type_name::(), &system_meta.component_access_set, - &state.component_access, + &component_access, world, ); - system_meta - .component_access_set - .add(state.component_access.clone()); - world.spawn(state).id() + system_meta.component_access_set.add(component_access); + + world.spawn(QueryStateWrapper::new(state)).id() } fn assert_component_access_compatibility( @@ -424,18 +427,19 @@ fn assert_component_access_compatibility( panic!("error[B0001]: Query<{}, {}> in system {system_name} accesses component(s) {accesses}in a way that conflicts with a previous system parameter. Consider using `Without` to create disjoint Queries or merging conflicting Queries into a `ParamSet`. See: https://bevyengine.org/learn/errors/b0001", ShortName(query_type), ShortName(filter_type)); } -/// SAFETY: Caller must ensure no other mutable access to QueryState -unsafe fn get_query_state<'w, D: QueryData + 'static, F: QueryFilter + 'static>( +fn get_query_state<'w, D: QueryData + 'static, F: QueryFilter + 'static>( entity: Entity, world: UnsafeWorldCell<'w>, ) -> Option<&'w QueryState> { - let state = world - .get_entity(entity) - .unwrap() - .get::>() - .unwrap(); + // SAFETY: QueryStateWrapper is immutable so no mutable access is possible + let state = unsafe { + world + .get_entity(entity) + .ok()? + .get::>() + }; - Some(state) + state.map(|state| state.inner()) } // SAFETY: Relevant query ComponentId and ArchetypeComponentId access is applied to SystemMeta. If diff --git a/crates/bevy_ecs/src/world/entity_ref.rs b/crates/bevy_ecs/src/world/entity_ref.rs index 58e242b62a6ee..d0b7bea4cf551 100644 --- a/crates/bevy_ecs/src/world/entity_ref.rs +++ b/crates/bevy_ecs/src/world/entity_ref.rs @@ -1994,7 +1994,7 @@ impl<'w> EntityWorldMut<'w> { let old_location = self.location; // SAFETY: `archetype_id` exists because it is referenced in the old `EntityLocation` which is valid, // components exist in `bundle_info` because `Bundles::init_info` initializes a `BundleInfo` containing all components of the bundle type `T` - let new_archetype_id = unsafe { + let mut new_archetype_id = unsafe { bundle_info.remove_bundle_from_archetype( &mut world.archetypes, storages, @@ -2181,7 +2181,7 @@ impl<'w> EntityWorldMut<'w> { // SAFETY: `archetype_id` exists because it is referenced in `location` which is valid // and components in `bundle_info` must exist due to this function's safety invariants. - let new_archetype_id = bundle_info + let mut new_archetype_id = bundle_info .remove_bundle_from_archetype( &mut world.archetypes, &mut world.storages, From 16a4791aec6ae4cc2714a4dfa0d1d6df1b314e5b Mon Sep 17 00:00:00 2001 From: notmd Date: Tue, 6 May 2025 14:56:55 +0700 Subject: [PATCH 04/16] Remove world access in `new_archetype` --- crates/bevy_ecs/macros/src/lib.rs | 4 +- crates/bevy_ecs/src/archetype.rs | 10 +-- crates/bevy_ecs/src/bundle.rs | 18 +++-- crates/bevy_ecs/src/system/commands/mod.rs | 2 - crates/bevy_ecs/src/system/function_system.rs | 13 +--- crates/bevy_ecs/src/system/system_param.rs | 75 ++++++------------- 6 files changed, 40 insertions(+), 82 deletions(-) diff --git a/crates/bevy_ecs/macros/src/lib.rs b/crates/bevy_ecs/macros/src/lib.rs index b6339e94c2a91..32dc7fde7db82 100644 --- a/crates/bevy_ecs/macros/src/lib.rs +++ b/crates/bevy_ecs/macros/src/lib.rs @@ -431,9 +431,9 @@ pub fn derive_system_param(input: TokenStream) -> TokenStream { } } - unsafe fn new_archetype(state: &Self::State, archetype: &#path::archetype::Archetype, system_meta: &mut #path::system::SystemMeta, world: &#path::world::World) { + unsafe fn new_archetype(state: &Self::State, archetype: &#path::archetype::Archetype, system_meta: &mut #path::system::SystemMeta) { // SAFETY: The caller ensures that `archetype` is from the World the state was initialized from in `init_state`. - unsafe { <#fields_alias::<'_, '_, #punctuated_generic_idents> as #path::system::SystemParam>::new_archetype(&state.state, archetype, system_meta, world) } + unsafe { <#fields_alias::<'_, '_, #punctuated_generic_idents> as #path::system::SystemParam>::new_archetype(&state.state, archetype, system_meta) } } fn apply(state: &mut Self::State, system_meta: &#path::system::SystemMeta, world: &mut #path::world::World) { diff --git a/crates/bevy_ecs/src/archetype.rs b/crates/bevy_ecs/src/archetype.rs index 0f7c9b519233c..a65dd231e34c8 100644 --- a/crates/bevy_ecs/src/archetype.rs +++ b/crates/bevy_ecs/src/archetype.rs @@ -825,26 +825,19 @@ pub(crate) struct ArchetypeIdState { impl ArchetypeIdState { /// Creates a new [`ArchetypeIdState`] with the given ID and marks it as old. - #[inline] pub(crate) fn old(id: ArchetypeId) -> Self { Self { id, is_new: false } } /// Get the [`ArchetypeId`]. - #[inline] pub(crate) fn id(&self) -> ArchetypeId { self.id } - #[inline] + /// Trigger the `ArchetypeCreated` event if the archetype is new. pub(crate) fn trigger_if_new(&mut self, world: &mut DeferredWorld) { if self.is_new { world.trigger(ArchetypeCreated(self.id)); - // let id = self.id; - // world.commands().queue(move |world: &mut World| { - // world.trigger(ArchetypeCreated(id)); - // std::println!("Trigger {:?}", id); - // }); self.is_new = false; } } @@ -1032,7 +1025,6 @@ impl Archetypes { id }); - // std::println!("After get_id_or_insert archetype {is_new}"); ArchetypeIdState { id: archetype_id, is_new, diff --git a/crates/bevy_ecs/src/bundle.rs b/crates/bevy_ecs/src/bundle.rs index 6e4b98aa9cd75..88e79171a59a4 100644 --- a/crates/bevy_ecs/src/bundle.rs +++ b/crates/bevy_ecs/src/bundle.rs @@ -1429,7 +1429,7 @@ impl<'w> BundleRemover<'w> { ) -> Option { let bundle_info = world.bundles.get_unchecked(bundle_id); // SAFETY: Caller ensures archetype and bundle ids are correct. - let new_archetype_id = unsafe { + let mut new_archetype_id = unsafe { bundle_info.remove_bundle_from_archetype( &mut world.archetypes, &mut world.storages, @@ -1439,11 +1439,13 @@ impl<'w> BundleRemover<'w> { !require_all, )? }; - if new_archetype_id == archetype_id { + if new_archetype_id.id() == archetype_id { return None; } - let (old_archetype, new_archetype) = - world.archetypes.get_2_mut(archetype_id, new_archetype_id); + + let (old_archetype, new_archetype) = world + .archetypes + .get_2_mut(archetype_id, new_archetype_id.id()); let tables = if old_archetype.table_id() == new_archetype.table_id() { None @@ -1455,13 +1457,17 @@ impl<'w> BundleRemover<'w> { Some((old.into(), new.into())) }; - Some(Self { + let remover = Self { bundle_info: bundle_info.into(), new_archetype: new_archetype.into(), old_archetype: old_archetype.into(), old_and_new_table: tables, world: world.as_unsafe_world_cell(), - }) + }; + + new_archetype_id.trigger_if_new(&mut remover.world.into_deferred()); + + Some(remover) } /// This can be passed to [`remove`](Self::remove) as the `pre_remove` function if you don't want to do anything before removing. diff --git a/crates/bevy_ecs/src/system/commands/mod.rs b/crates/bevy_ecs/src/system/commands/mod.rs index ffcdf80a23429..033f00e79463e 100644 --- a/crates/bevy_ecs/src/system/commands/mod.rs +++ b/crates/bevy_ecs/src/system/commands/mod.rs @@ -136,7 +136,6 @@ const _: () = { state: &Self::State, archetype: &bevy_ecs::archetype::Archetype, system_meta: &mut bevy_ecs::system::SystemMeta, - world: &World, ) { // SAFETY: Caller guarantees the archetype is from the world used in `init_state` unsafe { @@ -144,7 +143,6 @@ const _: () = { &state.state, archetype, system_meta, - world, ); }; } diff --git a/crates/bevy_ecs/src/system/function_system.rs b/crates/bevy_ecs/src/system/function_system.rs index 45cdb79ad1630..f45b9024cb562 100644 --- a/crates/bevy_ecs/src/system/function_system.rs +++ b/crates/bevy_ecs/src/system/function_system.rs @@ -479,9 +479,7 @@ impl SystemState { for archetype in &archetypes[old_generation..] { // SAFETY: The assertion above ensures that the param_state was initialized from `world`. - unsafe { - Param::new_archetype(&self.param_state, archetype, &mut self.meta, world.world()) - }; + unsafe { Param::new_archetype(&self.param_state, archetype, &mut self.meta) }; } } @@ -797,14 +795,7 @@ where for archetype in &archetypes[old_generation..] { // SAFETY: The assertion above ensures that the param_state was initialized from `world`. - unsafe { - F::Param::new_archetype( - &state.param, - archetype, - &mut self.system_meta, - world.world(), - ) - }; + unsafe { F::Param::new_archetype(&state.param, archetype, &mut self.system_meta) }; } } diff --git a/crates/bevy_ecs/src/system/system_param.rs b/crates/bevy_ecs/src/system/system_param.rs index c40cfc1bd8be5..f393c11130e69 100644 --- a/crates/bevy_ecs/src/system/system_param.rs +++ b/crates/bevy_ecs/src/system/system_param.rs @@ -237,7 +237,6 @@ pub unsafe trait SystemParam: Sized { state: &Self::State, archetype: &Archetype, system_meta: &mut SystemMeta, - world: &World, ) { } @@ -348,19 +347,10 @@ unsafe impl SystemParam for Qu } unsafe fn new_archetype( - state: &Self::State, - archetype: &Archetype, - system_meta: &mut SystemMeta, - world: &World, + _state: &Self::State, + _archetype: &Archetype, + _system_meta: &mut SystemMeta, ) { - let state = world - .entity(*state) - .get::>() - .unwrap(); - state.inner().update_archetype_component_access( - archetype, - &mut system_meta.archetype_component_access, - ); } #[inline] @@ -384,25 +374,23 @@ pub(crate) fn init_query_param system_meta: &mut SystemMeta, state: Option>, ) -> Entity { - let component_id = world.register_component::>(); let state: QueryState = state.unwrap_or_else(|| { QueryState::new_with_access(world, &mut system_meta.archetype_component_access) }); - let mut component_access = state.component_access.clone(); - // The Query need access to its own state. Just to be consistent, this will not - // cause a conflict since QueryStateWrapper is immutable - component_access.add_component_read(component_id); + // No need to update access to since `QueryStateWrapper` is immutable assert_component_access_compatibility( &system_meta.name, core::any::type_name::(), core::any::type_name::(), &system_meta.component_access_set, - &component_access, + &state.component_access, world, ); - system_meta.component_access_set.add(component_access); + system_meta + .component_access_set + .add(state.component_access.clone()); world.spawn(QueryStateWrapper::new(state)).id() } @@ -456,10 +444,9 @@ unsafe impl<'a, D: QueryData + 'static, F: QueryFilter + 'static> SystemParam fo state: &Self::State, archetype: &Archetype, system_meta: &mut SystemMeta, - world: &World, ) { // SAFETY: Delegate to existing `SystemParam` implementations. - unsafe { Query::::new_archetype(state, archetype, system_meta, world) }; + unsafe { Query::::new_archetype(state, archetype, system_meta) }; } #[inline] @@ -529,10 +516,9 @@ unsafe impl<'a, D: QueryData + 'static, F: QueryFilter + 'static> SystemParam state: &Self::State, archetype: &Archetype, system_meta: &mut SystemMeta, - world: &World, ) { // SAFETY: Delegate to existing `SystemParam` implementations. - unsafe { Single::::new_archetype(state, archetype, system_meta, &world) }; + unsafe { Single::::new_archetype(state, archetype, system_meta) }; } #[inline] @@ -613,10 +599,9 @@ unsafe impl SystemParam state: &Self::State, archetype: &Archetype, system_meta: &mut SystemMeta, - world: &World, ) { // SAFETY: Delegate to existing `SystemParam` implementations. - unsafe { Query::::new_archetype(state, archetype, system_meta, world) }; + unsafe { Query::::new_archetype(state, archetype, system_meta) }; } #[inline] @@ -831,9 +816,9 @@ macro_rules! impl_param_set { ($($param,)*) } - unsafe fn new_archetype(state: &Self::State, archetype: &Archetype, system_meta: &mut SystemMeta, world: &World) { + unsafe fn new_archetype(state: &Self::State, archetype: &Archetype, system_meta: &mut SystemMeta) { // SAFETY: The caller ensures that `archetype` is from the World the state was initialized from in `init_state`. - unsafe { <($($param,)*) as SystemParam>::new_archetype(state, archetype, system_meta, world); } + unsafe { <($($param,)*) as SystemParam>::new_archetype(state, archetype, system_meta); } } fn apply(state: &mut Self::State, system_meta: &SystemMeta, world: &mut World) { @@ -2043,7 +2028,7 @@ unsafe impl SystemParam for When { } unsafe fn new_archetype( - state: &mut Self::State, + state: &Self::State, archetype: &Archetype, system_meta: &mut SystemMeta, ) { @@ -2107,11 +2092,10 @@ unsafe impl SystemParam for Vec { state: &Self::State, archetype: &Archetype, system_meta: &mut SystemMeta, - world: &World, ) { for state in state { // SAFETY: The caller ensures that `archetype` is from the World the state was initialized from in `init_state`. - unsafe { T::new_archetype(state, archetype, system_meta, &world) }; + unsafe { T::new_archetype(state, archetype, system_meta) }; } } @@ -2159,11 +2143,10 @@ unsafe impl SystemParam for ParamSet<'_, '_, Vec> { state: &Self::State, archetype: &Archetype, system_meta: &mut SystemMeta, - world: &World, ) { for state in state { // SAFETY: The caller ensures that `archetype` is from the World the state was initialized from in `init_state`. - unsafe { T::new_archetype(state, archetype, system_meta, world) } + unsafe { T::new_archetype(state, archetype, system_meta) } } } @@ -2242,13 +2225,13 @@ macro_rules! impl_system_param_tuple { } #[inline] - unsafe fn new_archetype(($($param,)*): &Self::State, archetype: &Archetype, system_meta: &mut SystemMeta, world: &World) { + unsafe fn new_archetype(($($param,)*): &Self::State, archetype: &Archetype, system_meta: &mut SystemMeta) { #[allow( unused_unsafe, reason = "Zero-length tuples will not run anything in the unsafe block." )] // SAFETY: The caller ensures that `archetype` is from the World the state was initialized from in `init_state`. - unsafe { $($param::new_archetype($param, archetype, system_meta, world);)* } + unsafe { $($param::new_archetype($param, archetype, system_meta);)* } } #[inline] @@ -2424,10 +2407,9 @@ unsafe impl SystemParam for StaticSystemParam<'_, '_, state: &Self::State, archetype: &Archetype, system_meta: &mut SystemMeta, - world: &World, ) { // SAFETY: The caller guarantees that the provided `archetype` matches the World used to initialize `state`. - unsafe { P::new_archetype(state, archetype, system_meta, world) }; + unsafe { P::new_archetype(state, archetype, system_meta) }; } fn apply(state: &mut Self::State, system_meta: &SystemMeta, world: &mut World) { @@ -2674,12 +2656,7 @@ trait DynParamState: Sync + Send { /// /// # Safety /// `archetype` must be from the [`World`] used to initialize `state` in [`SystemParam::init_state`]. - unsafe fn new_archetype( - &self, - archetype: &Archetype, - system_meta: &mut SystemMeta, - world: &World, - ); + unsafe fn new_archetype(&self, archetype: &Archetype, system_meta: &mut SystemMeta); /// Applies any deferred mutations stored in this [`SystemParam`]'s state. /// This is used to apply [`Commands`] during [`ApplyDeferred`](crate::prelude::ApplyDeferred). @@ -2709,14 +2686,9 @@ impl DynParamState for ParamState { self } - unsafe fn new_archetype( - &self, - archetype: &Archetype, - system_meta: &mut SystemMeta, - world: &World, - ) { + unsafe fn new_archetype(&self, archetype: &Archetype, system_meta: &mut SystemMeta) { // SAFETY: The caller ensures that `archetype` is from the World the state was initialized from in `init_state`. - unsafe { T::new_archetype(&self.0, archetype, system_meta, world) }; + unsafe { T::new_archetype(&self.0, archetype, system_meta) }; } fn apply(&mut self, system_meta: &SystemMeta, world: &mut World) { @@ -2781,10 +2753,9 @@ unsafe impl SystemParam for DynSystemParam<'_, '_> { state: &Self::State, archetype: &Archetype, system_meta: &mut SystemMeta, - world: &World, ) { // SAFETY: The caller ensures that `archetype` is from the World the state was initialized from in `init_state`. - unsafe { state.0.new_archetype(archetype, system_meta, world) }; + unsafe { state.0.new_archetype(archetype, system_meta) }; } fn apply(state: &mut Self::State, system_meta: &SystemMeta, world: &mut World) { From ce8797d4174535ac1f483f9a886120e4271311a4 Mon Sep 17 00:00:00 2001 From: notmd Date: Tue, 6 May 2025 15:59:36 +0700 Subject: [PATCH 05/16] Remove unneeded test --- crates/bevy_ecs/src/query/state.rs | 7 +--- crates/bevy_ecs/src/system/mod.rs | 64 +----------------------------- 2 files changed, 2 insertions(+), 69 deletions(-) diff --git a/crates/bevy_ecs/src/query/state.rs b/crates/bevy_ecs/src/query/state.rs index 064ec43116ccc..3ef802990bf9a 100644 --- a/crates/bevy_ecs/src/query/state.rs +++ b/crates/bevy_ecs/src/query/state.rs @@ -1901,7 +1901,7 @@ impl From> for QueryState( // SAFETY: We keep a mutable reference but this method doesn't read that reference. state.0.update_archetypes_unsafe_world_cell(world); state.0.sync_by_observer = true; - - std::println!( - "Spawned observer for {}", - std::any::type_name::>() - ); }); } diff --git a/crates/bevy_ecs/src/system/mod.rs b/crates/bevy_ecs/src/system/mod.rs index 7ba3dba4fb33c..d27ae07a667c1 100644 --- a/crates/bevy_ecs/src/system/mod.rs +++ b/crates/bevy_ecs/src/system/mod.rs @@ -402,7 +402,7 @@ mod tests { use std::println; use crate::{ - archetype::{ArchetypeComponentId, Archetypes}, + archetype::Archetypes, bundle::Bundles, change_detection::DetectChanges, component::{Component, Components}, @@ -1553,68 +1553,6 @@ mod tests { } } - #[test] - fn update_archetype_component_access_works() { - use std::collections::HashSet; - - fn a_not_b_system(_query: Query<&A, Without>) {} - - let mut world = World::default(); - let mut system = IntoSystem::into_system(a_not_b_system); - let mut expected_ids = HashSet::::new(); - let a_id = world.register_component::(); - - // set up system and verify its access is empty - system.initialize(&mut world); - system.update_archetype_component_access(world.as_unsafe_world_cell()); - let archetype_component_access = system.archetype_component_access(); - assert!(expected_ids - .iter() - .all(|id| archetype_component_access.has_component_read(*id))); - - // add some entities with archetypes that should match and save their ids - expected_ids.insert( - world - .spawn(A) - .archetype() - .get_archetype_component_id(a_id) - .unwrap(), - ); - expected_ids.insert( - world - .spawn((A, C)) - .archetype() - .get_archetype_component_id(a_id) - .unwrap(), - ); - - // add some entities with archetypes that should not match - world.spawn((A, B)); - world.spawn((B, C)); - - // update system and verify its accesses are correct - system.update_archetype_component_access(world.as_unsafe_world_cell()); - let archetype_component_access = system.archetype_component_access(); - assert!(expected_ids - .iter() - .all(|id| archetype_component_access.has_component_read(*id))); - - // one more round - expected_ids.insert( - world - .spawn((A, D)) - .archetype() - .get_archetype_component_id(a_id) - .unwrap(), - ); - world.spawn((A, B, D)); - system.update_archetype_component_access(world.as_unsafe_world_cell()); - let archetype_component_access = system.archetype_component_access(); - assert!(expected_ids - .iter() - .all(|id| archetype_component_access.has_component_read(*id))); - } - #[test] fn commands_param_set() { // Regression test for #4676 From ecaf326db3e207c3a5a76b08f3efb055fdef5e5f Mon Sep 17 00:00:00 2001 From: notmd Date: Tue, 6 May 2025 16:11:30 +0700 Subject: [PATCH 06/16] Add test to demonstrate about possible missing data --- crates/bevy_ecs/src/query/state.rs | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/crates/bevy_ecs/src/query/state.rs b/crates/bevy_ecs/src/query/state.rs index 3ef802990bf9a..2e18e726cf979 100644 --- a/crates/bevy_ecs/src/query/state.rs +++ b/crates/bevy_ecs/src/query/state.rs @@ -1959,7 +1959,7 @@ fn on_add_query_state( .unwrap() }; // Its still possible archetype created after the state is created and before getting sync by - // the observer. The archetype [Observer, ObserverState] and [QueryState] for example. + // the observer. The archetype [Observer] and [QueryStateWrapper] for example. // SAFETY: We keep a mutable reference but this method doesn't read that reference. state.0.update_archetypes_unsafe_world_cell(world); state.0.sync_by_observer = true; @@ -2456,4 +2456,15 @@ mod tests { assert!(!query.is_dense); assert_eq!(1, query.iter(&world).count()); } + + #[test] + fn update_query_state_archetype() { + let mut world = World::new(); + + fn sys(query: Query) { + assert_eq!(query.iter().count(), 3); // This number can be changd as more things are entities + } + + let _ = world.run_system_cached(sys); + } } From c38d01ecc3918584d0ba7400b0209a42129ee56d Mon Sep 17 00:00:00 2001 From: notmd Date: Tue, 6 May 2025 19:55:07 +0700 Subject: [PATCH 07/16] Fix Query lifetime & clippy --- crates/bevy_audio/src/audio_output.rs | 6 +++--- crates/bevy_ecs/src/archetype.rs | 2 +- crates/bevy_ecs/src/bundle.rs | 4 ++-- crates/bevy_ecs/src/query/state.rs | 2 +- crates/bevy_ecs/src/system/system_param.rs | 2 +- crates/bevy_gizmos/src/gizmos.rs | 4 ++-- crates/bevy_input_focus/src/lib.rs | 6 +++--- crates/bevy_input_focus/src/tab_navigation.rs | 10 +++++----- .../src/mesh_picking/ray_cast/mod.rs | 4 ++-- crates/bevy_text/src/text2d.rs | 2 +- crates/bevy_text/src/text_access.rs | 14 ++++++------- crates/bevy_transform/src/helper.rs | 8 ++++---- .../src/experimental/ghost_hierarchy.rs | 20 +++++++++---------- crates/bevy_ui/src/render/mod.rs | 14 ++++++------- crates/bevy_ui/src/ui_node.rs | 10 +++++----- crates/bevy_ui/src/widget/text.rs | 2 +- 16 files changed, 55 insertions(+), 55 deletions(-) diff --git a/crates/bevy_audio/src/audio_output.rs b/crates/bevy_audio/src/audio_output.rs index 1869fb47555db..7b0766af546d7 100644 --- a/crates/bevy_audio/src/audio_output.rs +++ b/crates/bevy_audio/src/audio_output.rs @@ -54,10 +54,10 @@ pub struct PlaybackDespawnMarker; pub struct PlaybackRemoveMarker; #[derive(SystemParam)] -pub(crate) struct EarPositions<'w, 's> { - pub(crate) query: Query<'w, 's, (Entity, &'static GlobalTransform, &'static SpatialListener)>, +pub(crate) struct EarPositions<'w> { + pub(crate) query: Query<'w, 'w, (Entity, &'static GlobalTransform, &'static SpatialListener)>, } -impl<'w, 's> EarPositions<'w, 's> { +impl<'w> EarPositions<'w> { /// Gets a set of transformed ear positions. /// /// If there are no listeners, use the default values. If a user has added multiple diff --git a/crates/bevy_ecs/src/archetype.rs b/crates/bevy_ecs/src/archetype.rs index a65dd231e34c8..a07728bc2d5d2 100644 --- a/crates/bevy_ecs/src/archetype.rs +++ b/crates/bevy_ecs/src/archetype.rs @@ -968,7 +968,7 @@ impl Archetypes { /// /// # Safety /// - [`TableId`] must exist in tables - /// `table_components` and `sparse_set_components` must exist in `components` + /// `table_components` and `sparse_set_components` must exist in `components` /// - Caller must call `ArchetypeIdState::trigger_if_new` on the returned `ArchetypeIdState` #[must_use] pub(crate) unsafe fn get_id_or_insert( diff --git a/crates/bevy_ecs/src/bundle.rs b/crates/bevy_ecs/src/bundle.rs index 88e79171a59a4..ef9c40ecaf108 100644 --- a/crates/bevy_ecs/src/bundle.rs +++ b/crates/bevy_ecs/src/bundle.rs @@ -901,7 +901,7 @@ impl BundleInfo { }; let result = if let Some(result) = archetype_after_remove_result { // This bundle removal result is cached. Just return that! - result.map(|id| ArchetypeIdState::old(id)) + result.map(ArchetypeIdState::old) } else { let mut next_table_components; let mut next_sparse_set_components; @@ -964,7 +964,7 @@ impl BundleInfo { Some(new_archetype_id) }; let current_archetype = &mut archetypes[archetype_id]; - let archetype_id_result = result.as_ref().map(|id| id.id()); + let archetype_id_result = result.as_ref().map(ArchetypeIdState::id); // Cache the result in an edge. if intersection { current_archetype diff --git a/crates/bevy_ecs/src/query/state.rs b/crates/bevy_ecs/src/query/state.rs index 2e18e726cf979..70748f1e83b5f 100644 --- a/crates/bevy_ecs/src/query/state.rs +++ b/crates/bevy_ecs/src/query/state.rs @@ -1931,8 +1931,8 @@ fn on_add_query_state( .archetypes() .get(archetype_id) .expect("Invalid ArchetypeId"); + // SAFETY: This in intended mutation let Some(mut state) = world.get_entity_mut(entity).ok().and_then(|entity| unsafe { - // SAFETY: This in intended mutation entity.into_mut_assume_mutable::>() }) else { world.commands().entity(trigger.observer()).despawn(); diff --git a/crates/bevy_ecs/src/system/system_param.rs b/crates/bevy_ecs/src/system/system_param.rs index f393c11130e69..1a8676b7af0f7 100644 --- a/crates/bevy_ecs/src/system/system_param.rs +++ b/crates/bevy_ecs/src/system/system_param.rs @@ -427,7 +427,7 @@ fn get_query_state<'w, D: QueryData + 'static, F: QueryFilter + 'static>( .get::>() }; - state.map(|state| state.inner()) + state.map(QueryStateWrapper::inner) } // SAFETY: Relevant query ComponentId and ArchetypeComponentId access is applied to SystemMeta. If diff --git a/crates/bevy_gizmos/src/gizmos.rs b/crates/bevy_gizmos/src/gizmos.rs index b51dd672fe07d..26ef5da7dbecb 100644 --- a/crates/bevy_gizmos/src/gizmos.rs +++ b/crates/bevy_gizmos/src/gizmos.rs @@ -206,13 +206,13 @@ where } unsafe fn new_archetype( - state: &mut Self::State, + state: &Self::State, archetype: &bevy_ecs::archetype::Archetype, system_meta: &mut SystemMeta, ) { // SAFETY: The caller ensures that `archetype` is from the World the state was initialized from in `init_state`. unsafe { - GizmosState::::new_archetype(&mut state.state, archetype, system_meta); + GizmosState::::new_archetype(&state.state, archetype, system_meta); }; } diff --git a/crates/bevy_input_focus/src/lib.rs b/crates/bevy_input_focus/src/lib.rs index 3f7ecf9e7c5bc..fdbf1935ee500 100644 --- a/crates/bevy_input_focus/src/lib.rs +++ b/crates/bevy_input_focus/src/lib.rs @@ -289,13 +289,13 @@ pub trait IsFocused { /// /// When working with the entire [`World`], consider using the [`IsFocused`] instead. #[derive(SystemParam)] -pub struct IsFocusedHelper<'w, 's> { - parent_query: Query<'w, 's, &'static ChildOf>, +pub struct IsFocusedHelper<'w> { + parent_query: Query<'w, 'w, &'static ChildOf>, input_focus: Option>, input_focus_visible: Option>, } -impl IsFocused for IsFocusedHelper<'_, '_> { +impl IsFocused for IsFocusedHelper<'_> { fn is_focused(&self, entity: Entity) -> bool { self.input_focus .as_deref() diff --git a/crates/bevy_input_focus/src/tab_navigation.rs b/crates/bevy_input_focus/src/tab_navigation.rs index a5fe691458863..96724c1405b70 100644 --- a/crates/bevy_input_focus/src/tab_navigation.rs +++ b/crates/bevy_input_focus/src/tab_navigation.rs @@ -150,21 +150,21 @@ pub enum TabNavigationError { /// An injectable helper object that provides tab navigation functionality. #[doc(hidden)] #[derive(SystemParam)] -pub struct TabNavigation<'w, 's> { +pub struct TabNavigation<'w> { // Query for tab groups. - tabgroup_query: Query<'w, 's, (Entity, &'static TabGroup, &'static Children)>, + tabgroup_query: Query<'w, 'w, (Entity, &'static TabGroup, &'static Children)>, // Query for tab indices. tabindex_query: Query< 'w, - 's, + 'w, (Entity, Option<&'static TabIndex>, Option<&'static Children>), Without, >, // Query for parents. - parent_query: Query<'w, 's, &'static ChildOf>, + parent_query: Query<'w, 'w, &'static ChildOf>, } -impl TabNavigation<'_, '_> { +impl TabNavigation<'_> { /// Navigate to the desired focusable entity. /// /// Change the [`NavAction`] to navigate in a different direction. diff --git a/crates/bevy_picking/src/mesh_picking/ray_cast/mod.rs b/crates/bevy_picking/src/mesh_picking/ray_cast/mod.rs index c1f465b96a80a..73748f2fb95cb 100644 --- a/crates/bevy_picking/src/mesh_picking/ray_cast/mod.rs +++ b/crates/bevy_picking/src/mesh_picking/ray_cast/mod.rs @@ -179,7 +179,7 @@ pub struct MeshRayCast<'w, 's> { #[doc(hidden)] pub culling_query: Query< 'w, - 's, + 'w, ( Read, Read, @@ -192,7 +192,7 @@ pub struct MeshRayCast<'w, 's> { #[doc(hidden)] pub mesh_query: Query< 'w, - 's, + 'w, ( Option>, Option>, diff --git a/crates/bevy_text/src/text2d.rs b/crates/bevy_text/src/text2d.rs index a9419e89c0f0c..fdabaa6e021a9 100644 --- a/crates/bevy_text/src/text2d.rs +++ b/crates/bevy_text/src/text2d.rs @@ -131,7 +131,7 @@ impl From for Text2d { pub type Text2dReader<'w, 's> = TextReader<'w, 's, Text2d>; /// 2d alias for [`TextWriter`]. -pub type Text2dWriter<'w, 's> = TextWriter<'w, 's, Text2d>; +pub type Text2dWriter<'w> = TextWriter<'w, Text2d>; /// This system extracts the sprites from the 2D text components and adds them to the /// "render world". diff --git a/crates/bevy_text/src/text_access.rs b/crates/bevy_text/src/text_access.rs index 7aafa28ef63e2..7d3b306bb6800 100644 --- a/crates/bevy_text/src/text_access.rs +++ b/crates/bevy_text/src/text_access.rs @@ -52,7 +52,7 @@ pub struct TextReader<'w, 's, R: TextRoot> { scratch: Local<'s, TextIterScratch>, roots: Query< 'w, - 's, + 'w, ( &'static R, &'static TextFont, @@ -62,7 +62,7 @@ pub struct TextReader<'w, 's, R: TextRoot> { >, spans: Query< 'w, - 's, + 'w, ( &'static TextSpan, &'static TextFont, @@ -222,12 +222,12 @@ impl<'a, R: TextRoot> Drop for TextSpanIter<'a, R> { /// /// `R` is the root text component, and `S` is the text span component on children. #[derive(SystemParam)] -pub struct TextWriter<'w, 's, R: TextRoot> { +pub struct TextWriter<'w, R: TextRoot> { // This is a resource because two TextWriters can't run in parallel. scratch: ResMut<'w, TextIterScratch>, roots: Query< 'w, - 's, + 'w, ( &'static mut R, &'static mut TextFont, @@ -237,7 +237,7 @@ pub struct TextWriter<'w, 's, R: TextRoot> { >, spans: Query< 'w, - 's, + 'w, ( &'static mut TextSpan, &'static mut TextFont, @@ -245,10 +245,10 @@ pub struct TextWriter<'w, 's, R: TextRoot> { ), Without, >, - children: Query<'w, 's, &'static Children>, + children: Query<'w, 'w, &'static Children>, } -impl<'w, 's, R: TextRoot> TextWriter<'w, 's, R> { +impl<'w, R: TextRoot> TextWriter<'w, R> { /// Gets a mutable reference to a text span within a text block at a specific index in the flattened span list. pub fn get( &mut self, diff --git a/crates/bevy_transform/src/helper.rs b/crates/bevy_transform/src/helper.rs index d13822847fc40..7c7ebcd800bca 100644 --- a/crates/bevy_transform/src/helper.rs +++ b/crates/bevy_transform/src/helper.rs @@ -17,12 +17,12 @@ use crate::components::{GlobalTransform, Transform}; /// a [`GlobalTransform`] that reflects the changes made to any [`Transform`]s since /// the last time the transform propagation systems ran. #[derive(SystemParam)] -pub struct TransformHelper<'w, 's> { - parent_query: Query<'w, 's, &'static ChildOf>, - transform_query: Query<'w, 's, &'static Transform>, +pub struct TransformHelper<'w> { + parent_query: Query<'w, 'w, &'static ChildOf>, + transform_query: Query<'w, 'w, &'static Transform>, } -impl<'w, 's> TransformHelper<'w, 's> { +impl<'w> TransformHelper<'w> { /// Computes the [`GlobalTransform`] of the given entity from the [`Transform`] component on it and its ancestors. pub fn compute_global_transform( &self, diff --git a/crates/bevy_ui/src/experimental/ghost_hierarchy.rs b/crates/bevy_ui/src/experimental/ghost_hierarchy.rs index 9134f5eebac75..dda7284595adb 100644 --- a/crates/bevy_ui/src/experimental/ghost_hierarchy.rs +++ b/crates/bevy_ui/src/experimental/ghost_hierarchy.rs @@ -70,14 +70,14 @@ pub struct UiChildren<'w, 's> { #[cfg(not(feature = "ghost_nodes"))] /// System param that gives access to UI children utilities. #[derive(SystemParam)] -pub struct UiChildren<'w, 's> { - ui_children_query: Query<'w, 's, Option<&'static Children>, With>, - changed_children_query: Query<'w, 's, Entity, Changed>, - parents_query: Query<'w, 's, &'static ChildOf>, +pub struct UiChildren<'w> { + ui_children_query: Query<'w, 'w, Option<&'static Children>, With>, + changed_children_query: Query<'w, 'w, Entity, Changed>, + parents_query: Query<'w, 'w, &'static ChildOf>, } #[cfg(feature = "ghost_nodes")] -impl<'w, 's> UiChildren<'w, 's> { +impl<'w> UiChildren<'w> { /// Iterates the children of `entity`, skipping over [`GhostNode`]. /// /// Traverses the hierarchy depth-first to ensure child order. @@ -135,9 +135,9 @@ impl<'w, 's> UiChildren<'w, 's> { } #[cfg(not(feature = "ghost_nodes"))] -impl<'w, 's> UiChildren<'w, 's> { +impl<'w> UiChildren<'w> { /// Iterates the children of `entity`. - pub fn iter_ui_children(&'s self, entity: Entity) -> impl Iterator + 's { + pub fn iter_ui_children(&'w self, entity: Entity) -> impl Iterator + 'w { self.ui_children_query .get(entity) .ok() @@ -149,17 +149,17 @@ impl<'w, 's> UiChildren<'w, 's> { } /// Returns the UI parent of the provided entity. - pub fn get_parent(&'s self, entity: Entity) -> Option { + pub fn get_parent(&'w self, entity: Entity) -> Option { self.parents_query.get(entity).ok().map(ChildOf::parent) } /// Given an entity in the UI hierarchy, check if its set of children has changed, e.g if children has been added/removed or if the order has changed. - pub fn is_changed(&'s self, entity: Entity) -> bool { + pub fn is_changed(&'w self, entity: Entity) -> bool { self.changed_children_query.contains(entity) } /// Returns `true` if the given entity is either a [`Node`] or a [`GhostNode`]. - pub fn is_ui_node(&'s self, entity: Entity) -> bool { + pub fn is_ui_node(&'w self, entity: Entity) -> bool { self.ui_children_query.contains(entity) } } diff --git a/crates/bevy_ui/src/render/mod.rs b/crates/bevy_ui/src/render/mod.rs index 18532d288e627..1ba86cd1b7bde 100644 --- a/crates/bevy_ui/src/render/mod.rs +++ b/crates/bevy_ui/src/render/mod.rs @@ -263,13 +263,13 @@ impl ExtractedUiNodes { } #[derive(SystemParam)] -pub struct UiCameraMap<'w, 's> { - mapping: Query<'w, 's, RenderEntity>, +pub struct UiCameraMap<'w> { + mapping: Query<'w, 'w, RenderEntity>, } -impl<'w, 's> UiCameraMap<'w, 's> { +impl<'w> UiCameraMap<'w> { /// Get the default camera and create the mapper - pub fn get_mapper(&'w self) -> UiCameraMapper<'w, 's> { + pub fn get_mapper(&'w self) -> UiCameraMapper<'w> { UiCameraMapper { mapping: &self.mapping, camera_entity: Entity::PLACEHOLDER, @@ -278,13 +278,13 @@ impl<'w, 's> UiCameraMap<'w, 's> { } } -pub struct UiCameraMapper<'w, 's> { - mapping: &'w Query<'w, 's, RenderEntity>, +pub struct UiCameraMapper<'w> { + mapping: &'w Query<'w, 'w, RenderEntity>, camera_entity: Entity, render_entity: Entity, } -impl<'w, 's> UiCameraMapper<'w, 's> { +impl<'w> UiCameraMapper<'w> { /// Returns the render entity corresponding to the given `UiTargetCamera` or the default camera if `None`. pub fn map(&mut self, computed_target: &ComputedNodeTarget) -> Option { let camera_entity = computed_target.camera; diff --git a/crates/bevy_ui/src/ui_node.rs b/crates/bevy_ui/src/ui_node.rs index 55ce3eb6a2274..7bfd303891853 100644 --- a/crates/bevy_ui/src/ui_node.rs +++ b/crates/bevy_ui/src/ui_node.rs @@ -2689,13 +2689,13 @@ impl UiTargetCamera { pub struct IsDefaultUiCamera; #[derive(SystemParam)] -pub struct DefaultUiCamera<'w, 's> { - cameras: Query<'w, 's, (Entity, &'static Camera)>, - default_cameras: Query<'w, 's, Entity, (With, With)>, - primary_window: Query<'w, 's, Entity, With>, +pub struct DefaultUiCamera<'w> { + cameras: Query<'w, 'w, (Entity, &'static Camera)>, + default_cameras: Query<'w, 'w, Entity, (With, With)>, + primary_window: Query<'w, 'w, Entity, With>, } -impl<'w, 's> DefaultUiCamera<'w, 's> { +impl<'w> DefaultUiCamera<'w> { pub fn get(&self) -> Option { self.default_cameras.single().ok().or_else(|| { // If there isn't a single camera and the query isn't empty, there is two or more cameras queried. diff --git a/crates/bevy_ui/src/widget/text.rs b/crates/bevy_ui/src/widget/text.rs index 0153fa954c406..606243ae4ea98 100644 --- a/crates/bevy_ui/src/widget/text.rs +++ b/crates/bevy_ui/src/widget/text.rs @@ -132,7 +132,7 @@ impl From for Text { pub type TextUiReader<'w, 's> = TextReader<'w, 's, Text>; /// UI alias for [`TextWriter`]. -pub type TextUiWriter<'w, 's> = TextWriter<'w, 's, Text>; +pub type TextUiWriter<'w> = TextWriter<'w, Text>; /// Text measurement for UI layout. See [`NodeMeasure`]. pub struct TextMeasure { From 44584c5c445041584772d5da792164ba67f446c2 Mon Sep 17 00:00:00 2001 From: notmd Date: Tue, 6 May 2025 20:00:42 +0700 Subject: [PATCH 08/16] Add migration guide --- .../migration-guides/query_lifetime_change.md | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) create mode 100644 release-content/migration-guides/query_lifetime_change.md diff --git a/release-content/migration-guides/query_lifetime_change.md b/release-content/migration-guides/query_lifetime_change.md new file mode 100644 index 0000000000000..26a63bb2868ec --- /dev/null +++ b/release-content/migration-guides/query_lifetime_change.md @@ -0,0 +1,17 @@ +--- +title: Query lifetime change +pull_requests: [18860] +--- + +Query lifetimes have been change if you are deriving `SystemParam` with `Query` you will need to update. + +```diff +#[derive(SystemParam)] +-- struct MyParam<'w, 's> { +-- query: Query<'w, 's, Entity> +-- } +++ struct MyParam<'w> { +++ query: Query<'w, 'w, Entity> +++ } + +``` From 330794888bf7f659af5bf04e1531f6538a196b3f Mon Sep 17 00:00:00 2001 From: notmd Date: Tue, 6 May 2025 20:10:16 +0700 Subject: [PATCH 09/16] More query lifetime fixes --- .../src/experimental/ghost_hierarchy.rs | 42 +++++++++---------- 1 file changed, 21 insertions(+), 21 deletions(-) diff --git a/crates/bevy_ui/src/experimental/ghost_hierarchy.rs b/crates/bevy_ui/src/experimental/ghost_hierarchy.rs index dda7284595adb..4736ec165c181 100644 --- a/crates/bevy_ui/src/experimental/ghost_hierarchy.rs +++ b/crates/bevy_ui/src/experimental/ghost_hierarchy.rs @@ -29,19 +29,19 @@ pub struct GhostNode; /// /// A UI root node is either a [`Node`] without a [`ChildOf`], or with only [`GhostNode`] ancestors. #[derive(SystemParam)] -pub struct UiRootNodes<'w, 's> { - root_node_query: Query<'w, 's, Entity, (With, Without)>, - root_ghost_node_query: Query<'w, 's, Entity, (With, Without)>, - all_nodes_query: Query<'w, 's, Entity, With>, - ui_children: UiChildren<'w, 's>, +pub struct UiRootNodes<'w> { + root_node_query: Query<'w, 'w, Entity, (With, Without)>, + root_ghost_node_query: Query<'w, 'w, Entity, (With, Without)>, + all_nodes_query: Query<'w, 'w, Entity, With>, + ui_children: UiChildren<'w>, } #[cfg(not(feature = "ghost_nodes"))] -pub type UiRootNodes<'w, 's> = Query<'w, 's, Entity, (With, Without)>; +pub type UiRootNodes<'w> = Query<'w, 'w, Entity, (With, Without)>; #[cfg(feature = "ghost_nodes")] -impl<'w, 's> UiRootNodes<'w, 's> { - pub fn iter(&'s self) -> impl Iterator + 's { +impl<'w> UiRootNodes<'w> { + pub fn iter(&'w self) -> impl Iterator + 'w { self.root_node_query .iter() .chain(self.root_ghost_node_query.iter().flat_map(|root_ghost| { @@ -54,17 +54,17 @@ impl<'w, 's> UiRootNodes<'w, 's> { #[cfg(feature = "ghost_nodes")] /// System param that gives access to UI children utilities, skipping over [`GhostNode`]. #[derive(SystemParam)] -pub struct UiChildren<'w, 's> { +pub struct UiChildren<'w> { ui_children_query: Query< 'w, - 's, + 'w, (Option<&'static Children>, Has), Or<(With, With)>, >, - changed_children_query: Query<'w, 's, Entity, Changed>, - children_query: Query<'w, 's, &'static Children>, - ghost_nodes_query: Query<'w, 's, Entity, With>, - parents_query: Query<'w, 's, &'static ChildOf>, + changed_children_query: Query<'w, 'w, Entity, Changed>, + children_query: Query<'w, 'w, &'static Children>, + ghost_nodes_query: Query<'w, 'w, Entity, With>, + parents_query: Query<'w, 'w, &'static ChildOf>, } #[cfg(not(feature = "ghost_nodes"))] @@ -85,7 +85,7 @@ impl<'w> UiChildren<'w> { /// # Performance /// /// This iterator allocates if the `entity` node has more than 8 children (including ghost nodes). - pub fn iter_ui_children(&'s self, entity: Entity) -> UiChildrenIter<'w, 's> { + pub fn iter_ui_children(&'w self, entity: Entity) -> UiChildrenIter<'w> { UiChildrenIter { stack: self .ui_children_query @@ -98,14 +98,14 @@ impl<'w> UiChildren<'w> { } /// Returns the UI parent of the provided entity, skipping over [`GhostNode`]. - pub fn get_parent(&'s self, entity: Entity) -> Option { + pub fn get_parent(&'w self, entity: Entity) -> Option { self.parents_query .iter_ancestors(entity) .find(|entity| !self.ghost_nodes_query.contains(*entity)) } /// Iterates the [`GhostNode`]s between this entity and its UI children. - pub fn iter_ghost_nodes(&'s self, entity: Entity) -> Box + 's> { + pub fn iter_ghost_nodes(&'w self, entity: Entity) -> Box + 'w> { Box::new( self.children_query .get(entity) @@ -165,18 +165,18 @@ impl<'w> UiChildren<'w> { } #[cfg(feature = "ghost_nodes")] -pub struct UiChildrenIter<'w, 's> { +pub struct UiChildrenIter<'w> { stack: SmallVec<[Entity; 8]>, - query: &'s Query< + query: &'w Query< + 'w, 'w, - 's, (Option<&'static Children>, Has), Or<(With, With)>, >, } #[cfg(feature = "ghost_nodes")] -impl<'w, 's> Iterator for UiChildrenIter<'w, 's> { +impl<'w> Iterator for UiChildrenIter<'w> { type Item = Entity; fn next(&mut self) -> Option { loop { From c71a5415a7a011b817232b40801aae20163c5296 Mon Sep 17 00:00:00 2001 From: notmd Date: Tue, 6 May 2025 20:50:39 +0700 Subject: [PATCH 10/16] Impl validate for `Query` --- crates/bevy_ecs/src/system/system_param.rs | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/crates/bevy_ecs/src/system/system_param.rs b/crates/bevy_ecs/src/system/system_param.rs index 1a8676b7af0f7..0c747d35c767d 100644 --- a/crates/bevy_ecs/src/system/system_param.rs +++ b/crates/bevy_ecs/src/system/system_param.rs @@ -367,6 +367,21 @@ unsafe impl SystemParam for Qu // The caller ensures the world matches the one used in init_state. unsafe { state.query_unchecked_manual_with_ticks(world, system_meta.last_run, change_tick) } } + + #[inline] + unsafe fn validate_param( + state: &Self::State, + _system_meta: &SystemMeta, + world: UnsafeWorldCell, + ) -> Result<(), SystemParamValidationError> { + if get_query_state::(*state, world).is_none() { + return Err(SystemParamValidationError::invalid::( + "Query state not found", + )); + } + + Ok(()) + } } pub(crate) fn init_query_param( From 71bb7891c1f04783de6407a64a22e3d4cec9d1bc Mon Sep 17 00:00:00 2001 From: notmd Date: Tue, 6 May 2025 20:58:43 +0700 Subject: [PATCH 11/16] Fix lifetime again --- crates/bevy_ui/src/experimental/ghost_hierarchy.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/bevy_ui/src/experimental/ghost_hierarchy.rs b/crates/bevy_ui/src/experimental/ghost_hierarchy.rs index 4736ec165c181..73cc1635950e8 100644 --- a/crates/bevy_ui/src/experimental/ghost_hierarchy.rs +++ b/crates/bevy_ui/src/experimental/ghost_hierarchy.rs @@ -121,7 +121,7 @@ impl<'w> UiChildren<'w> { } /// Given an entity in the UI hierarchy, check if its set of children has changed, e.g if children has been added/removed or if the order has changed. - pub fn is_changed(&'s self, entity: Entity) -> bool { + pub fn is_changed(&'w self, entity: Entity) -> bool { self.changed_children_query.contains(entity) || self .iter_ghost_nodes(entity) @@ -129,7 +129,7 @@ impl<'w> UiChildren<'w> { } /// Returns `true` if the given entity is either a [`Node`] or a [`GhostNode`]. - pub fn is_ui_node(&'s self, entity: Entity) -> bool { + pub fn is_ui_node(&'w self, entity: Entity) -> bool { self.ui_children_query.contains(entity) } } From 99f84267ddfd0f073b1f0332764d0b880b19365b Mon Sep 17 00:00:00 2001 From: notmd Date: Tue, 6 May 2025 21:03:47 +0700 Subject: [PATCH 12/16] Fix example --- examples/ecs/system_param.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/examples/ecs/system_param.rs b/examples/ecs/system_param.rs index f32a783f72ca4..62735e1bf52f1 100644 --- a/examples/ecs/system_param.rs +++ b/examples/ecs/system_param.rs @@ -21,12 +21,12 @@ struct PlayerCount(usize); /// /// In this example, it includes a query and a mutable resource. #[derive(SystemParam)] -struct PlayerCounter<'w, 's> { - players: Query<'w, 's, &'static Player>, +struct PlayerCounter<'w> { + players: Query<'w, 'w, &'static Player>, count: ResMut<'w, PlayerCount>, } -impl<'w, 's> PlayerCounter<'w, 's> { +impl<'w> PlayerCounter<'w> { fn count(&mut self) { self.count.0 = self.players.iter().len(); } From 71454b387556d772c632e52954fbe6fabbb109bf Mon Sep 17 00:00:00 2001 From: notmd Date: Tue, 6 May 2025 21:22:48 +0700 Subject: [PATCH 13/16] Fix test --- .../compile_fail/tests/ui/system_param_derive_readonly.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/crates/bevy_ecs/compile_fail/tests/ui/system_param_derive_readonly.rs b/crates/bevy_ecs/compile_fail/tests/ui/system_param_derive_readonly.rs index 84691f7a2704a..408c80ec5a0e0 100644 --- a/crates/bevy_ecs/compile_fail/tests/ui/system_param_derive_readonly.rs +++ b/crates/bevy_ecs/compile_fail/tests/ui/system_param_derive_readonly.rs @@ -5,12 +5,11 @@ use bevy_ecs::system::{ReadOnlySystemParam, SystemParam, SystemState}; struct Foo; #[derive(SystemParam)] -struct Mutable<'w, 's> { - a: Query<'w, 's, &'static mut Foo>, +struct Mutable<'w> { + a: Query<'w, 'w, &'static mut Foo>, } fn main() { - let mut world = World::default(); let state = SystemState::::new(&mut world); state.get(&world); From 9c1bc69be518e44e335e7e0ee5394b7875b81e4d Mon Sep 17 00:00:00 2001 From: notmd Date: Tue, 6 May 2025 21:48:34 +0700 Subject: [PATCH 14/16] Fix test --- crates/bevy_input_focus/src/tab_navigation.rs | 32 +++++++++---------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/crates/bevy_input_focus/src/tab_navigation.rs b/crates/bevy_input_focus/src/tab_navigation.rs index 96724c1405b70..226c6d50f7815 100644 --- a/crates/bevy_input_focus/src/tab_navigation.rs +++ b/crates/bevy_input_focus/src/tab_navigation.rs @@ -365,8 +365,6 @@ pub fn handle_tab_navigation( #[cfg(test)] mod tests { - use bevy_ecs::system::SystemState; - use super::*; #[test] @@ -378,23 +376,25 @@ mod tests { let tab_entity_1 = world.spawn((TabIndex(0), ChildOf(tab_group_entity))).id(); let tab_entity_2 = world.spawn((TabIndex(1), ChildOf(tab_group_entity))).id(); - let mut system_state: SystemState = SystemState::new(world); - let tab_navigation = system_state.get(world); - assert_eq!(tab_navigation.tabgroup_query.iter().count(), 1); - assert_eq!(tab_navigation.tabindex_query.iter().count(), 2); + let system = move |tab_navigation: TabNavigation| { + assert_eq!(tab_navigation.tabgroup_query.iter().count(), 1); + assert_eq!(tab_navigation.tabindex_query.iter().count(), 2); + + let next_entity = + tab_navigation.navigate(&InputFocus::from_entity(tab_entity_1), NavAction::Next); + assert_eq!(next_entity, Ok(tab_entity_2)); - let next_entity = - tab_navigation.navigate(&InputFocus::from_entity(tab_entity_1), NavAction::Next); - assert_eq!(next_entity, Ok(tab_entity_2)); + let prev_entity = tab_navigation + .navigate(&InputFocus::from_entity(tab_entity_2), NavAction::Previous); + assert_eq!(prev_entity, Ok(tab_entity_1)); - let prev_entity = - tab_navigation.navigate(&InputFocus::from_entity(tab_entity_2), NavAction::Previous); - assert_eq!(prev_entity, Ok(tab_entity_1)); + let first_entity = tab_navigation.navigate(&InputFocus::default(), NavAction::First); + assert_eq!(first_entity, Ok(tab_entity_1)); - let first_entity = tab_navigation.navigate(&InputFocus::default(), NavAction::First); - assert_eq!(first_entity, Ok(tab_entity_1)); + let last_entity = tab_navigation.navigate(&InputFocus::default(), NavAction::Last); + assert_eq!(last_entity, Ok(tab_entity_2)); + }; - let last_entity = tab_navigation.navigate(&InputFocus::default(), NavAction::Last); - assert_eq!(last_entity, Ok(tab_entity_2)); + world.run_system_cached(system); } } From b7726282001fc063034f815b38b4e33733b47dc8 Mon Sep 17 00:00:00 2001 From: notmd Date: Tue, 6 May 2025 21:57:54 +0700 Subject: [PATCH 15/16] wip --- crates/bevy_input_focus/src/tab_navigation.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/bevy_input_focus/src/tab_navigation.rs b/crates/bevy_input_focus/src/tab_navigation.rs index 226c6d50f7815..9811311e85c2e 100644 --- a/crates/bevy_input_focus/src/tab_navigation.rs +++ b/crates/bevy_input_focus/src/tab_navigation.rs @@ -395,6 +395,6 @@ mod tests { assert_eq!(last_entity, Ok(tab_entity_2)); }; - world.run_system_cached(system); + let _ = world.run_system_cached(system); } } From cae1b7078d52760cf2ce2518f56f697f8376b2b5 Mon Sep 17 00:00:00 2001 From: notmd Date: Wed, 7 May 2025 21:05:54 +0700 Subject: [PATCH 16/16] Cache `ComponentId` in `Query` state --- crates/bevy_ecs/src/system/builder.rs | 5 +++-- crates/bevy_ecs/src/system/system_param.rs | 25 +++++++++++++--------- 2 files changed, 18 insertions(+), 12 deletions(-) diff --git a/crates/bevy_ecs/src/system/builder.rs b/crates/bevy_ecs/src/system/builder.rs index 880183cf5c3c9..ca20345bf217c 100644 --- a/crates/bevy_ecs/src/system/builder.rs +++ b/crates/bevy_ecs/src/system/builder.rs @@ -3,6 +3,7 @@ use bevy_utils::synccell::SyncCell; use variadics_please::all_tuples; use crate::{ + component::ComponentId, entity::Entity, prelude::QueryBuilder, query::{QueryData, QueryFilter, QueryState}, @@ -212,7 +213,7 @@ impl ParamBuilder { unsafe impl<'w, 's, D: QueryData + 'static, F: QueryFilter + 'static> SystemParamBuilder> for QueryState { - fn build(self, world: &mut World, system_meta: &mut SystemMeta) -> Entity { + fn build(self, world: &mut World, system_meta: &mut SystemMeta) -> (Entity, ComponentId) { self.validate_world(world.id()); init_query_param(world, system_meta, Some(self)) } @@ -290,7 +291,7 @@ unsafe impl< T: FnOnce(&mut QueryBuilder), > SystemParamBuilder> for QueryParamBuilder { - fn build(self, world: &mut World, system_meta: &mut SystemMeta) -> Entity { + fn build(self, world: &mut World, system_meta: &mut SystemMeta) -> (Entity, ComponentId) { let mut builder = QueryBuilder::new(world); (self.0)(&mut builder); let state = builder.build(); diff --git a/crates/bevy_ecs/src/system/system_param.rs b/crates/bevy_ecs/src/system/system_param.rs index 0c747d35c767d..017d7bb08c6d4 100644 --- a/crates/bevy_ecs/src/system/system_param.rs +++ b/crates/bevy_ecs/src/system/system_param.rs @@ -339,7 +339,7 @@ unsafe impl<'w, 's, D: ReadOnlyQueryData + 'static, F: QueryFilter + 'static> Re // SAFETY: Relevant query ComponentId and ArchetypeComponentId access is applied to SystemMeta. If // this Query conflicts with any prior access, a panic will occur. unsafe impl SystemParam for Query<'_, '_, D, F> { - type State = Entity; + type State = (Entity, ComponentId); type Item<'w, 's> = Query<'w, 'w, D, F>; fn init_state(world: &mut World, system_meta: &mut SystemMeta) -> Self::State { @@ -388,7 +388,8 @@ pub(crate) fn init_query_param world: &mut World, system_meta: &mut SystemMeta, state: Option>, -) -> Entity { +) -> (Entity, ComponentId) { + let component_id = world.register_component::>(); let state: QueryState = state.unwrap_or_else(|| { QueryState::new_with_access(world, &mut system_meta.archetype_component_access) }); @@ -407,7 +408,9 @@ pub(crate) fn init_query_param .component_access_set .add(state.component_access.clone()); - world.spawn(QueryStateWrapper::new(state)).id() + let entity = world.spawn(QueryStateWrapper::new(state)).id(); + + (entity, component_id) } fn assert_component_access_compatibility( @@ -430,8 +433,9 @@ fn assert_component_access_compatibility( panic!("error[B0001]: Query<{}, {}> in system {system_name} accesses component(s) {accesses}in a way that conflicts with a previous system parameter. Consider using `Without` to create disjoint Queries or merging conflicting Queries into a `ParamSet`. See: https://bevyengine.org/learn/errors/b0001", ShortName(query_type), ShortName(filter_type)); } -fn get_query_state<'w, D: QueryData + 'static, F: QueryFilter + 'static>( - entity: Entity, +/// Safety: Caller must ensure componet id is `QueryStateWrapper` +unsafe fn get_query_state<'w, D: QueryData + 'static, F: QueryFilter + 'static>( + (entity, component_id): (Entity, ComponentId), world: UnsafeWorldCell<'w>, ) -> Option<&'w QueryState> { // SAFETY: QueryStateWrapper is immutable so no mutable access is possible @@ -439,16 +443,17 @@ fn get_query_state<'w, D: QueryData + 'static, F: QueryFilter + 'static>( world .get_entity(entity) .ok()? - .get::>() + .get_by_id(component_id)? + .deref::>() }; - state.map(QueryStateWrapper::inner) + Some(state.inner()) } // SAFETY: Relevant query ComponentId and ArchetypeComponentId access is applied to SystemMeta. If // this Query conflicts with any prior access, a panic will occur. unsafe impl<'a, D: QueryData + 'static, F: QueryFilter + 'static> SystemParam for Single<'a, D, F> { - type State = Entity; + type State = (Entity, ComponentId); type Item<'w, 's> = Single<'w, D, F>; fn init_state(world: &mut World, system_meta: &mut SystemMeta) -> Self::State { @@ -520,7 +525,7 @@ unsafe impl<'a, D: QueryData + 'static, F: QueryFilter + 'static> SystemParam fo unsafe impl<'a, D: QueryData + 'static, F: QueryFilter + 'static> SystemParam for Option> { - type State = Entity; + type State = (Entity, ComponentId); type Item<'w, 's> = Option>; fn init_state(world: &mut World, system_meta: &mut SystemMeta) -> Self::State { @@ -603,7 +608,7 @@ unsafe impl<'a, D: ReadOnlyQueryData + 'static, F: QueryFilter + 'static> ReadOn unsafe impl SystemParam for Populated<'_, '_, D, F> { - type State = Entity; + type State = (Entity, ComponentId); type Item<'w, 's> = Populated<'w, 'w, D, F>; fn init_state(world: &mut World, system_meta: &mut SystemMeta) -> Self::State {