Skip to content
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

VuIkan: invalid Dynamic Rendering functions from the Instance-level loader #7554

Open
danylorudenko opened this issue May 4, 2024 · 3 comments

Comments

@danylorudenko
Copy link

danylorudenko commented May 4, 2024

Version/Branch of Dear ImGui:

Version 1.90.5, Branch: docking

Back-ends:

imgui_impl_vulkan.cpp + imgui_impl_win32.cpp

Compiler, OS:

Windows 10 + MSVC 2022

Full config/build information:

Dear ImGui 1.90.5 (19050)
--------------------------------
sizeof(size_t): 8, sizeof(ImDrawIdx): 2, sizeof(ImDrawVert): 20
define: __cplusplus=199711
define: _WIN32
define: _WIN64
define: _MSC_VER=1939
define: _MSVC_LANG=202002
define: IMGUI_HAS_VIEWPORT
define: IMGUI_HAS_DOCK
--------------------------------
io.BackendPlatformName: imgui_impl_win32
io.BackendRendererName: imgui_impl_vulkan
io.ConfigFlags: 0x00000443
 NavEnableKeyboard
 NavEnableGamepad
 DockingEnable
 ViewportsEnable
io.ConfigViewportsNoDecoration
io.ConfigInputTextCursorBlink
io.ConfigWindowsResizeFromEdges
io.ConfigMemoryCompactTimer = 60.0
io.BackendFlags: 0x00001C0E
 HasMouseCursors
 HasSetMousePos
 PlatformHasViewports
 HasMouseHoveredViewport
 RendererHasVtxOffset
 RendererHasViewports
--------------------------------
io.Fonts: 1 fonts, Flags: 0x00000000, TexSize: 512,64
io.DisplaySize: 1584.00,861.00
io.DisplayFramebufferScale: 1.00,1.00
--------------------------------
style.WindowPadding: 8.00,8.00
style.WindowBorderSize: 1.00
style.FramePadding: 4.00,3.00
style.FrameRounding: 0.00
style.FrameBorderSize: 0.00
style.ItemSpacing: 8.00,4.00
style.ItemInnerSpacing: 4.00,4.00

Details:

ImGuiImplVulkanFuncs_vkCmdBeginRenderingKHR and ImGuiImplVulkanFuncs_vkCmdEndRenderingKHR have invalid function pointers
Edit (see following comments): because they are requested via string with KHR postfix

Hi!
I'm currently in the process of integrating ImGui docking branch in my Vulkan renderer. In my app I link vulkan dynamically by loading function pointers via vkGetInstanceProcAddr and vkGetDeviceProcAddr

So I have defines that force ImGui to do almost the same thing:
#define IMGUI_IMPL_VULKAN_HAS_DYNAMIC_RENDERING
#define IMGUI_IMPL_VULKAN_NO_PROTOTYPES
#define VK_NO_PROTOTYPES

I provide function loader via ImGui_ImplVulkan_LoadFunctions:
image
The callback is called and all necessary functions seem to be loaded, no asserts triggered, etc.

For main viewport everything works great, the app handles surrounding render passes, command buffer is recorded by ImGui and then app does synchronization and submits the command buffer to the queue on it's own agenda.
image
Which just forwards call here
image

Though when I drag ImGui windows out of the main viewport, the Win32 window is created by ImGui with all associated resources (some of them I provide via callback, like VkSurface).
Drawing separate windows is delegated to ImGui_ImplVulkan_RenderWindow as you can see in the callstack of the following screenshot. Though I hit NULL function pointer directly under ImGuiImplVulkanFuncs_vkCmdBeginRenderingKHR (double checked for EndRendering, same thing)
image

I've managed to fix this issue by providing vulkan function pointers from my app to the vulkan_impl (red - new code, green - original):
image

The main difference between functions obtained via ImGui_ImplVulkan_LoadFunctions and my app function pointers is that I use pointers obtained from the device vkGetDeviceProcAddr while ImGui uses instance-level function vkGetInstanceProcAddr.

Another difference:

My thought is that Vulkan trampoline fails to dispatch instance-level functions correctly for dynamic rendering extension. Worth noting that in my app I allow ImGui to obtain Vulkan pointer after I initialize my app's function table. So that could be the reason why the loader is freaking out. But I'm not completely if it's the expected behavior.
(Read about vulkan loaders and trampolines here).

This could be hot-fixed on the side of application if I hack the callback for loading functions. But it's "meh".
image

Is there a particular reason why ImGui loads all functions on the Instance level?
Is it worth providing Device-level function loading?
Core functions that ImGui obtains from Instance seem to work just fine.

Screenshots/Video:

No response

Minimal, Complete and Verifiable Example code:

No response

@danylorudenko
Copy link
Author

danylorudenko commented May 4, 2024

Just as I pressed publish, another find:

While this can be a long discussion I decided to avoid modifying ImGui files and tried to settle with handling such cases via the callback. I give priority to loading device-level functions and if I fail, fallback to instance-level functions.
image

Spoiler: it didn't help. vkBeginRenderingKHR and vkEndRenderingKHR could not be found in device, so they're still requested from the instance.
image

But I found another difference in regards of obtaining vulkan functions:

ImGui uses KHR variant
image

While my ImportTable uses clean variant
image

So I returned to imgui_impl_vulkan.cpp and removed KHR postfix in the strings for Begin/End rendering and we're golden! Functions successfully obtained from the device
image
image

Note: For my app I create Vulkan 1.3 instance and have validation layers enabled for it

@danylorudenko
Copy link
Author

Because it can be very setup-specific, here's my Vulkan SDK version, driver and related hardware:

Vulkan SDK: 1.3.261.0
Nvidia GeForce RTX 3070
Driver version: 543.33

@ocornut ocornut changed the title Invalid Dynamic Rendering functions from the Instance-level loader VuIkan: invalid Dynamic Rendering functions from the Instance-level loader May 7, 2024
@ocornut
Copy link
Owner

ocornut commented May 7, 2024

Hello,

Thanks for the careful report! Dealing with fringe backend-related issues, and in particular Vulkan ones, has been too time consuming so for now I haven't really read this.
The (unfortunate :) goal is that the vulkan backend should ideally work for everyone in every condition.

I am open to PR with solution given than you carefully research the reason current things are in place this way (you can generally use git blame). The trail gives: 121072c, 7812e83, #5446, #5037, see discussions in those threads.

Thank you!

So I have defines that force ImGui to do almost the same thing:
#define IMGUI_IMPL_VULKAN_HAS_DYNAMIC_RENDERING

FYI you shouldn't need to define this, it's defined automatically when the Vulkan header version has it.

Is it worth providing Device-level function loading?

Unless I am mistaken, in your specific configuration IMGUI_IMPL_VULKAN_USE_LOADER will NOT be defined, therefore this code in imgui_impl_vulkan never runs:

ImGuiImplVulkanFuncs_vkCmdBeginRenderingKHR = reinterpret_cast<PFN_vkCmdBeginRenderingKHR>(vkGetInstanceProcAddr(info->Instance, "vkCmdBeginRenderingKHR"));
ImGuiImplVulkanFuncs_vkCmdEndRenderingKHR = reinterpret_cast<PFN_vkCmdEndRenderingKHR>(vkGetInstanceProcAddr(info->Instance, "vkCmdEndRenderingKHR"));

Therefore the only function loading code that runs is through your loader?
I am right? In that case it wouldn't be of any use if those two lines where complemented with the equivalent call to vkGetDeviceProcAddr() ?

So I returned to imgui_impl_vulkan.cpp and removed KHR postfix in the strings for Begin/End rendering and we're golden! Functions successfully obtained from the device

I noticed by looking at your screenshots that your version of the code is before b720c0f. This was meant to solve another issue but it interfere in same places of the code making this a little confusion.

    1. Could you update to latest?
    1. Then confirm things based on latest and help decide what we can do in the backend to fix this for you?

Thanks a lot!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants