Skip to content

Per world error handler #18810

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

SpecificProtagonist
Copy link
Contributor

@SpecificProtagonist SpecificProtagonist commented Apr 11, 2025

Objective

see original comment

Alternately, could we store it on the World instead of a global? I think we have a World nearby whenever we call default_error_handler(). That would avoid the need for atomics or locks, since we could do ordinary reads and writes to the World.

Global error handlers don't actually need to be global – per world is enough. This allows using different handlers for different worlds and also removes the restrictions on changing the handler only once.

Solution

Each World can now store its own error handler in a resource.

For convenience, you can also set the default error handler for an App, which applies it to the worlds of all SubApps. The old behavior of only being able to set the error handler once is kept for apps.

We also don't need the configurable_error_handler feature anymore now.

Testing

New/adjusted tests for failing schedule systems & observers.


Showcase

App::new()
    .set_error_handler(info)

@SpecificProtagonist SpecificProtagonist added A-ECS Entities, components, systems, and events M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Needs-Design This issue requires design work to think about how it would best be accomplished S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Apr 11, 2025
Copy link
Contributor

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

Please review the instructions for writing migration guides, then expand or revise the content in the migration guides directory to reflect your changes.

@SpecificProtagonist SpecificProtagonist added this to the 0.17 milestone Apr 11, 2025
@alice-i-cecile alice-i-cecile added the S-Needs-Benchmarking This set of changes needs performance benchmarking to double-check that they help label Apr 11, 2025
@SpecificProtagonist
Copy link
Contributor Author

SpecificProtagonist commented Apr 11, 2025

@alice-i-cecile Why the needs-benchmarking label? This doesn't do any synchronization at all because, not being global, it just uses normal Rust ownership. Also, as mentioned in #18801 (comment), we can't benchmark this well (unless there's just something screwed up with my computer and the relevant benchmarks are actually consistent enough for other people).

@Person-93
Copy link
Contributor

For convenience, you can also set the default error handler for an App, which applies it to the worlds of all SubApps. The question is: What should happen when the default handler of those worlds gets set from elsewhere, either before or after insertion into the App respectively?

Perhaps it can be more than just a convenience. Maybe have a per-App and and a per-World handler, and if the World handler isn't set, it falls back on the App's handler.

@alice-i-cecile alice-i-cecile removed the S-Needs-Benchmarking This set of changes needs performance benchmarking to double-check that they help label Apr 11, 2025
@SpecificProtagonist
Copy link
Contributor Author

SpecificProtagonist commented Apr 11, 2025

Something to that effect could work, but bevy_ecs can't know about Apps.

Edit: I've solved this by keeping the old behavior of not letting the handler to be set multiple times (for an app).

@Person-93
Copy link
Contributor

Something to that effect could work, but bevy_ecs can't know about Apps.

Perhaps a resource called AppErrorHandler with a function pointer, or a pointer-to-pointer.

@SpecificProtagonist SpecificProtagonist removed the S-Needs-Design This issue requires design work to think about how it would best be accomplished label Apr 12, 2025
@SpecificProtagonist SpecificProtagonist marked this pull request as ready for review April 12, 2025 12:56
@NthTensor
Copy link
Contributor

@cart I want to make sure you see this pr too. Imo it's probably the direction we should go, rather than the atomics.

Still probably too late to slip in to 0.16 but maybe a bit less of a risk because there's no issues with platform support.

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 M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Needs-Review Needs reviewer attention (from anyone!) to move forward
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants