-
-
Notifications
You must be signed in to change notification settings - Fork 10.4k
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
Conversation
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! |
Sure. Would you consider separate PR updating the SDL build matching the glfw build or should I just leave it out? |
a709840
to
46ba0d7
Compare
Thank you! I have uploaded a simplified version of that fix here: The problem is this needs to be merged and reworked for multi-viewport support in the Could you figure out the proper suitable fix applicable to |
Yep, currently Looking at this now since my next work is getting #3372 working with docking. |
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 But the issue is the change won't work as-is for
|
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 |
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.
Similar results with the code in https://github.com/ocornut/imgui/compare/features/vulkan_resize_fix branch.
Under
The failing code part is:
And my quick & dirty hack that solves it for me is:
|
Okay I updated, cherry-picked @ocornut @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) |
Thanks for the update @RoryO .
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:
This is right after the call to When resizing the main window the GDB backtrace is a bit different:
Nevertheless, it again happens right after the call to |
Oh drat I forgot that it's possible that |
Should be better now! |
Changed my mind and merged this today on |
@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.
Here is the gdb output for both, SDL and GLFW based apps:
|
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, 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 e: Actually from poking around a bit more, setting viewport->DrawData to null and calling viewport->DrawDataP.Clear() in |
Okay I went with nulling out the draw data approach. Hopefully that's the last of them. |
@RoryO, works fine for me now, no more crashes. |
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.) |
abee70f
to
2f394d6
Compare
4a6447b
to
6822493
Compare
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:
In backend itself:
I'm NOW going to run over those in details and attempt to merge something. Sorry for the delay! |
I'm NOW going [...] (worst typo :) |
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 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! |
Hey, I confirm this fixes the issue I had. Thanks and have a nice day ! |
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! |
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 vulkanClearValue
property.