Command error handling

[NathanSWard]

Fixes: #2004

Some new traits were introducted:

• FallibleCommand
  • this is for command that can potentially fail, and allow people to provide error handlers if an error occurs
• CommandErrorHandling
  • this is for function like types that can handle command errors.

If a command is fallible, then new return type is a Config type.
The Config type allows a user to optionally provide an error handler via on_err_do.
However, I added some builtin handlers (via CommandErrorHandler) like ignore() panic() and log() for some typical use cases of what someone might want to do with an error.
note: the default behavior is log(). None of the commands will panic! but will instead log the error via error!.

EXAMPLES:

fn system(mut commands: Commands) {
    commands.entity(e)
        .insert(32)
        .on_err_do(|error| { /*..*/ });

    command
        .remove_resource::<f32>()
        .on_err(CommandErrorHandler::ignore);

    commands
        .entity(e)
        .despawn()
        .on_err(CommandErrorHandler::panic);

    commands
        .entity(e)
        .insert(32)
        .insert(3.14); // you can also ignore the error handling and just chain regular commands
}

[alice-i-cecile]

Cool! Can you add an example for this? It feels like it belongs beside the testing example, but you'll need to change the folder name.

[mockersf]

I would prefer to use a System<In = Error, Out = ()> rather than introduce a new kind of closure with World access

[NathanSWard]

I would prefer to use a System<In = Error, Out = ()> rather than introduce a new kind of closure with World access

this is a fair concern, however is it possible to create a system without allocations?
Specifically, I went this route since I don't have to wrap the error handler in a Box<_>

Also, I feel this adds a lot of unnecessary baggage to what is supposed to be a simple error handler.
True exposing &mut World to a random lambda is weird, but unless there's a specific reason to wrap it in a System, should we?

[alice-i-cecile]

I was thinking that it might be valuable to have a global error handling for certain classes of error, but on reflection I feel that's an antipattern. I think that the better approach is going to be to provide nice functionality for enforcing error handling in whatever form as part of a Bevy-specific linter (#1602).

[mockersf]

this is a fair concern, however is it possible to create a system without allocations?

Not sure, it would probably be hard to capture all the types needed for a FunctionSystem

Specifically, I went this route since I don't have to wrap the error handler in a Box<_>

Why not? I'm not sure of the impact of Boxing a system.

Also, I feel this adds a lot of unnecessary baggage to what is supposed to be a simple error handler.
True exposing &mut World to a random lambda is weird, but unless there's a specific reason to wrap it in a System, should we?

It's more baggage in the implementation, but I feel it's less for the user. but then as there's world access, you end up using the same api in your error handler as in an exclusive system, right?

[alice-i-cecile]

True exposing &mut World to a random lambda is weird, but unless there's a specific reason to wrap it in a System, should we?

IME reading / writing normal systems is much easier than exclusive systems, in large part due to the implicit documentation of the function parameters.

This also makes any future migration to running these things in automatic parallel much less painful, since they're already scoped.

[NathanSWard]

It's more baggage in the implementation, but I feel it's less for the user. but then as there's world access, you end up using the same api in your error handler as in an exclusive system, right?

Yea that's fair :)
And as @alice-i-cecile said, it would be easier to parallelize these if that ever becomes possible in the future.

Why not? I'm not sure of the impact of Boxing a system

It's just extra memory allocations that I'm trying to avoid if possible.
However, I guess this'll be ok since the memory allocations only happens in the path where you actually specify an error handler.

[TheRawMeatball]

Personally I'd rather not make it so just adding an error handler requires an heap allocation, as this will discourage adding them. Another point is that while exclusive systems can run regular systems, and there are plans to make direct world access more ergonomic with a more system-like API, the reverse isn't true. I'd recommend having a barebones error handling mechanism directly based on &mut World, especially for this first PR.

[alice-i-cecile]

Having seen the results of your experiment, I think @TheRawMeatball is probably right above, as much as it pains me to encourage more "operate directly on the entire world" logic.

This needs to be ergonomic, and the heap allocation is a little 😬.

[alice-i-cecile]

Looks nice! This should be very useful.

In addition to my comments, I'd like to see:

1. User-facing example code.
2. Unhappy-path tests for each fallible command to verify that they do in fact error when we expect them to.

[alice-i-cecile]

This is a very serious improvement, and will make using commands much safer. I would expect us to persist with this style of API in some form even as we reduce our reliance on Commands per se moving forward.

[cart]

Just gave this a look. I think this is generally the right direction (until we overhaul Commands). I think the multiple on_err / on_err_do functions are a bit confusing. And the types + inference are a bit unwieldy.

I just pushed some (proposed) changes that simplify the implementation, remove the need for on_err_do, and improve ergonomics in some cases. With these changes, only a single on_err function is needed and the error type never needs to be specified (because it is inferred).

I think we should do a quick benchmark to see what this error handling costs us in practice compared to the previous impl. Probably something along the lines of:

1. Despawning LARGE_NUMBER entities with a error handler identical to the previous inlined "debug log" impl. This tests the "hard coded debug log" -> "command error debug log" case.
2. Spawning LARGE_NUMBER entities (without explicit error handling). This tests the "previously had no error handling" -> "now has implicit error handling" case.

[NathanSWard]

I think we should do a quick benchmark to see what this error handling costs us in practice compared to the previous impl. Probably something along the lines of:

We can use the commands bench-marking from #2334 once that gets merged.
(Or more specifically add additional benchmarks that test error handling)

[NathanSWard]

I ran some perf-tests for this and had the following results:

group                          DESPAWN-ERROR-MAIN                     DESPAWN-ERROR-NEW
-----                          ------------------                     -----------------
despawn_error/0_entities       1.00     10.0±0.46ns        ? ?/sec    1.03     10.2±0.26ns        ? ?/sec
despawn_error/1000_entities    1.00     54.6±2.26µs        ? ?/sec    1.07     58.7±2.70µs        ? ?/sec
despawn_error/1500_entities    1.00     77.1±1.92µs        ? ?/sec    1.12     86.5±2.70µs        ? ?/sec
despawn_error/2000_entities    1.00    103.4±3.29µs        ? ?/sec    1.10    114.2±4.72µs        ? ?/sec
despawn_error/500_entities     1.00     29.6±0.74µs        ? ?/sec    1.07     31.7±1.63µs        ? ?/sec

The error-handling version (unsurprisingly) took a little performance hit since now we store an error-handler, and there is extra logic for when you actually add an error handler.
I'm going to see if I can change anything to help reduce the perf impact, though if anyone has ideas, please shoot them my way :)

[NathanSWard]

pinging here.
Is this PR still a feature people would like implemented?
I've been trying to fix the performance degradation recently and have come up with a possible solution.
(specifically pinging @alice-i-cecile)

[alice-i-cecile]

Is this PR still a feature people would like implemented?

Yes please! I'd be happy to review this again once it's ready. There may be light changes to how commands work with upcoming scheduler work, but I don't think anything that's likely to conflict.

[alice-i-cecile]

@NathanSWard I'm looking to revive this; would you prefer I create new PR, make PRs to this or be granted write permissions to your fork?

[EngoDev]

To anyone who is interested, I picked up this PR and created a new one per the guidelines, you can find it here:
#11184

[alice-i-cecile]

Closing as adopted :)