Vulkan: Re-create main window pipeline (standalone)

[SuperRonan]

This is a standalone PR (from #8053, solve one problem at the time) to just recreate the main window pipeline when one of its properties changes (RenderPass / pDynamicRendering, MSAASamples, ...), but without any color correction (the colors might look off depending on the render target format / swapchain color space if directly forwarded to be presented).

A docking version is comming soon (without color correction).

API changes:

• Added ImGui_ImplVulkan_ReCreateMainPipeline(...) to explicitly re-create the main window pipeline (when some of its properties are changed).
• ImGui_ImplVulkan_CreatePipeline(...) does not implicitly use ImGui_ImplVulkan_InitInfo::PipelineRenderingCreateInfo, but a function parameter.
• The main window pipeline is created only if possible during ImGui_ImplVulkan_Init(...) (if a render pass or rendering info are given), else it should be created with ImGui_ImplVulkan_ReCreateMainPipeline(...) before rendering)

[ocornut]

Sorry this is still too complex for me to follow.

Can you:

• [1] Clarify how and when to use this (perhaps a commit to an example would be good)
• [2] Why is creation code in the ImGui_ImplVulkan_Init() function and not in ImGui_ImplVulkan_CreateDeviceObjects() ?
• [3] Can you explain the logic/need to allow v->PipelineRenderingCreateInfo.sType != VK_STRUCTURE_TYPE_PIPELINE_RENDERING_CREATE_INFO_KHR) when dynamic rendering is used? It was previously required? It is as a convenience to allow the user to create pipeline later than Init?
• [4] Could ImGui_ImplVulkan_PipelineRenderingInfo either be renamed to ImGui_ImplVulkan_PipelineRenderingCreateInfo to match VkPipelineRenderingCreateInfoKHR, or ideally removed? Maybe ifndef IMGUI_IMPL_VULKAN_HAS_DYNAMIC_RENDERING then we use typedef void VkPipelineRenderingCreateInfoKHR directly?
• [5] What is IMGUI_IMPL_VULKAN_HAS_MAIN_PIPELINE_RE_CREATION for?
• [6] Why commenting out //if (info->UseDynamicRendering == false) IM_ASSERT(info->RenderPass != VK_NULL_HANDLE); ?
• [7] It feels like the struct ImGui_ImplVulkan_MainPipelineCreateInfo is superfluous and those could be parameters to ImGui_ImplVulkan_ReCreateMainPipeline().
• [8] Likewise for ImGui_ImplVulkan_PipelineCreateInfo. Not in principle against using struct, and I agree they tends to play better in term of long-term API design, but this one is specifically an internal only API so it doesn't matter.

[SuperRonan]

Hi, Sorry for the late response.

Concerning your points:

• [1]: This is useful when some parameters that the pipeline depends on change (for example when the format of the target changes). So far we would need to shut down ImGui_Impl_Vulkan and init it again with the correct parameters. I think it is a bit overkill, and just re-creating the pipeline seems better. Here is an example usage in my engine.
• [2]: Indeed it should probably be in ImGui_ImplVulkan_CreateDeviceObjects(). I am moving it.
• [3]: Yes! I will add some comments to clarify it. v->PipelineRenderingCreateInfo is optional and considered valid iif its sType has the correct value.
• [4]: ImGui_ImplVulkan_PipelineRenderingCreateInfo it better indeed!
• [5]: It signals that this feature is available in the current version of ImGui, which can be useful when compiling with different versions of ImGui. It can be removed if you prefer.
• [6]: Bad habbit of commenting code rather than removing it... (done now)
• [7-8]: I don't like functions with too many parameters and prefer grouping them in structs. There could be more parameters in the future, and a struct is more extensible than many params in a function.

I pushed changes according to your comments. I also did the same on the docking version #8111.

[ocornut]

Thanks for your update!

Minor stuff:

• I feel ImGui_ImplVulkan_ReCreateMainPipeline() could be simply named ImGui_ImplVulkan_CreateMainPipeline().
• I believe [5] IMGUI_IMPL_VULKAN_HAS_MAIN_PIPELINE_RE_CREATION can go, there's rarely ever code that needs to be built for varying backend versions.
• Docking version has leftover ImGui_ImplVulkan_CreateFontsTexture() etc. which don't exist anymore.

If they are the only changes and if it's much work for you to rebase/push both branches don't worry I can apply those changes myself.

I agree in principle with [7] [8] but there are very few call sites to [7] and adding structs to the .h increases perceived complexity. For [8] it is all internal and is technically and unrelated refactor. I am undecided but I might rework this a little bit.

I admit I am a little bit puzzled by your main reason for this [1], sure it is in theory overkill but you do have any data suggesting it is a meaningful performance problem?
However I am generally favorable to adopt your PR partly because I appreciate that you have done a good work following up on it, especially considering how it awkwardly straddles the branches.

[ocornut]

Please don't make new edits right now because I'm working on this on my side, but I have a question:

in ImGui_ImplVulkan_ReCreateMainPipeline() you are doing:

    if (info.pDynamicRendering)
    {
        v->PipelineRenderingCreateInfo = *info.pDynamicRendering;
    }

Which seems a bit ambiguous, afaik the value is not used after _Init by other functions, but it seems generally sensible/sane to set this. But then if we set it (why not) then the equivalent parameter in ImGui_ImplVulkan_CreatePipeline()'s ImGui_ImplVulkan_PipelineCreateInfo seems like the same thing and misleading?

[ocornut]

Question 2:

We have:

VkPipelineRenderingCreateInfoKHR   ImGui_ImplVulkan_InitInfo::PipelineRenderingCreateInfo

vs

const VkPipelineRenderingCreateInfoKHR* ImGui_ImplVulkan_MainPipelineCreateInfo::pDynamicRendering

For essentially the same thing. Name and storage are inconsistent and something could be changed.
(I don't mind breaking API for the first one if needed, because it'll be an obvious fix in 1 place for users of this)

For consistency wouldn't it make sense to make the one in the new structure exactly the same as the one in the InitInfo struct, aka not a pointer, and rely on v->UseDynamicRendering && v->PipelineRenderingCreateInfo.sType == VK_STRUCTURE_TYPE_PIPELINE_RENDERING_CREATE_INFO_KHR ?

Supposedly we don't allow toggling v->UseDynamicRendering dynamically without shutting down backend (we probably could but that's not the topic here), so any call to ImGui_ImplVulkan_{Re}CreateMainPipeline() would set the field anyhow?

[SuperRonan]

I don't have more changes in mind (for now). I'll let you dot the changes you suggested when merging.

I would also suggest adding comments to explain the new behaviour

// The main window pipeline is created during Init() if the RenderPass is provided xor dynamic rendering is used and PipelineRenderingCreateInfo.sType is valid. 
// Else, the pipeline can be created, or re-created, using ImGui_ImplVulkan_CreateMainPipeline() before rendering. 

Concerning v->PipelineRenderingCreateInfo = *info.pDynamicRendering;: Yes we don't use that value anywhere else, but it is better to keep the up to date data rather than outdated one (for debugging at least).
Which makes me think that we should also copy the format array at this point like in #8282.
ImGui_ImplVulkan_InitInfo::PipelineRenderingCreateInfo contains information about the main rendering pipeline. But viewports have their own VkPipelineRenderingCreateInfoKHR. This is why ImGui_ImplVulkan_PipelineCreateInfo::pRenderingInfo is not necessarily ImGui_ImplVulkan_InitInfo::PipelineRenderingCreateInfo.
It is necessary if the main pipeline and the viewports have a different format.

[SuperRonan]

It is necessary to keep ImGui_ImplVulkan_InitInfo::PipelineRenderingCreateInfo and ImGui_ImplVulkan_MainPipelineCreateInfo::pDynamicRendering separate for the docking branch.

[ocornut]

It is necessary to keep ImGui_ImplVulkan_InitInfo::PipelineRenderingCreateInfo and ImGui_ImplVulkan_MainPipelineCreateInfo::pDynamicRendering separate for the docking branch.

I'm not sure I understand this part of the answer, the later should be written into the earlier.
My question was just, should they use the same name and syntax as the one in ImGui_ImplVulkan_InitInfo ?

[SuperRonan]

I'm not sure I understand this part of the answer, the later should be written into the earlier.

Yes, for the main window pipeline eclusively during ImGui_ImplVulkan_CreateMainPipeline().

You can rename ImGui_ImplVulkan_MainPipelineCreateInfo::pDynamicRendering to match ImGui_ImplVulkan_MainPipelineCreateInfo::pDynamicRendering if you prefer (don't forget to keep the p for the pointer one).

[ocornut]

(1) But does it even need to a pointer? Please re-read my question. It could use same mechanism as ImGui_ImplVulkan_InitInfo, aka not be a pointer and use sType check.

(2) Additionally, for consistency and toward being able to configure how secondary viewport pipeline is created, I wonder if we should, in theory, later:

• Remove InitInfo::RenderPass, Subpass, MSAASamples, PipelineRenderingCreateInfo
• And instead add two ~PipelineCreateInfo structs in InitInfo: for main viewport and for secondary viewports.
  (But I would tackle that later)

[SuperRonan]

(1) Sorry I didn't understand it the way you meant. No it does not need to be a pointer, I just thought it made more sense since it is optional. I agree it is preferable to use the same logic everywhere.

[SuperRonan]

(2) I did not change ImGui_ImplVulkan_InitInfo to not introduce any breaking change.
Also, we don't need a full PipelineCreateInfo for the viewports. They have their own RenderPass, their own MSAA (1x), ... .
We could add a desired secondary viewports format to ImGui_ImplVulkan_InitInfo (and eventually MSAA).

[ocornut]

Also, we don't need a full PipelineCreateInfo for the viewports. They have their own RenderPass, their own MSAA (1x), ... .
We could add a desired secondary viewports format to ImGui_ImplVulkan_InitInfo (and eventually MSAA).

(2) The assume the PipelineCreateInfo you added in the .cpp file to avoid using arguments, but I was thinking a PipelineCreateInfo dedicated to those 4 fields + ones potentially introduced by e.g. SuperRonan@5b9a62f

I'll finish the current thing for you to review first.

[SuperRonan]

Yes, the next step would be to add something like 5b9a62f.
I am not sure it is a good idea to use two instances of common PipelineCreateInfo struct in the InitInfo for the main window and secondary viewports. Some fields would one be used for one or the other usage, and I think it would confuse the user.

[ocornut]

Could you review the features/vk_create_main_pipeline branch and ensure it works with your setup?
https://github.com/ocornut/imgui/compare/docking...features/vk_create_main_pipeline?expand=1

This is based on docking. There are 6 commits:

• (1) 8014ab1 is essentially your commit
• (2) 54e46e9
• (3) e71f1d5
• (4) 9764c98 is diff between your commits on master vs docking.
• (5) 90fd35f
• (6) 0cf4bdf

docking won't build from before the 4th commits, but master can build with the first 3 cherry-picked.
Once we agree I will cherry-pick 1,2,3,5,6 into master, merge the whole into docking and cherry-pick 4.

Before merging I would like us to investigate the following "upcoming" changes:

• SuperRonan@5b9a62f
• Backends: Vulkan: Pipeline with dynamic rendering created for viewport should have its own rendering create info #8686

Based on the general intent, but it make sense in order to move forward to rework the ImGui_ImplVulkan_MainPipelineCreateInfo to be useful for both main and secondary viewports specs?

[SuperRonan]

LGTM.

One thing was missing: deep copy of the PipelineRenderingCreateInfo::pColorAttachmentFormats during ImGui_ImplVulkan_CreateMainPipeline(). I fixed it in a commit localy, but I don't have the permission to push it to features/vk_create_main_pipeline. Should I push it to a new branch on my fork a create a new PR for it?

And concerning the deep copy of the pColorAttachmentFormats, I think we could replace the manual memory management by an ImVector.

[SuperRonan]

Here is the commit on my fork branch: 2f10f9e

[SuperRonan]

I also added a fix: 8f62407
(fixing this error)

[ocornut]

I also added a fix: 8f62407 (fixing this error)

Err I don't think your linked change makes any difference - it's the same code - and i don't understand how it relates to the linke compile error?

[SuperRonan]

a PipelineRenderingCreateInfo must be accessed inside of #ifdef IMGUI_IMPL_VULKAN_HAS_DYNAMIC_RENDERING, else it does not exist. It was accessed outside of a #ifdef IMGUI_IMPL_VULKAN_HAS_DYNAMIC_RENDERING, hence the compilation error:

bd->Pipeline = ImGui_ImplVulkan_CreatePipeline(v->Device, v->Allocator, v->PipelineCache, v->RenderPass, v->MSAASamples, v->Subpass, v->UseDynamicRendering ? &info.PipelineRenderingCreateInfo : nullptr);

[ocornut]

My bad, fixing here.

[ocornut]

This should now be all merged in both branches! Thanks.

For the deep-copy I ended up doing this: 026d47c : using an ImVector<> and doing it in ImGui_ImplVulkan_CreateMainPipeline() so it's only done once. (EDIT might be an issue if calling multiple times... checking now)

[ocornut]

I'm closing this now, thanks!
But as mentioned in #8110 (comment) i think we may end up changing ImGui_ImplVulkan_MainPipelineCreateInfo:

I am not sure it is a good idea to use two instances of common PipelineCreateInfo struct in the InitInfo for the main window and secondary viewports. Some fields would one be used for one or the other usage, and I think it would confuse the user.

Yes but it's also trivial to comment which are used. We'll see how things go when we move toward those features.
(I don't mind breaking backend API as much as breaking main API because the fixes tends to be in 1 location.)

[ocornut]

I have pushed a couple of rework of the Vulkan backend related to the things discussed here.
It was much simpler to use a shared struct ImGui_ImplVulkan_PipelineInfo and now secondary viewports can be better customized.

• 01686c6 Backends: Vulkan: added a way to specify custom vertex/fragment shaders.
• 5fe9622 (Breaking) Backends: Vulkan: added VkImageUsageFlags image_usage parameter to ImGui_ImplVulkanH_CreateOrResizeWindow().
• cc0a669 Backends: Vulkan: minor internal renames/tweaks to reduce further patches.
• e312b99 (Breaking) Backends: Vulkan: renamed ImGui_ImplVulkan_MainPipelineCreateInfo to ImGui_ImplVulkan_PipelineInfo.
• 3470e61 (Breaking) Backends: Vulkan: moved fields in ImGui_ImplVulkan_InitInfo: RenderPass, Subpass, MSAASamples, PipelineRenderingCreateInfo.
• e4e3c2c Backends: Vulkan: amends for docking. Add PipelineInfoForViewports and SwapChainImageUsage.

Would you be able to update and confirm that everything works well for you?
(unfortunately it will probably affect your unpushed other changes)

[SuperRonan]

Thanks!
I will check that soon.

[SuperRonan]

@ocornut I found a few issues with multi-viewports. I will makes changes and submit a new PR to submit a the fix.

• Backends: Vulkan: added a way to specify custom vertex/fragment shaders.

Nice! In the end that's probably the best solution to fix the color space issues.