Vulkan backend: use PipelineRenderingCreateInfo for dynamic rendering

[shawnhatori]

No description provided.

[shawnhatori]

Tested locally, solves the problem referenced in #6855.
This also addresses documentation concerns discussed in #7140.

@ocornut Regarding 6695006, what is your process for patching the docking branch? Separate PR?

[shawnhatori]

This is obviously a breaking change, but to a small number of users for the new dynamic rendering support.

I'm not sure how ImGui handles breaking changes but it would also be nice to:

• Move optional members PipelineCache and Subpass down to their own section for clarity. This would break code for anyone who initializes the struct relying on member order.
• Move render_pass into ImGui_ImplVulkan_InitInfo. This breaks all users. Since modern Vulkan is trending in the direction of using dynamic rendering, this may make the API a little cleaner and less confusing.

[shawnhatori]

I made the above suggestions as additional commits that can be reverted if not desired.

[shawnhatori]

This has been rebased with the following additional change:
I removed the call to RemoveTexture(). That call tries to free a descriptor on shutdown, which requires the caller to provide a descriptor pool with VK_DESCRIPTOR_POOL_CREATE_FREE_DESCRIPTOR_SET_BIT set. Doing otherwise is a validation error. I don't think this should happen. Since it's a caller-supplied pool, it should be the caller's responsibility to free. Removing that call removes a major restriction on the pool a user can provide. Since I free my provided pool anyway, I have tested this new change and it runs and shuts down cleanly with no leaks.

[shawnhatori]

@ocornut Any feedback on this change? I'm not sure what the project's approach is to breaking changes, but that interface cleanup can be reverted if need be.

[ocornut]

Breaking changes to specific backends like Vulkan and WebGPU are treated in a little more relaxed way than breaking changes to main lib or simpler backends. It is understood that users of those platforms are development on moving ground anyway, and can be more reactive to change.

I removed the call to RemoveTexture().

I'm very confused by this. There are 5 commits in this PR (with 2 including unrelated addition/removal) but this change seems completely unrelated to the discussion? Please open separate PR as every change is subject to discussion and most often not as easy as meet the eyes, so separately the issue is welcome.

what is your process for patching the docking branch? Separate PR?

If the PR will affect it in this case I think I can manually apply corresponding change.

[ocornut]

I merged the main proposed change as 8901931.

I didn't merge the RenderPass change because the same change would require fixing examples. Also you removed use of Subpass but left the field. And the commit has an unrelated block of changes as well. I am open to make that breaking change.

I didn't merge the texture change I would prefer a separate PR for it.

[ocornut]

Also note your PR as-is was broken and required 11d73f0 to work.

[ocornut]

Also amended with e0ba0d0 as CI failed with older VulkanSDK.

[shawnhatori]

@ocornut Ack on the required fixes. Sorry about that - good catches.

I'm very confused by this. There are 5 commits in this PR (with 2 including unrelated addition/removal) but this change seems completely unrelated to the discussion? Please open separate PR as every change is subject to discussion and most often not as easy as meet the eyes, so separately the issue is welcome.

A number of things were discussed across two threads, which admittedly is a bit confusing. Adding to the confusion (for myself at least) is that this PR is two months old. So I will try to centralize everything here:

1. Backends: Vulkan: Modifiable depth and stencil formats #6855, which is about providing the VkPipelineRenderingCreateInfoKHR to prevent the validation errors. This is the PR you submitted your follow-up comment to and it has been addressed with the commit that has been merged.
2. Vulkan Backend: validation error/exception on ImGui_ImplVulkan_AddTexture() call to vkUpdateDescriptorSets() #7140 (comment), where I raise:

• Clarifications in the Vulkan backend documentation for how to use it and which members are optional (also merged)
• VkRenderPass being a separate parameter
• The RemoveTexture() issue

These all got rolled into one since I was living off of my branch and fixed any related validation errors I came across. Agreed that these last two should be part of separate PRs. I will do that.

Also you removed use of Subpass but left the field.

Yes, the confusion here was that Subpass is a member of both ImGui_ImplVulkan_InitInfo and ImGui_ImplVulkan_Data, where the Data member is just a copy of the Info member. There may be a good reason for this that I'm not aware of, in which case I can leave it as-is. Note that RenderPass will have the same problem if it is moved into the Info struct (as proposed, instead of taking it in as a parameter), as it is also in the Data struct. It seems to me that these members can stay in Info and be removed from Data, since they can always be accessed from the Info struct within imgui_impl_vulkan.cpp. But I will wait for your feedback on that.

[ocornut]

Yes, the confusion here was that Subpass is a member of both ImGui_ImplVulkan_InitInfo and ImGui_ImplVulkan_Data, where the Data member is just a copy of the Info member. There may be a good reason for this that I'm not aware of, in which case I can leave it as-is. Note that RenderPass will have the same problem if it is moved into the Info struct (as proposed, instead of taking it in as a parameter), as it is also in the Data struct. It seems to me that these members can stay in Info and be removed from Data, since they can always be accessed from the Info struct within imgui_impl_vulkan.cpp. But I will wait for your feedback on that.

Yes we don't need two copies, but your PR left an unused field.
And the RenderPass breaking would need examples's main.cpp to be updated accordingly.

[shawnhatori]

Okay the new PRs are up that should address the concerns raised above:
#7307
#7308

Since the change this PR was primarily about has been merged and we now have the other two, feel free to close this one.