-
Notifications
You must be signed in to change notification settings - Fork 27
Add exception handling for vk::SystemError #85
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
base: main
Are you sure you want to change the base?
Conversation
…tation functions - Catch `vk::SystemError` in `mainLoop` and presentation logic to handle `vk::Result::eErrorOutOfDateKHR` gracefully. - Log and ignore swapchain-related errors during shutdown or presentation to prevent crashes while rethrowing unexpected exceptions. - Updated multiple attachments for consistent error handling.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# Conflicts: # attachments/17_swap_chain_recreation.cpp # attachments/18_vertex_input.cpp # attachments/19_vertex_buffer.cpp # attachments/20_staging_buffer.cpp # attachments/26_texture_mapping.cpp # attachments/27_depth_buffering.cpp # attachments/30_multisampling.cpp # attachments/31_compute_shader.cpp
I just tried to fix 17. @SaschaWillems, do me a favor and try that swapchain resize chapter code in windows and see if it fixes resize. I'll bring it to all of the other files if it solves it. |
No change: WindowsTerminal_cdnpxr5dCh.mp4I don't see proper resize handling in that chapter (or any other chapter). |
… `ErrorOutOfDateKHR` with swapchain recreation.
I think there's a difference in our systems. Debugging in Linux, I'm seeing it correctly return the result of outofdate and does the recreate swapchain thing. However, you're getting spam, which means that it is throwing that error. Thus the "right" thing to do I think is to call the recreate swapchain in the catch block instead of just printing out to console. I mean that makes sense to me that it would rethrow each frame instead of sending out the error message which is admittedly what I'm used to. |
The latest version of chapter 17 does work indeed. No more issues, so feel free to apply the change to all chapters 👍🏻 |
Okay, I think Resizing is hopefully working for all. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works for all updated chapters. But chapters 33 and 34 are missing. both crash for me when trying to resize. Chapter 37 doesn't allow resizing (due to how the window is set up).
…tions for dynamic rendering.
- Add `framebufferResizeCallback` to monitor window resize events. - Implement `recreateSwapChain` to handle swapchain rebuild during resize scenarios. - Adjust `drawFrame` logic to manage `ErrorOutOfDateKHR` and `SuboptimalKHR` results robustly.
Okay, all of the chapters should handle resizing correctly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few comments on the WSI specifics inline.
glfwWaitEvents(); | ||
} | ||
} | ||
#endif | ||
// Wait for device to finish operations | ||
device.waitIdle(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note per recent spec updates, waiting for idle on the queue used for presentation is sufficient.
Also, it would be best if VK_EXT_swapchain_maintenance1's present fence were used here instead.
Is there a tutorial yet for smooth resize? I.e., using oldSwapchain to create the new swapchain and deferring cleanup until pending swaps have reached a point where it's safe to delete the old sawpchain(s)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, we should open 3 issues you presented here:
1.) remove not needed device.waitIdle();
2.) use swapchain_maintenance1
3.) request to create a smooth resize tutorial.
The first two will require making changes to the tutorial itself as we don't want to just change the code without making sure the tutorial explains what needs to be done to be correct. The last will require a full tutorial which takes more time and planning; however, it should be done as we do see the request from time to time to give a sample on smooth resizing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We also need to take into account that VK_EXT_swapchain_maintenance1 isn't universally supported, and the KHR variant even less so. So would prob. need to be optional for now.
@@ -1548,28 +1582,29 @@ class HelloTriangleApplication { | |||
}; | |||
queue.submit(submitInfo, *inFlightFences[currentFrame]); | |||
|
|||
const vk::PresentInfoKHR presentInfoKHR{ | |||
.waitSemaphoreCount = 1, | |||
.pWaitSemaphores = &*presentCompleteSemaphore[imageIndex], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does this example still have renderFinishedSemaphores, but also have these presentCompleteSemaphore array that seems to server the same purpose, but with a slightly misleading name (It seems to be used more as a "rendering complete/present-ready" indicator, rather than a "present complete" indicator, the latter of which I'm not sure is possible to measure using semaphores in current Vulkan, which is what lead me to look closely at this code).
The Android file below seems to still use renderFinishedSemaphores in the manner that fits its name for what appears to be the same steps renderFinishedSemaphores is used here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'opps' is the why :) I made a bug. Thanks for the report, I'll address it.
This is specific to the issue #73
vk::SystemError
inmainLoop
and presentation logic to handlevk::Result::eErrorOutOfDateKHR
gracefully.We likely will need to add better error detection everywhere; but in the spirit of just handling it when we get an out of date. It should be properly handled already in the draw function; but this makes it so we don't just throw an exception. I guess a better solution might be to handle the
vk::Result::eErrorOutOfDateKHR
by sending it to resize multiple times rather than the once. However, I think this is the right solution for now for this.