Add glfw wgpu desktop and multiviewport support

[Zelif]

Adding Multi viewport to the desktop version of WGPU

When using dawn/wgpu-native have the multi viewport enabled and rendering (fixes: #7110 )

Items required to get the wgpu backend working:

• Enable the backend setting and pluming
• Create required render functions
  • Create Window - needs work
  • Render Window
  • Swap Window
  • Set Window size
  • Destroy Window
• Store WGPU instance for surface creation
• Store WGPU surface for swapchain creation per veiwport
• Store WGPU swapchain for rendering per veiwport
• Add safe release functiosn for surface and swapchain
• Disable any multi viewport parts if emscripten is required
• Make sure wgpu-native compiles
• Make sure makefile works as well (include a desktop wgpu lib)
• Track down memory leak when window is undocked (potential in render window ->
  color_attachments.view = wgpuSwapChainGetCurrentTextureView(vd->Window.swapChain);)

Create window is the major area that still needs work due to surface creation being platform dependent. I was looking at the way vulkan handles it and there is a special function inside the glfw/sdl/etc backends (CreateVkSurface) I assume this is the approach that would be required going forward?

Example changes

Example now works on desktop and all emscripten stuff is behind preprocessor directives.

Added CMakeLists.txt and a CMakePresets.json, the latter doesn't have to be used but is handy (only works with CMake 3.19+), adding the location to a clone of the dawn repo is required in CMakeLists.txt:30. I am yet to test out wgpu-native vs dawn as of yet. Or if there is anywhere that has static builds to link against for dawn that could be included. (I am unsure how you would want this included)

The make file will need to be updated but I do not know my way around them, and as I only know how to get dawn working with CMake that is what I used.

Firefox/WGPU-Native
Google/Dawn

Video of it working:
https://github.com/ocornut/imgui/assets/666945/875315c9-5b61-42b6-b8ed-a31bc8179050

Hopefully this helps :D

[ocornut]

Hello,

Very interested in getting even-basic desktop support in.

Could you squash/rebase this and try to separate it into two commits?

• one commit to compile example on desktop
• one commit to add multi-viewport support
  If multi-viewport support is going to be complicated I would suggest aiming at merging the first part ahead of time.

Feedback:

• 1. What is the justification for using <memory> ? We typically don't rely on STL. It seems like all the std::unique_ptr<>/make_unique<> cruft could be changed into regular pointer?
• 2. What is the justification for changing e.g. WGPUDevice to e.g. wgpu::Device ? What is the difference?
• 3. Emscripten abstraction can use the same emscripten_mainloop_stub.h EMSCRIPTEN_MAINLOOP_BEGIN EMSCRIPTEN_MAINLOOP_END mechanism we use in e.g. example_glfw_opengl3, for consistency.

[ocornut]

FYI i have pushed some WebGPU related changes which will conflict with this (but ultimately simplify the diff).

• e3c7ff9 Examples: Emscripten+WebGPU: slightly refactor like other Emscripten compatible Desktop examples, as aiming to make this support desktop eventually. -> this include some of the cruft from this PR and will be the most breaking part, so I checked and it is relatively easy to resolve the conflicts they are pretty shallow.
• 831d42c WebGPU: ImGui_ImplWGPU_Init() now takes a ImGui_ImplWGPU_InitInfo structure instead of variety of parameters, allowing for easier further changes. (WebGPU pipeline is hardcoded to not use multisampling #7240) --> This will conflict with your addition of a parameter to ImGui_ImplWGPU_Init() but should be obvious to fix.
• 5fc0a36: WebGPU: added ImGui_ImplWGPU_InitInfo::PipelineMultisampleState. (WebGPU pipeline is hardcoded to not use multisampling #7240) -> I believe this shouldn't conflict once the previous one is merged.

To proceed I suggest that:

• you first merge docking branch just before those 3 changes.
• then you successively cherry-pick those 3 commits.
• now you have an up to date branch and we can easily squash/rebase into a neat 1 or 2 commits.
  Let me know if you need help with any of this.

[waywardmonkeys]

I'm working with the wgpu-native folks to make it easier to use it with cmake, so that'll be of help perhaps to you in the future.

But a couple of notes from reading through this:

• It'd be nice if everything used the C API / names rather than some using C and some using C++. That might be a good thing to do as a separate PR to clean up what's in the code now and then fix the merge conflicts? (I'm willing to do the change to what's in the code now...)
• The swap chain API is not in wgpu-native and has been merged with the surface instead, so that part of things will need some work. There's an open issue for Dawn to make the same changes in API (with a patch being developed): https://bugs.chromium.org/p/dawn/issues/detail?id=2320&q=swapchain&can=2

[Zelif]

@ocornut sorry for disappearing, just have a contract close to completion, that I need to take care of. I will sort those changes within the coming week or 2.

Surface creation is still something I am having issues with cross platform. That will require a bit of work to figure out.

@waywardmonkeys Awesome to hear about the cmake stuff and conflicts with swapchain. I had no idea that was coming. I will have to read up on it. The mix between the C and C++ was to get around surface creation stuff for half of it and getting the device and adapter callbacks. I have kept the implementation of webgpu as the C api only to keep it more inline, but the example has the C and C++ stuff. Tho it still has swapchain stuff in the dawn implementation that i used. So we will need to convert that.

What would the best approach be for aligning this? Pre processor for wgpu-native and dawn for how to use swapchain? But that would break when dawn changes it. My guess will be make it the way wgpu-native uses it and maybe release it once dawn releases that change.

I will try sort out some stuff today around the merge from docking, feel free to sort out the swapchain/surface reconciliation issue any help there is much appreciated :D

[eliasdaler]

Tested with Dawn e24033c.

Had to make a change specified below for it to work.

This one:

color_attachments.depthSlice = WGPU_DEPTH_SLICE_UNDEFINED;

Also when resizing the window, it crashes for me on Windows 10 and D3D12 backend here

[eliasdaler]

@ocornut, maybe it's worth renaming everything from WGPU to WebGPU? "wgpu" is a name of the Rust implementation and I see "WebGPU" being used everywhere (except for C API, which I find unfortunate).

Otherwise people might think that this backend can only be used with wgpu and not with Dawn.

[Zelif]

I had chosen to go with WGPU purely based on how the C API was naming things, so yeah WebGPU is fine to go with as well.

[eliasdaler]

These constructor and destructors are unnecessary and can be removed.

[Zelif]

That was a hang over of when I did have objects in there, yup should have been deleted

[eliasdaler]

Doesn't work without this:

color_attachments.depthSlice = WGPU_DEPTH_SLICE_UNDEFINED;

[Zelif]

I think this is a new requirement imposed in a version of dawn that I had yet to work with, If that is required then putting it in should be done.

[eliasdaler]

Probably needs a comment which will explain why "instance" is optional here (so that people don't need to read the implementation to know that).

[eliasdaler]

If it's not meant to be used by other people, then maybe let's move it into the example's main.cpp?

Actually, it would be pretty handy since you'll be able to use Dawn's webgpu_glfw.h + CreateSurfaceForWindow function that they provide.

[eliasdaler]

This is needed here too:

color_attachments.depthSlice = WGPU_DEPTH_SLICE_UNDEFINED;

[eliasdaler]

As others have said, there's no need to use C++ wrappers here since they don't simplify much in this example.

[eliasdaler]

This type of code is pretty hard to follow. Would be a bit easier to understand this, imo (note that wgpu_instance should be changed to a C type too):

int main(int, char**)
{
    wgpu_instance = wgpuCreateInstance(nullptr);
    WGPUAdapter adapter = requestAdapter(wgpu_instance);
    wgpu_device = requestDevice(adapter);

    SetupAndRun();
    return 0;
}

static WGPUAdapter requestAdapter(WGPUInstance instance)
{
    auto onAdapterRequestEnded = [](WGPURequestAdapterStatus status,
        WGPUAdapter adapter,
        char const* message,
        void* pUserData) {
            if (status == WGPURequestAdapterStatus_Success) {
                *static_cast<WGPUAdapter*>(pUserData) = adapter;
            }
            else {
                printf("Could not get WebGPU adapter: %s\n", message);
            }
        };

    WGPUAdapter adapter;
    wgpuInstanceRequestAdapter(instance, nullptr, onAdapterRequestEnded, (void*)&adapter);
    return adapter;
}

static WGPUDevice requestDevice(WGPUAdapter& adapter)
{
    auto onDeviceRequestEnded = [](WGPURequestDeviceStatus status,
        WGPUDevice device,
        char const* message,
        void* pUserData) {
            if (status == WGPURequestDeviceStatus_Success) {
                *static_cast<WGPUDevice*>(pUserData) = device;
            }
            else {
                printf("Could not get WebGPU device: %s\n", message);
            }
        };

    WGPUDevice device;
    wgpuAdapterRequestDevice(adapter, nullptr, onDeviceRequestEnded, (void*)&device);
    return device;
}

[Zelif]

Yup that looks way better

[eliasdaler]

This should probably be moved out of the backend and user should provide this instead - there's no easy way to handle this for every platform/WebGPU impl.

[Zelif]

100% this is the main thing I was struggling with trying to fix, would it be just a callback that a user would need to assign to ?

[eliasdaler]

Can be removed if ImGui_ImplWGPU_CreateWindow is not included in the backend.

[eliasdaler]

@Zelif
After some discussion with Omar, we have decided that it might be better to split this PR in at least two parts for ease of reviewing/testing. One part should be just about the desktop integration. The other one should be about adding support for multiple viewports and should be merged after the first one is done.

If you're currently busy with other work, I can do this myself and credit your contributions.

(P.S. I would also make some of the changes I've proposed myself, especially about C API usage and taming the callbacks in main.cpp)

[Zelif]

@eliasdaler
That would be great, I have been bogged down too much to contribute any further for a while now(always moving goal posts are fun :( ) Feel free to change it how ever you see fit, I don't need any credit for this, rather see it working than myself taking on extra load right now and failing. Originally I didn't think I would have got this far with it as wgpu is new for me and C++ is rusty.

The changes with swapchain I have yet to read up on and unsure if they have made their way to Dawn yet. I never got around to testing wgpu-native with it either.

What is the best way to move forward ? Do I add you or anyone else to my fork or did we want to move this to a new PR with someone else in control. I'm fine with anything wish I had time to sort it out.

[eliasdaler]

What is the best way to move forward ? Do I add you or anyone else to my fork or did we want to move this to a new PR with someone else in control. I'm fine with anything wish I had time to sort it out.

No problem, life happens, don't feel bad about it. I've abandoned quite a few PRs in my life myself and it's okay. :)

I'll start a new PR from my fork sometime later.

[Zelif]

@eliasdaler
Cheers, would be a load off my mind it has been bugging me not having time to spend time on it, when the creation of the instance and requiring of the adapter & device has been done. The index file can be removed and we can use the template one from:
libs/emscripten/shell_minimal.html

The only difference that was being done was the instance, adapter and was being captured in the html. (this was being swapped over in another pull request here: #7151
but that one never made it out.

Seems the depth stencil issue has already been resolved via:
#7232

[Zelif]

Welp I messed that up(I will have to fix the history of this PR or start another?),
@eliasdaler @ocornut
I had some time today to revise multiviewport until I fix what I did with the merge, the changes found here:
https://github.com/Zelif/imgui/tree/origin/features/multiviewport-wgpu
It has 2 commits the changes for multiviewport and then the change to the example.
Removed the swapchain stuff and changed to surface configuration, and in the example get the preferred format and present mode. This was due to surface config not allowing me to set FiFo on my integrated GPU (yet was working when I controlled the swapchain).
This should align more with the wgpu-native library since they do not use swapchain.

This still has the issue of surface creation which @eliasdaler said to change to a callback that a user creates. I am unsure how to structure this, but should hopefully help.

EDIT: I started another PR that focuses on just the multiviewport stuff #7557
Probably close this one off as the other desktop parts are already been handled. :D