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

Context dynamic dispatch #3051

Merged
merged 9 commits into from
Dec 20, 2022
Merged

Conversation

i509VCB
Copy link
Contributor

@i509VCB i509VCB commented Sep 27, 2022

Checklist

  • Run cargo clippy.
  • Run RUSTFLAGS=--cfg=web_sys_unstable_apis cargo clippy --target wasm32-unknown-unknown if applicable.
  • Add change to CHANGELOG.md. See simple instructions inside file.

Description
This pull request does most of the work required to allow wgpu backends to be set at runtime. In the future this would allow selection between webgl2 and webgpu on web and on native other backends (including my use case which crosses process boundaries).

What would need to be done after this is to solidify the internal Context trait and types so those can be publicly exported.

Testing
This pull request needs testing with crates across the ecosystem. The dynamic dispatch in the internals does pass the tests in wgpu, but best to be safe.

There hasn't been any testing for the web backend. There could be errors that may have been added during the dynamic dispatch internal mess. However the testing situation for the web has been grim for a while.

@i509VCB i509VCB force-pushed the wgpu-dyn-context branch 2 times, most recently from 166eed4 to e265d55 Compare September 27, 2022 03:23
@expenses
Copy link
Contributor

expenses commented Oct 3, 2022

This, is exciting! I've been wanting a way to switch between the WebGL 2 and WebGPU backends at runtime. On linux I'm getting an error when trying to run the hello-triangle example natively:

thread 'main' panicked at 'IdSendSync was downcast to the wrong type', wgpu/src/context.rs:38:14
stack backtrace:
   0: rust_begin_unwind
             at /rustc/a55dd71d5fb0ec5a6a3a9e8c27b2127ba491ce52/library/std/src/panicking.rs:584:5
   1: core::panicking::panic_fmt
             at /rustc/a55dd71d5fb0ec5a6a3a9e8c27b2127ba491ce52/library/core/src/panicking.rs:142:14
   2: core::panicking::panic_display
             at /rustc/a55dd71d5fb0ec5a6a3a9e8c27b2127ba491ce52/library/core/src/panicking.rs:72:5
   3: core::panicking::panic_str
             at /rustc/a55dd71d5fb0ec5a6a3a9e8c27b2127ba491ce52/library/core/src/panicking.rs:56:5
   4: core::option::expect_failed
             at /rustc/a55dd71d5fb0ec5a6a3a9e8c27b2127ba491ce52/library/core/src/option.rs:1874:5
   5: core::option::Option<T>::expect
             at /rustc/a55dd71d5fb0ec5a6a3a9e8c27b2127ba491ce52/library/core/src/option.rs:738:21
   6: wgpu::context::IdSendSync::downcast_id
             at ./src/context.rs:36:9
   7: <wgpu::backend::direct::Context as wgpu::Context>::instance_request_adapter::{{closure}}
             at ./src/backend/direct.rs:869:37
   8: core::option::Option<T>::map
             at /rustc/a55dd71d5fb0ec5a6a3a9e8c27b2127ba491ce52/library/core/src/option.rs:929:29
   9: <wgpu::backend::direct::Context as wgpu::Context>::instance_request_adapter
             at ./src/backend/direct.rs:867:37
  10: <T as wgpu::context::DynContext>::instance_request_adapter
             at ./src/context.rs:654:22
  11: wgpu::Instance::request_adapter
             at ./src/lib.rs:1809:23
  12: hello_triangle::run::{{closure}}
             at ./examples/hello-triangle/main.rs:12:19
  13: <core::future::from_generator::GenFuture<T> as core::future::future::Future>::poll
             at /rustc/a55dd71d5fb0ec5a6a3a9e8c27b2127ba491ce52/library/core/src/future/mod.rs:91:19
  14: pollster::block_on
             at /home/ashley/.cargo/registry/src/github.aaakk.us.kg-1ecc6299db9ec823/pollster-0.2.5/src/lib.rs:125:15
  15: hello_triangle::main
             at ./examples/hello-triangle/main.rs:145:9
  16: core::ops::function::FnOnce::call_once
             at /rustc/a55dd71d5fb0ec5a6a3a9e8c27b2127ba491ce52/library/core/src/ops/function.rs:248:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

@i509VCB
Copy link
Contributor Author

i509VCB commented Oct 4, 2022

@expenses I have fixed the bug and all the tests and examples (except for mipmap due to features) run on my system (radv Vulkan).

I managed to forget that a Box<dyn Any + Send + Sync> is &(dyn Any + Send + Sync), but a reference to the box is a borrow over the box, not the inner value. Hence the as_ref calls.

Copy link
Member

@cwfitzgerald cwfitzgerald left a comment

Choose a reason for hiding this comment

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

I need to take another look inside my IDE to see if I see anything weird, but here's some first pass comments.

wgpu/src/backend/direct.rs Outdated Show resolved Hide resolved
wgpu/src/backend/direct.rs Outdated Show resolved Hide resolved
wgpu/src/lib.rs Outdated Show resolved Hide resolved
wgpu/src/lib.rs Outdated Show resolved Hide resolved
wgpu/src/context.rs Outdated Show resolved Hide resolved
@i509VCB i509VCB force-pushed the wgpu-dyn-context branch 8 times, most recently from d3d174e to d8bc266 Compare October 24, 2022 02:14
@i509VCB i509VCB force-pushed the wgpu-dyn-context branch 6 times, most recently from 621bdf6 to a0dcf36 Compare November 1, 2022 05:55
@codecov-commenter
Copy link

codecov-commenter commented Nov 1, 2022

Codecov Report

Merging #3051 (2b59812) into master (f0f700c) will decrease coverage by 1.36%.
The diff coverage is 66.66%.

❗ Current head 2b59812 differs from pull request most recent head 10b5b6f. Consider uploading reports for the commit 10b5b6f to get more accurate results

@@            Coverage Diff             @@
##           master    #3051      +/-   ##
==========================================
- Coverage   65.64%   64.27%   -1.37%     
==========================================
  Files          82       83       +1     
  Lines       39479    42260    +2781     
==========================================
+ Hits        25915    27164    +1249     
- Misses      13564    15096    +1532     
Impacted Files Coverage Δ
wgpu-core/src/lib.rs 96.55% <ø> (ø)
wgpu-core/src/track/buffer.rs 89.08% <ø> (-3.07%) ⬇️
wgpu-core/src/track/metadata.rs 90.97% <ø> (+1.50%) ⬆️
wgpu-core/src/track/mod.rs 75.18% <ø> (ø)
wgpu-core/src/track/texture.rs 59.96% <ø> (-21.02%) ⬇️
wgpu-types/src/lib.rs 88.12% <ø> (ø)
wgpu/src/backend/direct.rs 57.14% <ø> (+0.70%) ⬆️
wgpu/src/context.rs 52.95% <ø> (ø)
wgpu/src/lib.rs 50.26% <ø> (-1.18%) ⬇️
wgpu/src/util/mod.rs 12.32% <0.00%> (-0.18%) ⬇️
... and 8 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@i509VCB i509VCB force-pushed the wgpu-dyn-context branch 7 times, most recently from 91f131d to cd74fd2 Compare November 4, 2022 22:48
@i509VCB i509VCB mentioned this pull request Nov 5, 2022
3 tasks
@i509VCB i509VCB force-pushed the wgpu-dyn-context branch 6 times, most recently from a61eacf to 14d2671 Compare November 17, 2022 04:49
Copy link
Member

@cwfitzgerald cwfitzgerald left a comment

Choose a reason for hiding this comment

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

Alright, I think I can basically say that this is good enough to merge, minus the mentioned comments. I don't think we can catch all the issues with this massive refactor by just staring at it, so I want to merge and improve iteratively.

I do think there might be some macro magic that makes the amount of boilerplate we need less, but we can stew on that at some future time.

.cargo/config.toml Outdated Show resolved Hide resolved
wgpu/src/assertions.rs Outdated Show resolved Hide resolved
wgpu/src/backend/web.rs Outdated Show resolved Hide resolved
wgpu/src/backend/web.rs Outdated Show resolved Hide resolved
wgpu/src/context.rs Outdated Show resolved Hide resolved
wgpu/src/context.rs Outdated Show resolved Hide resolved
wgpu/src/context.rs Outdated Show resolved Hide resolved
@i509VCB i509VCB force-pushed the wgpu-dyn-context branch 2 times, most recently from df1f94e to 225fcc7 Compare December 9, 2022 23:54
@i509VCB i509VCB requested a review from cwfitzgerald December 14, 2022 17:18
Copy link
Member

@cwfitzgerald cwfitzgerald left a comment

Choose a reason for hiding this comment

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

Needs conflicts resolved, but I think it's time to just pull the trigger on this.

@i509VCB i509VCB changed the title WIP: Context dynamic dispatch Context dynamic dispatch Dec 19, 2022
@cwfitzgerald cwfitzgerald merged commit 052bd17 into gfx-rs:master Dec 20, 2022
@cwfitzgerald cwfitzgerald mentioned this pull request Dec 20, 2022
14 tasks
@94bryanr
Copy link

94bryanr commented Feb 6, 2023

I am now getting an error when building a SurfaceConfiguration from a Web Worker (this use case worked before).

panicked at 'assertion failed: wasm.is_instance_of::<T>()', /home/b/.cargo/registry/src/github.aaakk.us.kg-1ecc6299db9ec823/wgpu-0.15.0/src/backend/web.rs:53:9

at <wgpu::backend::web::Identified<T> as core::convert::From<wgpu::context::ObjectId>>::from::h65779d5bfdededd6 (ccfe_bg.wasm:0x9a6d4f)

This happens when calling

adapter: &wgpu::Adapter ...

let caps = surface.get_capabilities(adapter);

The wgpu::Surface in this case was initialized via instance.create_surface_from_offscreen_canvas(...);

@i509VCB
Copy link
Contributor Author

i509VCB commented Feb 6, 2023

Yep the bug is known, it's being tracked in #3430

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants