Skip to content
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

refactor: separate out PanicHandlerPlugin #12557

Merged
merged 4 commits into from
Mar 19, 2024

Conversation

simbleau
Copy link
Contributor

@simbleau simbleau commented Mar 18, 2024

Objective

Solution

  • Separates the panic handler to a new plugin, PanicHandlerPlugin.
  • PanicHandlerPlugin was added to DefaultPlugins.
  • Can be disabled on DefaultPlugins, in the case someone needs to configure custom panic handlers.

Changelog

Added

  • A PanicHandlerPlugin was added to the DefaultPlugins, which now sets sensible target-specific panic handlers.

Changed

  • On WASM, the panic stack trace was output to the console through the BevyLogPlugin. Since this was separated out into PanicHandlerPlugin, you may need to add the new PanicHandlerPlugin (included in DefaultPlugins).

Migration Guide

  • If you used MinimalPlugins with LogPlugin for a WASM-target build, you will need to add the new PanicHandlerPlugin to set the panic behavior to output to the console. Otherwise, you will see the default panic handler (opaque, unreachable errors in the console).

@alice-i-cecile alice-i-cecile added O-Web Specific to web (WASM) builds C-Usability A targeted quality-of-life change that makes Bevy easier to use M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide labels Mar 18, 2024
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.

We should be able to remove the dependency on console_error_panic_hook from bevy_log now, right?

@alice-i-cecile
Copy link
Member

I think this basically makes sense as a separate crate, although it's a bit weird to have something that small. What about as part of bevy_app instead? I think that will reduce complexity and controversy here by quite a bit.

@simbleau
Copy link
Contributor Author

simbleau commented Mar 18, 2024

I think this basically makes sense as a separate crate, although it's a bit weird to have something that small. What about as part of bevy_app instead? I think that will reduce complexity and controversy here by quite a bit.

I am fine to go that way, but I am hoping in the future this could evolve to be configurable, e.g.

struct PanicHandlerPlugin {
    handler: Box<dyn Fn(&PanicInfo)>
}

For now I think this makes sense as a very simple separation that can satisfy and close #12546 , with more sensible configuration to Android and other targets in the future.

I'm also a little confused how you imagine this fitting into bevy_app - can you expand?

We should be able to remove the dependency on console_error_panic_hook from bevy_log now, right?

Correct, I just removed it.

@alice-i-cecile
Copy link
Member

alice-i-cecile commented Mar 18, 2024

I'm also a little confused how you imagine this fitting into bevy_app - can you expand?

I was just thinking drop the module in there directly; no changes beyond copy-paste. This is fine though: don't worry about changing it. More config would be great in the future.

@Vrixyz Vrixyz added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Mar 18, 2024
@JMS55
Copy link
Contributor

JMS55 commented Mar 18, 2024

I think we need to update the docs for default plugins right.

@simbleau
Copy link
Contributor Author

I think we need to update the docs for default plugins right.

Fixed! Good catch.

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Mar 19, 2024
Merged via the queue into bevyengine:main with commit 7c7d1e8 Mar 19, 2024
30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-Usability A targeted quality-of-life change that makes Bevy easier to use M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide O-Web Specific to web (WASM) builds 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.

LogPlugin silently overwrites the panic hook in WASM
4 participants