Improved Command Errors

[cart]

Objective

Rework / build on #17043 to simplify the implementation. #17043 should be merged first, and the diff from this PR will get much nicer after it is merged (this PR is net negative LOC).

Solution

1. Command and EntityCommand have been vastly simplified. No more marker components. Just one function.
2. Command and EntityCommand are now generic on the return type. This enables result-less commands to exist, and allows us to statically distinguish between fallible and infallible commands, which allows us to skip the "error handling overhead" for cases that don't need it.
3. There are now only two command queue variants: queue and queue_fallible. queue accepts commands with no return type. queue_fallible accepts commands that return a Result (specifically, one that returns an error that can convert to bevy_ecs::result::Error).
4. I've added the concept of the "default error handler", which is used by queue_fallible. This is a simple direct call to the panic() error handler by default. Users that want to override this can enable the configurable_error_handler cargo feature, then initialize the GLOBAL_ERROR_HANDLER value on startup. This is behind a flag because there might be minor overhead with OnceLock and I'm guessing this will be a niche feature. We can also do perf testing with OnceLock if someone really wants it to be used unconditionally, but I don't personally feel the need to do that.
5. I removed the "temporary error handler" on Commands (and all code associated with it). It added more branching, made Commands bigger / more expensive to initialize (note that we construct it at high frequencies / treat it like a pointer type), made the code harder to follow, and introduced a bunch of additional functions. We instead rely on the new default error handler used in queue_fallible for most things. In the event that a custom handler is required, handle_error_with can be used.
6. EntityCommand now only supports functions that take EntityWorldMut (and all existing entity commands have been ported). Removing the marker component from EntityCommand hinged on this change, but I strongly believe this is for the best anyway, as this sets the stage for more efficient batched entity commands.
7. I added EntityWorldMut::resource and the other variants for more ergonomic resource access on EntityWorldMut (removes the need for entity.world_scope, which also incurs entity-lookup overhead).

Open Questions

1. I believe we could merge queue and queue_fallible into a single queue which accepts both fallible and infallible commands (via the introduction of a QueueCommand trait). Is this desirable?

[spectria-limina]

I don't have time for a full review, but I skimmed, and two quick comments.

First, I think the output should be an associated type, not a parameter. Can't think of any good reason to allow a single type to implement the trait multiple times with different return types.

Second, if I read correctly, there isn't a way to make built in commands like insert return errors, right? I think that option needs to exist. Apologies if I missed it in the PR.

[cBournhonesque]

Would it be worth combining this with the impl HandleError above with

impl<C: Command<Result<T, E>>, T, E: Into<Error>> HandleError for C { 
   ...
}

Since it's essentially what we want in the end?

I see this function is only used once in queue_fallible to create a Command compatible with HandleError

[cBournhonesque]

I really like the leaner traits, great work!

1. I feel like the EntityCommands change to only support EntityWorldMut should be part of a separate PR. It's kind of independent to error-handling. Having this change in the same PR makes it harder to discuss implications specific to the EntityWorldMut change

2. I think queue and queue_fallible should be unified if possible. I have tons of commands in my project, some of them are tiny and return (), some of them are more complex and are fallible. I don't really want to have to remember if they return () or Result to see if I need to call queue or queue_fallible, I just want to queue them. If they return Result then I want the Result to be handled.

And in the rare case where you have a command which returns Result but you don't want it handled you can always apply the silent error-handler

[github-actions]

It looks like your PR is a breaking change, but you didn't provide a migration guide.

Could you add some context on what users should update when this change get released in a new version of Bevy?
It will be used to help writing the migration guide for the version. Putting it after a ## Migration Guide will help it get automatically picked up by our tooling.

[cart]

@spectria-limina

First, I think the output should be an associated type, not a parameter. Can't think of any good reason to allow a single type to implement the trait multiple times with different return types.

An associated type would prevent default values, which would force Command<Out = ()> instead of Command. Likewise Command<Out = Result> is just a bit nastier to write than Command<Result>. It also forces implementations that look like this:

impl Command for Foo {
  type Out = ();
  fn apply(self, world: &mut World) {
  }
}

Instead of this:

impl Command for Foo {
  fn apply(self, world: &mut World) {
  }
}

I think normal generics are the move for this one.

Second, if I read correctly, there isn't a way to make built in commands like insert return errors, right? I think that option needs to exist. Apologies if I missed it in the PR.

First: I'll note that most of the built in commands as phrased in #17043 do not (yet) return errors, and I think updating everything is out of scope for this PR.

That being said, multiple paths have been laid for built-in commands to return handled errors.

The first path: all built-in commands are now exported as functions in the command and entity_command modules. If any of them were rephrased to return an error, then this would immediately be possible:

entity.queue_fallible(insert(Team::Blue).handle_error_with(warn()));

The more common case of using the built-in shorthand functions would use the default global error handler, which defaults to panicking but can be configured:

entity.insert(Team::Blue);

@cBournhonesque

I feel like the EntityCommands change to only support EntityWorldMut should be part of a separate PR. It's kind of independent to error-handling. Having this change in the same PR makes it harder to discuss implications specific to the EntityWorldMut change

As mentioned in the description, the simplification of EntityCommands hinges on there being only one blanket function impl. We'd need to remove one of the two existing implementations anyway.

I think queue and queue_fallible should be unified if possible. I have tons of commands in my project, some of them are tiny and return (), some of them are more complex and are fallible. I don't really want to have to remember if they return () or Result to see if I need to call queue or queue_fallible, I just want to queue them. If they return Result then I want the Result to be handled.

Good points. I'm on "team unify" as well. I'll investigate.

[spectria-limina]

Normal generics would allow for both queue and queue_fallible to be the same function though, wouldn't they? Because the return type would be inferred from the type of the Command. So in that case, you wouldn't need to write Command<Out = ()> until you got to a point where you really needed that specifically. Code that doesn't care about the output type would never need to mention it.

There'd probably need to be some sort of trait bound on the output type, I guess, to ensure that it can be reduced to a (), say by logging and discarding the error or by panicking. But that again wouldn't need to be mentioned in most places.

[JaySpruce]

I feel like a queue_fallible_with (or queue_with) would help users without really costing anything. If someone wants to modify how an entity command reacts to the entity not existing, they have to do everything internal themselves:

commands.queue(entity_command::insert(bundle).with_entity(entity).handle_error_with(error_handler::warn());

Users don't have any reason to even know with_entity exists prior to this.

With queue_fallible_with, it's not really shorter, but users only have to worry about stuff they already know:

commands.entity(entity).queue_fallible_with(entity_command::insert(bundle), error_handler::warn());

[NthTensor]

Ok, starting on a review of this now. Having not yet looked at the code, I'm really pleased with the high-level goals. There is just one note, before I forget:

I've added the concept of the "default error handler", which is used by queue_fallible. This is a simple direct call to the panic() error handler by default. Users that want to override this can enable the configurable_error_handler cargo feature, then initialize the GLOBAL_ERROR_HANDLER value on startup.

Ok, I like this, I want default handlers. But it's weird that this is not unified with the system result handler, which I placed in the schedule runner here in #16589. Personally I don't really care where the default handler lives, and if we want to do a global-install-style handler that's fine with me, but imo we should pick one or the other.

This is behind a flag because there might be minor overhead with OnceLock and I'm guessing this will be a niche feature. We can also do perf testing with OnceLock if someone really wants it to be used unconditionally, but I don't personally feel the need to do that.

OnceLock is probably alright here. I had some idea that we could be returning these errors to the scheduler through the sync-point system, idk if that's a viable. I don't really think this will be a niche feature though. Don't we anticipate the editor using custom error handling to populate an error console? But that's nothing to do with the implementation.

Oh and off-the-cuff I would favor unifying queue and queue_fallible as well.

[cart]

I feel like a queue_fallible_with (or queue_with) would help users without really costing anything. If someone wants to modify how an entity command reacts to the entity not existing, they have to do everything internal themselves:

The code you provided works, but it also isn't necessary. This is already supported:

commands.entity(entity).queue_fallible(entity_command::insert(bundle).handle_error_with(error_handler::warn()));

Which is roughly as good as queue_fallible_with:

commands.entity(entity).queue_fallible_with(entity_command::insert(bundle), error_handler::warn());

I personally like keeping the variants down / having one way to do things. It gets even closer ergonomically if we unify queue and queue_fallible:

commands.entity(entity).queue(entity_command::insert(bundle).handle_error_with(error_handler::warn()));

[cart]

But it's weird that this is not unified with the system result handler

Hmm yeah we should think about how to unify these. I think applying the global handler to the system result handler by default makes some sense. I'd prefer that we tackle unification in a separate PR though.

[cart]

I had some idea that we could be returning these errors to the scheduler through the sync-point system, idk if that's a viable.

Worth considering all of our options. Imo the direct handler approach is nice because it gives us both granularity and nice performance (no need to queue results up, look up which handler to use, etc). I think we should start from there with essentially optimal performance and then compare against the fancier options later.

I don't really think this will be a niche feature though. Don't we anticipate the editor using custom error handling to populate an error console? But that's nothing to do with the implementation

Hmmm yeah thats true. Although thats an implementation we'd own ourselves, and editor integration would likely be behind a cargo feature (so we could abstract that out from a user perspective). I think the majority of users shouldn't need to explicitly think about custom global error handlers.

[NthTensor]

All fair points. I'm pretty happy with this, there's obviously going to be more work to do here but it will be in the long tail of gradual improvement. The changes to commands are really nice.

[JaySpruce]

This is already supported:

commands.entity(entity).queue_fallible(entity_command::insert(bundle).handle_error_with(error_handler::warn()));

Sorry, I'm still not seeing it. Wouldn't that result in an EntityFetchError being handled by the default error handler?

move |world: &mut World| {			   <- handle_error (default error handler)
    move |world: &mut World| {                     <- with_entity
	let entity = world.get_entity_mut(entity)?;
        move |entity: EntityWorldMut| {            <- handle_error_with (user's error handler)
	    move |mut entity: EntityWorldMut| {    <- entity_command::insert
	        entity.insert_with_caller(bundle, InsertMode::Replace);
            }
        }
    }
}

[cart]

I just pushed a commit that merges queue and queue_fallible.

Sorry, I'm still not seeing it. Wouldn't that result in an EntityFetchError being handled by the default error handler?

Hmmm yeah I see your point. You can't customize the error handler for an internal with_entity call, whereas the handle_error_with call could be added to the end of the chain internally with a specialized function.

[cart]

Fundamentally the problem @JaySpruce has called out is that the custom error handler resolves "too early" before we call entity_command.with_entity internally, resulting in the default error handler (panicking!) being used for the with_entity call. Given that with_entity is one of the more likely things to fail, we do need to prioritize resolving this.

As @JaySpruce said, adding queue_fallible_with does resolve this (I prefer queue_with_handler, queue_with, or queue_handled in the "unified queue" world). However I don't think adding that alone is optimal:

• It makes handle_error_with footgun-ey in the context of EntityCommands (its generally safe to use it in the context of Commands, unless you are manually replicating the same footgun-ey entity command order).
• It adds two way to do things, which I generally don't like
• The two ways to do things look functionally identical, but they have subtly different behaviors

I think we have a few options:

1. Remove the public handle_error_with function entirely (to resolve the issues above) and require using queue_handled if you want to define a custom handler, both for EntityCommands and Commands. This is a reduction in flexibility (as you can't pre-bake commands with custom handlers anymore), but I think in general it is fine.
2. Rework Command init to "tack on" the handler via a "command descriptor" object, deferring the final construction of the Command with the baked in handler until after with_entity is called internally.
3. Just add queue_handled and accept that the corner case of calling handle_error_with and passing the result into EntityCommands will not use the custom handler for with_entity errors.
4. Go back to using stateful Commands-scoped error handlers.

(1) does feel slightly inelegant / it removes functionality. But it does also make the UX simple / consistent / footgun free.
(2) provides the exact behavior we want at the cost of additional complexity and codegen (to generate the descriptor type). I don't love this solution.
(3) Is the easiest and allows people to do the right thing, but it complicates the UX and still allows people to shoot themselves in the foot.
I'm pretty strongly against (4) for the reasons stated in the description.

If anyone has other ideas let me know.

[hymm]

If anyone has other ideas let me know.

Not sure I like either of these ideas, but posting them since they have some merit.

5. Pass a Result<EntityWorldMut, EntityFetchError> instead of EntityWorldMut to EntityCommands. Not great, but just need to ? to get the EntityWorldMut.
6. Restructure things so you can attach an error handler to commands.entity(e).with_error_handler and then pass the EntityWorldMut to each entity command without having to reconstruct it every time. This feels like a pretty complex change and probably would need a EntityCommandQueue or maybe a intermediate buffer of components to be stored somewhere. It would probably slow down the case where you have only one entity command.

[cart]

I've opted to remove the HandleEntityError trait entirely, add Commands::queue_handled and EntityCommands::queue_handled, and leave HandleError as-is. This prevents the weirdness by embracing the fact that directly tacking on EntityCommand error handling is disallowed. HandleError is left in because this pattern is still valid and we need a public trait to drive this functionality.

Resolving some CI things and sorting out what try command variants should look like, then I think this is ready.

[cart]

I've made the try_ variants behave as expected (how they did previously). I think its worth reconsidering the try_x APIs as shorthand for manually calling queue_handled(entity_command::x, error_handler::warn()), but I think thats a topic for a separate PR.

I think this PR is now ready to merge.

[alice-i-cecile]

We're using boxed errors here, but in my ideal world this "add an error handler to a specific command" would handle receive the non-erased error type here. That gives us much better recovery options, beyond just log / panic / ignore.

Can we change that?

[cart]

I do agree that this could probably handle non-erased error types instead in the "specific-command error handler" case. I spent a bit of time investigating and got this largely working. I added a type Error associated type to HandleError and CommandWithEntity, and added an ErrorHandler<E> type alias for a generic error handler function. This works for adding error handlers to specific commands, but there is a problem: The "default error handler" fundamentally requires the general-purpose bevy Error type, making it (not directly) usable for a signature that expects a specific error type. We want queue to be "default error conversion compatible" and queue_handled to require the concrete type. I'm reasonably certain this requires a duplication of traits.

This is a big enough change that I think my preference is to defer it to a later PR.

[alice-i-cecile]

Good cleanup! The "EntityCommands ~= EntityWorldMut` changes are great, and I like the new helper methods on EntityWorldMut.

Requests for better documentation (shocking), and a more serious question about getting the concrete error type in queue_handled. I think this is an improvement and I wouldn't be upset to see this merged, but I think that getting that last detail right is valuable.