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

Management of Vulkan Images/Frames index in Docking branch. #7834

Open
NostraMagister opened this issue Jul 29, 2024 · 0 comments
Open

Management of Vulkan Images/Frames index in Docking branch. #7834

NostraMagister opened this issue Jul 29, 2024 · 0 comments

Comments

@NostraMagister
Copy link

NostraMagister commented Jul 29, 2024

Version/Branch of Dear ImGui:

Version 1.90.9 Docking branch

Back-ends:

imgui_impl_vulkan.cpp

Compiler, OS:

All

Full config/build information:

All

Details:

Dear ImGui's imgui_impl_vulkan backend, after building/rebuilding the swapchain retrieves the number of images in the swapchain and rebuilds the frames/semaphore set arrays. This determines the count of frames.

The ImGui_ImplVulkan_RenderWindow(ImGuiViewport* viewport, void*) function uses the vkAcquireNextImageKHR() function to find out what image/frame is next to use, within the boundaries of above mentioned frame count.

Yet, just before the vkAcquireNextImageKHR() the following line retrieves the frame at the CURRENT frame index:

ImGui_ImplVulkanH_Frame* fd = &wd->Frames[wd->FrameIndex];

This 'fd' variable content will NEVER be used because it is overwritten a few lines below vkAcquireNextImageKHR() by

fd = &wd->Frames[wd->FrameIndex];

The frame index is at that moment the one that vkAcquireNextImageKHR() retrieved. This overwrite is unconditional and CANNOT be avoided (which is OK because the index retrieved with vkAcquireNextImageKHR() is the one needed anyways).

To avoid confusion, as a first proposition I suggest to initialize fd as

ImGui_ImplVulkanH_Frame* fd = nullptr;

Yet, that brings another thing to light. The FrameIndex is only touched in one single other place and that is in
ImGui_ImplVulkan_SwapBuffers(ImGuiViewport* viewport, void*) where it is first retrieved to find out what frame to present with:

uint32_t present_index = wd->FrameIndex;

and then, at the end of the function, it is increased accompanied by a comment with:

wd->FrameIndex = (wd->FrameIndex + 1) % wd->ImageCount; // This is for the next vkWaitForFences()

We knows from above that what is in the comment about vkWaitForFences will never happen.

As a second proposition I suggest to remove this line as it is a defacto 'no-op'.
I checked, the frame index is touched nowhere else in the .cpp file.

I make this proposition because the Dear ImGui code often refers the developers to the backend code as examples of how to write ones own backend. If the above reasoning is correct and the suggestions adopted this will for sure take away one of the possible confusions.

NOTE: Although the above takes place in the same two functions as mentioned in #7825 and #7831, this is UNRELATED and
changes nothing to #7825 where what is explained still affects the semaphore index in case the above
suggestions would be accepted.

Screenshots/Video:

No response

Minimal, Complete and Verifiable Example code:

No response

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