From 795e273a9ac09ee482bfc1919e9b85ee8bdf95af Mon Sep 17 00:00:00 2001 From: Brian Reavis Date: Sun, 29 Jun 2025 09:34:20 -0700 Subject: [PATCH] Don't create errors for ignored failed commands (#19718) # Objective 1. Reduce overhead from error handling for ECS commands that intentionally ignore errors, such as `try_despawn`. These commands currently allocate error objects and pass them to a no-op handler (`ignore`), which can impact performance when many operations fail. 2. Fix a hang when removing `ChildOf` components during entity despawning. Excessive logging of these failures can cause significant hangs (I'm noticing around 100ms). - Fixes https://github.com/bevyengine/bevy/issues/19777 - Fixes https://github.com/bevyengine/bevy/issues/19753 image ## Solution * Added a `ignore_error` method to the `HandleError` trait to use instead of `handle_error_with(ignore)`. It swallows errors and does not create error objects. * Replaced `remove::` with `try_remove::` to suppress expected (?) errors and reduce log noise. ## Testing - I ran these changes on a local project. --- crates/bevy_ecs/src/error/command_handling.rs | 19 ++++++++++++ crates/bevy_ecs/src/relationship/mod.rs | 8 ++--- crates/bevy_ecs/src/system/commands/mod.rs | 30 ++++++++++++++----- 3 files changed, 46 insertions(+), 11 deletions(-) diff --git a/crates/bevy_ecs/src/error/command_handling.rs b/crates/bevy_ecs/src/error/command_handling.rs index c303b76d17..13ec866ec1 100644 --- a/crates/bevy_ecs/src/error/command_handling.rs +++ b/crates/bevy_ecs/src/error/command_handling.rs @@ -20,6 +20,8 @@ pub trait HandleError: Send + 'static { /// Takes a [`Command`] that returns a Result and uses the default error handler function to convert it into /// a [`Command`] that internally handles an error if it occurs and returns `()`. fn handle_error(self) -> impl Command; + /// Takes a [`Command`] that returns a Result and ignores any error that occurs. + fn ignore_error(self) -> impl Command; } impl HandleError> for C @@ -50,6 +52,12 @@ where ), } } + + fn ignore_error(self) -> impl Command { + move |world: &mut World| { + let _ = self.apply(world); + } + } } impl HandleError for C @@ -68,6 +76,13 @@ where self.apply(world); } } + + #[inline] + fn ignore_error(self) -> impl Command { + move |world: &mut World| { + self.apply(world); + } + } } impl HandleError for C @@ -82,6 +97,10 @@ where fn handle_error(self) -> impl Command { self } + #[inline] + fn ignore_error(self) -> impl Command { + self + } } /// Passes in a specific entity to an [`EntityCommand`], resulting in a [`Command`] that diff --git a/crates/bevy_ecs/src/relationship/mod.rs b/crates/bevy_ecs/src/relationship/mod.rs index 8830998663..12def083bd 100644 --- a/crates/bevy_ecs/src/relationship/mod.rs +++ b/crates/bevy_ecs/src/relationship/mod.rs @@ -14,7 +14,7 @@ pub use relationship_source_collection::*; use crate::{ component::{Component, Mutable}, entity::{ComponentCloneCtx, Entity, SourceComponent}, - error::{ignore, CommandWithEntity, HandleError}, + error::CommandWithEntity, lifecycle::HookContext, world::{DeferredWorld, EntityWorldMut}, }; @@ -187,7 +187,7 @@ pub trait Relationship: Component + Sized { world .commands() - .queue(command.with_entity(target_entity).handle_error_with(ignore)); + .queue_silenced(command.with_entity(target_entity)); } } } @@ -244,7 +244,7 @@ pub trait RelationshipTarget: Component + Sized { for source_entity in relationship_target.iter() { commands .entity(source_entity) - .remove::(); + .try_remove::(); } } @@ -255,7 +255,7 @@ pub trait RelationshipTarget: Component + Sized { let (entities, mut commands) = world.entities_and_commands(); let relationship_target = entities.get(entity).unwrap().get::().unwrap(); for source_entity in relationship_target.iter() { - commands.entity(source_entity).despawn(); + commands.entity(source_entity).try_despawn(); } } diff --git a/crates/bevy_ecs/src/system/commands/mod.rs b/crates/bevy_ecs/src/system/commands/mod.rs index 0751e26770..e066446b51 100644 --- a/crates/bevy_ecs/src/system/commands/mod.rs +++ b/crates/bevy_ecs/src/system/commands/mod.rs @@ -19,7 +19,7 @@ use crate::{ change_detection::{MaybeLocation, Mut}, component::{Component, ComponentId, Mutable}, entity::{Entities, Entity, EntityClonerBuilder, EntityDoesNotExistError, OptIn, OptOut}, - error::{ignore, warn, BevyError, CommandWithEntity, ErrorContext, HandleError}, + error::{warn, BevyError, CommandWithEntity, ErrorContext, HandleError}, event::{BufferedEvent, EntityEvent, Event}, observer::{Observer, TriggerTargets}, resource::Resource, @@ -641,6 +641,11 @@ impl<'w, 's> Commands<'w, 's> { self.queue_internal(command.handle_error_with(error_handler)); } + /// Pushes a generic [`Command`] to the queue like [`Commands::queue_handled`], but instead silently ignores any errors. + pub fn queue_silenced + HandleError, T>(&mut self, command: C) { + self.queue_internal(command.ignore_error()); + } + fn queue_internal(&mut self, command: impl Command) { match &mut self.queue { InternalQueue::CommandQueue(queue) => { @@ -1466,12 +1471,11 @@ impl<'a> EntityCommands<'a> { component_id: ComponentId, value: T, ) -> &mut Self { - self.queue_handled( + self.queue_silenced( // SAFETY: // - `ComponentId` safety is ensured by the caller. // - `T` safety is ensured by the caller. unsafe { entity_command::insert_by_id(component_id, value, InsertMode::Replace) }, - ignore, ) } @@ -1523,7 +1527,7 @@ impl<'a> EntityCommands<'a> { /// ``` #[track_caller] pub fn try_insert(&mut self, bundle: impl Bundle) -> &mut Self { - self.queue_handled(entity_command::insert(bundle, InsertMode::Replace), ignore) + self.queue_silenced(entity_command::insert(bundle, InsertMode::Replace)) } /// Adds a [`Bundle`] of components to the entity if the predicate returns true. @@ -1579,7 +1583,7 @@ impl<'a> EntityCommands<'a> { /// the resulting error will be ignored. #[track_caller] pub fn try_insert_if_new(&mut self, bundle: impl Bundle) -> &mut Self { - self.queue_handled(entity_command::insert(bundle, InsertMode::Keep), ignore) + self.queue_silenced(entity_command::insert(bundle, InsertMode::Keep)) } /// Removes a [`Bundle`] of components from the entity. @@ -1724,7 +1728,7 @@ impl<'a> EntityCommands<'a> { /// # bevy_ecs::system::assert_is_system(remove_combat_stats_system); /// ``` pub fn try_remove(&mut self) -> &mut Self { - self.queue_handled(entity_command::remove::(), ignore) + self.queue_silenced(entity_command::remove::()) } /// Removes a [`Bundle`] of components from the entity, @@ -1818,7 +1822,7 @@ impl<'a> EntityCommands<'a> { /// /// For example, this will recursively despawn [`Children`](crate::hierarchy::Children). pub fn try_despawn(&mut self) { - self.queue_handled(entity_command::despawn(), ignore); + self.queue_silenced(entity_command::despawn()); } /// Pushes an [`EntityCommand`] to the queue, @@ -1907,6 +1911,18 @@ impl<'a> EntityCommands<'a> { self } + /// Pushes an [`EntityCommand`] to the queue, which will get executed for the current [`Entity`]. + /// + /// Unlike [`EntityCommands::queue_handled`], this will completely ignore any errors that occur. + pub fn queue_silenced + CommandWithEntity, T, M>( + &mut self, + command: C, + ) -> &mut Self { + self.commands + .queue_silenced(command.with_entity(self.entity)); + self + } + /// Removes all components except the given [`Bundle`] from the entity. /// /// # Example