From af20cad83058842687c79dd3f9aaf5b45dbce9c7 Mon Sep 17 00:00:00 2001 From: Daniel McNab <36049421+DJMcNab@users.noreply.github.com> Date: Wed, 1 Sep 2021 20:59:25 +0000 Subject: [PATCH] Add error messages for the spooky insertions (#2581) # Objective Sometimes, the unwraps in `entity_mut` could fail here, if the entity was despawned *before* this command was applied. The simplest case involves two command buffers: ```rust use bevy::prelude::*; fn b(mut commands1: Commands, mut commands2: Commands) { let id = commands2.spawn().insert_bundle(()).id(); commands1.entity(id).despawn(); } fn main() { App::build().add_system(b.system()).run(); } ``` However, a more complicated version arises in the case of ambiguity: ```rust use std::time::Duration; use bevy::{app::ScheduleRunnerPlugin, prelude::*}; use rand::Rng; fn cleanup(mut e: ResMut>) { *e = None; } fn sleep_randomly() { let mut rng = rand::thread_rng(); std::thread::sleep(Duration::from_millis(rng.gen_range(0..50))); } fn spawn(mut commands: Commands, mut e: ResMut>) { *e = Some(commands.spawn().insert_bundle(()).id()); } fn despawn(mut commands: Commands, e: Res>) { let mut rng = rand::thread_rng(); std::thread::sleep(Duration::from_millis(rng.gen_range(0..50))); if let Some(e) = *e { commands.entity(e).despawn(); } } fn main() { App::build() .add_system(cleanup.system().label("cleanup")) .add_system(sleep_randomly.system().label("before_despawn")) .add_system(despawn.system().after("cleanup").after("before_despawn")) .add_system(sleep_randomly.system().label("before_spawn")) .add_system(spawn.system().after("cleanup").after("before_spawn")) .insert_resource(None::) .add_plugin(ScheduleRunnerPlugin::default()) .run(); } ``` In the cases where this example crashes, it's because `despawn` was ordered before `spawn` in the topological ordering of systems (which determines when buffers are applied). However, `despawn` actually ran *after* `spawn`, because these systems are ambiguous, so the jiggles in the sleeping time triggered a case where this works. ## Solution - Give a better error message --- crates/bevy_ecs/src/entity/mod.rs | 49 ++++++++++------------ crates/bevy_ecs/src/system/commands/mod.rs | 28 +++++++++++-- 2 files changed, 45 insertions(+), 32 deletions(-) diff --git a/crates/bevy_ecs/src/entity/mod.rs b/crates/bevy_ecs/src/entity/mod.rs index 350d83d86c699..4f4681757d3cb 100644 --- a/crates/bevy_ecs/src/entity/mod.rs +++ b/crates/bevy_ecs/src/entity/mod.rs @@ -173,6 +173,7 @@ pub struct Entities { /// Once `flush()` is done, `free_cursor` will equal `pending.len()`. pending: Vec, free_cursor: AtomicI64, + /// Stores the number of free entities for [`len`](Entities::len) len: u32, } @@ -369,12 +370,12 @@ impl Entities { } } + /// Returns true if the [`Entities`] contains [`entity`](Entity). + // This will return false for entities which have been freed, even if + // not reallocated since the generation is incremented in `free` pub fn contains(&self, entity: Entity) -> bool { - // Note that out-of-range IDs are considered to be "contained" because - // they must be reserved IDs that we haven't flushed yet. - self.meta - .get(entity.id as usize) - .map_or(true, |meta| meta.generation == entity.generation) + self.resolve_from_id(entity.id()) + .map_or(false, |e| e.generation() == entity.generation) } pub fn clear(&mut self) { @@ -399,31 +400,23 @@ impl Entities { } } - /// Panics if the given id would represent an index outside of `meta`. + /// Get the [`Entity`] with a given id, if it exists in this [`Entities`] collection + /// Returns `None` if this [`Entity`] is outside of the range of currently reserved Entities /// - /// # Safety - /// - /// Must only be called for currently allocated `id`s. - pub unsafe fn resolve_unknown_gen(&self, id: u32) -> Entity { - let meta_len = self.meta.len(); - - if meta_len > id as usize { - let meta = &self.meta[id as usize]; - Entity { - generation: meta.generation, - id, - } + /// Note: This method may return [`Entities`](Entity) which are currently free + /// Note that [`contains`](Entities::contains) will correctly return false for freed + /// entities, since it checks the generation + pub fn resolve_from_id(&self, id: u32) -> Option { + let idu = id as usize; + if let Some(&EntityMeta { generation, .. }) = self.meta.get(idu) { + Some(Entity { generation, id }) } else { - // See if it's pending, but not yet flushed. + // `id` is outside of the meta list - check whether it is reserved but not yet flushed. let free_cursor = self.free_cursor.load(Ordering::Relaxed); - let num_pending = std::cmp::max(-free_cursor, 0) as usize; - - if meta_len + num_pending > id as usize { - // Pending entities will have generation 0. - Entity { generation: 0, id } - } else { - panic!("entity id is out of range"); - } + // If this entity was manually created, then free_cursor might be positive + // Returning None handles that case correctly + let num_pending = usize::try_from(-free_cursor).ok()?; + (idu < self.meta.len() + num_pending).then(|| Entity { generation: 0, id }) } } @@ -508,7 +501,7 @@ impl EntityMeta { generation: 0, location: EntityLocation { archetype_id: ArchetypeId::INVALID, - index: usize::max_value(), // dummy value, to be filled in + index: usize::MAX, // dummy value, to be filled in }, }; } diff --git a/crates/bevy_ecs/src/system/commands/mod.rs b/crates/bevy_ecs/src/system/commands/mod.rs index 5e2873a29a39a..fc875f383c6bb 100644 --- a/crates/bevy_ecs/src/system/commands/mod.rs +++ b/crates/bevy_ecs/src/system/commands/mod.rs @@ -6,7 +6,7 @@ use crate::{ entity::{Entities, Entity}, world::World, }; -use bevy_utils::tracing::{debug, error}; +use bevy_utils::tracing::{error, warn}; pub use command_queue::CommandQueue; use std::marker::PhantomData; @@ -144,7 +144,13 @@ impl<'w, 's> Commands<'w, 's> { /// } /// # example_system.system(); /// ``` + #[track_caller] pub fn entity<'a>(&'a mut self, entity: Entity) -> EntityCommands<'w, 's, 'a> { + assert!( + self.entities.contains(entity), + "Attempting to create an EntityCommands for entity {:?}, which doesn't exist.", + entity + ); EntityCommands { entity, commands: self, @@ -371,7 +377,9 @@ pub struct Despawn { impl Command for Despawn { fn write(self, world: &mut World) { if !world.despawn(self.entity) { - debug!("Failed to despawn non-existent entity {:?}", self.entity); + warn!("Could not despawn entity {:?} because it doesn't exist in this World.\n\ + If this command was added to a newly spawned entity, ensure that you have not despawned that entity within the same stage.\n\ + This may have occurred due to system order ambiguity, or if the spawning system has multiple command buffers", self.entity); } } } @@ -386,7 +394,13 @@ where T: Bundle + 'static, { fn write(self, world: &mut World) { - world.entity_mut(self.entity).insert_bundle(self.bundle); + if let Some(mut entity) = world.get_entity_mut(self.entity) { + entity.insert_bundle(self.bundle); + } else { + panic!("Could not insert a bundle (of type `{}`) for entity {:?} because it doesn't exist in this World.\n\ + If this command was added to a newly spawned entity, ensure that you have not despawned that entity within the same stage.\n\ + This may have occurred due to system order ambiguity, or if the spawning system has multiple command buffers", std::any::type_name::(), self.entity); + } } } @@ -401,7 +415,13 @@ where T: Component, { fn write(self, world: &mut World) { - world.entity_mut(self.entity).insert(self.component); + if let Some(mut entity) = world.get_entity_mut(self.entity) { + entity.insert(self.component); + } else { + panic!("Could not add a component (of type `{}`) to entity {:?} because it doesn't exist in this World.\n\ + If this command was added to a newly spawned entity, ensure that you have not despawned that entity within the same stage.\n\ + This may have occurred due to system order ambiguity, or if the spawning system has multiple command buffers", std::any::type_name::(), self.entity); + } } }