EntityCommands should have methods to fail gracefully #3845
Comments
alice-i-cecile
added
A-ECS
alice-i-cecile
changed the title
|
The critical challenge here is that we need to check evaluation at the time of Command evaluation, not command issuing. |
alice-i-cecile
added
the
S-Needs-Design
I tried to solve this with:
As said on discord, I dislike this option as I worry people would jump on ignoring failures instead of fixing their system ordering, potentially leaving their game in an unexpected state |
|
I want to respectfully push back against this idea, or at least my perception of this idea, that those of us experiencing this just need to reorder our systems already. :) There really aren't any good tools for doing this at scale, without introducing all sorts of brittle cross-plugin dependencies. So say I have a damage/health system that despawns any entities with < 0 damage. That means I...run every single system before this To be clear, I'm fine with methods like |
|
Some other solutions that might help reduce these problems:
|
in the cases that panics now, there are two commands to apply on the same entity: a despawn, then a component insert. How would it work with refcounting? would it be the despawn that would panic? |
|
One graceful method ( |
## Objective Fixes #2004 Fixes #3845 Fixes #7118 Fixes #10166 ## Solution - The crux of this PR is the new `Command::with_error_handling` method. This wraps the relevant command in another command that, when applied, will apply the original command and handle any resulting errors. - To enable this, `Command::apply` and `EntityCommand::apply` now return `Result`. - `Command::with_error_handling` takes as a parameter an error handler of the form `fn(&mut World, CommandError)`, which it passes the error to. - `CommandError` is an enum that can be either `NoSuchEntity(Entity)` or `CommandFailed(Box<dyn Error>)`. ### Closures - Closure commands can now optionally return `Result`, which will be passed to `with_error_handling`. ### Commands - Fallible commands can be queued with `Commands::queue_fallible` and `Commands::queue_fallible_with`, which call `with_error_handling` before queuing them (using `Commands::queue` will queue them without error handling). - `Commands::queue_fallible_with` takes an `error_handler` parameter, which will be used by `with_error_handling` instead of a command's default. - The `command` submodule provides unqueued forms of built-in fallible commands so that you can use them with `queue_fallible_with`. - There is also an `error_handler` submodule that provides simple error handlers for convenience. ### Entity Commands - `EntityCommand` now automatically checks if the entity exists before executing the command, and returns `NoSuchEntity` if it doesn't. - Since all entity commands might need to return an error, they are always queued with error handling. - `EntityCommands::queue_with` takes an `error_handler` parameter, which will be used by `with_error_handling` instead of a command's default. - The `entity_command` submodule provides unqueued forms of built-in entity commands so that you can use them with `queue_with`. ### Defaults - In the future, commands should all fail according to the global error handling setting. That doesn't exist yet though. - For this PR, commands all fail the way they do on `main`. - Both now and in the future, the defaults can be overridden by `Commands::override_error_handler` (or equivalent methods on `EntityCommands` and `EntityEntryCommands`). - `override_error_handler` takes an error handler (`fn(&mut World, CommandError)`) and passes it to every subsequent command queued with `Commands::queue_fallible` or `EntityCommands::queue`. - The `_with` variants of the queue methods will still provide an error handler directly to the command. - An override can be reset with `reset_error_handler`. ## Future Work - After a universal error handling mode is added, we can change all commands to fail that way by default. - Once we have all commands failing the same way (which would require either the full removal of `try` variants or just making them useless while they're deprecated), `queue_fallible_with_default` could be removed, since its only purpose is to enable commands having different defaults.
…17043) ## Objective Fixes bevyengine#2004 Fixes bevyengine#3845 Fixes bevyengine#7118 Fixes bevyengine#10166 ## Solution - The crux of this PR is the new `Command::with_error_handling` method. This wraps the relevant command in another command that, when applied, will apply the original command and handle any resulting errors. - To enable this, `Command::apply` and `EntityCommand::apply` now return `Result`. - `Command::with_error_handling` takes as a parameter an error handler of the form `fn(&mut World, CommandError)`, which it passes the error to. - `CommandError` is an enum that can be either `NoSuchEntity(Entity)` or `CommandFailed(Box<dyn Error>)`. ### Closures - Closure commands can now optionally return `Result`, which will be passed to `with_error_handling`. ### Commands - Fallible commands can be queued with `Commands::queue_fallible` and `Commands::queue_fallible_with`, which call `with_error_handling` before queuing them (using `Commands::queue` will queue them without error handling). - `Commands::queue_fallible_with` takes an `error_handler` parameter, which will be used by `with_error_handling` instead of a command's default. - The `command` submodule provides unqueued forms of built-in fallible commands so that you can use them with `queue_fallible_with`. - There is also an `error_handler` submodule that provides simple error handlers for convenience. ### Entity Commands - `EntityCommand` now automatically checks if the entity exists before executing the command, and returns `NoSuchEntity` if it doesn't. - Since all entity commands might need to return an error, they are always queued with error handling. - `EntityCommands::queue_with` takes an `error_handler` parameter, which will be used by `with_error_handling` instead of a command's default. - The `entity_command` submodule provides unqueued forms of built-in entity commands so that you can use them with `queue_with`. ### Defaults - In the future, commands should all fail according to the global error handling setting. That doesn't exist yet though. - For this PR, commands all fail the way they do on `main`. - Both now and in the future, the defaults can be overridden by `Commands::override_error_handler` (or equivalent methods on `EntityCommands` and `EntityEntryCommands`). - `override_error_handler` takes an error handler (`fn(&mut World, CommandError)`) and passes it to every subsequent command queued with `Commands::queue_fallible` or `EntityCommands::queue`. - The `_with` variants of the queue methods will still provide an error handler directly to the command. - An override can be reset with `reset_error_handler`. ## Future Work - After a universal error handling mode is added, we can change all commands to fail that way by default. - Once we have all commands failing the same way (which would require either the full removal of `try` variants or just making them useless while they're deprecated), `queue_fallible_with_default` could be removed, since its only purpose is to enable commands having different defaults.
What problem does this solve or what need does it fill?
Commands failing due to an entity being despawned is one of the most common and frustrating sources of panics in Bevy at the moment.
What solution would you like?
What alternative(s) have you considered?
Try
For each of the existing methods on
EntityCommands, unwrap this option. Create atry_Xvariant for each method.This creates significant API bloat and doesn't capture the way that entity commands are chained: after the first operation has succeeded, the rest of the operations in the chain are infallible.
Verified entity flag
Store a
verified_entity_existsOption flag onEntityCommands. Each of the existing methods onEntityCommandsshould attempt to set this flag by unwrapping it toSome(true) if it isNone`, and be skipped if it is false.Then, create a few methods on
EntityCommandsthat set the value of the verification flag with varying behavior:commands.entity(entity).silently_fail_if_missing().insert(Foo)commands.entity(entity).log_if_missing().insert(Foo)commands.entity(entity).warn_if_missing().insert(Foo)This doesn't work though, because the check is occuring at command-issuing time.
Additional context
#2241 proposes a much more general approach to this, but has significant complexity and overhead.
#3757 aims for a similar goal, but doesn't actually create a reliable API: entities can be despawned after we check for their existence, resulting in the same panic.
@EmiOnGit expressed interest in this problem in #3840.
The text was updated successfully, but these errors were encountered: