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

Update vulkan swapchain recreation examples #3390

Closed
wants to merge 12 commits into from

Conversation

RoryO
Copy link

@RoryO RoryO commented Aug 6, 2020

Following up on #2626, this PR does a couple things. First, it changes all the Vulkan examples on the timing of resizing the swapchain. Currently the sdl and glfw examples respond to the library window resize events and then trigger the swapchain resize on those events. This accidentally works on Windows as Windows does not repaint a window using a GPU accelerated surface as it's resized. It treats accelerated windows differently as Windows repaints other windows as they're resized. Window managers on Linux and OSX will always resize the window contents as it's resized. This leads to a disconnect in timing where the display system redraws the window with different dimensions before processing the event and resizing the swapchain. This causes a crash with VK_ERROR_OUT_OF_DATE_KHR.

The best thing with Vulkan swapchain resizing is don't trigger recreating the swapchain outside the render loop. Instead, always present the swapchain images and check for VK_ERROR_OUT_OF_DATE. This is the last possible moment in the Vulkan pipeline. If it's out of date, resize the swapchain and go on to the next render loop.

Also made a minor updates. Fixed the clear color reference, the clear_color variable exists and was not copied to the vulkan ClearValue property.

@ocornut
Copy link
Owner

ocornut commented Aug 6, 2020

Hello,

I'm not sure I understand how the issue as labelled relates to adding a full precompiled SDL into the repository. I can't accept it into the repo, but I presume the other changes are very worth looking at. Could you force-push this without the unrelated SDL changes?

Thank you!

@RoryO
Copy link
Author

RoryO commented Aug 6, 2020

Sure. Would you consider separate PR updating the SDL build matching the glfw build or should I just leave it out?

@ocornut
Copy link
Owner

ocornut commented Aug 7, 2020

I pushed a commit for the clear color issue which got broken by a06eb83. Fix is ede8825 (will conflict with yours which is more verbose).

@ocornut
Copy link
Owner

ocornut commented Aug 7, 2020

Thank you!

I have uploaded a simplified version of that fix here:
https://github.com/ocornut/imgui/compare/features/vulkan_resize_fix
For new code please try to stay within the code style.

The problem is this needs to be merged and reworked for multi-viewport support in the docking branch.
With the separation of rendering and platform back-end it'll require some work and testing.
The "swapbuffer" function of the Vulkan back-ends for owned secondary viewport would need to go via the PlatformIO functions to retrieve the size back from the platform back-end.

Could you figure out the proper suitable fix applicable to docking branch with multi-viewport enabled? Since I presume the current fix won't work for secondary viewports owned by the back-end.

@RoryO
Copy link
Author

RoryO commented Aug 15, 2020

Yep, currently docking head crashes with the glfw vulkan example and resize

Peek 2020-08-15 14-36

Looking at this now since my next work is getting #3372 working with docking.

@RoryO
Copy link
Author

RoryO commented Aug 15, 2020

I have uploaded a simplified version of that fix here:
https://github.com/ocornut/imgui/compare/features/vulkan_resize_fix
For new code please try to stay within the code style.

Sorry, I'm a bit lost on what you want me to do here. Do you want me to cherry pick and merge your commit into this branch and open a separate PR for docking?

@ocornut
Copy link
Owner

ocornut commented Aug 17, 2020

Sorry, I'm a bit lost on what you want me to do here. Do you want me to cherry pick and merge your commit into this branch and open a separate PR for docking?

For master, I believe my branch is a simpler replacement to your branch, and could be merged as-is later.

But the issue is the change won't work as-is for docking, so ideally you would:

  • start from docking, cherry-pick my simplified version of the change
  • then figure out what it would take to make that same fix work with multi-viewports and make a new PR (I don't think you can force-push into this same PR if the base branch changed?)
  • when it works I can merge the first commit into master+docking and the other commits you'll produce into docking.

@RoryO
Copy link
Author

RoryO commented Aug 17, 2020

For master, I believe my branch is a simpler replacement to your branch, and could be merged as-is later.

I think yours is fine. Can you go ahead and merge it to master now, and I'll work on the viewports?

@ocornut
Copy link
Owner

ocornut commented Aug 18, 2020

I think yours is fine. Can you go ahead and merge it to master now, and I'll work on the viewports?

I'd prefer to first be sure the technique is going to work in a similar manner with multi-viewport. In your branch based from docking you can cherry-pick that commit as a first step, and we merge on both side when ready.

You should be able to call platform_io.Platform_GetWindowSize() to get window size in the renderer back-end.

@hinxx
Copy link

hinxx commented Aug 18, 2020

Tried this PR on my Linux Mint 19.3 system, and I still get the app to crash if window resize is attempted. Happens for both SDL and GLFW with vulkan.

hinxx@obzen ~/Code/x11imgui/examples/example_x11_vulkan $ git checkout update-vulkan-examples 
Switched to branch 'update-vulkan-examples'
Your branch is up to date with 'origin/update-vulkan-examples'.
hinxx@obzen ~/Code/x11imgui/examples/example_sdl_vulkan $ ./build.sh 
hinxx@obzen ~/Code/x11imgui/examples/example_sdl_vulkan $ ./Debug/example_sdl_vulkan 
Xlib:  extension "NV-GLX" missing on display ":0.0".
[vulkan] Error: VkResult = -1000001004
Aborted
hinxx@obzen ~/Code/x11imgui/examples/example_glfw_vulkan $ git checkout update-vulkan-examples 
Already on 'update-vulkan-examples'
Your branch is up to date with 'origin/update-vulkan-examples'.
hinxx@obzen ~/Code/x11imgui/examples/example_glfw_vulkan $ make
hinxx@obzen ~/Code/x11imgui/examples/example_glfw_vulkan $ ./example_glfw_vulkan 
Xlib:  extension "NV-GLX" missing on display ":0.0".
[vulkan] Error: VkResult = -1000001004
Aborted

Similar results with the code in https://github.com/ocornut/imgui/compare/features/vulkan_resize_fix branch.

hinxx@obzen ~/Code/imgui/examples/example_sdl_vulkan $ git checkout features/vulkan_resize_fix 
Switched to branch 'features/vulkan_resize_fix'
Your branch is up to date with 'origin/features/vulkan_resize_fix'.
hinxx@obzen ~/Code/imgui/examples/example_sdl_vulkan $ ./build.sh 
hinxx@obzen ~/Code/imgui/examples/example_sdl_vulkan $ ./Debug/example_sdl_vulkan 
Xlib:  extension "NV-GLX" missing on display ":0.0".
[vulkan] Error: VkResult = -1000001004
Aborted
hinxx@obzen ~/Code/imgui/examples/example_sdl_vulkan $ 
hinxx@obzen ~/Code/imgui/examples/example_glfw_vulkan $ git checkout features/vulkan_resize_fix 
M	examples/example_glfw_opengl3/main.cpp
Already on 'features/vulkan_resize_fix'
Your branch is up to date with 'origin/features/vulkan_resize_fix'.
hinxx@obzen ~/Code/imgui/examples/example_glfw_vulkan $ make 
hinxx@obzen ~/Code/imgui/examples/example_glfw_vulkan $ ./example_glfw_vulkan 
Xlib:  extension "NV-GLX" missing on display ":0.0".
[vulkan] Error: VkResult = -1000001004
Aborted

Under gdb I got this backtrace for SDL & vulkan code in this PR:

hinxx@obzen ~/Code/x11imgui/examples/example_sdl_vulkan $ gdb ./Debug/example_sdl_vulkan 
GNU gdb (Ubuntu 8.1-0ubuntu3.2) 8.1.0.20180409-git
...
[vulkan] Error: VkResult = -1000001004
[Thread 0x7fffcf10c700 (LWP 10197) exited]

Thread 1 "example_sdl_vul" received signal SIGABRT, Aborted.
__GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:51
51	../sysdeps/unix/sysv/linux/raise.c: No such file or directory.
(gdb) bt
#0  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:51
#1  0x00007ffff6b648b1 in __GI_abort () at abort.c:79
#2  0x0000555555558284 in check_vk_result (err=VK_ERROR_OUT_OF_DATE_KHR) at main.cpp:48
#3  0x0000555555558a7a in FrameRender (wd=0x555555829060 <g_MainWindowData>, draw_data=0x555555e400e8) at main.cpp:251
#4  0x00005555555597a0 in main () at main.cpp:524
(gdb) 

The failing code part is:

static void FrameRender(ImGui_ImplVulkanH_Window* wd, ImDrawData* draw_data)
{
    VkResult err;

    VkSemaphore image_acquired_semaphore  = wd->FrameSemaphores[wd->SemaphoreIndex].ImageAcquiredSemaphore;
    VkSemaphore render_complete_semaphore = wd->FrameSemaphores[wd->SemaphoreIndex].RenderCompleteSemaphore;
    err = vkAcquireNextImageKHR(g_Device, wd->Swapchain, UINT64_MAX, image_acquired_semaphore, VK_NULL_HANDLE, &wd->FrameIndex);
    check_vk_result(err);
...

And my quick & dirty hack that solves it for me is:

hinxx@obzen ~/Code/x11imgui/examples/example_sdl_vulkan $ git diff .
diff --git a/examples/example_sdl_vulkan/main.cpp b/examples/example_sdl_vulkan/main.cpp
index b5ce9c4f..4a479c51 100644
--- a/examples/example_sdl_vulkan/main.cpp
+++ b/examples/example_sdl_vulkan/main.cpp
@@ -248,7 +248,13 @@ static void FrameRender(ImGui_ImplVulkanH_Window* wd, ImDrawData* draw_data)
     VkSemaphore image_acquired_semaphore  = wd->FrameSemaphores[wd->SemaphoreIndex].ImageAcquiredSemaphore;
     VkSemaphore render_complete_semaphore = wd->FrameSemaphores[wd->SemaphoreIndex].RenderCompleteSemaphore;
     err = vkAcquireNextImageKHR(g_Device, wd->Swapchain, UINT64_MAX, image_acquired_semaphore, VK_NULL_HANDLE, &wd->FrameIndex);
-    check_vk_result(err);
+    if(err == VK_ERROR_OUT_OF_DATE_KHR)
+    {
+        SDL_GetWindowSize(g_Window, &g_SwapChainResizeWidth, &g_SwapChainResizeHeight);
+        g_SwapChainRebuild = true;
+        return;
+    }
+     check_vk_result(err);
 
     ImGui_ImplVulkanH_Frame* fd = &wd->Frames[wd->FrameIndex];
     {
@@ -522,7 +528,8 @@ int main(int, char**)
         if (!is_minimized)
         {
             FrameRender(wd, draw_data);
-            FramePresent(wd);
+            if (!g_SwapChainRebuild)
+                FramePresent(wd);
         }
     }

@RoryO RoryO changed the base branch from master to docking August 18, 2020 21:36
@RoryO
Copy link
Author

RoryO commented Aug 18, 2020

Okay I updated, cherry-picked @ocornut a65a6cae41d172c01d813da310e6b25d4fce5d80 and retargeted to docking.

@hinxx Thank you for trying this out! As of this comment resizing the main window works, resizing any child windows crashes like the above capture. That's what I'm fixing next, hopefully end of today.

(None of the animations are half drawn when viewing locally, seems like the capture utility I use sometimes captures a half drawn frame)

Peek 2020-08-18 15-49

Peek 2020-08-18 15-51

@RoryO
Copy link
Author

RoryO commented Aug 19, 2020

Yeah, it works fully with viewports! I think this is good to go now.
Peek 2020-08-18 17-46

@hinxx
Copy link

hinxx commented Aug 20, 2020

Thanks for the update @RoryO .
I still have the same issue:

hinxx@obzen ~/Code/x11imgui/examples/example_sdl_vulkan $ ./Debug/example_sdl_vulkan 
Xlib:  extension "NV-GLX" missing on display ":0.0".
[vulkan] Error: VkResult = -1000001004
Aborted

Peek 2020-08-20 11-19

The last resize of the 'Demo ImGui Demo' window killed the app because the mouse went out of the main window.

I was also experiencing random crashes of the app with the same error if I tried to move the window around (not resizing it, just moving). Sometimes resizing the ImGui windows inside the main window would cause the crash as well. I did not manage to capture those.

When going outside the main window by resizing the internal window GDB backtrace says:

Thread 1 "example_sdl_vul" received signal SIGABRT, Aborted.
__GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:51
51	../sysdeps/unix/sysv/linux/raise.c: No such file or directory.
(gdb) bt
#0  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:51
#1  0x00007ffff6b648b1 in __GI_abort () at abort.c:79
#2  0x0000555555558a14 in check_vk_result (err=VK_ERROR_OUT_OF_DATE_KHR) at main.cpp:47
#3  0x000055555555c164 in check_vk_result (err=VK_ERROR_OUT_OF_DATE_KHR) at ../imgui_impl_vulkan.cpp:248
#4  0x000055555556069f in ImGui_ImplVulkan_RenderWindow (viewport=0x555555a81ec0) at ../imgui_impl_vulkan.cpp:1360
#5  0x000055555558a26e in ImGui::RenderPlatformWindowsDefault (platform_render_arg=0x0, renderer_render_arg=0x0) at ../../imgui.cpp:11533
#6  0x0000555555559f7a in main () at main.cpp:532

This is right after the call to vkAcquireNextImageKHR().

When resizing the main window the GDB backtrace is a bit different:

Thread 1 "example_sdl_vul" received signal SIGABRT, Aborted.
__GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:51
51	../sysdeps/unix/sysv/linux/raise.c: No such file or directory.
(gdb) bt
#0  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:51
#1  0x00007ffff6b648b1 in __GI_abort () at abort.c:79
#2  0x0000555555558a14 in check_vk_result (err=VK_ERROR_OUT_OF_DATE_KHR) at main.cpp:47
#3  0x000055555555920a in FrameRender (wd=0x555555850060 <g_MainWindowData>, draw_data=0x555555d8ba98) at main.cpp:250
#4  0x0000555555559f54 in main () at main.cpp:526

Nevertheless, it again happens right after the call to vkAcquireNextImageKHR().

@RoryO
Copy link
Author

RoryO commented Aug 20, 2020

Oh drat I forgot that it's possible that vkAcquireNextImageKHR could also return out of date. That's a rarer case though what @hinxx uses for a window manager trips it immediately. It takes some playing around with the window on KDE and I did reproduce it. It's the same fix, wherever we call vkAcquireNextImageKHR check for out of date, if so rebuild the swapchain and continue on to the next frame. I'll have that up tomorrow.

@RoryO
Copy link
Author

RoryO commented Aug 21, 2020

Should be better now!

@ocornut
Copy link
Owner

ocornut commented Aug 21, 2020

I think yours is fine. Can you go ahead and merge it to master now, and I'll work on the viewports?

Changed my mind and merged this today on master, better than nothing (but would of course be good to get the multi-viewport equivalent fixed and tested!). Thanks again.

@hinxx
Copy link

hinxx commented Aug 22, 2020

@RoryO , we're getting there.. not yet there though. This time it took a little longer to get it to crash. It happens only after the internal window is moved out of the main window, and then resized. Resizing the main window does not crash the app anymore.

hinxx@obzen ~/Code/x11imgui/examples/example_sdl_vulkan $ git log -n1
commit a2118b1a138db0d5ae641345d7b40b0cb20cdaa3 (HEAD -> update-vulkan-examples, origin/update-vulkan-examples)
Author: Rory OConnell <19547+RoryO@users.noreply.github.com>
Date:   Thu Aug 20 20:29:05 2020 -0700

    Remove unused window parameter
hinxx@obzen ~/Code/x11imgui/examples/example_sdl_vulkan $ git branch 
  master
* update-vulkan-examples
  x11-samples

Here is the gdb output for both, SDL and GLFW based apps:

hinxx@obzen ~/Code/x11imgui/examples/example_sdl_vulkan $ gdb ./Debug/example_sdl_vulkan 

Thread 1 "example_sdl_vul" received signal SIGABRT, Aborted.
__GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:51
51	../sysdeps/unix/sysv/linux/raise.c: No such file or directory.
(gdb) bt
#0  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:51
#1  0x00007ffff6b648b1 in __GI_abort () at abort.c:79
#2  0x0000555555558a14 in check_vk_result (err=VK_ERROR_OUT_OF_DATE_KHR) at main.cpp:45
#3  0x000055555555c16e in check_vk_result (err=VK_ERROR_OUT_OF_DATE_KHR) at ../imgui_impl_vulkan.cpp:248
#4  0x00005555555606a9 in ImGui_ImplVulkan_RenderWindow (viewport=0x555555a90d40) at ../imgui_impl_vulkan.cpp:1360
#5  0x000055555558a278 in ImGui::RenderPlatformWindowsDefault (platform_render_arg=0x0, renderer_render_arg=0x0) at ../../imgui.cpp:11533
#6  0x0000555555559f7c in main () at main.cpp:536

hinxx@obzen ~/Code/x11imgui/examples/example_glfw_vulkan $ gdb ./example_glfw_vulkan 

[vulkan] Error: VkResult = -1000001004

Thread 1 "example_glfw_vu" received signal SIGABRT, Aborted.
__GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:51
51	../sysdeps/unix/sysv/linux/raise.c: No such file or directory.
(gdb) bt
#0  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:51
#1  0x00007ffff65338b1 in __GI_abort () at abort.c:79
#2  0x000055555557e204 in check_vk_result (err=VK_ERROR_OUT_OF_DATE_KHR) at /home/hinxx/Code/x11imgui/examples/example_glfw_vulkan/main.cpp:53
#3  0x0000555555581aec in check_vk_result (err=VK_ERROR_OUT_OF_DATE_KHR) at /home/hinxx/Code/x11imgui/examples/imgui_impl_vulkan.cpp:248
#4  0x0000555555586027 in ImGui_ImplVulkan_RenderWindow (viewport=0x555555932b90) at /home/hinxx/Code/x11imgui/examples/imgui_impl_vulkan.cpp:1360
#5  0x00005555555afbc5 in ImGui::RenderPlatformWindowsDefault (platform_render_arg=0x0, renderer_render_arg=0x0) at /home/hinxx/Code/x11imgui/imgui.cpp:11533
#6  0x000055555557f6eb in main () at /home/hinxx/Code/x11imgui/examples/example_glfw_vulkan/main.cpp:537
(gdb) 

@RoryO
Copy link
Author

RoryO commented Aug 22, 2020

That one has me concerned. Viewports are rendered in two functions RenderWindow and SwapBuffers. RenderWindow does a render pass and SwapBuffers does the page flip. I'm trying to think through what can potentially happen in the next step, ImGui_ImplVulkan_SwapBuffers if we do the same catch and return in ImGui_ImplVulkan_RenderWindow. We may get out of date again when presenting the image, which is okay and we'll skip this frame, we may get a garbage looking frame drawn, which is ugly and unprofessional, or we may get a validation error I'm not thinking of at the moment which puts us in undefined behavior.

One easy way to solve it is turn SwapBuffers for the default vulkan renderer a no-op and perform the swapchain image presentation at the end of RenderWindow. Another way is keeping them separate and creating a flag, probably within ImGuiViewportDataVulkan, that signals don't perform the page flip because that swapchain image may be garbage data. I'd probably do the former as I can't see a reason for separating the render pass from the queue presentation here. I'll leave it for @ocornut if he has any other insight.

e: Actually from poking around a bit more, setting viewport->DrawData to null and calling viewport->DrawDataP.Clear() in RenderWindow might be the answer.

@RoryO
Copy link
Author

RoryO commented Aug 23, 2020

Okay I went with nulling out the draw data approach. Hopefully that's the last of them.

@hinxx
Copy link

hinxx commented Aug 29, 2020

@RoryO, works fine for me now, no more crashes.

@ocornut
Copy link
Owner

ocornut commented Aug 29, 2020

I don't fully understand what's going on, it seems like this is cancelling out some the refactor done and committed in df89a16 so it's dififcult to understand. Would be good to rebase it on latest and then ensure only the strict necessary amount of changes are in the commits (some of the clear color changes were reverted etc.)

@ocornut
Copy link
Owner

ocornut commented May 7, 2024

I went back to this as #2626 #3390 #3758 #7508 #7513 are all circling around the same problem and this seems like the first solution to it, with ample commentary.

I am forced to harshly divide and conquer to move forward with the amount of task, so any issues/pr with extraneous noise is unfortunately always delayed.

As of today (and it may not have been the case at the time of the PR),

In examples's main.cpp:

  • the ClearValue related changes are not needed (this was changed already)
  • the if (g_SwapChainRebuild) continue; are not needed (the only thing coming afterwards is a call to FramePresent() which does the same thing.
  • so there are no remaining changes in this PR for the main.cpp files.

In backend itself:

I'm NOW going to run over those in details and attempt to merge something. Sorry for the delay!

@ocornut
Copy link
Owner

ocornut commented May 7, 2024

I'm not going to run over those in details and attempt to merge something. Sorry for the delay!

I'm NOW going [...] (worst typo :)

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

ocornut commented May 7, 2024

I have merged the remaining parts of your PR as 8b2c6dd, based on comparing with #7513 I came to the conclusion they did the same thing.

One difference if your PR defer actual call to ImGui_ImplVulkanH_CreateOrResizeWindow to next ImGui_ImplVulkan_RenderWindow() call, while the other PR did the call immediately but set the flag to avoid swapping. They are essentially equivalent but I prefer the approach of calling from a single spot instead of two spots.

I've credited you both.

Given the complexity of this, I would appreciate if everyone affected could confirm that it fixes the situation for them.

Thanks everyone!

@InsideBSITheSecond
Copy link

Hey,

I confirm this fixes the issue I had.

Thanks and have a nice day !

@RoryO
Copy link
Author

RoryO commented May 10, 2024

Wow this is lovely! I have sadly not been able to do gfx work in years and would have a difficult time picking this back up again if I had to because I don't remember even doing this. Glad it all worked out!

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.

None yet

4 participants