From caab2dd480247faadf746e197b2b6c5e360a96a5 Mon Sep 17 00:00:00 2001 From: rudderbucky Date: Thu, 3 Oct 2024 09:21:05 -0700 Subject: [PATCH] Add try_despawn methods to World/Commands (#15480) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit # Objective Fixes #14511. `despawn` allows you to remove entities from the world. However, if the entity does not exist, it emits a warning. This may not be intended behavior for many users who have use cases where they need to call `despawn` regardless of if the entity actually exists (see the issue), or don't care in general if the entity already doesn't exist. (Also trying to gauge interest on if this feature makes sense, I'd personally love to have it, but I could see arguments that this might be a footgun. Just trying to help here 😄 If there's no contention I could also implement this for `despawn_recursive` and `despawn_descendants` in the same PR) ## Solution Add `try_despawn`, `try_despawn_recursive` and `try_despawn_descendants`. Modify `World::despawn_with_caller` to also take in a `warn` boolean argument, which is then considered when logging the warning. Set `log_warning` to `true` in the case of `despawn`, and `false` in the case of `try_despawn`. ## Testing Ran `cargo run -p ci` on macOS, it seemed fine. --- .../bevy_ecs/world/despawn_recursive.rs | 2 +- crates/bevy_ecs/src/system/commands/mod.rs | 25 +++- crates/bevy_ecs/src/world/mod.rs | 15 +- crates/bevy_hierarchy/src/hierarchy.rs | 131 +++++++++++++----- crates/bevy_ui/src/layout/mod.rs | 2 +- 5 files changed, 135 insertions(+), 40 deletions(-) diff --git a/benches/benches/bevy_ecs/world/despawn_recursive.rs b/benches/benches/bevy_ecs/world/despawn_recursive.rs index 3c2b523b5faa3..482086ab17444 100644 --- a/benches/benches/bevy_ecs/world/despawn_recursive.rs +++ b/benches/benches/bevy_ecs/world/despawn_recursive.rs @@ -29,7 +29,7 @@ pub fn world_despawn_recursive(criterion: &mut Criterion) { group.bench_function(format!("{}_entities", entity_count), |bencher| { bencher.iter(|| { ents.iter().for_each(|e| { - despawn_with_children_recursive(&mut world, *e); + despawn_with_children_recursive(&mut world, *e, true); }); }); }); diff --git a/crates/bevy_ecs/src/system/commands/mod.rs b/crates/bevy_ecs/src/system/commands/mod.rs index 4f941c07a6721..3d6a6cf536c66 100644 --- a/crates/bevy_ecs/src/system/commands/mod.rs +++ b/crates/bevy_ecs/src/system/commands/mod.rs @@ -1411,6 +1411,14 @@ impl EntityCommands<'_> { self.queue(despawn()); } + /// Despawns the entity. + /// This will not emit a warning if the entity does not exist, essentially performing + /// the same function as [`Self::despawn`] without emitting warnings. + #[track_caller] + pub fn try_despawn(&mut self) { + self.queue(try_despawn()); + } + /// Pushes an [`EntityCommand`] to the queue, which will get executed for the current [`Entity`]. /// /// # Examples @@ -1697,7 +1705,22 @@ where fn despawn() -> impl EntityCommand { let caller = Location::caller(); move |entity: Entity, world: &mut World| { - world.despawn_with_caller(entity, caller); + world.despawn_with_caller(entity, caller, true); + } +} + +/// A [`Command`] that despawns a specific entity. +/// This will not emit a warning if the entity does not exist. +/// +/// # Note +/// +/// This won't clean up external references to the entity (such as parent-child relationships +/// if you're using `bevy_hierarchy`), which may leave the world in an invalid state. +#[track_caller] +fn try_despawn() -> impl EntityCommand { + let caller = Location::caller(); + move |entity: Entity, world: &mut World| { + world.despawn_with_caller(entity, caller, false); } } diff --git a/crates/bevy_ecs/src/world/mod.rs b/crates/bevy_ecs/src/world/mod.rs index 6f1774e264833..8a665e51ece6b 100644 --- a/crates/bevy_ecs/src/world/mod.rs +++ b/crates/bevy_ecs/src/world/mod.rs @@ -1385,7 +1385,15 @@ impl World { #[track_caller] #[inline] pub fn despawn(&mut self, entity: Entity) -> bool { - self.despawn_with_caller(entity, Location::caller()) + self.despawn_with_caller(entity, Location::caller(), true) + } + + /// Performs the same function as [`Self::despawn`] but does not emit a warning if + /// the entity does not exist. + #[track_caller] + #[inline] + pub fn try_despawn(&mut self, entity: Entity) -> bool { + self.despawn_with_caller(entity, Location::caller(), false) } #[inline] @@ -1393,13 +1401,16 @@ impl World { &mut self, entity: Entity, caller: &'static Location, + log_warning: bool, ) -> bool { self.flush(); if let Some(entity) = self.get_entity_mut(entity) { entity.despawn(); true } else { - warn!("error[B0003]: {caller}: Could not despawn entity {:?} because it doesn't exist in this World. See: https://bevyengine.org/learn/errors/b0003", entity); + if log_warning { + warn!("error[B0003]: {caller}: Could not despawn entity {:?} because it doesn't exist in this World. See: https://bevyengine.org/learn/errors/b0003", entity); + } false } } diff --git a/crates/bevy_hierarchy/src/hierarchy.rs b/crates/bevy_hierarchy/src/hierarchy.rs index 9f090d8ce4b91..351f36ae7d747 100644 --- a/crates/bevy_hierarchy/src/hierarchy.rs +++ b/crates/bevy_hierarchy/src/hierarchy.rs @@ -11,6 +11,8 @@ use bevy_utils::tracing::debug; pub struct DespawnRecursive { /// Target entity pub entity: Entity, + /// Whether or not this command should output a warning if the entity does not exist + pub warn: bool, } /// Despawns the given entity's children recursively @@ -18,10 +20,12 @@ pub struct DespawnRecursive { pub struct DespawnChildrenRecursive { /// Target entity pub entity: Entity, + /// Whether or not this command should output a warning if the entity does not exist + pub warn: bool, } /// Function for despawning an entity and all its children -pub fn despawn_with_children_recursive(world: &mut World, entity: Entity) { +pub fn despawn_with_children_recursive(world: &mut World, entity: Entity, warn: bool) { // first, make the entity's own parent forget about it if let Some(parent) = world.get::(entity).map(|parent| parent.0) { if let Some(mut children) = world.get_mut::(parent) { @@ -30,26 +34,30 @@ pub fn despawn_with_children_recursive(world: &mut World, entity: Entity) { } // then despawn the entity and all of its children - despawn_with_children_recursive_inner(world, entity); + despawn_with_children_recursive_inner(world, entity, warn); } -// Should only be called by `despawn_with_children_recursive`! -fn despawn_with_children_recursive_inner(world: &mut World, entity: Entity) { +// Should only be called by `despawn_with_children_recursive` and `try_despawn_with_children_recursive`! +fn despawn_with_children_recursive_inner(world: &mut World, entity: Entity, warn: bool) { if let Some(mut children) = world.get_mut::(entity) { for e in core::mem::take(&mut children.0) { - despawn_with_children_recursive_inner(world, e); + despawn_with_children_recursive_inner(world, e, warn); } } - if !world.despawn(entity) { + if warn { + if !world.despawn(entity) { + debug!("Failed to despawn entity {:?}", entity); + } + } else if !world.try_despawn(entity) { debug!("Failed to despawn entity {:?}", entity); } } -fn despawn_children_recursive(world: &mut World, entity: Entity) { +fn despawn_children_recursive(world: &mut World, entity: Entity, warn: bool) { if let Some(children) = world.entity_mut(entity).take::() { for e in children.0 { - despawn_with_children_recursive_inner(world, e); + despawn_with_children_recursive_inner(world, e, warn); } } } @@ -60,10 +68,11 @@ impl Command for DespawnRecursive { let _span = bevy_utils::tracing::info_span!( "command", name = "DespawnRecursive", - entity = bevy_utils::tracing::field::debug(self.entity) + entity = bevy_utils::tracing::field::debug(self.entity), + warn = bevy_utils::tracing::field::debug(self.warn) ) .entered(); - despawn_with_children_recursive(world, self.entity); + despawn_with_children_recursive(world, self.entity, self.warn); } } @@ -73,10 +82,12 @@ impl Command for DespawnChildrenRecursive { let _span = bevy_utils::tracing::info_span!( "command", name = "DespawnChildrenRecursive", - entity = bevy_utils::tracing::field::debug(self.entity) + entity = bevy_utils::tracing::field::debug(self.entity), + warn = bevy_utils::tracing::field::debug(self.warn) ) .entered(); - despawn_children_recursive(world, self.entity); + + despawn_children_recursive(world, self.entity, self.warn); } } @@ -87,6 +98,12 @@ pub trait DespawnRecursiveExt { /// Despawns all descendants of the given entity. fn despawn_descendants(&mut self) -> &mut Self; + + /// Similar to [`Self::despawn_recursive`] but does not emit warnings + fn try_despawn_recursive(self); + + /// Similar to [`Self::despawn_descendants`] but does not emit warnings + fn try_despawn_descendants(&mut self) -> &mut Self; } impl DespawnRecursiveExt for EntityCommands<'_> { @@ -94,46 +111,90 @@ impl DespawnRecursiveExt for EntityCommands<'_> { /// This will emit warnings for any entity that does not exist. fn despawn_recursive(mut self) { let entity = self.id(); - self.commands().queue(DespawnRecursive { entity }); + self.commands() + .queue(DespawnRecursive { entity, warn: true }); } fn despawn_descendants(&mut self) -> &mut Self { let entity = self.id(); - self.commands().queue(DespawnChildrenRecursive { entity }); + self.commands() + .queue(DespawnChildrenRecursive { entity, warn: true }); + self + } + + /// Despawns the provided entity and its children. + /// This will never emit warnings. + fn try_despawn_recursive(mut self) { + let entity = self.id(); + self.commands().queue(DespawnRecursive { + entity, + warn: false, + }); + } + + fn try_despawn_descendants(&mut self) -> &mut Self { + let entity = self.id(); + self.commands().queue(DespawnChildrenRecursive { + entity, + warn: false, + }); self } } +fn despawn_recursive_inner(world: EntityWorldMut, warn: bool) { + let entity = world.id(); + + #[cfg(feature = "trace")] + let _span = bevy_utils::tracing::info_span!( + "despawn_recursive", + entity = bevy_utils::tracing::field::debug(entity), + warn = bevy_utils::tracing::field::debug(warn) + ) + .entered(); + + despawn_with_children_recursive(world.into_world_mut(), entity, warn); +} + +fn despawn_descendants_inner<'v, 'w>( + world: &'v mut EntityWorldMut<'w>, + warn: bool, +) -> &'v mut EntityWorldMut<'w> { + let entity = world.id(); + + #[cfg(feature = "trace")] + let _span = bevy_utils::tracing::info_span!( + "despawn_descendants", + entity = bevy_utils::tracing::field::debug(entity), + warn = bevy_utils::tracing::field::debug(warn) + ) + .entered(); + + world.world_scope(|world| { + despawn_children_recursive(world, entity, warn); + }); + world +} + impl<'w> DespawnRecursiveExt for EntityWorldMut<'w> { /// Despawns the provided entity and its children. /// This will emit warnings for any entity that does not exist. fn despawn_recursive(self) { - let entity = self.id(); - - #[cfg(feature = "trace")] - let _span = bevy_utils::tracing::info_span!( - "despawn_recursive", - entity = bevy_utils::tracing::field::debug(entity) - ) - .entered(); - - despawn_with_children_recursive(self.into_world_mut(), entity); + despawn_recursive_inner(self, true); } fn despawn_descendants(&mut self) -> &mut Self { - let entity = self.id(); + despawn_descendants_inner(self, true) + } - #[cfg(feature = "trace")] - let _span = bevy_utils::tracing::info_span!( - "despawn_descendants", - entity = bevy_utils::tracing::field::debug(entity) - ) - .entered(); + /// Despawns the provided entity and its children. + /// This will not emit warnings. + fn try_despawn_recursive(self) { + despawn_recursive_inner(self, false); + } - self.world_scope(|world| { - despawn_children_recursive(world, entity); - }); - self + fn try_despawn_descendants(&mut self) -> &mut Self { + despawn_descendants_inner(self, false) } } diff --git a/crates/bevy_ui/src/layout/mod.rs b/crates/bevy_ui/src/layout/mod.rs index fe9ccca5b43b9..1965b24096d89 100644 --- a/crates/bevy_ui/src/layout/mod.rs +++ b/crates/bevy_ui/src/layout/mod.rs @@ -793,7 +793,7 @@ mod tests { } // despawn the parent entity and its descendants - despawn_with_children_recursive(&mut world, ui_parent_entity); + despawn_with_children_recursive(&mut world, ui_parent_entity, true); ui_schedule.run(&mut world);