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<Option<Entity>>) {
    *e = None;
}

fn sleep_randomly() {
    let mut rng = rand::thread_rng();
    std:🧵:sleep(Duration::from_millis(rng.gen_range(0..50)));
}

fn spawn(mut commands: Commands, mut e: ResMut<Option<Entity>>) {
    *e = Some(commands.spawn().insert_bundle(()).id());
}

fn despawn(mut commands: Commands, e: Res<Option<Entity>>) {
    let mut rng = rand::thread_rng();
    std:🧵: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::<Entity>)
        .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
This commit is contained in:
Daniel McNab 2021-09-01 20:59:25 +00:00
parent 35979922df
commit af20cad830
2 changed files with 45 additions and 32 deletions

View File

@ -173,6 +173,7 @@ pub struct Entities {
/// Once `flush()` is done, `free_cursor` will equal `pending.len()`. /// Once `flush()` is done, `free_cursor` will equal `pending.len()`.
pending: Vec<u32>, pending: Vec<u32>,
free_cursor: AtomicI64, free_cursor: AtomicI64,
/// Stores the number of free entities for [`len`](Entities::len)
len: u32, 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 { pub fn contains(&self, entity: Entity) -> bool {
// Note that out-of-range IDs are considered to be "contained" because self.resolve_from_id(entity.id())
// they must be reserved IDs that we haven't flushed yet. .map_or(false, |e| e.generation() == entity.generation)
self.meta
.get(entity.id as usize)
.map_or(true, |meta| meta.generation == entity.generation)
} }
pub fn clear(&mut self) { 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 /// Note: This method may return [`Entities`](Entity) which are currently free
/// /// Note that [`contains`](Entities::contains) will correctly return false for freed
/// Must only be called for currently allocated `id`s. /// entities, since it checks the generation
pub unsafe fn resolve_unknown_gen(&self, id: u32) -> Entity { pub fn resolve_from_id(&self, id: u32) -> Option<Entity> {
let meta_len = self.meta.len(); let idu = id as usize;
if let Some(&EntityMeta { generation, .. }) = self.meta.get(idu) {
if meta_len > id as usize { Some(Entity { generation, id })
let meta = &self.meta[id as usize];
Entity {
generation: meta.generation,
id,
}
} else { } 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 free_cursor = self.free_cursor.load(Ordering::Relaxed);
let num_pending = std::cmp::max(-free_cursor, 0) as usize; // If this entity was manually created, then free_cursor might be positive
// Returning None handles that case correctly
if meta_len + num_pending > id as usize { let num_pending = usize::try_from(-free_cursor).ok()?;
// Pending entities will have generation 0. (idu < self.meta.len() + num_pending).then(|| Entity { generation: 0, id })
Entity { generation: 0, id }
} else {
panic!("entity id is out of range");
}
} }
} }
@ -508,7 +501,7 @@ impl EntityMeta {
generation: 0, generation: 0,
location: EntityLocation { location: EntityLocation {
archetype_id: ArchetypeId::INVALID, archetype_id: ArchetypeId::INVALID,
index: usize::max_value(), // dummy value, to be filled in index: usize::MAX, // dummy value, to be filled in
}, },
}; };
} }

View File

@ -6,7 +6,7 @@ use crate::{
entity::{Entities, Entity}, entity::{Entities, Entity},
world::World, world::World,
}; };
use bevy_utils::tracing::{debug, error}; use bevy_utils::tracing::{error, warn};
pub use command_queue::CommandQueue; pub use command_queue::CommandQueue;
use std::marker::PhantomData; use std::marker::PhantomData;
@ -144,7 +144,13 @@ impl<'w, 's> Commands<'w, 's> {
/// } /// }
/// # example_system.system(); /// # example_system.system();
/// ``` /// ```
#[track_caller]
pub fn entity<'a>(&'a mut self, entity: Entity) -> EntityCommands<'w, 's, 'a> { 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 { EntityCommands {
entity, entity,
commands: self, commands: self,
@ -371,7 +377,9 @@ pub struct Despawn {
impl Command for Despawn { impl Command for Despawn {
fn write(self, world: &mut World) { fn write(self, world: &mut World) {
if !world.despawn(self.entity) { 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, T: Bundle + 'static,
{ {
fn write(self, world: &mut World) { 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::<T>(), self.entity);
}
} }
} }
@ -401,7 +415,13 @@ where
T: Component, T: Component,
{ {
fn write(self, world: &mut World) { 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::<T>(), self.entity);
}
} }
} }