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.