Skip to content

feat(ecs): configurable error handling for fallible systems #17753

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

Merged
merged 8 commits into from
Feb 11, 2025

Conversation

JeanMertz
Copy link
Contributor

@JeanMertz JeanMertz commented Feb 9, 2025

You can now configure error handlers for fallible systems. These can be configured on several levels:

  • Globally via App::set_systems_error_handler
  • Per-schedule via Schedule::set_error_handler
  • Per-system via a piped system (this is existing functionality)

The default handler of panicking on error keeps the same behavior as before this commit.

The "fallible_systems" example demonstrates the new functionality.

This builds on top of #17731, #16589, #17051.

You can now configure error handlers for fallible systems. These can be
configured on several levels:

- Globally via `App::set_systems_error_handler`
- Per-schedule via `Schedule::set_error_handler`
- Per-system via a piped system (this is existing functionality)

The "fallible_systems" example demonstrates the new functionality.

This builds on top of bevyengine#17731, bevyengine#16589, bevyengine#17051.

Signed-off-by: Jean Mertz <[email protected]>
@alice-i-cecile alice-i-cecile added this to the 0.16 milestone Feb 9, 2025
@alice-i-cecile alice-i-cecile added C-Feature A new feature, making something new possible A-ECS Entities, components, systems, and events M-Needs-Release-Note Work that should be called out in the blog due to impact S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Feb 9, 2025
@NthTensor
Copy link
Contributor

Wonderful, thanks! I will try very hard to find the time to review this sometime this week.

Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basic strategy looks good to me. Exposing the different error handlers at the schedule level is definitely the most natural way to do this, even if I think that users will almost never set error handlers for specific schedules independently.

That said, I think we should:

  1. Improve the documentation to more clearly explain the core model. The bevy_ecs::result module feels like the best place for a detailed explanation, which should be linked wherever these setters are exposed.
  2. Cut the error handling setter from World: it's neither convenient nor natural, and it's super easy to implement externally.
  3. Add a few conveniences to the result module to make setting this behavior to info/warn/error/ignore easier.

3 can easily happen in follow-up though :)

Signed-off-by: Jean Mertz <[email protected]>
@JeanMertz
Copy link
Contributor Author

JeanMertz commented Feb 10, 2025

edit applied the change in 5c30e11.

I realized #17731 does not give us access to ScheduleSystem (because observer systems aren't "scheduled" in the ECS the way regular systems are).

So it probably makes sense to review and merge #17731 first, then update this PR to change the error handler signature from fn(Error, &ScheduledSystem) to (e.g.) fn(Error, SystemErrorContext), which allows us to pass some context such as the system name for both scheduled systems and observer systems.

It also warrants a discussion on where our default error_handler should live. As I can fetch the Schedules resource to get the default error handler for observer systems, but for obvious reasons that's a weird place to get the data from.

Given that we're already doing a world.get_resource::<Schedules> to get the global error handler, I'm thinking we could instead introduce a new DefaultSystemsErrorHandler resource that we pull instead, both for scheduled systems and observer systems. That resource would be fetched once when initializing a schedule, or when spawning an observer, so it's not in any significantly hot path.

@alice-i-cecile
Copy link
Member

Given that we're already doing a world.get_resource:: to get the global error handler, I'm thinking we could instead introduce a new DefaultSystemsErrorHandler resource that we pull instead, both for scheduled systems and observer systems. That resource would be fetched once when initializing a schedule, or when spawning an observer, so it's not in any significantly hot path.

I like this!

Copy link
Member

@cart cart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is the right approach. Well done!

Signed-off-by: Jean Mertz <[email protected]>
@notmd
Copy link
Contributor

notmd commented Feb 11, 2025

Nice work, just small question. How does Rust Analyzer work with this when suggesting autocomplete? Does it conflict with the warn!, error macro etc from tracing? If it conflicts I think we should wrap them in a SystemErrorHandler struct. As this probably use one time so additional namespace is fine.

@JeanMertz
Copy link
Contributor Author

Nice work, just small question. How does Rust Analyzer work with this when suggesting autocomplete? Does it conflict with the warn!, error macro etc from tracing? If it conflicts I think we should wrap them in a SystemErrorHandler struct. As this probably use one time so additional namespace is fine.

The bare functions aren’t exposed in the prelude, so it shouldn’t be an issue.

Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice refinement!

@alice-i-cecile alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Feb 11, 2025
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Feb 11, 2025
Merged via the queue into bevyengine:main with commit fd67ca7 Feb 11, 2025
30 checks passed
@JeanMertz JeanMertz deleted the fallibility-handler branch February 11, 2025 19:00
@alice-i-cecile
Copy link
Member

Thank you to everyone involved with the authoring or reviewing of this PR! This work is relatively important and needs release notes! Head over to bevyengine/bevy-website#1993 if you'd like to help out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events C-Feature A new feature, making something new possible M-Needs-Release-Note Work that should be called out in the blog due to impact S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants