Backends: Vulkan: Re-organize custom shaders and added control over secondary viewports #8980
Conversation
…econdary viewports - Added `ImGui_ImplVulkan_CustomShadersInfo` - Replaced `ImGui_ImplVulkan_InitInfo::SecondaryViewportsInfo` by struct `ImGui_ImplVulkan_SecondaryViewportsInfo` - Custom shaders: directly feed a VkShaderModule rather than a VkShaderModuleCreateInfo - Fixed some inconsistencies: All viewports share the same format and Pipeline, PipelineLayout, ... - Added `ImGui_ImplVulkan_SetSecondaryViewportsOptions()` to control secondary viewports without reinitializing the backend (like `ImGui_ImplVulkan_CreateMainPipeline()`)
|
You seem to be overlooking that there are vastly too many changes in there. It's impossible to review and merge a PR that modify hundreds of lines of the most complex rendering backend, in a way that arguably further increase complexity. There are dozens of design decisions taken, each of them would required a careful separate reviewed. I am sorry but I cannot even begin to consider looking at this. You'd need at minimum to split your proposal in smaller chunks, providing reasoning and test code. |
|
I'll give you an example of required divide of conquer:
You will need to justify and explain every decision, e.g.
Where/how is this giving color correction options? Don't we already support custom shaders?
Which, what? Why those aren't corrected in a separate commit?
Why do you think so? This seems to contradict the comment at #8585 (comment) and the intent of the original commit.
What's the intended benefit? If the only benefit is to always reinitializing backend it doesn't seems like a good tradeoff. You are adding complexity to this code that will be maintained forever, that is fragile and which I am already struggling to maintain, for a fringe use cases which you can workaround on your side? What's the issue with reinitializing the backend? Please go through changes and split/justify them one by one. You don't need to do everything at once. Pick one or two things and aim for them to be merged. Try to make things as simple as possible, state your design choices and reasoning. |
|
I am trying to split in smaller changes. I will keep this PR to justify the big picture of why these changes are useful. |
|
You can split into separate commits as part of the same part. Use git interactive rebase to split the commits, and force-push into branch PR. This way we have the separate commit but it's a single PR. |
|
So one big PR with multiple commits is good for you? (I remember you wanted it all to be squashed last year) |
It needs to be squashed when the commits are random updates, we don't need the intermediate history especially if the commits are spammy. It needs to be split when the complexity/noise is high. I can always decide to cherry-pick individual commits from the PR, and the PR becomes smaller. If the features are too orthogonal multiple PR may be better but feel free to keep it a same PR in this case it seems easier for both you and me. e.g. when I make big changes internally, if there is a "noisy" side to the change (e.g. member rename, indentation change) i try to split the noise into a one commit, so next commit only has the useful stuff. e.g. in the case #8985 i'd be willing to merge this commit but only if I have a good understanding that we are going to follow on the remaining changes where this needs to be a separate function. If for some reasons we end up ditching the reason for calling |
|
I am breaking this PR in 3 (sequencial) smaller PRs. Keeping this one open for now as a big picture view. |
2 participants
This PR improves the vulkan impl:
This PR should not add any breaking change for normal users, only "Advanced" parameters have been changed.
Custom Shaders
I replaced the init's
CustomShader(Vert|Frag)CreateInfowith a completeImGui_ImplVulkan_CustomShadersInfo. I think it makes more sense for the user to provide pre-packagedVkShaderModulerather than aVkShaderModuleCreateInfothat will be created by the vulkan impl. The new struct also provides more shader customization information, that can be useful for the user (e.g. managing color correction without needing a complete shader re-compilation).I think these are reasonable additions for the customization possibilities is opens.
User control over secondary viewports
ImGui_ImplVulkan_SetSecondaryViewportsOptions()has been added to dynamically control secondary viewports options (format, present mode, custom shader, ...) much likeImGui_ImplVulkan_CreateMainPipeline()introduced in #8110. The main difference is that the action of re-creating viewports render objects (RenderPass, Pipeline, ...) is performed later when creating or rendering a viewport.Secondary viewports custom push constant data can be passed to
ImGui::RenderPlatformWindowsDefault(nullptr, pc_data).Secondary viewports format
So far, each viewport was selecting its own surface format and creates its own RenderPass, but every viewports were sharing the same Pipeline (created with the first viewport's RenderPass). It works because every viewports ends up selecting the same format, but it is not ideal.
With this PR, every viewport now explicitely uses the same common format, and Pipeline. The RenderPass is still per viewport because it depends on the clear parameter (different
VkRenderPassare compatible with different LoadOp/StoreOp, but not with different formats).Check this PR in action
ImGui_ImplVulkan_CustomShadersInfoImGui_ImplVulkan_InitInfo::SecondaryViewportsInfoby structImGui_ImplVulkan_SecondaryViewportsInfoImGui_ImplVulkan_CustomShadersInfoImGui_ImplVulkan_SetSecondaryViewportsOptions()to control secondary viewports without reinitializing the backend (likeImGui_ImplVulkan_CreateMainPipeline())