ocornut
Vulkan: Re-create main window pipeline (standalone) with docking
Extension of #8110 to the docking branch (the merge is nearly straightforward, but not totally). To keep one PR = one feature, I will soon push another branch (and PR) to give the application the control over the viewports surface format (and some bug fixes I found) (much like #8061, but without the whole color correction stuff).
As per my comment in https://github.com/ocornut/imgui/pull/8085#issuecomment-2435710272
- Could you rebase/squash to remove fix up commits (aka squash the intermediary "replace some tabs" etc.). Multiple commits only if there is a reason so splitting the work in two steps.
- Then review diff from the base using your local git ui and ensure there's no unnecessary changes. e.g. Shifting the alignment of declaration in imgui_impl_vulkan.h is unnecessary.
- Then you can force-push into the PR branch.
- AFAIK if #8111 is a followup to #8110 it could be in the same PR as per my comment above, no need for #8110.
I use this process of reviewing and sculpting history all the time myself. Never committing random bunch of changes bundled together. When there are too many shallow changes needed (e.g. realignement, moving blocks, renaming something) for a larger feature which is it harder to review, I try to sculpt history to move the shallow change to a first commit, then the feature as a second commit, so both are individual easier to review than both together.
Sorry this is tedious but you'll quickly learn how to make neat PR, it'll be nicer both for you and for any of your future contribution to open source. As stated in my recent 10 years of dear imgui post, I don't have mental capacity to review bundled changes and Vulkan is particular difficult for me as I'm not a user. Thank you!
I squashed #8110 and #8111 down to one commit each. As for your comment that the two PR can be done in one PR: is that possible? We can only do one branch per PR. I agree #8111 is mostly redundant with #8110 since it is a merge of #8110 in docking, but also with a few changes to adapt the multi viewport code to the changes of #8110 (see the cpp arround line 1700/1770). And if I push only this PR, you won't be able to integrate it in master without merging the whole docking branch?
And if I push only this PR, you won't be able to integrate it in master without merging the whole docking branch?
If the PR has xx commits expected for master and then subsequent commits expected for docking, I can cherry-pick them individually. It's actually easier both for you and me in the end. If the xx commits expected for master then written over docking would create a conflict on master it's a little trickier, but happy to resolve minor/obvious conflict on cherry-pick. As per my comment in 8085: "I think it'll be easier both for you and me to open a single PR for docking (close the master one), but with commit history intentionally crafted so that some commits are expected to be picked in master (minor obvious conflicts I can fix), and some only in docking after picking the earlier ones. This way we discuss/update in a single location."
Please generally review my comments. Otherwise I'll catch up anyhow later, but I tend to get sidetracked too easily when I spot a mistake or unnecessary change in a PR.
As per my comment in 8085: _"I think it'll be easier both for you and me to open a single PR for docking (close the master one), but with commit history intentionally crafted so that some commits are expected to be picked in master (minor obvious conflicts I can fix), and some only in docking after picking the earlier ones. This way we discuss/update in a single location."
I did see that, but I still though it would be easier for you since the merge was not automatic, but if it is less of a issue for you than having two PR, I'll keep working on docking.
I also have some other changes (SuperRonan/imgui@5b9a62f2cf10669e9c24c0dc40f208f6353d3341) ready to be pushed (ability to control viewports format / present mode, but without color correction), so this might be considered a new feature. Should I integrate it to this PR, or make a second one (that would be built on top of this one)?
The same commit also fixes a bug in the vulkan impl:
Each viewport had its own VkRenderPass, but shared a common VkPipeline (which was created with the first viewport's VkRenderPass). It was not an issue as long as each viewport VkRenderPass was compatible with the first one used by the VkPipeline. But if the compatibility is lost (for example if the format of the render target changes), then we get some validation error and the GUI might not be rendered properly.
This issue did not necessarily arise on the current version docking since all viewports share the same format (or if we don't change the main viewport format with #8111), but it relies on an implicit/shaky assumption.
The docking version of the commit has this block missing from ImGui_ImplVulkan_Init():
{
bool create_pipeline = false;
const ImGui_ImplVulkan_PipelineRenderingCreateInfo * p_dynamic_rendering = nullptr;
if (v->RenderPass)
{
create_pipeline = true;
}
else
{
#ifdef IMGUI_IMPL_VULKAN_HAS_DYNAMIC_RENDERING
if (v->UseDynamicRendering && v->PipelineRenderingCreateInfo.sType == VK_STRUCTURE_TYPE_PIPELINE_RENDERING_CREATE_INFO_KHR)
{
p_dynamic_rendering = &v->PipelineRenderingCreateInfo;
create_pipeline = true;
}
#endif
}
if (create_pipeline)
{
ImGui_ImplVulkan_MainPipelineCreateInfo mp_info = {};
mp_info.RenderPass = v->RenderPass;
mp_info.Subpass = v->Subpass;
mp_info.MSAASamples = info->MSAASamples;
mp_info.pDynamicRendering = p_dynamic_rendering;
ImGui_ImplVulkan_ReCreateMainPipeline(mp_info);
}
}
Is that a mistake?
It was moved to ImGui_ImplVulkan_CreateDeviceObjects() as you suggested.
Oh yes indeed the block is also in ImGui_ImplVulkan_Init, merging error. You can remove it from ImGui_ImplVulkan_Init