From a9785ae87b7ba7c62ef13638f866f42bb8acfc0f Mon Sep 17 00:00:00 2001 From: rewin Date: Thu, 3 Oct 2024 23:35:08 +0300 Subject: [PATCH] Add method to remove component and all required components for removed component (#15026) ## Objective The new Required Components feature (#14791) in Bevy allows spawning a fixed set of components with a single method with cool require macro. However, there's currently no corresponding method to remove all those components together. This makes it challenging to keep insertion and removal code in sync, especially for simple using cases. ```rust #[derive(Component)] #[require(Y)] struct X; #[derive(Component, Default)] struct Y; world.entity_mut(e).insert(X); // Spawns both X and Y world.entity_mut(e).remove::(); world.entity_mut(e).remove::(); // We need to manually remove dependencies without any sync with the `require` macro ``` ## Solution Simplifies component management by providing operations for removal required components. This PR introduces simple 'footgun' methods to removes all components of this bundle and its required components. Two new methods are introduced: For Commands: ```rust commands.entity(e).remove_with_requires::(); ``` For World: ```rust world.entity_mut(e).remove_with_requires::(); ``` For performance I created new field in Bundels struct. This new field "contributed_bundle_ids" contains cached ids for dynamic bundles constructed from bundle_info.cintributed_components() ## Testing The PR includes three test cases: 1. Removing a single component with requirements using World. 2. Removing a bundle with requirements using World. 3. Removing a single component with requirements using Commands. 4. Removing a single component with **runtime** requirements using Commands These tests ensure the feature works as expected across different scenarios. ## Showcase Example: ```rust use bevy_ecs::prelude::*; #[derive(Component)] #[require(Y)] struct X; #[derive(Component, Default)] #[require(Z)] struct Y; #[derive(Component, Default)] struct Z; #[derive(Component)] struct W; let mut world = World::new(); // Spawn an entity with X, Y, Z, and W components let entity = world.spawn((X, W)).id(); assert!(world.entity(entity).contains::()); assert!(world.entity(entity).contains::()); assert!(world.entity(entity).contains::()); assert!(world.entity(entity).contains::()); // Remove X and required components Y, Z world.entity_mut(entity).remove_with_requires::(); assert!(!world.entity(entity).contains::()); assert!(!world.entity(entity).contains::()); assert!(!world.entity(entity).contains::()); assert!(world.entity(entity).contains::()); ``` ## Motivation for PR #15580 ## Performance I made simple benchmark ```rust let mut world = World::default(); let entity = world.spawn_empty().id(); let steps = 100_000_000; let start = std::time::Instant::now(); for _ in 0..steps { world.entity_mut(entity).insert(X); world.entity_mut(entity).remove::<(X, Y, Z, W)>(); } let end = std::time::Instant::now(); println!("normal remove: {:?} ", (end - start).as_secs_f32()); println!("one remove: {:?} micros", (end - start).as_secs_f64() / steps as f64 * 1_000_000.0); let start = std::time::Instant::now(); for _ in 0..steps { world.entity_mut(entity).insert(X); world.entity_mut(entity).remove_with_requires::(); } let end = std::time::Instant::now(); println!("remove_with_requires: {:?} ", (end - start).as_secs_f32()); println!("one remove_with_requires: {:?} micros", (end - start).as_secs_f64() / steps as f64 * 1_000_000.0); ``` Output: CPU: Amd Ryzen 7 2700x ```bash normal remove: 17.36135 one remove: 0.17361348299999999 micros remove_with_requires: 17.534006 one remove_with_requires: 0.17534005400000002 micros ``` NOTE: I didn't find any tests or mechanism in the repository to update BundleInfo after creating new runtime requirements with an existing BundleInfo. So this PR also does not contain such logic. ## Future work (outside this PR) Create cache system for fast removing components in "safe" mode, where "safe" mode is remove only required components that will be no longer required after removing root component. --------- Co-authored-by: a.yamaev Co-authored-by: Carter Anderson --- crates/bevy_ecs/src/bundle.rs | 32 +++++ crates/bevy_ecs/src/lib.rs | 132 +++++++++++++++++++++ crates/bevy_ecs/src/system/commands/mod.rs | 71 +++++++++++ crates/bevy_ecs/src/world/entity_ref.rs | 14 +++ 4 files changed, 249 insertions(+) diff --git a/crates/bevy_ecs/src/bundle.rs b/crates/bevy_ecs/src/bundle.rs index 446eb30921225b..56e11e854e13e9 100644 --- a/crates/bevy_ecs/src/bundle.rs +++ b/crates/bevy_ecs/src/bundle.rs @@ -1302,6 +1302,8 @@ pub struct Bundles { bundle_infos: Vec, /// Cache static [`BundleId`] bundle_ids: TypeIdMap, + /// Cache bundles, which contains both explicit and required components of [`Bundle`] + contributed_bundle_ids: TypeIdMap, /// Cache dynamic [`BundleId`] with multiple components dynamic_bundle_ids: HashMap, BundleId>, dynamic_bundle_storages: HashMap>, @@ -1351,6 +1353,36 @@ impl Bundles { id } + /// Registers a new [`BundleInfo`], which contains both explicit and required components for a statically known type. + /// + /// Also registers all the components in the bundle. + pub(crate) fn register_contributed_bundle_info( + &mut self, + components: &mut Components, + storages: &mut Storages, + ) -> BundleId { + if let Some(id) = self.contributed_bundle_ids.get(&TypeId::of::()).cloned() { + id + } else { + let explicit_bundle_id = self.register_info::(components, storages); + // SAFETY: reading from `explicit_bundle_id` and creating new bundle in same time. Its valid because bundle hashmap allow this + let id = unsafe { + let (ptr, len) = { + // SAFETY: `explicit_bundle_id` is valid and defined above + let contributed = self + .get_unchecked(explicit_bundle_id) + .contributed_components(); + (contributed.as_ptr(), contributed.len()) + }; + // SAFETY: this is sound because the contributed_components Vec for explicit_bundle_id will not be accessed mutably as + // part of init_dynamic_info. No mutable references will be created and the allocation will remain valid. + self.init_dynamic_info(components, core::slice::from_raw_parts(ptr, len)) + }; + self.contributed_bundle_ids.insert(TypeId::of::(), id); + id + } + } + /// # Safety /// A [`BundleInfo`] with the given [`BundleId`] must have been initialized for this instance of `Bundles`. pub(crate) unsafe fn get_unchecked(&self, id: BundleId) -> &BundleInfo { diff --git a/crates/bevy_ecs/src/lib.rs b/crates/bevy_ecs/src/lib.rs index 69cbcfdafb986c..a62d23afb69cdc 100644 --- a/crates/bevy_ecs/src/lib.rs +++ b/crates/bevy_ecs/src/lib.rs @@ -2029,6 +2029,138 @@ mod tests { assert!(e.contains::()); } + #[test] + fn remove_component_and_his_runtime_required_components() { + #[derive(Component)] + struct X; + + #[derive(Component, Default)] + struct Y; + + #[derive(Component, Default)] + struct Z; + + #[derive(Component)] + struct V; + + let mut world = World::new(); + world.register_required_components::(); + world.register_required_components::(); + + let e = world.spawn((X, V)).id(); + assert!(world.entity(e).contains::()); + assert!(world.entity(e).contains::()); + assert!(world.entity(e).contains::()); + assert!(world.entity(e).contains::()); + + //check that `remove` works as expected + world.entity_mut(e).remove::(); + assert!(!world.entity(e).contains::()); + assert!(world.entity(e).contains::()); + assert!(world.entity(e).contains::()); + assert!(world.entity(e).contains::()); + + world.entity_mut(e).insert(X); + assert!(world.entity(e).contains::()); + assert!(world.entity(e).contains::()); + assert!(world.entity(e).contains::()); + assert!(world.entity(e).contains::()); + + //remove `X` again and ensure that `Y` and `Z` was removed too + world.entity_mut(e).remove_with_requires::(); + assert!(!world.entity(e).contains::()); + assert!(!world.entity(e).contains::()); + assert!(!world.entity(e).contains::()); + assert!(world.entity(e).contains::()); + } + + #[test] + fn remove_component_and_his_required_components() { + #[derive(Component)] + #[require(Y)] + struct X; + + #[derive(Component, Default)] + #[require(Z)] + struct Y; + + #[derive(Component, Default)] + struct Z; + + #[derive(Component)] + struct V; + + let mut world = World::new(); + + let e = world.spawn((X, V)).id(); + assert!(world.entity(e).contains::()); + assert!(world.entity(e).contains::()); + assert!(world.entity(e).contains::()); + assert!(world.entity(e).contains::()); + + //check that `remove` works as expected + world.entity_mut(e).remove::(); + assert!(!world.entity(e).contains::()); + assert!(world.entity(e).contains::()); + assert!(world.entity(e).contains::()); + assert!(world.entity(e).contains::()); + + world.entity_mut(e).insert(X); + assert!(world.entity(e).contains::()); + assert!(world.entity(e).contains::()); + assert!(world.entity(e).contains::()); + assert!(world.entity(e).contains::()); + + //remove `X` again and ensure that `Y` and `Z` was removed too + world.entity_mut(e).remove_with_requires::(); + assert!(!world.entity(e).contains::()); + assert!(!world.entity(e).contains::()); + assert!(!world.entity(e).contains::()); + assert!(world.entity(e).contains::()); + } + + #[test] + fn remove_bundle_and_his_required_components() { + #[derive(Component, Default)] + #[require(Y)] + struct X; + + #[derive(Component, Default)] + struct Y; + + #[derive(Component, Default)] + #[require(W)] + struct Z; + + #[derive(Component, Default)] + struct W; + + #[derive(Component)] + struct V; + + #[derive(Bundle, Default)] + struct TestBundle { + x: X, + z: Z, + } + + let mut world = World::new(); + let e = world.spawn((TestBundle::default(), V)).id(); + + assert!(world.entity(e).contains::()); + assert!(world.entity(e).contains::()); + assert!(world.entity(e).contains::()); + assert!(world.entity(e).contains::()); + assert!(world.entity(e).contains::()); + + world.entity_mut(e).remove_with_requires::(); + assert!(!world.entity(e).contains::()); + assert!(!world.entity(e).contains::()); + assert!(!world.entity(e).contains::()); + assert!(!world.entity(e).contains::()); + assert!(world.entity(e).contains::()); + } + #[test] fn runtime_required_components() { // Same as `required_components` test but with runtime registration diff --git a/crates/bevy_ecs/src/system/commands/mod.rs b/crates/bevy_ecs/src/system/commands/mod.rs index fb7c0485abf93c..fb4f1a31189b9b 100644 --- a/crates/bevy_ecs/src/system/commands/mod.rs +++ b/crates/bevy_ecs/src/system/commands/mod.rs @@ -1369,6 +1369,34 @@ impl EntityCommands<'_> { self.queue(remove::) } + /// Removes all components in the [`Bundle`] components and remove all required components for each component in the [`Bundle`] from entity. + /// + /// # Example + /// + /// ``` + /// use bevy_ecs::prelude::*; + /// + /// #[derive(Component)] + /// #[require(B)] + /// struct A; + /// #[derive(Component, Default)] + /// struct B; + /// + /// #[derive(Resource)] + /// struct PlayerEntity { entity: Entity } + /// + /// fn remove_with_requires_system(mut commands: Commands, player: Res) { + /// commands + /// .entity(player.entity) + /// // Remove both A and B components from the entity, because B is required by A + /// .remove_with_requires::(); + /// } + /// # bevy_ecs::system::assert_is_system(remove_with_requires_system); + /// ``` + pub fn remove_with_requires(&mut self) -> &mut Self { + self.queue(remove_with_requires::) + } + /// Removes a component from the entity. pub fn remove_by_id(&mut self, component_id: ComponentId) -> &mut Self { self.queue(remove_by_id(component_id)) @@ -1826,6 +1854,13 @@ fn remove_by_id(component_id: ComponentId) -> impl EntityCommand { } } +/// An [`EntityCommand`] that remove all components in the bundle and remove all required components for each component in the bundle. +fn remove_with_requires(entity: Entity, world: &mut World) { + if let Some(mut entity) = world.get_entity_mut(entity) { + entity.remove_with_requires::(); + } +} + /// An [`EntityCommand`] that removes all components associated with a provided entity. fn clear() -> impl EntityCommand { move |entity: Entity, world: &mut World| { @@ -2190,6 +2225,42 @@ mod tests { assert!(world.contains_resource::>()); } + #[test] + fn remove_component_with_required_components() { + #[derive(Component)] + #[require(Y)] + struct X; + + #[derive(Component, Default)] + struct Y; + + #[derive(Component)] + struct Z; + + let mut world = World::default(); + let mut queue = CommandQueue::default(); + let e = { + let mut commands = Commands::new(&mut queue, &world); + commands.spawn((X, Z)).id() + }; + queue.apply(&mut world); + + assert!(world.get::(e).is_some()); + assert!(world.get::(e).is_some()); + assert!(world.get::(e).is_some()); + + { + let mut commands = Commands::new(&mut queue, &world); + commands.entity(e).remove_with_requires::(); + } + queue.apply(&mut world); + + assert!(world.get::(e).is_none()); + assert!(world.get::(e).is_none()); + + assert!(world.get::(e).is_some()); + } + fn is_send() {} fn is_sync() {} diff --git a/crates/bevy_ecs/src/world/entity_ref.rs b/crates/bevy_ecs/src/world/entity_ref.rs index fbc58a91048f4c..57d45366c6940f 100644 --- a/crates/bevy_ecs/src/world/entity_ref.rs +++ b/crates/bevy_ecs/src/world/entity_ref.rs @@ -1558,6 +1558,20 @@ impl<'w> EntityWorldMut<'w> { self } + /// Removes all components in the [`Bundle`] and remove all required components for each component in the bundle + pub fn remove_with_requires(&mut self) -> &mut Self { + let storages = &mut self.world.storages; + let components = &mut self.world.components; + let bundles = &mut self.world.bundles; + + let bundle_id = bundles.register_contributed_bundle_info::(components, storages); + + // SAFETY: the dynamic `BundleInfo` is initialized above + self.location = unsafe { self.remove_bundle(bundle_id) }; + + self + } + /// Removes any components except those in the [`Bundle`] (and its Required Components) from the entity. /// /// See [`EntityCommands::retain`](crate::system::EntityCommands::retain) for more details.