From 058497e0bb79abbc3a1037c8669591e715996271 Mon Sep 17 00:00:00 2001 From: JaySpruce Date: Thu, 27 Feb 2025 15:05:16 -0600 Subject: [PATCH] Change `Commands::get_entity` to return `Result` and remove panic from `Commands::entity` (#18043) ## Objective Alternative to #18001. - Now that systems can handle the `?` operator, `get_entity` returning `Result` would be more useful than `Option`. - With `get_entity` being more flexible, combined with entity commands now checking the entity's existence automatically, the panic in `entity` isn't really necessary. ## Solution - Changed `Commands::get_entity` to return `Result`. - Removed panic from `Commands::entity`. --- crates/bevy_ecs/src/relationship/mod.rs | 2 +- crates/bevy_ecs/src/system/commands/mod.rs | 74 +++++++++----------- crates/bevy_input/src/gamepad.rs | 4 +- crates/bevy_pbr/src/render/light.rs | 6 +- crates/bevy_pbr/src/wireframe.rs | 2 +- crates/bevy_picking/src/hover.rs | 2 +- crates/bevy_sprite/src/mesh2d/wireframe2d.rs | 2 +- examples/ecs/observers.rs | 2 +- 8 files changed, 45 insertions(+), 49 deletions(-) diff --git a/crates/bevy_ecs/src/relationship/mod.rs b/crates/bevy_ecs/src/relationship/mod.rs index e88a4428fe..da9e66d1a8 100644 --- a/crates/bevy_ecs/src/relationship/mod.rs +++ b/crates/bevy_ecs/src/relationship/mod.rs @@ -128,7 +128,7 @@ pub trait Relationship: Component + Sized { { relationship_target.collection_mut_risky().remove(entity); if relationship_target.len() == 0 { - if let Some(mut entity) = world.commands().get_entity(target_entity) { + if let Ok(mut entity) = world.commands().get_entity(target_entity) { // this "remove" operation must check emptiness because in the event that an identical // relationship is inserted on top, this despawn would result in the removal of that identical // relationship ... not what we want! diff --git a/crates/bevy_ecs/src/system/commands/mod.rs b/crates/bevy_ecs/src/system/commands/mod.rs index 8db032a996..28fd253bd1 100644 --- a/crates/bevy_ecs/src/system/commands/mod.rs +++ b/crates/bevy_ecs/src/system/commands/mod.rs @@ -20,7 +20,7 @@ use crate::{ bundle::{Bundle, InsertMode, NoBundleEffect}, change_detection::{MaybeLocation, Mut}, component::{Component, ComponentId, Mutable}, - entity::{Entities, Entity, EntityClonerBuilder}, + entity::{Entities, Entity, EntityClonerBuilder, EntityDoesNotExistError}, event::Event, observer::{Observer, TriggerTargets}, resource::Resource, @@ -404,13 +404,9 @@ impl<'w, 's> Commands<'w, 's> { /// Returns the [`EntityCommands`] for the requested [`Entity`]. /// - /// This method does not guarantee that commands queued by the `EntityCommands` + /// This method does not guarantee that commands queued by the returned `EntityCommands` /// will be successful, since the entity could be despawned before they are executed. /// - /// # Panics - /// - /// This method panics if the requested entity does not exist. - /// /// # Example /// /// ``` @@ -442,33 +438,21 @@ impl<'w, 's> Commands<'w, 's> { #[inline] #[track_caller] pub fn entity(&mut self, entity: Entity) -> EntityCommands { - #[inline(never)] - #[cold] - #[track_caller] - fn panic_no_entity(entities: &Entities, entity: Entity) -> ! { - panic!( - "Attempting to create an EntityCommands for entity {entity}, which {}", - entities.entity_does_not_exist_error_details(entity) - ); - } - - if self.get_entity(entity).is_some() { - EntityCommands { - entity, - commands: self.reborrow(), - } - } else { - panic_no_entity(self.entities, entity) + EntityCommands { + entity, + commands: self.reborrow(), } } /// Returns the [`EntityCommands`] for the requested [`Entity`], if it exists. /// - /// Returns `None` if the entity does not exist. - /// - /// This method does not guarantee that commands queued by the `EntityCommands` + /// This method does not guarantee that commands queued by the returned `EntityCommands` /// will be successful, since the entity could be despawned before they are executed. /// + /// # Errors + /// + /// Returns [`EntityDoesNotExistError`] if the requested entity does not exist. + /// /// # Example /// /// ``` @@ -476,29 +460,41 @@ impl<'w, 's> Commands<'w, 's> { /// /// #[derive(Component)] /// struct Label(&'static str); - /// fn example_system(mut commands: Commands) { - /// // Create a new, empty entity + /// fn example_system(mut commands: Commands) -> Result { + /// // Create a new, empty entity. /// let entity = commands.spawn_empty().id(); /// - /// // Get the entity if it still exists, which it will in this case - /// if let Some(mut entity_commands) = commands.get_entity(entity) { - /// // adds a single component to the entity - /// entity_commands.insert(Label("hello world")); - /// } + /// // Get the entity if it still exists, which it will in this case. + /// // If it didn't, the `?` operator would propagate the returned error + /// // to the system, and the system would pass it to an error handler. + /// let mut entity_commands = commands.get_entity(entity)?; + /// + /// // Add a single component to the entity. + /// entity_commands.insert(Label("hello world")); + /// + /// // Return from the system with a success. + /// Ok(()) /// } /// # bevy_ecs::system::assert_is_system(example_system); /// ``` /// /// # See also /// - /// - [`entity`](Self::entity) for the panicking version. + /// - [`entity`](Self::entity) for the infallible version. #[inline] #[track_caller] - pub fn get_entity(&mut self, entity: Entity) -> Option { - self.entities.contains(entity).then_some(EntityCommands { - entity, - commands: self.reborrow(), - }) + pub fn get_entity( + &mut self, + entity: Entity, + ) -> Result { + if self.entities.contains(entity) { + Ok(EntityCommands { + entity, + commands: self.reborrow(), + }) + } else { + Err(EntityDoesNotExistError::new(entity, self.entities)) + } } /// Pushes a [`Command`] to the queue for creating entities with a particular [`Bundle`] type. diff --git a/crates/bevy_input/src/gamepad.rs b/crates/bevy_input/src/gamepad.rs index cdacd458f6..7d2b5e815d 100644 --- a/crates/bevy_input/src/gamepad.rs +++ b/crates/bevy_input/src/gamepad.rs @@ -1460,7 +1460,7 @@ pub fn gamepad_connection_system( vendor_id, product_id, } => { - let Some(mut gamepad) = commands.get_entity(id) else { + let Ok(mut gamepad) = commands.get_entity(id) else { warn!("Gamepad {} removed before handling connection event.", id); continue; }; @@ -1475,7 +1475,7 @@ pub fn gamepad_connection_system( info!("Gamepad {} connected.", id); } GamepadConnection::Disconnected => { - let Some(mut gamepad) = commands.get_entity(id) else { + let Ok(mut gamepad) = commands.get_entity(id) else { warn!("Gamepad {} removed before handling disconnection event. You can ignore this if you manually removed it.", id); continue; }; diff --git a/crates/bevy_pbr/src/render/light.rs b/crates/bevy_pbr/src/render/light.rs index 5dc225d466..c17b9d9355 100644 --- a/crates/bevy_pbr/src/render/light.rs +++ b/crates/bevy_pbr/src/render/light.rs @@ -525,7 +525,7 @@ pub(crate) fn add_light_view_entities( trigger: Trigger, mut commands: Commands, ) { - if let Some(mut v) = commands.get_entity(trigger.target()) { + if let Ok(mut v) = commands.get_entity(trigger.target()) { v.insert(LightViewEntities::default()); } } @@ -535,7 +535,7 @@ pub(crate) fn extracted_light_removed( trigger: Trigger, mut commands: Commands, ) { - if let Some(mut v) = commands.get_entity(trigger.target()) { + if let Ok(mut v) = commands.get_entity(trigger.target()) { v.try_remove::(); } } @@ -548,7 +548,7 @@ pub(crate) fn remove_light_view_entities( if let Ok(entities) = query.get(trigger.target()) { for v in entities.0.values() { for e in v.iter().copied() { - if let Some(mut v) = commands.get_entity(e) { + if let Ok(mut v) = commands.get_entity(e) { v.despawn(); } } diff --git a/crates/bevy_pbr/src/wireframe.rs b/crates/bevy_pbr/src/wireframe.rs index 68862bbf71..dae770bf25 100644 --- a/crates/bevy_pbr/src/wireframe.rs +++ b/crates/bevy_pbr/src/wireframe.rs @@ -157,7 +157,7 @@ fn apply_wireframe_material( global_material: Res, ) { for e in removed_wireframes.read().chain(no_wireframes.iter()) { - if let Some(mut commands) = commands.get_entity(e) { + if let Ok(mut commands) = commands.get_entity(e) { commands.remove::>(); } } diff --git a/crates/bevy_picking/src/hover.rs b/crates/bevy_picking/src/hover.rs index 69edb6d9aa..d989ddfa65 100644 --- a/crates/bevy_picking/src/hover.rs +++ b/crates/bevy_picking/src/hover.rs @@ -244,7 +244,7 @@ pub fn update_interactions( for (hovered_entity, new_interaction) in new_interaction_state.drain() { if let Ok(mut interaction) = interact.get_mut(hovered_entity) { *interaction = new_interaction; - } else if let Some(mut entity_commands) = commands.get_entity(hovered_entity) { + } else if let Ok(mut entity_commands) = commands.get_entity(hovered_entity) { entity_commands.try_insert(new_interaction); } } diff --git a/crates/bevy_sprite/src/mesh2d/wireframe2d.rs b/crates/bevy_sprite/src/mesh2d/wireframe2d.rs index 4547f83d9b..6d62a04675 100644 --- a/crates/bevy_sprite/src/mesh2d/wireframe2d.rs +++ b/crates/bevy_sprite/src/mesh2d/wireframe2d.rs @@ -163,7 +163,7 @@ fn apply_wireframe_material( global_material: Res, ) { for e in removed_wireframes.read().chain(no_wireframes.iter()) { - if let Some(mut commands) = commands.get_entity(e) { + if let Ok(mut commands) = commands.get_entity(e) { commands.remove::>(); } } diff --git a/examples/ecs/observers.rs b/examples/ecs/observers.rs index 0f057db100..f1cc7c2eff 100644 --- a/examples/ecs/observers.rs +++ b/examples/ecs/observers.rs @@ -144,7 +144,7 @@ fn on_remove_mine( fn explode_mine(trigger: Trigger, query: Query<&Mine>, mut commands: Commands) { // If a triggered event is targeting a specific entity you can access it with `.target()` let id = trigger.target(); - let Some(mut entity) = commands.get_entity(id) else { + let Ok(mut entity) = commands.get_entity(id) else { return; }; info!("Boom! {} exploded.", id.index());