Despawning entities should silently fail if the entity does not exist

[alice-i-cecile]

What problem does this solve or what need does it fill?

Panics when working with commands are extremely frustrating, and difficult to debug or guard against.

What solution would you like?

We can reduce the pain in the common case by making commands.entity(my_entity).despawn() fail silently if the entity does not exist.

There is no real risk to doing so, as the desired state is completed. This is also useful as it makes despawn commands idempotent, making them significantly more robust and easier to work with.

What alternative(s) have you considered?

#2004 represents a more complete solution, but is dramatically more complex. This is also a better default for this particular command.

Additional context

We may also want to change the behavior of other commands, but that should be done in separate PRs to avoid blocking this less controversial change.

[mockersf]

I'm very against commands silently failing.

While it's not an issue in the case of despawn, it still means the game is not in the state expected by the system (an entity has already been despawned when the system thought it was still present) and so could hide a bigger bug in the user code.

Letting the user choose to ignore it could be good, but the default must be to panic

[Nilirad]

Could warning instead silently fail be a less controvertial alternative to this?

[alice-i-cecile]

IMO the correct level would be dbg!; it's perfectly reasonable to rely on this behavior in production.

[mockersf]

Could warning instead silently fail be a less controvertial alternative to this?

Not for me at least 🤷

For log level, I guess at least warning. Anyway it wouldn't surface in a released game because the log levels would have been appropriately set to not surface bevy logs as they aren't relevant.

[arnavc52]

Another approach would be to keep the existing despawn unchanged and create another method (maybe despawn_unchecked or despawn_silent?) that silently fails. That way, people can use whichever one they want.

The documentation for despawn_unchecked/despawn_silent should probably mention that people should use the normal (i.e., panicking) one whenever possible.

[Nilirad]

That would increase the API complexity for no actual benefit. Why would someone use despawn_unchecked instead of despawn? To bypass an error feedback that's for the user's advantage?

In the case of a user always using despawn_unchecked instead, we're giving them a footgun to play with.

IMO the best we can do is leave things as is (panicking is not a big issue, you fix the bug and you go), or you get a warning (app still goes, so you can continue to develop/debug, but you get a reminder that you have to fix it).

[tkgalk]

I agree with others. When I call despawn() and there was nothing to despawn() it should be counted as an error. It's always better to be explicit and check before despawning if it exists or not.

[arnavc52]

If people want it to silently fail, they can already use World::despawn. EntityCommands::despawn is simply a wrapper around it.

Another approach would be for EntityCommands::despawn to return a Result instead of panic!ing. IMO this is just a better way of doing error handling and aligns better with the general philosophy of Rust.

[alice-i-cecile]

That's useful data about the behaviour of World::despawn.

Proper result-based error handling for Commands is very high up on my list of "critical problems that need fixing", but the design is nontrivial due to the deferred nature of the work.

[mockersf]

Another approach would be for EntityCommands::despawn to return a Result instead of panic!ing. IMO this is just a better way of doing error handling and aligns better with the general philosophy of Rust.

If commands were able to return anything about their execution that would be great, but commands are executed only once the system is finished...

[JoJoJet]

I don't see how adding a despawn_unchecked or try_despawn method would present a potential footgun. The meaning is fairly clear: "despawn this entity if it exists. If it doesn't exist, that's fine". It's useful for situations in which you just want to make sure that an entity does not exist anymore, but you don't really care whether or not a different system happened to despawn it in the meantime.

[kaphula]

I think despawn should not panic by default since the default parallel nature of Bevy makes it hard to reason and verify the timeline of entity lifetimes (not Rust lifetimes) regardless unless you go all in planning your world sync points carefully, which I personally don't do if I can get away with it. By getting away with it I mean that most of the ECS code is written to update entities and their components when they exist and are ready to be updated, otherwise the code just does something else or nothing while waiting for the entities to appear (which can happen whenever).

[alice-i-cecile]

Despawn no longer panics, but it does warn (which I think it shouldn't). These logs are super noisy with state scoped entities as well.

[Wabtey]

In favor argument for no panic/silence.
If you despawn the child 1v1 of entity 1v0 and then despawn_recursive the entity 1v0 then the link between 1v0 and 1v1 will trigger the warning bevy_ecs::world: error[B0003].

[nixpulvis]

I'm getting a lot of these warnings, but I'm not quite sure why. I have a system with an argument query: Query<Entity, With<MyMarker>> then:

for entity in query.iter() {
    commands.entity(entity).despawn_recursive()
}

My only though is that I might be overflowing the entity ID space?

[alice-i-cecile]

If you're querying for multiple entities per tree, many of the children will be despawned by the time the corresponding trees are processed.

[nixpulvis]

I get it without _recursive as well. I'm calling it within an Update system which listens for data to essentially clear the clear before I put new data into the world. It happens when many such updates happen quickly back to back.

[spacemen0]

Hello! Anyone has a good practice to get rid of this warning? I've been working around this for several hours and can't really find a solution. In my case, the bullet in the game despawns both after hitting an enemy and after reaching its own lifespan. i printed the entity id in two separate systems and it became apparent that it was both systems attempting to despawn the same entity that caused this warning.

[alice-i-cecile]

Writing your own custom despawn command is easy, and this warning won't occur simply when calling world.despawn. For now, I'd recommend that to anyone annoyed by this warning.

[spacemen0]

took a look at the source code and i think world.despawn still raises the warning:

    #[inline]
    pub fn despawn(&mut self, entity: Entity) -> bool {
        self.flush();
        if let Some(entity) = self.get_entity_mut(entity) {
            entity.despawn();
            true
        } else {
            warn!("error[B0003]: Could not despawn entity {:?} because it doesn't exist in this World. See: https://bevyengine.org/learn/errors/#b0003", entity);
            false
        }
    }

but i did this and it worked:

commands.add(move |world: &mut World| {
                if let Some(entity) = world.get_entity_mut(e) {
                    entity.despawn();
                }
            });

Thanks!

Writing your own custom despawn command is easy, and this warning won't occur simply when calling world.despawn. For now, I'd recommend that to anyone annoyed by this warning.