From 2b00904978d2adfd65fe7f57e89c0a9fdaa2609c Mon Sep 17 00:00:00 2001 From: Daniel McNab <36049421+DJMcNab@users.noreply.github.com> Date: Sun, 1 Aug 2021 18:01:57 +0100 Subject: [PATCH 1/3] Add error messages for the spooky insertions --- crates/bevy_ecs/src/entity/mod.rs | 2 +- crates/bevy_ecs/src/system/commands/mod.rs | 16 ++++++++++++++-- 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/crates/bevy_ecs/src/entity/mod.rs b/crates/bevy_ecs/src/entity/mod.rs index 350d83d86c699..1d3d4bc95f548 100644 --- a/crates/bevy_ecs/src/entity/mod.rs +++ b/crates/bevy_ecs/src/entity/mod.rs @@ -508,7 +508,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..98cf39acc4874 100644 --- a/crates/bevy_ecs/src/system/commands/mod.rs +++ b/crates/bevy_ecs/src/system/commands/mod.rs @@ -386,7 +386,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 for entity {:?} because it no longer exists.\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); + } } } @@ -401,7 +407,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 to entity {:?} because it no longer exists.\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); + } } } From 8624bbe0ff348f0a3fcc1eade30e3cff1fcc20a0 Mon Sep 17 00:00:00 2001 From: Daniel McNab <36049421+DJMcNab@users.noreply.github.com> Date: Mon, 2 Aug 2021 22:18:41 +0100 Subject: [PATCH 2/3] Rework `resolve_unknown_gen` Previously, it paniced when the value was out of the range, it now returns an Option. It was also erroniously marked as `unsafe`, even though it is a safe method I've renamed it - hopefully this rename makes sense I've also reworked `contains` to use this method, which allows it to be more precise about out-of-range entities. --- crates/bevy_ecs/src/entity/mod.rs | 47 +++++++++------------- crates/bevy_ecs/src/system/commands/mod.rs | 20 ++++++--- 2 files changed, 34 insertions(+), 33 deletions(-) diff --git a/crates/bevy_ecs/src/entity/mod.rs b/crates/bevy_ecs/src/entity/mod.rs index 1d3d4bc95f548..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 }) } } diff --git a/crates/bevy_ecs/src/system/commands/mod.rs b/crates/bevy_ecs/src/system/commands/mod.rs index 98cf39acc4874..a5e821e58c525 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 no longer exists.\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); } } } @@ -389,9 +397,9 @@ where if let Some(mut entity) = world.get_entity_mut(self.entity) { entity.insert_bundle(self.bundle); } else { - panic!("Could not insert a bundle for entity {:?} because it no longer exists.\n\ + panic!("Could not insert a bundle (of type `{}`) for entity {:?} because it no longer exists.\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); + This may have occurred due to system order ambiguity, or if the spawning system has multiple command buffers", std::any::type_name::(), self.entity); } } } @@ -410,9 +418,9 @@ where if let Some(mut entity) = world.get_entity_mut(self.entity) { entity.insert(self.component); } else { - panic!("Could not add a component to entity {:?} because it no longer exists.\n\ + panic!("Could not add a component (of type `{}`) to entity {:?} because it no longer exists.\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); + This may have occurred due to system order ambiguity, or if the spawning system has multiple command buffers", std::any::type_name::(), self.entity); } } } From 8a2103e1706042d04b412d6d5c5d8651bcb73b44 Mon Sep 17 00:00:00 2001 From: Daniel McNab <36049421+DJMcNab@users.noreply.github.com> Date: Wed, 1 Sep 2021 21:45:16 +0100 Subject: [PATCH 3/3] Make error messages less useful --- crates/bevy_ecs/src/system/commands/mod.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/crates/bevy_ecs/src/system/commands/mod.rs b/crates/bevy_ecs/src/system/commands/mod.rs index a5e821e58c525..fc875f383c6bb 100644 --- a/crates/bevy_ecs/src/system/commands/mod.rs +++ b/crates/bevy_ecs/src/system/commands/mod.rs @@ -377,7 +377,7 @@ pub struct Despawn { impl Command for Despawn { fn write(self, world: &mut World) { if !world.despawn(self.entity) { - warn!("Could not despawn entity {:?} because it no longer exists.\n\ + 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); } @@ -397,7 +397,7 @@ where 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 no longer exists.\n\ + 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); } @@ -418,7 +418,7 @@ where 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 no longer exists.\n\ + 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); }