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

Properly handle the case where Navigator.gpu is undefined and WebGPU is the only compiled backend #6197

Merged
merged 8 commits into from
Sep 15, 2024

Conversation

BGR360
Copy link
Contributor

@BGR360 BGR360 commented Sep 2, 2024

Connections
Fixes #6196.

Description
This PR fixes a bug where wgpu would "crash" in the browser (JavaScript TypeError exception) when compiled without webgl support and the browser doesn't support WebGPU.

Implementation
This PR changes ContextWebGPU to avoid calling wgpu_sys with an undefined Gpu object. In ContextWebGpu::request_adapter, this involves immediately returning None if self.gpu is undefined in the browser.

Testing
Please advise. I have no idea how to write an automated regression test for this kind of change. I could not find any CONTRIBUTING.md or likewise providing any guidelines.

Additionally, I have not yet manually tested that this works. I will work on that.
EDIT: Okay I applied the patch to wgpu 0.20.1 locally and patched eframe to use it, and it does successfully get past the problem.

Checklist

  • Run cargo fmt.
  • Run cargo clippy. If applicable, add:
    • --target wasm32-unknown-unknown
    • --target wasm32-unknown-emscripten
  • Run cargo xtask test to run tests.
    • ?? Do I need to do this on --target wasm32-unknown-unknown as well ??
  • Add change to CHANGELOG.md. See simple instructions inside file.

@BGR360 BGR360 requested a review from a team as a code owner September 2, 2024 04:26
wgpu/src/backend/webgpu.rs Outdated Show resolved Hide resolved
wgpu/src/backend/webgpu.rs Outdated Show resolved Hide resolved
Copy link
Member

@Wumpf Wumpf left a comment

Choose a reason for hiding this comment

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

great catch! Not convinced of the Option wrapper on gpu, but looking good otherwise.
Unfortunate that this needs the OptionFuture wrapper, but don't see a better solution either

wgpu/src/backend/webgpu.rs Outdated Show resolved Hide resolved
wgpu/src/backend/webgpu.rs Outdated Show resolved Hide resolved
@BGR360 BGR360 force-pushed the issue-6196-undefined-webgpu branch from e4001a5 to 3ab65b4 Compare September 14, 2024 21:47
@Wumpf Wumpf self-requested a review September 15, 2024 07:54
Copy link
Member

@Wumpf Wumpf left a comment

Choose a reason for hiding this comment

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

nice!

wgpu/src/api/instance.rs Show resolved Hide resolved
wgpu/src/backend/webgpu.rs Outdated Show resolved Hide resolved
@Wumpf Wumpf enabled auto-merge (squash) September 15, 2024 07:56
@Wumpf Wumpf merged commit 2fac5e9 into gfx-rs:trunk Sep 15, 2024
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants