Logic error in ImGui_ImplVulkan_RenderWindow() function. #7831
Description
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