Logic error in ImGui_ImplVulkan_RenderWindow() function.

[NostraMagister]

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:

Partial Code:

    {
            err = vkAcquireNextImageKHR(v->Device, wd->Swapchain, UINT64_MAX, fsd->ImageAcquiredSemaphore, VK_NULL_HANDLE, &wd->FrameIndex);
            if (err == VK_ERROR_OUT_OF_DATE_KHR)
            {
                // Since we are not going to swap this frame anyway, it's ok that recreation happens on next frame.
                vd->SwapChainNeedRebuild = true;
                return;
            }
            check_vk_result(err);
            fd = &wd->Frames[wd->FrameIndex];
        }

Comment: In the code above the vkAcquireNextImageKHR() can ALSO return VK_SUBOPTIMAL_KHR. It cannot return VK_TIMEOUT or VK_NOT_READY because the 'timeout' argument has been set to UINT64_MAX.
In the code above the VK_SUBOPTIMAL_KHR is correctly treated (unless see ATTENTION 2 below), as if VK_SUCCESS was returned, however the Vulkan specification say this:

VK_SUBOPTIMAL_KHR is returned if an image became available, and the swapchain no longer matches the surface properties exactly, but can still be used to present to the surface successfully.

  NOTE
  This may happen, for example, if the platform surface has been resized but the platform is able to scale 
  the presented images to the new size to produce valid surface updates. It is up to the application to 
  decide whether it prefers to continue using the current swapchain indefinitely or temporarily in this 
  state, or to re-create the swapchain to better match the platform surface properties.

Source: [https://registry.khronos.org/vulkan/specs/1.3-extensions/html/vkspec.html#swapchain-acquire-forward-progress]
Therefor, IMO, an extra line afther check_vk_result(err) is needed if(err == VK_SUBOPTIMAL_KHR) vd->SwapChainNeedRebuild = true;

        ATTENTION 1: Considering the above and #7825 the current code here lets the VK_SUBOPTIMAL_KHR situation
                 exist (no rebuild flag set) and hence VK_SUBOPTIMAL_KHR will be returned in the code of #7825, 
                 resulting in the indices problem described in that ticket.

    ATTENTION 2: The check_vk_result(err) is a black spot because it isn't part of the backend but executes 
                 code set as a function address. If this code returns if err==VK_SUCCESS and otherwise performs 
             the error procedure, then VK_SUBOPTIMAL_KHR will be treated as an error (which it isn't). 
             If this code only logs something its harmless, but if it calls exit() at some point well then...

        ATTENTION 3: If it is an EXPLICIT backend design choice to not rebuild the swapchain on sub-optimal, then maybe 
                     a flag should be provided to allow for a choice. When very large buffers remain in existence and are only 
                     partially used it consumes (keeps allocated) a lot of memory for nothing.

Screenshots/Video:

No response

Minimal, Complete and Verifiable Example code:

No response

[ocornut]

Hello, Thanks for reporting. Please move this to #7825. Ideally you'd need to git blame and track the various issues/discussion which led to this code to ensure we are not making mistakes.

[NostraMagister]

@ocornut, this is a completely separated problem from what I reported with #7825. I mentioned my #7825 here because this one can create the conditions needed to make the indices problem of #7825 to occur. They are in the same .cpp but its different functions.

There will probably be others and I will report them as in my attempt to go to the bottom of my #7602 ticket I decided to write a Vulkan/SDL3 backend from scratch (because as commented in #7602 it is a strange problem), and hence I walk the complete Dear ImGui Vulkan/SDL3 backends because i want to understand every single line of it. It is a quite simple and straight forward job but the EuroCup, Tour de France and the Olympics delay me :)

If it is OK with you I would like to keep this #7831 separated from #7825, because IMO it is a separate issue. I have found several other things but I'll report on them when I am more certain of what I think I found.

[ocornut]

Understood.

Could you clarify in which setup you managed to create this situation where vkAcquireNextImageKHR() returns VK_SUBOPTIMAL_KHR ? I would need to create myself a test-bed where I can reproduce this.

Also note the FrameRender() calls used by main viewports in example_glfw_vulkan/main.cpp and example_sdl2_vulkan/main.cpp (commit 6e4770ea5, #3881).

On "Attention 2: The check_vk_result(err) is a black spot" I think we should avoid calling check_vk_result() on VK_SUBOPTIMAL_KHR or any result that we are handling.

[NostraMagister]

I cannot provide an exact situation because my backend for the docking branch is a 'chantier ouvert' since it isn't completely finished and can't run yet. But here is how you could proceed.

Create one of the situations below using the Dear ImGui standard SDL3/Vulkan backends (I didn't check the others because I only do those two) that in all cases need to be prepared for the VK_SUBOPTIMAL_KHR return value even if currently only Harry Potter could provoke one.

A VK_SUBOPTIMAL_KHR can come in one of the following situations (you could add a log to see it happen):

Surface Size Change The surface size has changed but is still compatible with the swapchain's image sizes. For example, the window may have been resized, but the swapchain images can still be presented without issues, even though they may not match the new size optimally.

Surface Transformation Change The transform applied to the surface may have changed. For instance, the user may have rotated their display.

Presentation Mode Change The presentation mode could be suboptimal due to changes in the display configuration, such as resolution changes, refresh rate changes or other alterations in the display settings. For instance, the user could change the display resolution (a smaller one), resulting in buffers that can still accommodate for the rendered image size but are much to large (hence sub-optimal). A higher refresh rate could increase the minimum images for the swapchain, etc.

Multi-Monitor Setup Changes Changes in multi-monitor configurations, like moving a windowed application from one monitor to another :), which may have different refresh rates, resolutions, or other properties. This last one will be be important when you add monitor selection as I saw is in your plans.

[NostraMagister]

Additional info: In the ImGui_ImplVulkan_SwapBuffers() the following line checks the rebuild flag.

if (vd->SwapChainNeedRebuild) // Frame data became invalid in the middle of rendering
        return;

This means that you will need an EXTRA flag to qualify why the swapchain needs rebuild. If it is for ANY other reason then a VK_SUBOPTIMAL_KHR then the existing code is OK. But if you adopt the suggestion of setting the swapchain rebuild flag on the VK_SUBOPTIMAL_KHR return code, then the extra flag may be used to Present the frame no matter the fact that the SwapChainNeedRebuild is true in the line above. UNLESS of course you decide to loose that frame as happens when an VK_ERROR_OUT_OF_DATE_KHR return code is the reason why the rebuild flag is set.