Skip to content

fix: avoid wasm panic without GL context (WebGL2 only) #12690

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 8 commits into
base: main
Choose a base branch
from

Conversation

pietrosophya
Copy link
Contributor

@pietrosophya pietrosophya commented Mar 24, 2024

Objective

Solution

If the context lost is detected (checking for the canvas, then for the WebGL2 rendering context, then for the is_context_lost()), it pauses the subapps.

This has the effect of avoid calling GL related operations, thus letting the rest of the app work.

I don't know:

  • if the pausing/resuming mechanism can be replaced by something else already existing
  • if only specific subapps (e.g. the RenderApp) can be paused

This fixes only WebGL2 because there's yet no support for WebGPU to check context lost (there's a promise handler for Chrome but it's still an experimental feature).

@alice-i-cecile alice-i-cecile added this to the 0.13.2 milestone Mar 24, 2024
@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior A-Rendering Drawing game state to the screen O-WebGL2 Specific to the WebGL2 render API labels Mar 24, 2024
@pietrosophya
Copy link
Contributor Author

pietrosophya commented Mar 24, 2024

I checked if we can at least retrieve the GpuCanvasContext for WebGPU, but this is still unstable and would require a special config when running the project.

@mockersf mockersf modified the milestones: 0.13.2, 0.13.3 Apr 4, 2024
@JMS55 JMS55 modified the milestones: 0.13.3, 0.14 Apr 25, 2024
@alice-i-cecile
Copy link
Member

@maximetinu @beoboo if either of you have relevant expertise here, a review would be very welcome. Even validating that this does fix your issue would be great.

@alice-i-cecile alice-i-cecile removed this from the 0.14 milestone May 16, 2024
@pietrosophya
Copy link
Contributor Author

Yep, it does fix our issue. since it disables the render app if there is no GL available (and thus avoids a crash).

@alice-i-cecile alice-i-cecile added S-Needs-Review Needs reviewer attention (from anyone!) to move forward D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes labels May 17, 2024
@pietrosophya
Copy link
Contributor Author

I fixed all conflicts.

Copy link
Contributor

@maximetinu maximetinu left a comment

Choose a reason for hiding this comment

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

if either of you have relevant expertise here, a review would be very welcome. Even validating that this does fix your issue would be great.

Sorry for the delayed review @alice-i-cecile

So, on one hand, this does fix the issue because Bevy doesn't panic anymore when it loses the webgl2 context.

But, on another hand, I don't think this is a general-enough solution worth contributing upstream for a couple of reasons:

  • it pauses not only the render sub-app, but all the sub apps. Other Bevy users could be defining their own sub app for some other non-graphics related thing, and they may not want to pause it when the webgl2 context is lost.
  • ideally, we should just pause the render app, but this would introduce a coupling between bevy_winit and bevy_render
  • so, in reality, pausing all the sub apps for this reason is introducing a hidden coupling between bevy_winit and bevy_render anyway, but preventing a graphic crash like this should be responsibility only of bevy_render (if not of the wgpu crate itself, ideally).

I think that a good solution would require:

  • allow pausing only specific sub apps
  • do this check as part of bevy_render, so it pauses itself

I have discussed this offline with @pietrosophya, and he has an idea to allow each app or sub app to implement a customizable fn should_update(..) -> bool system that checks for pre-conditions like this. In the case of the RenderApp sub app, in wasm+webgl2, it'd check that the webgl2 context still exists / has not been lost. That sounds very good to me, but it's a bigger feature out of scope for this PR

@alice-i-cecile alice-i-cecile added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jun 3, 2024
@pietrosophya
Copy link
Contributor Author

After thinking more about this, and after trying to introduce a filter for SubApps, I think this remains a good solution for now, since the crash can happen in any app that contains the RenderApp (maybe 99% of the apps out there), so IMO it's ok to be merged as it is now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Rendering Drawing game state to the screen C-Bug An unexpected or incorrect behavior D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes O-WebGL2 Specific to the WebGL2 render API S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

WASM: let the app work if the GL context is lost Bevy should recover from WebGL Context Lost or WebGPU Device Lost
5 participants