From fe24652cc0da00b4d1bf377de583bc71318eb133 Mon Sep 17 00:00:00 2001 From: JaySpruce Date: Tue, 21 Jan 2025 17:21:32 -0600 Subject: [PATCH] Change `World::try_despawn` and `World::try_insert_batch` to return `Result` (#17376) ## Objective Most `try` methods on `World` return a `Result`, but `try_despawn` and `try_insert_batch` don't. Since Bevy's error handling is advancing, these should be brought in line. ## Solution - Added `TryDespawnError` and `TryInsertBatchError`. - `try_despawn`, `try_insert_batch`, and `try_insert_batch_if_new` now return their respective errors. - Fixed slightly incorrect behavior in `try_insert_batch_with_caller`. - The method was always meant to continue with the rest of the batch if an entity was missing, but that only worked after the first entity; if the first entity was missing, the method would exit early. This has been resolved. ## Migration Guide - `World::try_despawn` now returns a `Result` rather than a `bool`. - `World::try_insert_batch` and `World::try_insert_batch_if_new` now return a `Result` where they previously returned nothing. --- crates/bevy_ecs/src/lib.rs | 8 +- .../bevy_ecs/src/system/commands/command.rs | 33 +-- crates/bevy_ecs/src/system/commands/mod.rs | 21 +- crates/bevy_ecs/src/world/error.rs | 27 ++ crates/bevy_ecs/src/world/mod.rs | 245 +++++++++++------- 5 files changed, 198 insertions(+), 136 deletions(-) diff --git a/crates/bevy_ecs/src/lib.rs b/crates/bevy_ecs/src/lib.rs index 21fc5cddd0c62..f99f73c1220b6 100644 --- a/crates/bevy_ecs/src/lib.rs +++ b/crates/bevy_ecs/src/lib.rs @@ -1878,7 +1878,9 @@ mod tests { let values = vec![(e0, (A(1), B(0))), (e1, (A(0), B(1)))]; - world.try_insert_batch(values); + let error = world.try_insert_batch(values).unwrap_err(); + + assert_eq!(e1, error.entities[0]); assert_eq!( world.get::(e0), @@ -1900,7 +1902,9 @@ mod tests { let values = vec![(e0, (A(1), B(0))), (e1, (A(0), B(1)))]; - world.try_insert_batch_if_new(values); + let error = world.try_insert_batch_if_new(values).unwrap_err(); + + assert_eq!(e1, error.entities[0]); assert_eq!( world.get::(e0), diff --git a/crates/bevy_ecs/src/system/commands/command.rs b/crates/bevy_ecs/src/system/commands/command.rs index 71875f9d41edd..b29535367bf5e 100644 --- a/crates/bevy_ecs/src/system/commands/command.rs +++ b/crates/bevy_ecs/src/system/commands/command.rs @@ -126,46 +126,27 @@ where } /// A [`Command`] that consumes an iterator to add a series of [`Bundles`](Bundle) to a set of entities. -/// If any entities do not exist in the world, this command will panic. /// -/// This is more efficient than inserting the bundles individually. -#[track_caller] -pub fn insert_batch(batch: I, mode: InsertMode) -> impl Command -where - I: IntoIterator + Send + Sync + 'static, - B: Bundle, -{ - #[cfg(feature = "track_location")] - let caller = Location::caller(); - move |world: &mut World| { - world.insert_batch_with_caller( - batch, - mode, - #[cfg(feature = "track_location")] - caller, - ); - } -} - -/// A [`Command`] that consumes an iterator to add a series of [`Bundles`](Bundle) to a set of entities. -/// If any entities do not exist in the world, this command will ignore them. +/// If any entities do not exist in the world, this command will return a +/// [`TryInsertBatchError`](crate::world::error::TryInsertBatchError). /// /// This is more efficient than inserting the bundles individually. #[track_caller] -pub fn try_insert_batch(batch: I, mode: InsertMode) -> impl Command +pub fn insert_batch(batch: I, insert_mode: InsertMode) -> impl Command where I: IntoIterator + Send + Sync + 'static, B: Bundle, { #[cfg(feature = "track_location")] let caller = Location::caller(); - move |world: &mut World| { + move |world: &mut World| -> Result { world.try_insert_batch_with_caller( batch, - mode, + insert_mode, #[cfg(feature = "track_location")] caller, - ); + )?; + Ok(()) } } diff --git a/crates/bevy_ecs/src/system/commands/mod.rs b/crates/bevy_ecs/src/system/commands/mod.rs index b68e46667f350..59cb74ed8f544 100644 --- a/crates/bevy_ecs/src/system/commands/mod.rs +++ b/crates/bevy_ecs/src/system/commands/mod.rs @@ -705,7 +705,7 @@ impl<'w, 's> Commands<'w, 's> { /// Pushes a [`Command`] to the queue for adding a [`Bundle`] type to a batch of [`Entities`](Entity). /// /// A batch can be any type that implements [`IntoIterator`] containing `(Entity, Bundle)` tuples, - /// such as a [`Vec<(Entity, Bundle)>`] or an array `[(Entity, Bundle); N]`. + /// such as a [`Vec<(Entity, Bundle)>`](alloc::vec::Vec) or an array `[(Entity, Bundle); N]`. /// /// When the command is applied, for each `(Entity, Bundle)` pair in the given batch, /// the `Bundle` is added to the `Entity`, overwriting any existing components shared by the `Bundle`. @@ -732,7 +732,7 @@ impl<'w, 's> Commands<'w, 's> { /// Pushes a [`Command`] to the queue for adding a [`Bundle`] type to a batch of [`Entities`](Entity). /// /// A batch can be any type that implements [`IntoIterator`] containing `(Entity, Bundle)` tuples, - /// such as a [`Vec<(Entity, Bundle)>`] or an array `[(Entity, Bundle); N]`. + /// such as a [`Vec<(Entity, Bundle)>`](alloc::vec::Vec) or an array `[(Entity, Bundle); N]`. /// /// When the command is applied, for each `(Entity, Bundle)` pair in the given batch, /// the `Bundle` is added to the `Entity`, except for any components already present on the `Entity`. @@ -759,7 +759,7 @@ impl<'w, 's> Commands<'w, 's> { /// Pushes a [`Command`] to the queue for adding a [`Bundle`] type to a batch of [`Entities`](Entity). /// /// A batch can be any type that implements [`IntoIterator`] containing `(Entity, Bundle)` tuples, - /// such as a [`Vec<(Entity, Bundle)>`] or an array `[(Entity, Bundle); N]`. + /// such as a [`Vec<(Entity, Bundle)>`](alloc::vec::Vec) or an array `[(Entity, Bundle); N]`. /// /// When the command is applied, for each `(Entity, Bundle)` pair in the given batch, /// the `Bundle` is added to the `Entity`, overwriting any existing components shared by the `Bundle`. @@ -769,7 +769,7 @@ impl<'w, 's> Commands<'w, 's> { /// and passing the bundle to [`insert`](EntityCommands::insert), /// but it is faster due to memory pre-allocation. /// - /// This command silently fails by ignoring any entities that do not exist. + /// This command will send a warning if any of the given entities do not exist. /// /// For the panicking version, see [`insert_batch`](Self::insert_batch). #[track_caller] @@ -778,13 +778,16 @@ impl<'w, 's> Commands<'w, 's> { I: IntoIterator + Send + Sync + 'static, B: Bundle, { - self.queue(command::try_insert_batch(batch, InsertMode::Replace)); + self.queue( + command::insert_batch(batch, InsertMode::Replace) + .handle_error_with(error_handler::warn()), + ); } /// Pushes a [`Command`] to the queue for adding a [`Bundle`] type to a batch of [`Entities`](Entity). /// /// A batch can be any type that implements [`IntoIterator`] containing `(Entity, Bundle)` tuples, - /// such as a [`Vec<(Entity, Bundle)>`] or an array `[(Entity, Bundle); N]`. + /// such as a [`Vec<(Entity, Bundle)>`](alloc::vec::Vec) or an array `[(Entity, Bundle); N]`. /// /// When the command is applied, for each `(Entity, Bundle)` pair in the given batch, /// the `Bundle` is added to the `Entity`, except for any components already present on the `Entity`. @@ -794,7 +797,7 @@ impl<'w, 's> Commands<'w, 's> { /// and passing the bundle to [`insert_if_new`](EntityCommands::insert_if_new), /// but it is faster due to memory pre-allocation. /// - /// This command silently fails by ignoring any entities that do not exist. + /// This command will send a warning if any of the given entities do not exist. /// /// For the panicking version, see [`insert_batch_if_new`](Self::insert_batch_if_new). #[track_caller] @@ -803,7 +806,9 @@ impl<'w, 's> Commands<'w, 's> { I: IntoIterator + Send + Sync + 'static, B: Bundle, { - self.queue(command::try_insert_batch(batch, InsertMode::Keep)); + self.queue( + command::insert_batch(batch, InsertMode::Keep).handle_error_with(error_handler::warn()), + ); } /// Pushes a [`Command`] to the queue for inserting a [`Resource`] in the [`World`] with an inferred value. diff --git a/crates/bevy_ecs/src/world/error.rs b/crates/bevy_ecs/src/world/error.rs index 1c6b5043bcea8..7609b15e019ae 100644 --- a/crates/bevy_ecs/src/world/error.rs +++ b/crates/bevy_ecs/src/world/error.rs @@ -1,5 +1,6 @@ //! Contains error types returned by bevy's schedule. +use alloc::vec::Vec; use thiserror::Error; use crate::{ @@ -15,6 +16,32 @@ use crate::{ #[error("The schedule with the label {0:?} was not found.")] pub struct TryRunScheduleError(pub InternedScheduleLabel); +/// The error type returned by [`World::try_despawn`] if the provided entity does not exist. +/// +/// [`World::try_despawn`]: crate::world::World::try_despawn +#[derive(Error, Debug, Clone, Copy)] +#[error("Could not despawn the entity with ID {entity} because it {details}")] +pub struct TryDespawnError { + /// The entity's ID. + pub entity: Entity, + /// Details on why the entity does not exist, if available. + pub details: EntityDoesNotExistDetails, +} + +/// The error type returned by [`World::try_insert_batch`] and [`World::try_insert_batch_if_new`] +/// if any of the provided entities do not exist. +/// +/// [`World::try_insert_batch`]: crate::world::World::try_insert_batch +/// [`World::try_insert_batch_if_new`]: crate::world::World::try_insert_batch_if_new +#[derive(Error, Debug, Clone)] +#[error("Could not insert bundles of type {bundle_type} into the entities with the following IDs because they do not exist: {entities:?}")] +pub struct TryInsertBatchError { + /// The bundles' type name. + pub bundle_type: &'static str, + /// The IDs of the provided entities that do not exist. + pub entities: Vec, +} + /// An error that occurs when dynamically retrieving components from an entity. #[derive(Error, Debug, Clone, Copy, PartialEq, Eq)] pub enum EntityComponentError { diff --git a/crates/bevy_ecs/src/world/mod.rs b/crates/bevy_ecs/src/world/mod.rs index e34e2752c2b25..5c619d706464c 100644 --- a/crates/bevy_ecs/src/world/mod.rs +++ b/crates/bevy_ecs/src/world/mod.rs @@ -51,19 +51,22 @@ use crate::{ system::Commands, world::{ command_queue::RawCommandQueue, - error::{EntityFetchError, TryRunScheduleError}, + error::{EntityFetchError, TryDespawnError, TryInsertBatchError, TryRunScheduleError}, }, }; use alloc::{boxed::Box, vec::Vec}; use bevy_platform_support::sync::atomic::{AtomicU32, Ordering}; use bevy_ptr::{OwningPtr, Ptr}; -use core::{any::TypeId, fmt, panic::Location}; +use core::{any::TypeId, fmt}; use log::warn; use unsafe_world_cell::{UnsafeEntityCell, UnsafeWorldCell}; #[cfg(feature = "track_location")] use bevy_ptr::UnsafeCellDeref; +#[cfg(feature = "track_location")] +use core::panic::Location; + /// Stores and exposes operations on [entities](Entity), [components](Component), resources, /// and their associated metadata. /// @@ -1264,9 +1267,11 @@ impl World { Ok(result) } - /// Despawns the given `entity`, if it exists. This will also remove all of the entity's - /// [`Component`]s. Returns `true` if the `entity` is successfully despawned and `false` if - /// the `entity` does not exist. + /// Despawns the given [`Entity`], if it exists. This will also remove all of the entity's + /// [`Components`](Component). + /// + /// Returns `true` if the entity is successfully despawned and `false` if + /// the entity does not exist. /// /// # Note /// @@ -1291,36 +1296,55 @@ impl World { #[track_caller] #[inline] pub fn despawn(&mut self, entity: Entity) -> bool { - self.despawn_with_caller(entity, Location::caller(), true) + if let Err(error) = self.despawn_with_caller( + entity, + #[cfg(feature = "track_location")] + Location::caller(), + ) { + warn!("{error}"); + false + } else { + true + } } - /// Performs the same function as [`Self::despawn`] but does not emit a warning if - /// the entity does not exist. + /// Despawns the given `entity`, if it exists. This will also remove all of the entity's + /// [`Components`](Component). + /// + /// Returns a [`TryDespawnError`] if the entity does not exist. + /// + /// # Note + /// + /// This will also despawn the entities in any [`RelationshipTarget`](crate::relationship::RelationshipTarget) that is configured + /// to despawn descendants. For example, this will recursively despawn [`Children`](crate::hierarchy::Children). #[track_caller] #[inline] - pub fn try_despawn(&mut self, entity: Entity) -> bool { - self.despawn_with_caller(entity, Location::caller(), false) + pub fn try_despawn(&mut self, entity: Entity) -> Result<(), TryDespawnError> { + self.despawn_with_caller( + entity, + #[cfg(feature = "track_location")] + Location::caller(), + ) } #[inline] pub(crate) fn despawn_with_caller( &mut self, entity: Entity, - caller: &'static Location, - log_warning: bool, - ) -> bool { + #[cfg(feature = "track_location")] caller: &'static Location, + ) -> Result<(), TryDespawnError> { self.flush(); if let Ok(entity) = self.get_entity_mut(entity) { entity.despawn_with_caller( #[cfg(feature = "track_location")] caller, ); - true + Ok(()) } else { - if log_warning { - warn!("error[B0003]: {caller}: Could not despawn entity {entity}, which {}. See: https://bevyengine.org/learn/errors/b0003", self.entities.entity_does_not_exist_error_details(entity)); - } - false + Err(TryDespawnError { + entity, + details: self.entities().entity_does_not_exist_error_details(entity), + }) } } @@ -2344,7 +2368,7 @@ impl World { /// /// This function will panic if any of the associated entities do not exist. /// - /// For the non-panicking version, see [`World::try_insert_batch`]. + /// For the fallible version, see [`World::try_insert_batch`]. #[track_caller] pub fn insert_batch(&mut self, batch: I) where @@ -2374,7 +2398,7 @@ impl World { /// /// This function will panic if any of the associated entities do not exist. /// - /// For the non-panicking version, see [`World::try_insert_batch_if_new`]. + /// For the fallible version, see [`World::try_insert_batch_if_new`]. #[track_caller] pub fn insert_batch_if_new(&mut self, batch: I) where @@ -2395,12 +2419,10 @@ impl World { /// This can be called by: /// - [`World::insert_batch`] /// - [`World::insert_batch_if_new`] - /// - [`Commands::insert_batch`] - /// - [`Commands::insert_batch_if_new`] #[inline] pub(crate) fn insert_batch_with_caller( &mut self, - iter: I, + batch: I, insert_mode: InsertMode, #[cfg(feature = "track_location")] caller: &'static Location, ) where @@ -2408,22 +2430,20 @@ impl World { I::IntoIter: Iterator, B: Bundle, { - self.flush(); + struct InserterArchetypeCache<'w> { + inserter: BundleInserter<'w>, + archetype_id: ArchetypeId, + } + self.flush(); let change_tick = self.change_tick(); - let bundle_id = self .bundles .register_info::(&mut self.components, &mut self.storages); - struct InserterArchetypeCache<'w> { - inserter: BundleInserter<'w>, - archetype_id: ArchetypeId, - } + let mut batch_iter = batch.into_iter(); - let mut batch = iter.into_iter(); - - if let Some((first_entity, first_bundle)) = batch.next() { + if let Some((first_entity, first_bundle)) = batch_iter.next() { if let Some(first_location) = self.entities().get(first_entity) { let mut cache = InserterArchetypeCache { // SAFETY: we initialized this bundle_id in `register_info` @@ -2449,7 +2469,7 @@ impl World { ) }; - for (entity, bundle) in batch { + for (entity, bundle) in batch_iter { if let Some(location) = cache.inserter.entities().get(entity) { if location.archetype_id != cache.archetype_id { cache = InserterArchetypeCache { @@ -2496,11 +2516,11 @@ impl World { /// This will overwrite any previous values of components shared by the `Bundle`. /// See [`World::try_insert_batch_if_new`] to keep the old values instead. /// - /// This function silently fails by ignoring any entities that do not exist. + /// Returns a [`TryInsertBatchError`] if any of the provided entities do not exist. /// /// For the panicking version, see [`World::insert_batch`]. #[track_caller] - pub fn try_insert_batch(&mut self, batch: I) + pub fn try_insert_batch(&mut self, batch: I) -> Result<(), TryInsertBatchError> where I: IntoIterator, I::IntoIter: Iterator, @@ -2511,7 +2531,7 @@ impl World { InsertMode::Replace, #[cfg(feature = "track_location")] Location::caller(), - ); + ) } /// For a given batch of ([`Entity`], [`Bundle`]) pairs, /// adds the `Bundle` of components to each `Entity` without overwriting. @@ -2523,11 +2543,11 @@ impl World { /// This is the same as [`World::try_insert_batch`], but in case of duplicate /// components it will leave the old values instead of replacing them with new ones. /// - /// This function silently fails by ignoring any entities that do not exist. + /// Returns a [`TryInsertBatchError`] if any of the provided entities do not exist. /// /// For the panicking version, see [`World::insert_batch_if_new`]. #[track_caller] - pub fn try_insert_batch_if_new(&mut self, batch: I) + pub fn try_insert_batch_if_new(&mut self, batch: I) -> Result<(), TryInsertBatchError> where I: IntoIterator, I::IntoIter: Iterator, @@ -2538,7 +2558,7 @@ impl World { InsertMode::Keep, #[cfg(feature = "track_location")] Location::caller(), - ); + ) } /// Split into a new function so we can differentiate the calling location. @@ -2546,91 +2566,116 @@ impl World { /// This can be called by: /// - [`World::try_insert_batch`] /// - [`World::try_insert_batch_if_new`] + /// - [`Commands::insert_batch`] + /// - [`Commands::insert_batch_if_new`] /// - [`Commands::try_insert_batch`] /// - [`Commands::try_insert_batch_if_new`] #[inline] pub(crate) fn try_insert_batch_with_caller( &mut self, - iter: I, + batch: I, insert_mode: InsertMode, #[cfg(feature = "track_location")] caller: &'static Location, - ) where + ) -> Result<(), TryInsertBatchError> + where I: IntoIterator, I::IntoIter: Iterator, B: Bundle, { - self.flush(); - - let change_tick = self.change_tick(); - - let bundle_id = self - .bundles - .register_info::(&mut self.components, &mut self.storages); - struct InserterArchetypeCache<'w> { inserter: BundleInserter<'w>, archetype_id: ArchetypeId, } - let mut batch = iter.into_iter(); + self.flush(); + let change_tick = self.change_tick(); + let bundle_id = self + .bundles + .register_info::(&mut self.components, &mut self.storages); - if let Some((first_entity, first_bundle)) = batch.next() { - if let Some(first_location) = self.entities().get(first_entity) { - let mut cache = InserterArchetypeCache { - // SAFETY: we initialized this bundle_id in `register_info` - inserter: unsafe { - BundleInserter::new_with_id( - self, - first_location.archetype_id, - bundle_id, - change_tick, + let mut invalid_entities = Vec::::new(); + let mut batch_iter = batch.into_iter(); + + // We need to find the first valid entity so we can initialize the bundle inserter. + // This differs from `insert_batch_with_caller` because that method can just panic + // if the first entity is invalid, whereas this method needs to keep going. + let cache = loop { + if let Some((first_entity, first_bundle)) = batch_iter.next() { + if let Some(first_location) = self.entities().get(first_entity) { + let mut cache = InserterArchetypeCache { + // SAFETY: we initialized this bundle_id in `register_info` + inserter: unsafe { + BundleInserter::new_with_id( + self, + first_location.archetype_id, + bundle_id, + change_tick, + ) + }, + archetype_id: first_location.archetype_id, + }; + // SAFETY: `entity` is valid, `location` matches entity, bundle matches inserter + unsafe { + cache.inserter.insert( + first_entity, + first_location, + first_bundle, + insert_mode, + #[cfg(feature = "track_location")] + caller, ) - }, - archetype_id: first_location.archetype_id, - }; - // SAFETY: `entity` is valid, `location` matches entity, bundle matches inserter - unsafe { - cache.inserter.insert( - first_entity, - first_location, - first_bundle, - insert_mode, - #[cfg(feature = "track_location")] - caller, - ) - }; + }; + break Some(cache); + } + invalid_entities.push(first_entity); + } else { + // We reached the end of the entities the caller provided and none were valid. + break None; + } + }; - for (entity, bundle) in batch { - if let Some(location) = cache.inserter.entities().get(entity) { - if location.archetype_id != cache.archetype_id { - cache = InserterArchetypeCache { - // SAFETY: we initialized this bundle_id in `register_info` - inserter: unsafe { - BundleInserter::new_with_id( - self, - location.archetype_id, - bundle_id, - change_tick, - ) - }, - archetype_id: location.archetype_id, - } + if let Some(mut cache) = cache { + for (entity, bundle) in batch_iter { + if let Some(location) = cache.inserter.entities().get(entity) { + if location.archetype_id != cache.archetype_id { + cache = InserterArchetypeCache { + // SAFETY: we initialized this bundle_id in `register_info` + inserter: unsafe { + BundleInserter::new_with_id( + self, + location.archetype_id, + bundle_id, + change_tick, + ) + }, + archetype_id: location.archetype_id, } - // SAFETY: `entity` is valid, `location` matches entity, bundle matches inserter - unsafe { - cache.inserter.insert( - entity, - location, - bundle, - insert_mode, - #[cfg(feature = "track_location")] - caller, - ) - }; } + // SAFETY: `entity` is valid, `location` matches entity, bundle matches inserter + unsafe { + cache.inserter.insert( + entity, + location, + bundle, + insert_mode, + #[cfg(feature = "track_location")] + caller, + ) + }; + } else { + invalid_entities.push(entity); } } } + + if invalid_entities.is_empty() { + Ok(()) + } else { + Err(TryInsertBatchError { + bundle_type: core::any::type_name::(), + entities: invalid_entities, + }) + } } /// Temporarily removes the requested resource from this [`World`], runs custom user code,