[Merged by Bors] - Add error messages for the spooky insertions #2581

DJMcNab · 2021-08-01T17:05:32Z

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:

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:

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

crates/bevy_ecs/src/system/commands/mod.rs

cart · 2021-08-01T19:22:24Z

crates/bevy_ecs/src/system/commands/mod.rs

+        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\


"no longer" implies that it did exist at some point, which isn't necessarily true (although this should generally be true)

Perhaps? I think that making error messages 'worse' because someone has manually created an entity id is not the tradeoff I'd prefer to make.

We can change this to "doesn't exist" though if you're adamant.

For docs and error messages I'd prefer to not say things that are "approximately true". Omitting information in the interest of clarity is ok, but I'd prefer to keep this crisp, especially given that the new renderer "spawns at specific entities" / "shares entities across worlds".

alice-i-cecile · 2021-08-02T20:27:20Z

This should be added for despawn as well, as the entity can fail to exist in the same way.

mockersf · 2021-08-09T19:28:31Z

crates/bevy_ecs/src/entity/mod.rs

@@ -162,6 +162,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)


wouldn't that Entities::len link just be a link to itself?

I don't think so? There's a function also called len, which this should point to

I can check though at some point. It's only an internal documentation anyway, since it's on a private field.

Oh I didn't see the method with the same name in this diff 👍

Anyway I can't get rust-analyser to correctly link to something from Entities, not sure why...

rust-analyser's intra doc links syntax is a subset of valid links, from what I've found.

The way to try this is with cargo doc -- --document-private-items (or whatever the precise correct syntax is)

I have confirmed that it links correctly

There are several other bugs which running cargo doc --open --document-private-items makes appear, but that's not the job of this PR.

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.

cart · 2021-09-01T20:59:10Z

bors r+

# 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

bors · 2021-09-01T21:21:35Z

Pull request successfully merged into main.

Build succeeded:

github-actions bot added the S-Needs-Triage This issue needs to be labelled label Aug 1, 2021

alice-i-cecile reviewed Aug 1, 2021

View reviewed changes

crates/bevy_ecs/src/system/commands/mod.rs Outdated Show resolved Hide resolved

alice-i-cecile reviewed Aug 1, 2021

View reviewed changes

crates/bevy_ecs/src/system/commands/mod.rs Outdated Show resolved Hide resolved

alice-i-cecile added C-Usability A simple quality-of-life change that makes Bevy easier to use A-ECS Entities, components, systems, and events and removed S-Needs-Triage This issue needs to be labelled labels Aug 1, 2021

alice-i-cecile approved these changes Aug 1, 2021

View reviewed changes

cart reviewed Aug 1, 2021

View reviewed changes

alice-i-cecile approved these changes Aug 3, 2021

View reviewed changes

mockersf reviewed Aug 9, 2021

View reviewed changes

This was referenced Aug 17, 2021

thread 'main' panicked at 'Entity does not exist' #1743

Closed

thread 'main' panicked at 'Entity does not exist' #2668

Closed

DJMcNab mentioned this pull request Aug 19, 2021

[Merged by Bors] - Reduce visibility of various types and fields #2690

Closed

DJMcNab added 2 commits August 31, 2021 23:20

Add error messages for the spooky insertions

2b00904

DJMcNab force-pushed the jeepers-creepers branch from bbe568e to 8624bbe Compare August 31, 2021 22:24

Make error messages less useful

8a2103e

bors bot changed the title ~~Add error messages for the spooky insertions~~ [Merged by Bors] - Add error messages for the spooky insertions Sep 1, 2021

bors bot closed this Sep 1, 2021

DJMcNab deleted the jeepers-creepers branch September 1, 2021 21:24

DJMcNab mentioned this pull request Oct 4, 2021

LD49 Post-Mortem -- First use of Bevy & learnings #2914

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Merged by Bors] - Add error messages for the spooky insertions #2581

[Merged by Bors] - Add error messages for the spooky insertions #2581

DJMcNab commented Aug 1, 2021

cart Aug 1, 2021

DJMcNab Aug 1, 2021

cart Sep 1, 2021

DJMcNab Sep 1, 2021

cart Sep 1, 2021

alice-i-cecile commented Aug 2, 2021

mockersf Aug 9, 2021

DJMcNab Aug 10, 2021

mockersf Aug 10, 2021

DJMcNab Aug 10, 2021

DJMcNab Aug 31, 2021

cart commented Sep 1, 2021

bors bot commented Sep 1, 2021

[Merged by Bors] - Add error messages for the spooky insertions #2581

[Merged by Bors] - Add error messages for the spooky insertions #2581

Conversation

DJMcNab commented Aug 1, 2021

Objective

Solution

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alice-i-cecile commented Aug 2, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cart commented Sep 1, 2021

bors bot commented Sep 1, 2021