Return an error instead of panicking when canvas context is unavailable #3052
Conversation
Yeah, through wasm-bindgen/wasm-pack, it's on my list to implement soon.
We can take a dep on hal, that's fine. |
Codecov Report
@@ Coverage Diff @@
## master #3052 +/- ##
===========================================
- Coverage 65.41% 47.14% -18.28%
===========================================
Files 82 82
Lines 39455 39439 -16
===========================================
- Hits 25809 18592 -7217
- Misses 13646 20847 +7201
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
|
Needs conflict resolution, then g2g |
…ebGL 2). Part of fixing gfx-rs#3017. This commit changes the handling of `web_sys` errors and nulls from `expect()` to returning `Err`, but it doesn't actually affect the public API — that will be done in the next commit.
…s errors. Part of fixing gfx-rs#3017.
|
I've rebased, resolving the conflict and squashing the previous changes from review. The 2 commits now on this branch are separate because they are intentionally self-contained to separate the implementation logic and the breaking type change. (By the way, it would be nice if there was a CONTRIBUTING.md or other documentation that specified project preferences for commit/PR management, such as whether I ought to have just fully squashed unconditionally and not thought about it.) Also, please check on how I resolved the CHANGELOG conflict — I wasn't sure from the beginning whether this is really “major” (and now, deserving of its own entire section heading), but that does seem to be how breaking changes are reported, so perhaps it's just right. |
That would be great to have.
Yeah any breaking changes we should call out explicitly even if they aren't "major" in scope. |
Checklist
cargo clippy.RUSTFLAGS=--cfg=web_sys_unstable_apis cargo clippy --target wasm32-unknown-unknownif applicable.Connections
Fixes #3017 "WebGL and WebGPU backends should not panic if canvas context is unavailable".
Description
Resultreturn value towgpu::Instance::create_surface()and related functions.wgpu::CreateSurfaceError.create_surface().Testing
There is no current way to run automated tests under WASM, but I ran
cargo checkwith and without thewebglfeature;cargo nextest run; and somerun-wasmexamples.Further notes and concerns
This is a breaking change (changing return types to be
Result<Surface>instead ofSurface) — is there anything to improve about the new API?It would be really nice to have more details from the error, so that it is diagnosable rather than “Sorry, something wrong happened”. The main obstacle to that is the non-specificity of
wgpu_hal::InstanceError. However, this PR doesn't strictly need that, and it can be enhanced later withoutwgpuAPI changes.The docs for
create_surface()say more things than they used to. I think they're better, but since this is anunsafe fn, everything it says is important. Particularly, I moved the remark about panicking on Metal from Safety to Panics on the assumption that if it's a panic it's not unsound (and therefore should not be in the Safety section), but perhaps that's wrong.This is a draft because some changes are missing forResolved.features = ["webgl"]. I discovered thatwgpu_halis not depended on directly fromwgpu(when the target is wasm32) even though it is indirectly depended on, sowgpu_hal::InstanceErrorisn't usable for communicating errors fromwgpu_coretowgpuin the WebGL backend's context creation. So, a different error type will be needed, or that dependency added, or a reexport. I'm posting this PR anyway (as a draft) because it's mostly complete from an API and docs perspective.