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

Logic error in ImGui_ImplVulkan_SwapBuffers() function. #7825

Open
NostraMagister opened this issue Jul 28, 2024 · 2 comments
Open

Logic error in ImGui_ImplVulkan_SwapBuffers() function. #7825

NostraMagister opened this issue Jul 28, 2024 · 2 comments

Comments

@NostraMagister
Copy link

NostraMagister commented Jul 28, 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:

Function: ImGui_ImplVulkan_SwapBuffers()

Partial Code:

    err = vkQueuePresentKHR(v->Queue, &info);
    if (err == VK_ERROR_OUT_OF_DATE_KHR || err == VK_SUBOPTIMAL_KHR)
    {
        vd->SwapChainNeedRebuild = true;
        return;
    }
    check_vk_result(err);

    wd->FrameIndex = (wd->FrameIndex + 1) % wd->ImageCount;             // This is for the next vkWaitForFences()
    wd->SemaphoreIndex = (wd->SemaphoreIndex + 1) % wd->SemaphoreCount; // Now we can use the next set of semaphores
	

Comment:
The return value VK_SUBOPTIMAL_KHR must/should, IMO, be treated like VK_SUCCESS after the SwapChainNeedRebuild
flag has been set. The Vulkan specs qualify it as success which means the vkQueuePresentKHR() has properly
executed and the Frame and Semaphore indices must be increased. In the code above with VK_SUBOPTIMAL_KHR the indices are not increased.

Info Source: [https://registry.khronos.org/vulkan/specs/1.3-extensions/man/html/vkQueuePresentKHR.html]

Screenshots/Video:

No response

Minimal, Complete and Verifiable Example code:

@ocornut
Copy link
Owner

ocornut commented Jul 29, 2024

Hello,

Thanks for the report. I generally have a principle issue where I don't want to "blind fix" issues based on theory or documentation only, I would like to confirm that this can run correctly when it happens.

At this point I'm not even sure why we don't increase the two values even on the VK_ERROR_OUT_OF_DATE_KHR path?

Pushed e212511, this amends 085cff2 (#3881)

@NostraMagister
Copy link
Author

NostraMagister commented Jul 29, 2024

The answer is partially in a new ticket with two suggestions i just posted. The frame-index shouldn't be increased at all IMO, as i explain. There are IMO quite some other issues in the vulkan backend code. I'll report them when I get there and then you can decide whether you do something with them or not.

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