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

Add wgpu multiviewport support #7557

Open
wants to merge 4 commits into
base: docking
Choose a base branch
from

Conversation

Zelif
Copy link

@Zelif Zelif commented May 4, 2024

Supersedes #7132

Add Multiviewport support to the WebGPU backend and example.

Tasks outstanding:

  • Have user defined callback for creating a surface when create window is called
  • Remove wgpu_instance & headers needed for surface creation
  • Change the format and presentation mode better (select preferred rather than first available but fall back to first available)
  • Fix present mode assert to use ~0 not 0

Note

Linux users(using Dawn):
Due to an intel bug on linux/vulkan, the Mailbox present mode is the only thing available using vulkan backend. This disables Vsync. Using both AMD/Nvidia seems fine to edit dawn's code directly but that isn't a solution.
The old manual swapchain creation and usage and selecting FiFo is still working.
Fixed with this commit:
https://dawn.googlesource.com/dawn/+/780bbd6fce6b8e81569406fdb4b490613d158733%5E%21/#F2

@Zelif Zelif marked this pull request as draft May 4, 2024 20:35
@Zelif Zelif changed the title Add glfw wgpu multiviewport support Add wgpu multiviewport support May 4, 2024
@Zelif
Copy link
Author

Zelif commented May 10, 2024

@ocornut & @eliasdaler
Unsure if that is how you would want the callback, but seems to work.

The changes that have been done are:

  • To move away from Swapchain and use SurfaceConfigure . (Dawn has finally updated to be inline with wgpu-native)
  • Add a callback for the user to define how to handle the surface creation after the window has been made
  • Updated the example to include the callback using the glfw surface creation helper so the wgpu impl can be independent. (If SDL was used per say they could create the surface from the platform handle if that is what they are using)

Hopefully this sorts out multiviewport for desktop versions.

Warning

It seems to hang when trying to clean up the surface in some sort of race condition I assume. If I put a breakpoint inside the surface release code and resume the program hang is gone. I think the vulkan surface gets cleaned up before the internal swapchain gets deleted, this results in the program hanging.

This issue seems to not happen on my windows machine, as it seems to be an issue with the vulkan part.

Oddly enough it only happens when I drag the child window in and drop it, if I use the main window to encompass the child window there is no hang/crash.

@Zelif Zelif marked this pull request as ready for review May 10, 2024 23:07
@@ -318,7 +330,10 @@ static bool InitWGPU(GLFWwindow* window)
wgpu::Surface surface = wgpu::glfw::CreateSurfaceForWindow(instance, window);
if (!surface)
return false;
wgpu_preferred_fmt = WGPUTextureFormat_BGRA8Unorm;
wgpu::SurfaceCapabilities capabilities;
Copy link
Author

Choose a reason for hiding this comment

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

This might be questioned, so I will explain this.
This gets the first available texture format and presentation mode for the surface, on linux current with dawn since there is no FiFo presentation mode for vulkan the example will not work for linux.
This solves it, it will get the first available presentation mode which in most system the first one is FiFo and on linux will be Mailbox.

commit 3cd29e4
Author: Zelif <jarodk.1988@gmail.com>
Date:   Wed May 15 09:30:41 2024 +1000

    Add Docking flag

commit fb98ed9
Author: Zelif <jarodk.1988@gmail.com>
Date:   Sat May 11 08:53:39 2024 +1000

    Changed window creation to be a user callback

commit 8d08e0b
Author: Zelif <jarodk.1988@gmail.com>
Date:   Sun May 5 05:34:59 2024 +1000

    Updated wgpu example for multiviewport
    - Added instance to init model
    - Added Present mode for any new window to adhere by
    - Added Multiviewport render and update code
    - Changed Texture format and Present mode to select the first available mode for capabilities (some surface creation such as laptops might not support FiFo)
@Zelif Zelif force-pushed the origin/features/multiviewport-wgpu branch from 3cd29e4 to 14a89bc Compare May 14, 2024 23:44
@ocornut
Copy link
Owner

ocornut commented Nov 8, 2024

Hello,

If you have time would mind:

  • rebasing this on latest.
  • squashing, or splitting into multiple commits if there is are possible distinct changes to make.
  • confirming that this compile with all 3 targets (Emscripten, WGPU? Dawn?) and which of them may support multi-viewport (obv not Emscripten). As you now we now use IMGUI_IMPL_WEBGPU_BACKEND_DAWN/IMGUI_IMPL_WEBGPU_BACKEND_WGPU define to support those targets which haven't converged.

Thank you!

@ocornut
Copy link
Owner

ocornut commented Nov 8, 2024

About the WGPUSurface (*CreateViewportWindowFn)(ImGuiViewport* viewport) = nullptr; function:

The Vulkan backend has been facing a similar problem, which I have solved with a function pointer in ImGuiPlatformIO:

int (*Platform_CreateVkSurface)(ImGuiViewport* vp, ImU64 vk_inst, const void* vk_allocators, ImU64* out_vk_surface); // (Optional) For a Vulkan Renderer to call into Platform code (since the surface creation needs to tie them both).

In our case this is filled by the platform backend:

platform_io.Platform_CreateVkSurface = ImGui_ImplSDL3_CreateVkSurface;

// Vulkan support (the Vulkan renderer needs to call a platform-side support function to create the surface)
// SDL is graceful enough to _not_ need <vulkan/vulkan.h> so we can safely include this.
#include <SDL3/SDL_vulkan.h>
static int ImGui_ImplSDL3_CreateVkSurface(ImGuiViewport* viewport, ImU64 vk_instance, const void* vk_allocator, ImU64* out_vk_surface)
{
    ImGui_ImplSDL3_ViewportData* vd = (ImGui_ImplSDL3_ViewportData*)viewport->PlatformUserData;
    (void)vk_allocator;
    bool ret = SDL_Vulkan_CreateSurface(vd->Window, (VkInstance)vk_instance, (VkAllocationCallbacks*)vk_allocator, (VkSurfaceKHR*)out_vk_surface);
    return ret ? 0 : 1; // ret ? VK_SUCCESS : VK_NOT_READY
}

I would be open to potentially e.g.

  • add a dedicated function in PlatformIO (but its signature would need to be portable, so not use WGPU type)
  • see if we can merge the functions (if it makes sense)

But your solution may the best one because in the case of wgpu the desirable function is simpler to implement.

@Zelif
Copy link
Author

Zelif commented Nov 14, 2024

* rebasing this on latest.

* squashing, _or_ splitting into multiple commits if there is are possible distinct changes to make.

Will do.

* confirming that this compile with all 3 targets (Emscripten, WGPU? Dawn?) and which of them may support multi-viewport (obv not Emscripten). As you now we now use IMGUI_IMPL_WEBGPU_BACKEND_DAWN/IMGUI_IMPL_WEBGPU_BACKEND_WGPU define to support those targets which haven't converged.

I will have a look over it and see what changes need sorting, I am quite behind with the WGPU releases(last I touched it was these changes)

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