Skip to content

Commit

Permalink
Add error messages for the spooky insertions (#2581)
Browse files Browse the repository at this point in the history
# 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::thread::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::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::<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
  • Loading branch information
DJMcNab committed Sep 1, 2021
1 parent 3597992 commit af20cad
Show file tree
Hide file tree
Showing 2 changed files with 45 additions and 32 deletions.
49 changes: 21 additions & 28 deletions crates/bevy_ecs/src/entity/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,7 @@ pub struct Entities {
/// Once `flush()` is done, `free_cursor` will equal `pending.len()`.
pending: Vec<u32>,
free_cursor: AtomicI64,
/// Stores the number of free entities for [`len`](Entities::len)
len: u32,
}

Expand Down Expand Up @@ -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) {
Expand All @@ -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<Entity> {
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 })
}
}

Expand Down Expand Up @@ -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
},
};
}
Expand Down
28 changes: 24 additions & 4 deletions crates/bevy_ecs/src/system/commands/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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);
}
}
}
Expand All @@ -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::<T>(), self.entity);
}
}
}

Expand All @@ -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::<T>(), self.entity);
}
}
}

Expand Down

0 comments on commit af20cad

Please sign in to comment.