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

Update webgpu backend to latest dawn #7977

Closed
wants to merge 7 commits into from

Conversation

acgaudette
Copy link
Contributor

these changes ensure compatibility between imgui's webgpu backend and the webgpu.h header provided by the dawn webgpu implementation.

in addition, the dawn native example in examples/example_glfw_wgpu currently does not build; with this patch, the example builds and runs (although note that it uses a fair amount of deprecated API and is a ways off spec).

unfortunately, this change will break compatibility between imgui's webgpu backend and wgpu-native, as far as I can tell. this will almost certainly impact downstream wgpu users. I have not checked emscripten.

see also:
#6602
#6188
#7523

- Rename usage of `WGPUSType_ShaderModuleWGSLDescriptor`
  to `WGPUSType_ShaderSourceWGSL`
- Use `WGPUOptionalBool`
  for `WGPUDepthStencilState.depthWriteEnabled`
@ocornut
Copy link
Owner

ocornut commented Sep 12, 2024

unfortunately, this change will break compatibility between imgui's webgpu backend and wgpu-native, as far as I can tell. this will almost certainly impact downstream wgpu users. I have not checked emscripten.

Err, then what do you suggest? Couldn't both be supported via some ifdef or version check?
Emscripten should be checked as well.

Also see #7969

@acgaudette
Copy link
Contributor Author

at the moment, dawn users are getting the short end of the stick (it's always someone 😁). it may indeed be time to revisit the ifdef proposal in the issues mentioned above. taking a look.

note that the current example is pretty heavily tied to dawn. #6188 appears to be an attempt to resolve this.

@ocornut
Copy link
Owner

ocornut commented Sep 12, 2024

Thank you for your help!
I have to admit I lack knowledge in those ecosystems and I'm struggling to follow.
I can compile the Emscripten stuff but not very acquainted with the rest.

@ocornut ocornut changed the title update webgpu backend to latest dawn Update webgpu backend to latest dawn Sep 12, 2024
@acgaudette
Copy link
Contributor Author

it's definitely pretty wild west! truly, the crux of the issue is that everyone ships a slightly different version of the same interface (webgpu.h). although technically there is a single source of truth (webgpu-native/webgpu-headers), the webgpu spec has not yet reached 1.0, and the three vendors are in various states of being ahead or behind spec changes. eventually the idea is to converge on an equivalent header. in the meantime, keeping up with this whack a mole is necessary to support where everyone is on the overall timeline. the need for truly implementation-specific code seems to be disappearing, thankfully.

I have confirmed that these changes break emscripten, which was passing before.

@acgaudette
Copy link
Contributor Author

OK. neither dawn nor wgpu expose anything -- official, that is -- that would allow us to key on the implementation. eventually this problem will go away. in the meantime, providing defines of the form IMGUI_IMPL_WEBGPU_BACKEND_... is probably the best bet. it's a bummer to have to push this requirement downstream to everyone, but the only other solutions I came up with are effectively hacks. I'm open to suggestions.

note that __EMSCRIPTEN__ is well defined -- it's dawn versus wgpu that concerns us here.

we have two options in this case. we can provide a default, i.e. prefer one implementation over the other. it would make some sense to use dawn, given that the current example only supports dawn.

alternatively, we can force the user to define one or the other. this is what I've added to the PR currently.

@ocornut
Copy link
Owner

ocornut commented Sep 12, 2024

This is a good idea, probably the best we can do the meanwhile.

I would add some commentary in imgui_impl_wgpu.h to increase visibility.
Maybe reflect this selection in io.BackendRendererName setup.
Is IMGUI_IMPL_WEBGPU_BACKEND_WGPU likely to be needed then?

Thanks!

@acgaudette
Copy link
Contributor Author

ah, yes, I had meant to add a comment! thank you for the notes. everything look OK? changes are welcome.

IMGUI_IMPL_WEBGPU_BACKEND_WGPU is not strictly necessary, but it seems prudent to keep both, if only to force the user to pick one. otherwise we imply something like the existence of an uncountable number of possible backends versus a single source of truth. one define would make the build process pretty asymmetric -- one of the backends would silently pass whereas the other would require a special build flag. I also figure having both is useful in case we ever want to signal code specific to either one... the kind of thing you'd wrap in just a single ifdef ... endif block. but I'm certainly open to alternatives.

I have tested this patch on the repo example with both dawn and emscripten. we have no path currently to test with wgpu, but I tested it locally. everything looks good to go there.

@acgaudette
Copy link
Contributor Author

thinking about the changelog -- a rename of this PR might be in order.

ocornut pushed a commit that referenced this pull request Sep 16, 2024
@ocornut
Copy link
Owner

ocornut commented Sep 16, 2024

Merged as 1ac162f, thank you!

@ocornut ocornut closed this Sep 16, 2024
@ocornut
Copy link
Owner

ocornut commented Sep 20, 2024

Someone mentioned not being able to build this, presumably because they use a slightly older version of DAWN.
imgui_impl_wgpu.cpp:262:29: error: use of undeclared identifier 'WGPUSType_ShaderSourceWGSL' 262 | wgsl_desc.chain.sType = WGPUSType_ShaderSourceWGSL;

Do we have access to some compile-time version number that we can use to better support them?

@acgaudette
Copy link
Contributor Author

as far as I can tell (I peeked into this recently) google doesn't version dawn in any way, at least externally. or rather, they version by hash. it's a rolling release. so there's not really much to work off, there.

wgpu is a little better, but things still feel fast and loose. I recommend only supporting latest, at least until the implementations become more stable. where would we draw the line? we probably shouldn't be beholden to every user's specific checkout.

as far as your someone goes -- where are they getting their dawn? it seems unlikely that they are tied to any specific version as such, but perhaps they are dependent on a third party's release cycle (some homebrew projects package ad hoc binaries afaik, but aren't officially supported). the ShaderSource rename is about a month old, I think.

a quick fix could be to try compiling with IMGUI_IMPL_WEBGPU_BACKEND_WGPU instead of _DAWN -- that might work here, until they upgrade. otherwise, if I was a user in this situation, I would maintain a fork / patch.

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

Successfully merging this pull request may close these issues.

2 participants