Introduce ash-swapchain helper crate#506
Conversation
| &[vk::SubmitInfo::builder() | ||
| .wait_semaphores(&[acq.ready]) | ||
| .wait_dst_stage_mask(&[vk::PipelineStageFlags::TRANSFER]) | ||
| .signal_semaphores(&[self.frames[acq.frame_index].complete]) | ||
| .command_buffers(&[cmd]) | ||
| .build()], |
There was a problem hiding this comment.
When needing a slice of a single element that is created by a builder, I typically use std::slice::as_ref() which allows the builder to Deref without calling .build().
Edit: Same for a few other single-element slice constructs in this PR.
There was a problem hiding this comment.
That seems like extraneous ceremony to me, since this build call is lexically within the statement that consumes it and therefore sound regardless.
Ralith
force-pushed
the
swapchain
branch
2 times, most recently
from
678186d to
b5d68ce
Compare
|
I'm now confident the resize handling here is as well-behaved as the presentation API permits. Expanded the doc comments a good bit too. |
Ralith
force-pushed
the
swapchain
branch
3 times, most recently
from
f303def to
fcbaa31
Compare
As per the readme `.build()` should only be called as late as possible, and only if absolutely necessary; such cases include slices that are passed directly to functions. More precisely, such build calls and the creation of temporary slices should happen inside the same expression as the function call to be sound and completely lifetime-checked. This pattern of `&[my_builder.build()]` is however not possible when constructing intermediary Vulkan objects that reference the slice. In the first place this slice goes out of scope after the expression that creates the Vulkan object, which is caught and disallowed by rustc (unless this expression itself ends in `.build()`, which is completely unsound as it makes rustc unable to validate this lifetime dependency). In the second place - and as is most relevant for this patch that removes `.build()` calls that were not surrounded by temporary slice constructors - said expression drops the lifetime checks on anything held by `my_builder` which _could_ go out of scope before the newly constructed Vulkan object is used, resulting yet again in Undefined Behaviour. Fortunately, for slices of size 1 which are typical in Vulkan, `std::slice::as_ref` exists which is analogous to taking a pointer to an object and considering it an array of length 1 in C(++). This maintains the lifetime through `Deref` and makes rustc able to fully check all lifetimes and prevent unsound code. Albeit improving overall consistency, the `&[my_builder.build()]` pattern is not substituted in aforementioned Vulkan function-call expressions as that is considered "extraneous" [1] and demonstrates the various ways to safely construct Vulkan objects for the observant reader. [1]: #506 (comment)
As per the readme `.build()` should only be called as late as possible, and only if absolutely necessary; such cases include slices that are passed directly to functions. More precisely, such build calls and the creation of temporary slices should happen inside the same expression as the function call to be sound and completely lifetime-checked. This pattern of `&[my_builder.build()]` is however not possible when constructing intermediary Vulkan objects that reference the slice. In the first place this slice goes out of scope after the expression that creates the Vulkan object, which is caught and disallowed by rustc (unless this expression itself ends in `.build()`, which is completely unsound as it makes rustc unable to validate this lifetime dependency). In the second place - and as is most relevant for this patch that removes `.build()` calls that were not surrounded by temporary slice constructors - said expression drops the lifetime checks on anything held by `my_builder` which _could_ go out of scope before the newly constructed Vulkan object is used, resulting yet again in Undefined Behaviour. Fortunately, for slices of size 1 which are typical in Vulkan, `std::slice::as_ref` exists which is analogous to taking a pointer to an object and considering it an array of length 1 in C(++). This maintains the lifetime through `Deref` and makes rustc able to fully check all lifetimes and prevent unsound code. Albeit improving overall consistency, the `&[my_builder.build()]` pattern is not substituted in aforementioned Vulkan function-call expressions as that is considered "extraneous" [1] and demonstrates the various ways to safely construct Vulkan objects for the observant reader. [1]: #506 (comment)
|
A few high level comments: This would need a readme that quickly explains the purpose of this library (The why and how). Similar to what ash-window has. We should probably also promote our helper crates in the main readme On another note, should we use this in the ash examples as well? |
|
Added a brief README.
I'm kinda tempted to drop ash's current example entirely in favor of something like this demo.rs; if we don't try to teach Vulkan in general and focus on maximal simplicity, the (currently empirically unsustainable) maintenance effort required to keep the example in good shape will be much reduced. |
|
Replaced the |
Ralith
force-pushed
the
swapchain
branch
5 times, most recently
from
4304185 to
36d99ec
Compare
| }; | ||
| self.needs_rebuild = true; | ||
|
|
||
| // Rebuild swapchain |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
| impl Swapchain { | ||
| /// Construct a new [`Swapchain`] for rendering at most `frames_in_flight` frames | ||
| /// concurrently. `extent` should be the current dimensions of `surface`. | ||
| pub fn new( |
There was a problem hiding this comment.
I think this needs to be unsafe:
VUID-VkSwapchainCreateInfoKHR-surface-01270 states the device must be compatible with the surface. This function arguably has no way to test for that
Swapchain support courtesy of Ralith ash-rs/ash#506 - thanks!
|
This should be heavily revised to take advantage of VK_EXT_swapchain_maintenance1, which resolves some key ambiguities in the base extension. |
|
I think this should draw inspiration from #1019 to load and cache the function pointer tables internally rather than requiring the user to awkwardly pass them in. Storing some redundant function pointers isn't a big deal. AFAIK this is still not implemented by all major vendors on desktop, so maybe not a good target :( |
6 participants
This implements a reusable, lightly-opinionated helper for typical resizable windowed swapchain operation, which is otherwise error-prone.