From a034dae26b3a5a9c963e3c0d8c4b686dbd922ba9 Mon Sep 17 00:00:00 2001 From: Aceeri Date: Sat, 13 Aug 2022 16:48:58 -0700 Subject: [PATCH] Not working yet, replace with an events for removed components Refactor to use a resource of component ids to events Removed components as an iterator over events Clean up unnecessary code in event, update events on clear_trackers Correct documentation Clarify documentation Fix test to expect despawned entity as well Fix some CI and docs Fix links in docs Rename to match previous type names Rename in docs/tests Unused import Fix removal detection example Fix example docs Fix up rebase issue, make panic message more in line with other ones Fix docs for RemovedComponents --- crates/bevy_ecs/src/lib.rs | 96 +-------- crates/bevy_ecs/src/removal_detection.rs | 217 +++++++++++++++++++++ crates/bevy_ecs/src/system/mod.rs | 10 +- crates/bevy_ecs/src/system/system_param.rs | 3 + crates/bevy_ecs/src/world/entity_ref.rs | 30 +-- crates/bevy_ecs/src/world/mod.rs | 39 +--- examples/ecs/removal_detection.rs | 7 +- 7 files changed, 256 insertions(+), 146 deletions(-) create mode 100644 crates/bevy_ecs/src/removal_detection.rs diff --git a/crates/bevy_ecs/src/lib.rs b/crates/bevy_ecs/src/lib.rs index 46f1a8dc4281c6..8a61bb061baa27 100644 --- a/crates/bevy_ecs/src/lib.rs +++ b/crates/bevy_ecs/src/lib.rs @@ -13,6 +13,7 @@ pub mod event; pub mod query; #[cfg(feature = "bevy_reflect")] pub mod reflect; +pub mod removal_detection; pub mod schedule; pub mod storage; pub mod system; @@ -33,6 +34,7 @@ pub mod prelude { entity::Entity, event::{EventReader, EventWriter, Events}, query::{Added, AnyOf, ChangeTrackers, Changed, Or, QueryState, With, Without}, + removal_detection::{RemovedComponentEvents, RemovedComponents}, schedule::{ IntoSystemDescriptor, RunCriteria, RunCriteriaDescriptorCoercion, RunCriteriaLabel, Schedule, Stage, StageLabel, State, SystemLabel, SystemSet, SystemStage, @@ -708,100 +710,6 @@ mod tests { assert!(i32_query.get(&world, a).is_err()); } - #[test] - fn query_get_works_across_sparse_removal() { - // Regression test for: https://github.com/bevyengine/bevy/issues/6623 - let mut world = World::new(); - let a = world.spawn((TableStored("abc"), SparseStored(123))).id(); - let b = world.spawn((TableStored("def"), SparseStored(456))).id(); - let c = world - .spawn((TableStored("ghi"), SparseStored(789), B(1))) - .id(); - - let mut query = world.query::<&TableStored>(); - assert_eq!(query.get(&world, a).unwrap(), &TableStored("abc")); - assert_eq!(query.get(&world, b).unwrap(), &TableStored("def")); - assert_eq!(query.get(&world, c).unwrap(), &TableStored("ghi")); - - world.entity_mut(b).remove::(); - world.entity_mut(c).remove::(); - - assert_eq!(query.get(&world, a).unwrap(), &TableStored("abc")); - assert_eq!(query.get(&world, b).unwrap(), &TableStored("def")); - assert_eq!(query.get(&world, c).unwrap(), &TableStored("ghi")); - } - - #[test] - fn remove_tracking() { - let mut world = World::new(); - - let a = world.spawn((SparseStored(0), A(123))).id(); - let b = world.spawn((SparseStored(1), A(123))).id(); - - world.entity_mut(a).despawn(); - assert_eq!( - world.removed::().collect::>(), - &[a], - "despawning results in 'removed component' state for table components" - ); - assert_eq!( - world.removed::().collect::>(), - &[a], - "despawning results in 'removed component' state for sparse set components" - ); - - world.entity_mut(b).insert(B(1)); - assert_eq!( - world.removed::().collect::>(), - &[a], - "archetype moves does not result in 'removed component' state" - ); - - world.entity_mut(b).remove::(); - assert_eq!( - world.removed::().collect::>(), - &[a, b], - "removing a component results in a 'removed component' state" - ); - - world.clear_trackers(); - assert_eq!( - world.removed::().collect::>(), - &[], - "clearning trackers clears removals" - ); - assert_eq!( - world.removed::().collect::>(), - &[], - "clearning trackers clears removals" - ); - assert_eq!( - world.removed::().collect::>(), - &[], - "clearning trackers clears removals" - ); - - // TODO: uncomment when world.clear() is implemented - // let c = world.spawn(("abc", 123)).id(); - // let d = world.spawn(("abc", 123)).id(); - // world.clear(); - // assert_eq!( - // world.removed::(), - // &[c, d], - // "world clears result in 'removed component' states" - // ); - // assert_eq!( - // world.removed::<&'static str>(), - // &[c, d, b], - // "world clears result in 'removed component' states" - // ); - // assert_eq!( - // world.removed::(), - // &[b], - // "world clears result in 'removed component' states" - // ); - } - #[test] fn added_tracking() { let mut world = World::new(); diff --git a/crates/bevy_ecs/src/removal_detection.rs b/crates/bevy_ecs/src/removal_detection.rs new file mode 100644 index 00000000000000..01e7995049124f --- /dev/null +++ b/crates/bevy_ecs/src/removal_detection.rs @@ -0,0 +1,217 @@ +//! Removal event types. + +use crate as bevy_ecs; +use crate::component::{Component, ComponentId, Components}; +use crate::entity::Entity; +use crate::event::{EventId, Events, ManualEventReader}; +use crate::storage::SparseSet; +use crate::system::{Local, Res, Resource, SystemParam}; +use std::marker::PhantomData; + +/// A [`SystemParam`] that grants access to the entities that had their `T` [`Component`] removed. +/// +/// Note that this does not allow you to see which data existed before removal. +/// If you need this, you will need to track the component data value on your own, +/// using a regularly scheduled system that requests `Query<(Entity, &T), Changed>` +/// and stores the data somewhere safe to later cross-reference. +/// +/// If you are using `bevy_ecs` as a standalone crate, +/// note that the [`RemovedComponentEvents`] events will not be automatically updated/cleared for you, +/// and will need to be manually flushed using [`crate::world::World::clear_trackers`] or [`RemovedComponentEvents::update`] +/// +/// For users of `bevy` itself, this is automatically done in a system added by `MinimalPlugins` +/// or `DefaultPlugins` at the end of each pass of the game loop during the `CoreStage::Last` +/// stage. +/// +/// # Examples +/// +/// Basic usage: +/// +/// ``` +/// # use bevy_ecs::component::Component; +/// # use bevy_ecs::system::IntoSystem; +/// # use bevy_ecs::removal_detection::RemovedComponents; +/// # +/// # #[derive(Component)] +/// # struct MyComponent; +/// +/// fn react_on_removal(mut removed: RemovedComponents) { +/// removed.iter().for_each(|removed_entity| println!("{:?}", removed_entity)); +/// } +/// +/// # bevy_ecs::system::assert_is_system(react_on_removal); +/// ``` +#[derive(SystemParam)] +pub struct RemovedComponents<'w, 's, T: Component> { + components: &'w Components, + events: Res<'w, RemovedComponentEvents>, + readers: Local<'s, RemovedComponentReaders>, + #[system_param(ignore)] + phantom: PhantomData<&'s T>, +} + +impl<'w, 's, T: Component> RemovedComponents<'w, 's, T> { + pub fn component_id(&self) -> ComponentId { + self.components + .component_id::() + .expect("component does not exist in the world") + } + + pub fn iter(&mut self) -> impl Iterator + '_ { + self.iter_with_id().map(|(e, _)| e) + } + + pub fn iter_with_id(&mut self) -> impl Iterator)> + '_ { + let component_id = self.component_id(); + let events = self.events.get(component_id); + let reader = self.readers.get_mut(component_id); + events + .map(|events| reader.iter_with_id(events)) + .into_iter() + .flatten() + .map(|(e, id)| (*e, id)) + } + + pub fn len(&self) -> usize { + let component_id = self.component_id(); + let events = self.events.get(component_id); + let reader = self.readers.get(component_id); + match (reader, events) { + (Some(reader), Some(events)) => reader.len(events), + (None, Some(events)) => events.len(), + _ => 0, + } + } + + pub fn is_empty(&self) -> bool { + self.len() == 0 + } +} + +#[derive(Default, Debug, Resource)] +pub struct RemovedComponentEvents { + events: SparseSet>, +} + +impl RemovedComponentEvents { + pub fn get(&self, component_id: ComponentId) -> Option<&Events> { + self.events.get(component_id) + } + + pub fn get_mut(&mut self, component_id: ComponentId) -> &mut Events { + self.events + .get_or_insert_with(component_id, Default::default) + } + + pub fn extend( + &mut self, + component_id: ComponentId, + entities: impl IntoIterator, + ) { + self.get_mut(component_id).extend(entities); + } + + pub fn push(&mut self, component_id: ComponentId, entity: Entity) { + self.get_mut(component_id).send(entity); + } + + pub fn update(&mut self) { + for events in self.events.values_mut() { + events.update(); + } + } +} + +#[derive(Default, Debug, Resource)] +pub struct RemovedComponentReaders { + readers: SparseSet>, +} + +impl RemovedComponentReaders { + pub fn get(&self, component_id: ComponentId) -> Option<&ManualEventReader> { + self.readers.get(component_id) + } + + pub fn get_mut(&mut self, component_id: ComponentId) -> &mut ManualEventReader { + self.readers + .get_or_insert_with(component_id, Default::default) + } +} + +#[cfg(test)] +mod tests { + use crate as bevy_ecs; + use crate::{ + component::Component, + removal_detection::RemovedComponents, + system::{Resource, SystemState}, + world::World, + }; + + #[derive(Component, Resource, Debug, PartialEq, Eq, Clone, Copy)] + struct A(usize); + #[derive(Component, Debug, PartialEq, Eq, Clone, Copy)] + struct B(usize); + #[derive(Component, Debug, PartialEq, Eq, Clone, Copy)] + struct C; + + #[derive(Component, Copy, Clone, PartialEq, Eq, Debug)] + #[component(storage = "Table")] + struct TableStored(&'static str); + #[derive(Component, Copy, Clone, PartialEq, Eq, Debug)] + #[component(storage = "SparseSet")] + struct SparseStored(u32); + + #[test] + fn remove_tracking() { + let mut world = World::new(); + + let a = world.spawn().insert_bundle((SparseStored(0), A(123))).id(); + let b = world.spawn().insert_bundle((SparseStored(1),)).id(); + let c = world + .spawn() + .insert_bundle((SparseStored(0), A(123), B(1), C)) + .id(); + + let mut a_system: SystemState> = SystemState::new(&mut world); + let mut c_system: SystemState> = SystemState::new(&mut world); + let mut sparse_system: SystemState> = + SystemState::new(&mut world); + world.entity_mut(a).despawn(); + world.entity_mut(b).despawn(); + assert_eq!( + a_system.get_mut(&mut world).iter().collect::>(), + &[a], + "despawning results in 'removed component' state for table components" + ); + assert_eq!( + sparse_system.get_mut(&mut world).iter().collect::>(), + &[a, b], + "despawning results in 'removed component' state for sparse set components" + ); + + // When we check again the event iterator should be ahead now. + assert_eq!( + a_system.get_mut(&mut world).iter().collect::>(), + &[], + "reading from the same system state will result in no new removal events" + ); + assert_eq!( + sparse_system.get_mut(&mut world).iter().collect::>(), + &[], + "reading from the same system state will result in no new removal events" + ); + + world.entity_mut(c).remove::(); + assert_eq!( + c_system.get_mut(&mut world).iter().collect::>(), + &[c], + "removing specific component should result in removal event" + ); + assert_eq!( + c_system.get_mut(&mut world).iter().collect::>(), + &[], + "reading from same system state will result in no new removal events" + ); + } +} diff --git a/crates/bevy_ecs/src/system/mod.rs b/crates/bevy_ecs/src/system/mod.rs index 0dcc806c6d44ab..1810b52dc92323 100644 --- a/crates/bevy_ecs/src/system/mod.rs +++ b/crates/bevy_ecs/src/system/mod.rs @@ -58,8 +58,8 @@ //! - [`NonSend`] and `Option` //! - [`NonSendMut`] and `Option` //! - [`&World`](crate::world::World) -//! - [`RemovedComponents`] //! - [`SystemName`] +//! - [`RemovedComponents`](crate::removal_detection::RemovedComponents) //! - [`SystemChangeTick`] //! - [`Archetypes`](crate::archetype::Archetypes) (Provides Archetype metadata) //! - [`Bundles`](crate::bundle::Bundles) (Provides Bundles metadata) @@ -114,6 +114,7 @@ mod tests { entity::{Entities, Entity}, prelude::AnyOf, query::{Added, Changed, Or, With, Without}, + removal_detection::RemovedComponents, schedule::{Schedule, Stage, SystemStage}, system::{ Commands, IntoSystem, Local, NonSend, NonSendMut, ParamSet, Query, QueryComponentError, @@ -577,7 +578,7 @@ mod tests { world.entity_mut(spurious_entity).despawn(); fn validate_despawn( - removed_i32: RemovedComponents>, + mut removed_i32: RemovedComponents>, despawned: Res, mut n_systems: ResMut, ) { @@ -602,13 +603,14 @@ mod tests { world.entity_mut(entity_to_remove_w_from).remove::>(); fn validate_remove( - removed_i32: RemovedComponents>, + mut removed_i32: RemovedComponents>, + despawned: Res, removed: Res, mut n_systems: ResMut, ) { assert_eq!( removed_i32.iter().collect::>(), - &[removed.0], + &[despawned.0, removed.0], "removing a component causes the correct entity to show up in the 'RemovedComponent' system parameter." ); diff --git a/crates/bevy_ecs/src/system/system_param.rs b/crates/bevy_ecs/src/system/system_param.rs index 7099029fcc0c04..8d79b76b3bd0b8 100644 --- a/crates/bevy_ecs/src/system/system_param.rs +++ b/crates/bevy_ecs/src/system/system_param.rs @@ -810,6 +810,7 @@ unsafe impl SystemParamState for LocalState { } } +<<<<<<< HEAD /// A [`SystemParam`] that grants access to the entities that had their `T` [`Component`] removed. /// /// Note that this does not allow you to see which data existed before removal. @@ -906,6 +907,8 @@ unsafe impl SystemParamState for RemovedComponentsState { } } +======= +>>>>>>> 0a7b6378e (Not working yet, replace with an events for removed components) /// Shared borrow of a non-[`Send`] resource. /// /// Only `Send` resources may be accessed with the [`Res`] [`SystemParam`]. In case that the diff --git a/crates/bevy_ecs/src/world/entity_ref.rs b/crates/bevy_ecs/src/world/entity_ref.rs index 458564beb4cfe3..b1e039a545d427 100644 --- a/crates/bevy_ecs/src/world/entity_ref.rs +++ b/crates/bevy_ecs/src/world/entity_ref.rs @@ -387,7 +387,6 @@ impl<'w> EntityMut<'w> { let storages = &mut self.world.storages; let components = &mut self.world.components; let entities = &mut self.world.entities; - let removed_components = &mut self.world.removed_components; let bundle_info = self.world.bundles.init_info::(components, storages); let old_location = self.location; @@ -416,12 +415,12 @@ impl<'w> EntityMut<'w> { let result = unsafe { T::from_components(storages, &mut |storages| { let component_id = bundle_components.next().unwrap(); + to_remove.push((entity, component_id)); // SAFETY: entity location is valid and table row is removed below take_component( components, storages, old_archetype, - removed_components, component_id, entity, old_location, @@ -443,6 +442,11 @@ impl<'w> EntityMut<'w> { ); } + for (entity, component_id) in to_remove { + self.world + .remove_component_from_entity(entity, component_id); + } + Some(result) } @@ -513,7 +517,6 @@ impl<'w> EntityMut<'w> { let storages = &mut self.world.storages; let components = &mut self.world.components; let entities = &mut self.world.entities; - let removed_components = &mut self.world.removed_components; let bundle_info = self.world.bundles.init_info::(components, storages); let old_location = self.location; @@ -540,9 +543,7 @@ impl<'w> EntityMut<'w> { let entity = self.entity; for component_id in bundle_info.component_ids.iter().cloned() { if old_archetype.contains(component_id) { - removed_components - .get_or_insert_with(component_id, Vec::new) - .push(entity); + to_remove.push((entity, component_id)); // Make sure to drop components stored in sparse sets. // Dense components are dropped later in `move_to_and_drop_missing_unchecked`. @@ -569,6 +570,11 @@ impl<'w> EntityMut<'w> { new_archetype_id, ); } + + for (entity, component_id) in to_remove { + self.world + .remove_component_from_entity(entity, component_id); + } } pub fn despawn(self) { @@ -584,10 +590,7 @@ impl<'w> EntityMut<'w> { { let archetype = &mut world.archetypes[location.archetype_id]; for component_id in archetype.components() { - let removed_components = world - .removed_components - .get_or_insert_with(component_id, Vec::new); - removed_components.push(self.entity); + to_remove.push((self.entity, component_id)); } let remove_result = archetype.swap_remove(location.archetype_row); if let Some(swapped_entity) = remove_result.swapped_entity { @@ -614,6 +617,10 @@ impl<'w> EntityMut<'w> { world.archetypes[moved_location.archetype_id] .set_entity_table_row(moved_location.archetype_row, table_row); } + + for (entity, component_id) in to_remove { + world.remove_component_from_entity(entity, component_id); + } } #[inline] @@ -811,14 +818,11 @@ unsafe fn take_component<'a>( components: &Components, storages: &'a mut Storages, archetype: &Archetype, - removed_components: &mut SparseSet>, component_id: ComponentId, entity: Entity, location: EntityLocation, ) -> OwningPtr<'a> { let component_info = components.get_info_unchecked(component_id); - let removed_components = removed_components.get_or_insert_with(component_id, Vec::new); - removed_components.push(entity); match component_info.storage_type() { StorageType::Table => { let table = &mut storages.tables[archetype.table_id()]; diff --git a/crates/bevy_ecs/src/world/mod.rs b/crates/bevy_ecs/src/world/mod.rs index 0e8009916715cb..935e5b7e03aeba 100644 --- a/crates/bevy_ecs/src/world/mod.rs +++ b/crates/bevy_ecs/src/world/mod.rs @@ -57,7 +57,6 @@ pub struct World { pub(crate) archetypes: Archetypes, pub(crate) storages: Storages, pub(crate) bundles: Bundles, - pub(crate) removed_components: SparseSet>, /// Access cache used by [WorldCell]. pub(crate) archetype_component_access: ArchetypeComponentAccess, main_thread_validator: MainThreadValidator, @@ -67,21 +66,23 @@ pub struct World { impl Default for World { fn default() -> Self { - Self { + let mut world = Self { id: WorldId::new().expect("More `bevy` `World`s have been created than is supported"), entities: Entities::new(), components: Default::default(), archetypes: Archetypes::new(), storages: Default::default(), bundles: Default::default(), - removed_components: Default::default(), archetype_component_access: Default::default(), main_thread_validator: Default::default(), // Default value is `1`, and `last_change_tick`s default to `0`, such that changes // are detected on first system runs and for direct world queries. change_tick: AtomicU32::new(1), last_change_tick: 0, - } + }; + + world.init_resource::(); + world } } @@ -636,9 +637,10 @@ impl World { /// /// [`RemovedComponents`]: crate::system::RemovedComponents pub fn clear_trackers(&mut self) { - for entities in self.removed_components.values_mut() { - entities.clear(); - } + let mut removals = self + .get_resource_mut::() + .expect("removed component events"); + removals.update(); self.last_change_tick = self.increment_change_tick(); } @@ -734,29 +736,6 @@ impl World { QueryState::new(self) } - /// Returns an iterator of entities that had components of type `T` removed - /// since the last call to [`World::clear_trackers`]. - pub fn removed(&self) -> std::iter::Cloned> { - if let Some(component_id) = self.components.get_id(TypeId::of::()) { - self.removed_with_id(component_id) - } else { - [].iter().cloned() - } - } - - /// Returns an iterator of entities that had components with the given `component_id` removed - /// since the last call to [`World::clear_trackers`]. - pub fn removed_with_id( - &self, - component_id: ComponentId, - ) -> std::iter::Cloned> { - if let Some(removed) = self.removed_components.get(component_id) { - removed.iter().cloned() - } else { - [].iter().cloned() - } - } - /// Inserts a new resource with standard starting values. /// /// If the resource already exists, nothing happens. diff --git a/examples/ecs/removal_detection.rs b/examples/ecs/removal_detection.rs index f931066a4a4ec3..86f3e1893090b4 100644 --- a/examples/ecs/removal_detection.rs +++ b/examples/ecs/removal_detection.rs @@ -3,10 +3,7 @@ use bevy::prelude::*; fn main() { - // Information regarding removed `Component`s is discarded at the end of each frame, so you need - // to react to the removal before the frame is over. - // - // Also, `Components` are removed via a `Command`. `Command`s are applied after a stage has + // `Components` are removed via a `Command`. `Command`s are applied after a stage has // finished executing. So you need to react to the removal at some stage after the // `Component` is removed. // @@ -51,7 +48,7 @@ fn remove_component( } } -fn react_on_removal(removed: RemovedComponents, mut query: Query<&mut Sprite>) { +fn react_on_removal(mut removed: RemovedComponents, mut query: Query<&mut Sprite>) { // `RemovedComponents::iter()` returns an interator with the `Entity`s that had their // `Component` `T` (in this case `MyComponent`) removed at some point earlier during the frame. for entity in removed.iter() {