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

Vulkan: Fixing secondary viewport resize problems on linux #7513

Closed

Conversation

InsideBSITheSecond
Copy link

@InsideBSITheSecond InsideBSITheSecond commented Apr 20, 2024

this fixes #7508
and maybe #3758 as well

@InsideBSITheSecond InsideBSITheSecond changed the title this fix the issue https://github.com/ocornut/imgui/issues/7508 Fixing secondary viewport resize problems on linux Apr 20, 2024
@InsideBSITheSecond
Copy link
Author

InsideBSITheSecond commented Apr 20, 2024

This happens because when vkAcquireNextImageKHR returns VK_ERROR_OUT_OF_DATE_KHR, the following err check cause the example to abort.

If the swap chain turns out to be out of date when attempting to acquire an image, then it is no longer possible to present to it. Therefore we should immediately recreate the swap chain and try again in the next drawFrame call. (source)

There is probably a better way to skip the current frame other than adding a new bool in wd & returning 2 functions, but I'm not familiar enough with imgui's backend to think about one atm.
(I did it this way because between ImGui_ImplVulkan_RenderWindow & ImGui_ImplVulkan_SwapBuffers the execution is handled back to imgui.cpp and there was no reason to edit that so I just found a quick and easy way to keep the edit in a single file. & we have to skip any semaphore calls to avoid validation errors)

But this pr makes resizing secondary viewports work as intended on linux (or whatever else my system & #3758 have in common that make this issue happen in the first place)

I know this is an annoying issue to reproduce on your side so I just went ahead and attempted to fix it myself, feel free to edit it to your likings.
(and btw it's been tested with validation layers enabled, and works without issues on my end at least)
(and it should not break anything)

@ocornut ocornut changed the title Fixing secondary viewport resize problems on linux Vulkan: Fixing secondary viewport resize problems on linux Apr 22, 2024
@ocornut ocornut added the vulkan label Apr 22, 2024
else
return;
}
else
check_vk_result(err);
Copy link
Owner

@ocornut ocornut May 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the purpose of returning here and not incrementing both indexes?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you also confirm that both paths calling ImGui_ImplVulkanH_CreateOrResizeWindow() are needed?
Sorry for the amount of details requested, but since this is rather subtle and brittle and hard to reproduce, any extra details are useful for our records.

Copy link
Author

@InsideBSITheSecond InsideBSITheSecond May 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

none of this is needed that's just how I handle it in my own backend.

wd->CanPresent = true;
return;
}

VkResult err;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you also clarify how this is needed?
Our main.cpp example do a similar thing via setting g_SwapChainRebuild = true but it is primarily because the swap chain recreation is deferred to later.
Can you confirm what happens if you attempt the vkQueuePresentKHR() ? for my records. Thanks!

Copy link
Author

@InsideBSITheSecond InsideBSITheSecond May 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and yeah answers to this are in #3390

ocornut pushed a commit that referenced this pull request May 7, 2024
@ocornut
Copy link
Owner

ocornut commented May 7, 2024

This should now be fixed, see other PR:
#3390 (comment)

Both PR were AFAIK near identical in results, given the complexity of this, I would appreciate if everyone affected could confirm that it fixes the situation for them. Thanks everyone!

@ocornut ocornut closed this May 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants