#5037 for docking branch: Support for dynamic_rendering

[spnda]

Version of #5037 for docking branch as requested by https://github.com/ocornut/imgui/pull/5037#issuecomment-1152482524. Sorry for the delay, completely forgot about my original PR.

As we now load function pointers manually, it's possible for vkCmdBeginRenderingKHR and vkCmdEndRenderingKHR to be nullptr. Is it fine to just cause a segfault in ImGui_ImplVulkan_RenderWindow when UseDynamicRendering is enabled but the extension wasn't enabled on the device and the function pointers therefore possibly be nullptr?

[spnda]

Oh and I would also like to extend my comment about the function pointers: As they're from an extension and not part of core, the way it's currently setup will result in linker issues if VK_NO_PROTOTYPES is not defined. I couldn't find any manual function pointer loading in regards to the other functions from the KHR_swapchain and KHR_surface extensions. So what is the recommended way to do this in the already present backend, if it's not something totally new?

[ocornut]

Hello,

Is it fine to just cause a segfault in ImGui_ImplVulkan_RenderWindow when UseDynamicRendering is enabled but the extension wasn't enabled on the device and the function pointers therefore possibly be nullptr?

Best to assert if possible.

the way it's currently setup will result in linker issues if VK_NO_PROTOTYPES is not defined.

I am not sure I follow, are you suggesting the PR things won't link by default, making this unmergeable as-is ?

So what is the recommended way to do this in the already present backend, if it's not something totally new?

I have no idea, I'm not so familiar with Vulkan ecosystem.

[spnda]

I am not sure I follow, are you suggesting the PR things won't link by default, making this unmergeable as-is ?

I have no idea, I'm not so familiar with Vulkan ecosystem.

With Vulkan one has 2 options:

• You can link against the Vulkan loader (vulkan-1.lib on Windows) which contains method stubs for every core Vulkan function and some essential functions from extensions like VK_KHR_swapchain and VK_KHR_surface. As dynamic rendering is an extension (before 1.3), we will require the user to have a Vulkan lib that is up to date if we link against the library. This only happens when VK_NO_PROTOTYPES is not defined.
• You cannot link against the function prototypes with VK_NO_PROTOTYPES defined, forcing you to have to load the function pointers at runtime. In my opinion, this is the best way to do things, as it decreases link time and overall reduces runtime speeds because of less indirection. However, this only happens when you define IMGUI_IMPL_VULKAN_NO_PROTOTYPES when building ImGui which a lot of users probably don't do.

The PR works in its current state, assuming that the user has a fairly recent version of the Vulkan loader to link against. With older versions, the user would need to define IMGUI_IMPL_VULKAN_NO_PROTOTYPES so that ImGui loads the function pointers at runtime. Otherwise, there will be link failures with vkCmdBeginRenderingKHR and vkCmdEndRenderingKHR with older versions of the Vulkan loader lib. This happens no matter if the user chooses to use dynamic rendering or not.

I hope what I said clarifies how this works. So, we can either leave this as is and just tell users to upgrade to a new Vulkan SDK that ships with the most up to date Vulkan loader, or we have to choose another solution. My suggestion would be to have a struct which the user has to pass to the Vulkan backend which contains function pointers to all the function the backend will have to use. This avoids the problem completely, as the backend is not responsible for anything related to the function loading. Or we just go the simplest route and unconditionally load the two offending functions at runtime, no matter if the rest is also loaded at runtime or statically linked. However, I feel like such a change is out of the scope of this PR.

[Joshua-Ashton]

Any movement on this? Would be nice to have.

[cmarcelo]

@spnda @ocornut

Suggestion to move forward with this, based on the previous comments:

• For this PR, we always load vkCmdBeginRenderingKHR and vkCmdEndRenderingKHR at runtime as Sean suggested. Because this will be used with and without prototypes, we'll have to store them in different names to avoid conflict.

• For another PR, consider changing so the backend implementation to always load functions, i.e. always compiled with VK_NO_PROTOTYPES. If the user doesn't set the loader function, we use the default one from the loader (we can provide our own prototype for what's needed here). This would allow us to get rid of the IMGUI_IMPL_VULKAN_NO_PROTOTYPES. At this point we can use the regular names for the two functions of this PR. The reasoning here is that always loading is not significantly worse, and possibly better (because skips the trampoline).

If @spnda don't plan to further work on this, I'm happy to update the PR (or make another one cherry-picking these changes).

[spnda]

@cmarcelo I am still invested and effectively still waiting on a response from @ocornut. If I don't hear back within a few days I will just load the pointers at runtime no matter what, making this work 100%. So any users can checkout my fork to use these features if desired. And I like your idea of a separate PR to always use dynamic dispatch, so we (or you) could look into that.

[ocornut]

Hello, Your October post seemingly pointed at no solution so I wasn't sure how to answer.

• I'm absolutely fine moving toward a backend that always load functions if this the first step to better solution. I'm happy to first merge a change doing that. I didn't know it was possible without user providing a loader function, but your sentence "If the user doesn't set the loader function, we use the default one from the loader (we can provide our own prototype for what's needed here)." suggest so.
• But I'm not entirely sure how to implement that considering VK_NO_PROTOTYPES would leak into imgui_impl_vulkan.h and potentially affect user application which may want to use regular prototypes. As a test-bed the examples's main.cpp file shouldn't be affected and should be able to use the stubs if they want to.
• Ideally: we would need to cater for unity/jumbo build with the possibility of user including imgui_impl_vulkan.cpp in the middle of their code. Which may involve using unique function names, aka using the IMGUI_VULKAN_FUNC_MAP macro to generate uniquely named symbols + redirecting calls in imgui_impl_vulkan.cpp to those names + undoing the redirect at the end of the file. This shouldn't be too difficult.

I would suggest making the always-load-function-pointer in 1 commit, then changes related to dynamic_rendering in a second commit, and force-push the result into this branch. I will cherry-pick the first one into both branches before merging second one.

Optionally/ideally we would need for reference a third commit that demonstrate using dynamic-rendering in one of the examples's main.cpp. I won't merge this one but I would need it as reference and for testing.

[cmarcelo]

@spnda Good to hear you are still invested, I'll focus on the loading part then.

I said earlier "This would allow us to get rid of the IMGUI_IMPL_VULKAN_NO_PROTOTYPES." but looking deeper here, we can't, we need it to control whether we can access the vkGetInstanceProcAddr() "directly" (i.e. expect the linker to plug it for us). We could avoid it if we manually dlsym (or Windows equivalent).

macro to generate uniquely named symbols + redirecting calls in imgui_impl_vulkan.cpp to those names + undoing the redirect at the end of the file

@ocornut The undoing part here is the tricky one. The default solution would be #define all symbols we use to the unique names and later #undef them, but in both cases we can't use the FUNC_MAP macro to achieve that (can't define/undef inside a macro itself) -- would have to provide the defines on top and undefs on bottom of the file. Did you had something more specific in mind for the undoing?

We could side-step both this and the "don't leak VK_NO_PROTOTYPES" by storing the functions we care about in a global static, and access them via such global or maybe even a macro (that we can undef later). So something like g_vulkan.vkCmd...(...), g_vk.Cmd...(...), API(vkCmd...)(...), etc. Nothing of this would be visible from the user of the backend.cpp code. Would that be an acceptable option?

[cmarcelo]

Update: I've implemented the API() approach and didn't liked it, so I've tried something different: put the whole implementation inside a namespace. The function pointers can have the regular names, and the backend functions will use those instead of the ones in global namespace. This works, the downside is having to "bridge" the calls to inside the namespace, e.g.

// In the imgui_impl_vulkan.cpp file
bool                 ImGui_ImplVulkan_Init(ImGui_ImplVulkan_InitInfo* info, VkRenderPass render_pass) { return imgui_impl_vulkan::ImGui_ImplVulkan_Init(info, render_pass); }

Here are the two attempts so you can compare:

• API() macro: https://github.com/cmarcelo/imgui/commits/vulkan-backend-always-load-function-pointers-access-with-api-macro
• Namespace: https://github.com/cmarcelo/imgui/commits/vulkan-backend-always-load-function-pointers-access-with-namespace

I've only ported GLFW example for this comparison. Why the example changed? I needed all the "entrypoint" functions to pass the Instance, so we can load the pointers. Since those are "internal" it won't cause a break.

Given all the options I considered in this and previous comment, my favorite solution so far is the namespace one.

[ocornut]

If the initial suggestion of just loading those 2 functions with a specific name is easier we can consider it.

I would also consider requiring users to update VulkanSDK if this simplifies our problem meaningfully.

[cmarcelo]

If the initial suggestion of just loading those 2 functions with a specific name is easier we can consider it.

I expect it to be easier because we can store the pointers in a struct and just live with that for the two calls -- no worries about make it extremely convenient to access. My understanding is @spnda will update this PR to achieve that.

[spnda]

I would suggest making the always-load-function-pointer in 1 commit, then changes related to dynamic_rendering in a second commit, and force-push the result into this branch. I will cherry-pick the first one into both branches before merging second one.

I thought that within this PR I will merely load the two new function pointers to make this work. Then @cmarcelo will "clean it up" later by loading loading everything or making it more dynamic to fix issues like these.

Though I think I would currently just change the original commit to reflect these changes.

I expect it to be easier because we can store the pointers in a struct and just live with that for the two calls -- no worries about make it extremely convenient to access.

My mockup currently just adds imgui_vkCmdBeginRenderingKHR and imgui_vkCmdEndRenderingKHR as global pointers for that to work with and without prototypes. The following commit will fix all of the issues by always loading those two functions, regardless of VK_NO_PROTOTYPES, meaning that this PR should be good to go.

[cmarcelo]

@spnda I've tried to run the code with the examples I had and found some more issues. I've posted the suggestions above and a few fixes to the branch https://github.com/cmarcelo/imgui/commits/dynamic-rendering could you take a look?

@ocornut I've also made two extra "DO NOT MERGE" patches on top that change Vulkan GLFW example, just for testing. The docking behavior works (moving an imgui window OUT to create a new native window, and then back again) in both.

• Use VK_KHR_dynamic_rendering as an extension (you see the example code needs to load the begin/end functions by itself).
• Use VK_KHR_dynamic_rendering requiring Vulkan 1.3, and using the symbols directly in the example app.

Do you think is worth keeping at least one of these variants as a separate example of its own in the repo?

[ocornut]

Do you think is worth keeping at least one of these variants as a separate example of its own in the repo?

Thanks! If anything it might be a #define in the same example. My gut-feeling is that we won't merge now but I'll keep a copy of this into a private branch for now.

Let me know when you feel there's something for me to do. Quite overwhelmed with this (most things Vulkan related tbh) so better when it's neat and bite-sized.

Again, I'm not entirely opposed to requiring SDK 1.3 is that's helping the code. I am assuming there wouldn't be any strong incentive for people to NOT update? Though I guess it is possible some platforms (e.g. consoles) may not support API 1.3.

[spnda]

@spnda I've tried to run the code with the examples I had and found some more issues. I've posted the suggestions above and a few fixes to the branch https://github.com/cmarcelo/imgui/commits/dynamic-rendering could you take a look?

Yeah most of these I already covered before reading that message and seeing you committed the stuff. I've just locally cherry picked your commit and squashed them together and also changed some stuff. There was a single issue in your commit on line 1722 and 1775 with the comma (yeah I know it compiles but this is not what the comma operator is useful for):

            VkImageMemoryBarrier barrier = {};
            barrier.sType = VK_STRUCTURE_TYPE_IMAGE_MEMORY_BARRIER,
                    barrier.dstAccessMask = VK_ACCESS_COLOR_ATTACHMENT_WRITE_BIT;
            barrier.oldLayout = VK_IMAGE_LAYOUT_UNDEFINED;

@ocornut I've also made two extra "DO NOT MERGE" patches on top that change Vulkan GLFW example, just for testing. The docking behavior works (moving an imgui window OUT to create a new native window, and then back again) in both.

• Use VK_KHR_dynamic_rendering as an extension (you see the example code needs to load the begin/end functions by itself).
• Use VK_KHR_dynamic_rendering requiring Vulkan 1.3, and using the symbols directly in the example app.

Do you think is worth keeping at least one of these variants as a separate example of its own in the repo?

I also have essentially the same thing locally, but yeah I would personally also vouch for a separate example that uses the extension or 1.3, as you can have the same code use both because they are identical besides function/struct naming. However, the example would need a bit more code than what you currently have because of extension dependencies on other extensions or Vulkan versions.

And if @cmarcelo won't find another tiny detail in the implementation to complain about (thanks though 😄) I am always on the side of 'this is good now'.

[cmarcelo]

@spnda @ocornut Here's an idea: would you prefer this to be a compile time option instead? Similar to what's done for NO_PROTOTYPES. Removes one "variable" of the whole equation, and might be less overwhelming to integrate.

This contains the example code updated to consider the same variable.

https://github.com/cmarcelo/imgui/commits/dynamic-rendering-compile-time-option

What do you think?

[spnda]

@cmarcelo Definitely not. This restricts the feature to being enabled or disabled at runtime and therefore wouldn't work on multiple different machines where one might not support dynamic rendering for whatever reason. I would vastly prefer the previous runtime flag to give the possibility to enable/disable the feature based on device properties. Of course, going forward this could be done in a few years when most devices will support 1.3 or the extension but I don't see it really being viable currently to be really honest with you.

[cmarcelo]

@cmarcelo Definitely not. This restricts the feature to being enabled or disabled at runtime and therefore wouldn't work on multiple different machines where one might not support dynamic rendering for whatever reason. I would vastly prefer the previous runtime flag to give the possibility to enable/disable the feature based on device properties. Of course, going forward this could be done in a few years when most devices will support 1.3 or the extension but I don't see it really being viable currently to be really honest with you.

This is a good point. I agree the runtime choice here makes sense.

I'll test your latest version of the PR later today.

[cmarcelo]

If anything it might be a #define in the same example.

I've updated the example in https://github.com/cmarcelo/imgui/commits/dynamic-rendering to use a #define to select the Dynamic Rendering. I think it is worth being included in this PR.

Again, I'm not entirely opposed to requiring SDK 1.3 is that's helping the code. I am assuming there wouldn't be any strong incentive for people to NOT update? Though I guess it is possible some platforms (e.g. consoles) may not support API 1.3.

Yeah, some platforms are still on older versions of Vulkan.

And if @cmarcelo won't find another tiny detail in the implementation to complain about (thanks though smile) I am always on the side of 'this is good now'.

PR looks good to me. :tada:

[ocornut]

Hello, Looking now. The branches are all based on a quite old version but I should be able to recover that. It seems a bit confusing at first sight where to look, I am assuming spdna/docking_dynamic_rendering for PR and top commit of cmarcelo/dynamic-rendering for the main change?

[ocornut]

Mostly all the extension handling system in examples has changed/improved quite a bit, so if I manually reapply one of them I want to make sure I use the right one.

[spnda]

The branches are all based on a quite old version but I should be able to recover that.

If needed I can also do that to assure everything is rebased correctly.

It seems a bit confusing at first sight where to look, I am assuming spdna/docking_dynamic_rendering for PR and top commit of cmarcelo/dynamic-rendering for the main change?

No. If I remember correctly I cherry-picked everything relevant from cmarcelo's fork (and then squashed) for this PR. They have other changes but those would be part of a different PR I think. So currently only my fork (this branch) is relevant.

[ocornut]

If needed I can also do that to assure everything is rebased correctly. [...] No. If I remember correctly I cherry-picked everything relevant from cmarcelo's fork for this PR. They have other changes but those would be part of a different PR I think. So currently only my fork (this branch) is relevant.

Managed to merge the main.cpp bits. What I meant is that I won't push the main.cpp changes yet/soon but I need a version of it it to test locally.

Should I credit you both in the changelog for this work?

I can get it to work, but it worries me bit that if I checkout the main.cpp changes cmarcelo/dynamic_rendering I get validation errors as it doesn't set UseDynamicRendering=true at all, suggesting that the tree as pushed was not tested properly.

[spnda]

Should I credit you both in the changelog for this work?

For the docking branch definitely, yes. I already added the Co-authored-by label to my commit after squashing.

I can get it to work, but it worries me bit that if I checkout the main.cpp changes cmarcelo/dynamic_rendering I get validation errors as it doesn't set UseDynamicRendering=true at all, suggesting that the tree as pushed was not tested properly.

Don't know about that fork exactly but would seem correct to assume that that's not valid, yes. I think that might've happened because cmarcelo pushed an experimental change earlier that enabled the functionality of UseDynamicRendering statically through a macro, and was then wrongly rebased.

[cmarcelo]

Sorry for the confusion here. I'm fixing up the example and double checking works in both cases.

[ocornut]

Thank you, I think I have everything I need. I will need to perform a little bit of mumbo-jumbo to first commit the master version then merge into docking and add remainder stuff, but should be good.

Do you see an issue with me rewording those:

#if defined(VK_VERSION_1_3) || defined(VK_KHR_dynamic_rendering)

Into e.g.

#if defined(VK_VERSION_1_3) || defined(VK_KHR_dynamic_rendering)
#define IMGUI_IMPL_VULKAN_HAS_DYNAMIC_RENDERING
#endif

And then elsewhere in file use that single ifdef ?

I'll try to finish this tomorrow. Thanks again!

[spnda]

No, that looks fine.

[cmarcelo]

Updated my branch in case it still helps.

[ocornut]

Pushed 7812e83 + small amends 121072c + docking amends ac85738

Than you!

[peter1745]

Maybe it should be noted that this doesn't work with viewports? Currently it will cause a crash if you try to move an imgui window outside of the main window