Make WebGPU backend portable to emscripten, Dawn, and wgpu-native #6188

eliemichel · 2023-02-23T08:01:16Z

The wgpu backend was only compatible with emscripten's flavor of WebGPU, which does not exactly match the officiel webgpu native headers.

This PR adds support for the two main (and only?) implementations of WebGPU that one can use when building native binary applications instead of wasm:

Dawn (Chrome's C++ backend)
wgpu-native (binding Firefox' Rust backend)

The header assumes that the following macros are defined:

WEBGPU_BACKEND_WGPU iff wgpu-native backend is used
WEBGPU_BACKEND_DAWN iff Dawn backend is used
If none of the above is defined, WEBGPU_BACKEND_EMSCRIPTEN is assumed

NB These macros are automatically defined in the WebGPU-binaries distributions (main branch uses wgpu-native, dawn branch uses Dawn) which are used in my LearnWebGPU for native C++ tutorial series. This gives the context in which I adapted this wgpu backend.

Notable changes

Switch to WGSL shaders instead of SPIR-V binary blobs
Add gamma correction when the target texture format requires it (ending with "Srgb")
Handle the Release methods, which are not specified in the standard webgpu.h header and have different names in the wgpu-native implem.
Align buffer sizes to 16 bytes

ocornut · 2023-02-23T14:31:56Z

Hello,

Thanks for the PR.

There are too many changes here. Each individual change would need to be carefully explained and discussed in its own commit.

Your changelog mention "Adapter for LearnWebGPU tutorial series" which seems different from what is stated here and in the change.

I believe the WEBGPU_NONSTD_RELEASE mess may be beneficial to change to a single #ifdef WEBGPU_BACKEND_WGPU block that use defines to redirect the name we used to names using Drop. That would make the change less noisy and simpler.

Why was the a wgpuBufferRelease() call removed?

eliemichel · 2023-02-23T23:40:29Z

Thx for the feedback:

Split in 6 commits
Removed WEBGPU_NONSTD_RELEASE
Only WEBGPU_BACKEND_WGPU is needed (when relevant). I suggest we keep WEBGPU_BACKEND_DAWN in the doc anyways, in case emscripten and Dawn backend start to diverge. (Tbh I don't like making the wgpu-native implem look like an exception while it's actually closer to the officiel webgpu native headers but let's not care).
Fixed the mention from the changelog that was indeed not appropriate
Restored the missing call to wgpuBufferRelease()

Tested with emscripten, wgpu-native + Windows, Dawn + Windows, and got feedback that it works on wgpu-native + macOS but could not test myself.

ocornut · 2023-03-06T09:34:57Z

Hello,

The only change that I can potentially merging without further explanation is the one with the defines (for WEBGPU_BACKEND_WGPU). For each change I would otherwise needs an explanation as to why you are making the change. e.g.

what's the reason and purpose of switching to WGSL shaders?
why aligning the buffer? if this is supposedly needed, why did it seemingly worked until now?

Thank you.

eliemichel · 2023-03-08T23:23:49Z

Hello,

I understand, here are some details:

Switch to WGSL. Hard to track down were the official discussion is about this right now because it is not a consensual choice, but last time I checked the possibility to use SPIR-V shaders in WebGPU was only transitional:

once WGSL and Tint are ready, the SPIR-V path will be removed from Chromium

From @Kangz (source) (BTW hi Corentin if you'd have some heads up this'd help :) )

Using SPIR-V would remain possible when building Desktop applications (using wgpu-native or Dawn) but not when cross-compiling for the web (emscripten).

(On a side note, I believe that binary blobs in source code are not convenient to maintain but as I'm not the maintainer here this isn't really a point.)

Alignment. I first added it because the WebGPU device was raising a runtime error complaining about it. Then when squashing it seemed to no longer be a problem, then I got the error again... I'm still investigating but at this stage it might be hardware and/or backend dependent. According to the alignment and size rules, the ImDrawVert struct must be aliged on 16 bytes [edit: 4, not 16]. The uniform buffer also really requires alignment, on 16 bytes (it was naturally aligned before because it was only containing a mat4). Aligning the index buffer however is likely overkill.

Release vs Drop. The different names in wgpu-native vs Dawn correspond to slightly different semantics in their role. The issue lasts from the very beginning of the project and is still open and both Drop and Release have been proposed but remain unmerged. I believe it does not affect the way it is used here but I mention it FYI.

Gamma correction. Dawn only allow BGRAUnorm swap chain format, and afaik the surface returned by emscripten's binding is also always BGRAUnorm (at least when testing with Chrome it uses Dawn under the hood so it cannot return sth else), so this was not a problem until now. wgpu-native supports Srgb formats, for which the shader is expected to return colors on an "sRGB scale".

Kangz · 2023-03-08T23:32:59Z

From @Kangz (gpuweb/gpuweb#847 (comment)) (BTW hi Corentin if you'd have some heads up this'd help :) )

This is still the case in browsers. WGSL has become a decent language now and all WebGPU implementation (in native or Web) will support it. So IMHO imgui should just use that for the small amount of shaders it needs.

Alignment Vertex buffers and index buffers only need an alignment of 4. I'm not sure what the 16 alignment is for ImDrawVert. Is it a uniform buffer? Or maybe I misremember the alignment constraints on vertex attributes (IIRC it is only the base type alignment that matters).

Release v Drop: we really need to settle this between Dawn and wgpu :/

Gamma correction: on the Web the GPUCanvasContext can be configured with bgra8unorm with a view format of bgra8unorm-srgb so that the texture view can reinterpret the format as SRGB. Dawn doesn't implement this for native swapchains yet, but that's something we should fix eventually.

Kangz · 2023-03-08T23:33:30Z

backends/imgui_impl_wgpu.cpp

+@group(1) @binding(0) var t: texture_2d<f32>;
+
+@fragment
+fn main(in: VertexOutput) -> @location(0) vec4<f32> {


note: in the future when wgpu supports them, you could use shorthand types like vec4f

Yep I'm looking fwd to it :)

eliemichel · 2023-03-09T19:32:56Z

Thanks a lot for your review :)

(about alignment, I wrote 16 instead of 4 for vertex buffer, but in the code it's aligned to 4 bytes as expected)

eliemichel · 2023-03-20T21:12:17Z

Needs confirmation but it seems that the globals (around line 80) don't do well with multiple ImGui contexts. But this deserves a whole new PR I think.

Amend from original pr to use a macro.

)

ocornut · 2023-04-11T14:01:23Z

Thanks both for your kind explanation.

Needs confirmation but it seems that the globals (around line 80) don't do well with multiple ImGui contexts. But this deserves a whole new PR I think.

That's correct, I've pushed those changes in cbdac1e + rebased this PR over this.
Then I have merged: e67f2f4 and 7b15fa7. There are 3 commits left in the branch.

For alignment, I used a macro instead
So (fr->VertexBufferSize * sizeof(ImDrawVert) + 3) & ~3 becomes MEMALIGN(fr->VertexBufferSize * sizeof(ImDrawVert), 4).

For the remaining:

Do you think imgui_impl_wgpu.xxx files and symbols should be renamed to imgui_impl_webgpu.xxx ?
How relevant are the WEBGPU_BACKEND_XXX defines in the larger ecosystem, as per your explanation in first post ("NB These macros are automatically defined....") shall we use those exact name or shall we use ours IMGUI_IMPL_WEBGPU_BACKEND_XXXX?

…d Gamma uniform. (#6188) Add gamma correction uniform Group uniforms in a single binding The second binding was not satisfying the minimum BufferBindingType::Uniform alignment (256) and since this alignment is large it is more idiomatic to group uniforms tegether. Also ensures that the size of the uniform buffer is aligned to 16 bytes.

Fix include for wgpu-native

ocornut · 2023-04-11T14:07:42Z

Amend: I made a mistake merging the WGSL change before the Gamma one as they depended on each others.
I amend this with a squash. Leaving 2 commits in the branch.

backends/imgui_impl_wgpu.cpp

eliemichel · 2023-04-11T21:22:42Z

Do you think imgui_impl_wgpu.xxx files and symbols should be renamed to imgui_impl_webgpu.xxx ?

It feels to me that "wgpu" tends to refer to Firefox' implementation of WebGPU so imgui_impl_webgpu.xxx would make more sense, but I don't know how much renaming this may break things, and this feeling may require some confirmation; I think it has low priority.

How relevant are the WEBGPU_BACKEND_XXX defines in the larger ecosystem, as per your explanation in first post ("NB These macros are automatically defined....") shall we use those exact name or shall we use ours IMGUI_IMPL_WEBGPU_BACKEND_XXXX?

I believe it would be good to settle on macros at the scale of the whole ecosystem. The choice of WEBGPU_BACKEND_XXX is the one I use personnally, and I am pushing for it both in my programming guide and through PRs upstream (ping @Kangz for the Dawn counterpart) but I am open to alternatives: I don't really care what the consensus is, I just strongly believe we need one so I made a proposal.

Kangz · 2023-04-12T13:08:03Z

I believe it would be good to settle on macros at the scale of the whole ecosystem. The choice of WEBGPU_BACKEND_XXX is the one I use personnally, and I am pushing for it both in my programming guide and through PRs gfx-rs/wgpu-native#251 (ping @Kangz for the Dawn counterpart) but I am open to alternatives: I don't really care what the consensus is, I just strongly believe we need one so I made a proposal.

I strongly believe that Dawn and wgpu should be targeting the same header eventually. There is basically a single issue to resolve, and some tech debt. So the WEBGPU_BACKEND_* macros should disappear eventually. But this PR is not the correct place to discuss this at length.

Amend from original pr to use a macro.

…d Gamma uniform. (ocornut#6188) Add gamma correction uniform Group uniforms in a single binding The second binding was not satisfying the minimum BufferBindingType::Uniform alignment (256) and since this alignment is large it is more idiomatic to group uniforms tegether. Also ensures that the size of the uniform buffer is aligned to 16 bytes.

eliemichel · 2023-08-29T20:34:11Z

This is now fully merged, there is no more backend discrepencies requiring some #ifdef 🥳

ocornut · 2023-08-30T12:40:03Z

Hello Elie,
Can you clarify, do you mean that all 3 tech have now converged to use same names?

eliemichel · 2023-09-02T12:51:00Z

For what is used in this backend, yes they do. So what you progressively merged for Dawn/emscripten just works with wgpu-native as well (no more need for the custom preprocessor macros).

I stopped using my branch and switched my projects to the official ImGui v1.89.8; it works in all 3 build cases!

eliemichel changed the title ~~Make WebGPU backend portable to both emscripten, Dawn, and wgpu-native backends~~ Make WebGPU backend portable to both emscripten, Dawn, and wgpu-native Feb 23, 2023

ocornut added backends osx/ios labels Feb 23, 2023

eliemichel changed the title ~~Make WebGPU backend portable to both emscripten, Dawn, and wgpu-native~~ Make WebGPU backend portable to emscripten, Dawn, and wgpu-native Feb 23, 2023

eliemichel force-pushed the eliemichel/portable_wgpu_backend branch from b349a02 to 4dd88e8 Compare February 23, 2023 23:36

Kangz reviewed Mar 8, 2023

View reviewed changes

ocornut force-pushed the eliemichel/portable_wgpu_backend branch from 582e4d7 to fd944ec Compare April 11, 2023 13:33

ocornut pushed a commit that referenced this pull request Apr 11, 2023

Backends: WebGPU: Align buffer sizes upon creation. (#6188)

e67f2f4

Amend from original pr to use a macro.

ocornut pushed a commit that referenced this pull request Apr 11, 2023

Backends: WebGPU: Use WGSL shaders instead of precompiled SPIR-V. (#6188

7b15fa7

)

ocornut force-pushed the eliemichel/portable_wgpu_backend branch from fd944ec to c8d770b Compare April 11, 2023 13:52

ocornut force-pushed the master branch from 7b15fa7 to d81f2ae Compare April 11, 2023 14:03

eliemichel added 2 commits April 11, 2023 16:05

Adapt to wgpu-native Drop methods

33b8978

Fix include for wgpu-native

Use WGPUMipmapFilterMode enum when available

11d34d5

ocornut force-pushed the eliemichel/portable_wgpu_backend branch from c8d770b to 11d34d5 Compare April 11, 2023 14:05

Kangz reviewed Apr 11, 2023

View reviewed changes

backends/imgui_impl_wgpu.cpp Outdated Show resolved Hide resolved

Kangz reviewed Apr 11, 2023

View reviewed changes

backends/imgui_impl_wgpu.cpp Outdated Show resolved Hide resolved

eliemichel mentioned this pull request Apr 24, 2023

Branch step095-emscripten doesn't build eliemichel/LearnWebGPU-Code#3

Closed

kjblanchard pushed a commit to kjblanchard/imgui that referenced this pull request May 5, 2023

Backends: WebGPU: Align buffer sizes upon creation. (ocornut#6188)

4d97cdd

Amend from original pr to use a macro.

ocornut pushed a commit that referenced this pull request Jul 13, 2023

Backends: WebGPU: fix webgpu changes for Dawn. (#6602, #6188)

3dc3aef

ocornut mentioned this pull request Jul 13, 2023

fix webgpu changes #6602

Closed

Merge branch 'master' into eliemichel/portable_wgpu_backend

6e62247

eliemichel closed this Aug 29, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make WebGPU backend portable to emscripten, Dawn, and wgpu-native #6188

Make WebGPU backend portable to emscripten, Dawn, and wgpu-native #6188

eliemichel commented Feb 23, 2023

ocornut commented Feb 23, 2023

eliemichel commented Feb 23, 2023 •

edited

ocornut commented Mar 6, 2023

eliemichel commented Mar 8, 2023 •

edited

Kangz commented Mar 8, 2023

Kangz Mar 8, 2023

eliemichel Mar 9, 2023

eliemichel commented Mar 9, 2023 •

edited

eliemichel commented Mar 20, 2023

ocornut commented Apr 11, 2023

ocornut commented Apr 11, 2023

eliemichel commented Apr 11, 2023 •

edited

Kangz commented Apr 12, 2023

eliemichel commented Aug 29, 2023

ocornut commented Aug 30, 2023

eliemichel commented Sep 2, 2023 •

edited

Make WebGPU backend portable to emscripten, Dawn, and wgpu-native #6188

Make WebGPU backend portable to emscripten, Dawn, and wgpu-native #6188

Conversation

eliemichel commented Feb 23, 2023

ocornut commented Feb 23, 2023

eliemichel commented Feb 23, 2023 • edited

ocornut commented Mar 6, 2023

eliemichel commented Mar 8, 2023 • edited

Kangz commented Mar 8, 2023

Kangz Mar 8, 2023

Choose a reason for hiding this comment

eliemichel Mar 9, 2023

Choose a reason for hiding this comment

eliemichel commented Mar 9, 2023 • edited

eliemichel commented Mar 20, 2023

ocornut commented Apr 11, 2023

ocornut commented Apr 11, 2023

eliemichel commented Apr 11, 2023 • edited

Kangz commented Apr 12, 2023

eliemichel commented Aug 29, 2023

ocornut commented Aug 30, 2023

eliemichel commented Sep 2, 2023 • edited

eliemichel commented Feb 23, 2023 •

edited

eliemichel commented Mar 8, 2023 •

edited

eliemichel commented Mar 9, 2023 •

edited

eliemichel commented Apr 11, 2023 •

edited

eliemichel commented Sep 2, 2023 •

edited