Potential surprising "entity does not exist" panic due to asynchronous command execution

[infmagic2047]

Bevy version

0.9.1

What went wrong

These two systems look innocent on their own, but may cause panic if used together.

fn insert(mut commands: Commands, query: Query<Entity, With<Marker>>, ...) {
    for entity in query.iter() {
        if ... {
            // We obtained the entity from the query, so it must be valid, right?
            commands.entity(entity).insert(Foo);
        }
    }
}

fn despawn(mut commands: Commands, query: Query<Entity, With<Marker>>, ...) {
    for entity in query.iter() {
        if ... {
            // This command may execute before the insertion and cause a panic there!
            commands.entity(entity).despawn();
        }
    }
}

This panic is hard to avoid in user code, as there is no way to know if there is a pending despawn command for a certain entity.

Additional information

The Insert command is, as far as I know, the only place where a missing entity in command execution is turned into a panic. We should at least replace the panic with a soft error or warning like the other commands.

We are also currently inconsistent in the way missing entities are reported. Insert uses panics, Remove uses no error reporting at all (we may want to change this), Despawn uses warnings, and DespawnRecursive use debug messages.

[infmagic2047]

Related issue: #3845.

I strongly disagree with the suggestion of "just use system ordering" in this case. There's no explicit indicator that the two systems are order-independent (the system order ambiguity reporter doesn't catch it, for example), and the panic can be hard to reproduce and only occurs in specific conditions, so unless one is extremely careful with system ordering, they may end up with a game that just randomly crashes for no obvious reasons.

In my code I work around this problem by deferring all despawns to a later stage. I use events to mark entities for removal, and ban all despawning commands outside a dedicated despawning system that reads these events. Clearly this is not the solution we want for all users.

I think most such issues can be solved by applying despawning commands after other commands. Is this a solution we want? It may add more overhead to commands and needs a good benchmark to test.

[mockersf]

I think most such issues can be solved by applying despawning commands after other commands. Is this a solution we want? It may add more overhead to commands and needs a good benchmark to test.

Reordering commands across system could be hard as there are independent queue for each system. Doing something specific for one command, like keeping Despawn commands to the end, would not be simple as commands are kept as a dynamic type until their execution time. And then what of other commands that despawn, like DespawnRecursive and friends? And for custom commands that can despawn too?

Currently the consensus is that commands that comes from a "legit" entity (for example, one from a query) should not panic, but just be ignored if the entity is not live when time comes to execute it. It would also be nice to have command batching.

[Preston-Harrison]

Would I be correct in using the following as a workaround?

if !commands.get_entity(entity).is_none() {
    // continue with rest of logic
}