-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Configurable error handling in commands #2004
Comments
The "robustness" argument actually backfires here in some circumstances. Things might not get done for various reason, but the logic might rely on them being done. That means we get extremally hard to detect bugs, and often impossible to solve when no opportunity to handle those failure modes is given. An especially involved example comes from drafted relations RFC: commands.entity(target_mass).change_target::<Spring>(m1, m2);
Here we intend to modify
Doing nothing at all in all those situations by default is a little scary. Some of those failures might be unexpected, and influence correctness of implemented game logic if they happen. Saying "things might get done if all stars align" sounds very fragile and hard to rely on. Also in some cases, there are still decisions to be made at the last second, like what to do with the relation that was supposed to be moved out, but there is nowhere to go? One way to fix that would be to allow adding extra commands.entity(target_mass)
.change_target::<Spring>(m1, m2)
.on_failure(|failure_mode| {
match failure_mode {
// when move failed, remove relation that was supposed to be moved
ChangeTargetFailure::NewTargetMissing(decision) => decision.remove_relation(),
// expected, do nothing
ChangeTargetFailure::OldTargetMissing | ChangeTargetFailure::RelationMissing => {},
_ => panic!("unexpected failure {:?}", failure_mode)
}
}); A lot of things to bikeshed here of course, but I hope this gets the general idea through. This would allow logging on some unexpected behaviour, panic on straight up algorithm correctness violations, or do last-second decisions on what action to take. This of course applies not only to relations, but things like moving a component between entities or other actions that can fail. Things that can be done are of course limited by the closure lifetime, but this can be further expanded if needed. Adding some more context to that closure could possibly allow more complex behaviours in response to fails, e.g. by allowing to issue more commands. |
I like the idea of having a callback for failures. This means that something more robust can be built with MPSC using |
In theory these callbacks could have arbitrary World access, as they would be executed during command application. They could even be Systems (although this would likely incur overhead if every entity needs to initialize the same system). That flexibility might also change if we ever adopt more granular command synchronization (ex: the "stageless" design we've been discussing). |
We should note #2219 as being an example of this issue directly affecting a developer and causing a quite bad UX. |
I think that's a very good idea. We should do #2192 first, and then error handling could build on that? |
I'm not the biggest fan of this necessarily, mostly because then we have to wrap the error handler in a |
If anyone if curious, I have an initial implementation of this at #2241 |
I've implemented a few simple methods (try_insert, try_remove and try_add_child) in this PR: Leafwing-Studios/Emergence#765 externally. Feel free to steal it directly (MIT license) either for Bevy or your own projects. Works perfectly for what I need. |
What problem does this solve or what need does it fill?
Commands (like removing components or despawning entities) may fail for various reasons. Right now, for the sake of idempotency (the same function applied twice should do the same thing as the function applied once), robustness and not spamming the user endlessly, these silently fail.
This is not always the desired behavior, and should be configurable at the call site.
Possible solutions
Approach to config
Global config is convenient for reducing boilerplate, but the desired behavior may fail by call site.
Local config is useful for both debugging and persistent configuration.
Default behavior
This could panic, warn or fail silently.
Patterns
More methods: this is bad for API explosion.
Extra argument: this is very verbose for common operations because Rust doesn't have default arguments
Builder pattern: nice, as long as we don't have to use it.
Additional context
@Frizi, @BoxyUwU and myself ran into this while discussing new commands for relations in bevyengine/rfcs#18
The text was updated successfully, but these errors were encountered: