Simplified system error handling #18297
Conversation
alice-i-cecile
added
A-ECS
|
You added a new example but didn't add metadata for it. Please update the root Cargo.toml file. |
| @@ -449,7 +440,7 @@ impl Schedule { | |||
| self.initialize(world) | |||
| .unwrap_or_else(|e| panic!("Error when initializing schedule {:?}: {e}", self.label)); | |||
|
|
|||
| let error_handler = self.error_handler.expect("schedule initialized"); | |||
| let error_handler = world.get_resource_or_init::<FallbackErrorHandler>().0; | |||
There was a problem hiding this comment.
The FallbackErrorHandler is mentioned as the thing that bevy will use if nothing else is provided- I assume this PR is an initial step and then this code will be updated later to grab the appropriate configured error handler for use at runtime?
There was a problem hiding this comment.
This particular line of code is in its final form. There's other localized mechanisms of error handling, like system piping, which is what the docs on FallbackErrorHandling refer to. When those are used, this error handler is never called at all.
There was a problem hiding this comment.
Ahh, I see because when systems are piped only the outermost system, if it returns a Result::Err will trigger this. Thanks for the context!
|
many deleted lines make grug brain dev happy. The resource initialization pattern without providing the redundant helper methods makes a ton of sense. |
…dling Update example metadata
| @@ -10,12 +10,19 @@ pub struct SystemErrorContext { | |||
| pub last_run: Tick, | |||
| } | |||
|
|
|||
| /// The default systems error handler stored as a resource in the [`World`](crate::world::World). | |||
| pub struct DefaultSystemErrorHandler(pub fn(BevyError, SystemErrorContext)); | |||
| /// The error handler of last resort used for [`bevy_ecs::error::Result`]s returned by systems, commands and observers, | |||
There was a problem hiding this comment.
This is not currently used by commands (largely for performance reasons).
| /// | ||
| /// See [`bevy_ecs::error`] for more information on error handling, | ||
| /// and [`bevy_ecs::error::handler`] for an assortment of built-in error handlers. | ||
| pub struct FallbackErrorHandler(pub fn(BevyError, SystemErrorContext)); |
There was a problem hiding this comment.
I see the appeal of removing System, but as a user if I was going to change this, I would ask myself "how do I set the default bevy error handler". I also think it "fits". People tend to consider defaults "overridable".
There was a problem hiding this comment.
As I understand it, this would no longer be "overridable" in the sense of inserting a resource that takes precedence- is that correct? If so then avoiding the term "default" may fit.
There was a problem hiding this comment.
Or I guess you can override it but you'd be overriding it by instantiating a new instance of this struct. So a custom error handler doing world.insert_resource(DefaultErrorHandler(... would feel weird.
Maybe a name like ScheduleErrorHandler or RuntimeErrorHandler for the struct makes more sense?
|
The generated |
@alice-i-cecile I was going to pr another change to run this, but it looks like the table formatting may have changed in an update to the generation script- if this looks intentional I can send a PR for it? |
|
Closing out; going to try moving to the command-based design instead. |
Objective
While working towards #17272, I found a few things to clean up for system error handling. Since I intend to mostly use the system error handling approach for both systems and commands, I wanted to clean it up and reduce complexity before porting commands over to it to.
This is a good, simple stopping point for review, so here's the second PR in the series :)
Solution
fallible_systemsexample to the less mysterious and more generalerror_handlingDefaultSystemErrorHandlerto the more general and more informativeFallbackErrorHandlerAppandSubAppfor configuring this: all they do is set a value of a resourceTesting
error_handlingexample works as intended