Configurable error handling in commands

alice-i-cecile commented 3 years ago

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 https://github.com/bevyengine/rfcs/pull/18

Frizi commented 3 years ago

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 Spring relation target_mass -> m1 to target_mass -> m2. There are multiple ways it can fail:

the new target m2 might not exist at command execution time, what should we do with relation target_mass -> m1?
the old target m1 might already be deleted, which also deletes it's relations. We end up with no relation on pointing to m2.
any other combination of m1, m2, or target_mass can be deleted
there might be no relation to move (relation target_mass -> m1 was not there to begin with)

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 FnOnce + 'commands closure parameter to handle those failures.

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.

TehPers commented 3 years ago

I like the idea of having a callback for failures. This means that something more robust can be built with MPSC using Sender in the callback and another system that reads for failures from a Receiver. That way if a command fails, you can have more advanced error handling like updating the game state to an error state or changing how some entities are spawned or such.

cart commented 3 years ago

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).

NathanSWard commented 3 years ago

We should note #2219 as being an example of this issue directly affecting a developer and causing a quite bad UX. Once I get settled on a better bevy-development-workflow, this appears to be once of the issues we should prioritize. I have some testing going locally for possible solutions to this and may present them in the near future.

mockersf commented 3 years ago

They could even be Systems

I think that's a very good idea. We should do https://github.com/bevyengine/bevy/issues/2192 first, and then error handling could build on that?

NathanSWard commented 3 years ago

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 Box where a currently implementation I'm working on doesn't require this extra allocation. Also, a Command might have a specific Error type that it wants to hand the error_handler but this won't be easily done if the handler has to be a system.

NathanSWard commented 3 years ago

If anyone if curious, I have an initial implementation of this at #2241

alice-i-cecile commented 1 year ago

I've implemented a few simple methods (try_insert, try_remove and try_add_child) in this PR: https://github.com/Leafwing-Studios/Emergence/pull/765 externally.

Feel free to steal it directly (MIT license) either for Bevy or your own projects. Works perfectly for what I need.

bevyengine / bevy