-
-
Notifications
You must be signed in to change notification settings - Fork 11.2k
Vulkan backend: use PipelineRenderingCreateInfo for dynamic rendering #7166
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
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:
|
I made the above suggestions as additional commits that can be reverted if not desired. |
This has been rebased with the following additional change: |
@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. |
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'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.
If the PR will affect it in this case I think I can manually apply corresponding change. |
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. |
Also note your PR as-is was broken and required 11d73f0 to work. |
Also amended with e0ba0d0 as CI failed with older VulkanSDK. |
@ocornut Ack on the required fixes. Sorry about that - good catches.
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:
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.
Yes, the confusion here was that |
Yes we don't need two copies, but your PR left an unused field. |
No description provided.