imgui icon indicating copy to clipboard operation
imgui copied to clipboard

Backends: Vulkan: Create a custom pipeline for viewports

Open skaman opened this issue 1 year ago • 5 comments

The main render pass may have a different configuration than the viewport window render pass. Vulkan can reuse the pipeline between different render passes only if the render pass configuration is the same. To fix it, when it's time to RenderDrawData for the viewport window and a pipeline is not yet available, a new pipeline is created for that window to be sure that is compatible with the render pass.

This is related to the issue #6305

skaman avatar Apr 12 '23 20:04 skaman

Hello, Thanks for your PR.

  • Could you post a patch (or separate commit which I won't merge) to repro the issue in one of the existing Vulkan example?
  • It seems like a shared pipeline could be used for all secondary viewports, in which case it may make more sense to create it during Init and reuse that?

ocornut avatar Apr 13 '23 10:04 ocornut

Hello, Thanks for your PR.

  • Could you post a patch (or separate commit which I won't merge) to repro the issue in one of the existing Vulkan example?

I created a commit into a different branch: https://github.com/skaman/imgui/commit/f974caa5ddcef16d2e890d4e5e46c34a3ed3addb

Is it ok?

I created a custom render pass that i pass to the ImGui_ImplVulkan_Init function. To reproduce the issue the render pass should differ from the internal one, so i changed the color format from UNORM to SRGB (the colors are weird but it's just for test). In the real world probably the user have a depth buffer or multiple render passes.

  • It seems like a shared pipeline could be used for all secondary viewports, in which case it may make more sense to create it during Init and reuse that?

Yes you're right. I'll change the fix to have a shared pipeline for all secondary windows. I'll let you know when it's done.

skaman avatar Apr 13 '23 19:04 skaman

Ok, committed the shared pipeline. Now there's only one pipeline for all the secondary windows.

skaman avatar Apr 13 '23 20:04 skaman

Is this being merged? I'm experiencing an issue that would be solved with this. Particularly the RenderPass I'm using in ImGui_ImplVulkan_Init has a VK_FORMAT_B8G8R8A8_SRGB format, but the created window defaults to VK_FORMAT_B8G8R8A8_UNORM, or any of const VkFormat requestSurfaceImageFormat[] = { VK_FORMAT_B8G8R8A8_UNORM, VK_FORMAT_R8G8B8A8_UNORM, VK_FORMAT_B8G8R8_UNORM, VK_FORMAT_R8G8B8_UNORM };.

I could see this as a solution, or some way to request a custom surface format when creating the multi-viewport renderpass.

raaavioli avatar Sep 24 '23 14:09 raaavioli

Any update on this?

theunwisewolf avatar Apr 19 '24 22:04 theunwisewolf

I created a commit into a different branch: https://github.com/skaman/imgui/commit/f974caa5ddcef16d2e890d4e5e46c34a3ed3addb

Thank you @skaman and sorry for slow answer.

Why is the if (wd->Pipeline) { vkDestroyPipeline(device, wd->Pipeline, allocator); } code being removed?

ocornut avatar Apr 30 '24 12:04 ocornut

Why is the if (wd->Pipeline) { vkDestroyPipeline(device, wd->Pipeline, allocator); } code being removed?

I realized that:

  • wd->Pipeline is misleadingly essentially never used by the backend so far, and therefore always null.
  • If anything, it can then only be potentially fed by the application using the ImGui_ImplVulkanH_Window helper, but our own example apps don't use it, our backend doesn't use it anymore, and we discourage users from using those helpers anyway.

Therefore the whole thing is a fishy leftover that once worked. Initially was added in 41e2aa2e for #3459. That initial version had the ImGui_ImplVulkan_CreatePipeline() call in ImGui_ImplVulkanH_CreateWindowSwapChain(), it made sense.

However it was immediately removed by e8447dea also for #3459 and I believe I messed up there. @FunMiles themselves updated later and eventually reacted to it https://github.com/ocornut/imgui/pull/3459#issuecomment-856827913 but I guess it went unnoticed then :(

While it seems to be my fault I believe it highlight that I need clear repros of the more intricate use cases of Vulkan. (I maintain them in private branches and could probably aim to publish them somewhere). Thanks for providing one here!

ocornut avatar Apr 30 '24 13:04 ocornut

The PR is incorrect as is, as ImGui_ImplVulkanH_XXXX() functions aren't allowed to use backend data. In fact it makes the main.cpp examples asserts because they call ImGui_ImplVulkanH_CreateOrResizeWindow() first.

I have moved the creation in ImGui_ImplVulkan_CreateWindow() instead: ebb8d78 and it works. I have also vaguely confirmed that it works with my setup for using dynamic rendering.

Apologies @FunMiles ! Thanks all!

ocornut avatar Apr 30 '24 13:04 ocornut