ash-window: Keep loaded fns around via new SurfaceFactory object
#1019
Conversation
MarijnS95
force-pushed
the
ash-window-store-loaded-extension
branch
from
93c61ff to
9b92e12
Compare
StarLederer
left a comment
StarLederer
left a comment
There was a problem hiding this comment.
Coming from #974, I looked at the source code and figured that it probably doesn't matter if ash::khr::Surface::Instance instance is the same or not whenever it is being used to destroy surfaces, but I think it would be better if create_surface() returned both the vk::SurfaceKHR and ash::khr::Surface::Instance. This would be a very obvious clue that we can use the loader that is returned to destroy the surface later. This would also let us avoid executing redundant init code when creating the loader again right before or after one is already created inside create_surface().
I think that is very close to what this PR is doing but the PR also adds the ability to reuse the same extension/loader instance to create more surfaces later, if I understood correctly, so I think it is a good change.
Btw, I don't want to make any false impressions, I am very new to Vulkan and my experience with Rust is also limited so consider my comments with this in mind
MarijnS95
force-pushed
the
ash-window-store-loaded-extension
branch
from
9b92e12 to
1d26882
Compare
MarijnS95
changed the title
SurfaceExtension objectSurfaceFactory object
Even though `create_surface()` is typically off the hot path, we're repeatedly loading all platform-specific surface function pointers each time a new surface has to be created (sometimes for multiple windows, and on Android after every suspend-resume cycle when the app becomes invisible). In a subsequent patch these loaded extensions can be used to call their respective `get_present_support()` without reloading the entire function-pointer struct too. Additionally some users seem to be confused about object lifetimes when seeing this `Instance::new()` being dropped inside the implementation. This patch only marginally addresses that and users are more than free to just drop their `SurfaceFactory` object if no longer used (or recreate it later) without having any impact on the "parent/child relationship" mentioned in the `fn create_surface()` documentation. For now the choice has been made to store the passed `DisplayHandle` field with the enum variant when used in `create_surface()` to reduce ambiguity by not requiring it to be passed again. The user might still pass an incompatible `WindowHandle` in which case this returns `ERROR_EXTENSION_NOT_PRESENT`.
MarijnS95
force-pushed
the
ash-window-store-loaded-extension
branch
from
1d26882 to
989eb03
Compare
| impl SurfaceFactory { | ||
| /// Load the relevant surface extension for a given [`RawDisplayHandle`]. | ||
| /// | ||
| // TODO: Safety docs about Entry/Instance lifetime? |
There was a problem hiding this comment.
Because neither new nor drop traverse display_handle, I don't think we need a formal safety comment here. The safety comment on create_surface is sufficient.
| pub fn new( | ||
| entry: &Entry, | ||
| instance: &Instance, | ||
| // TODO: Return "lifetimed" on DisplayHandle? |
There was a problem hiding this comment.
I like this idea, but it might give users a false sense of security if we don't also track the lifetime of the window handle, which would require bundling a lifetime with the SurfaceKHR somehow. I'd leave this for future work -- updating ash-window should be much less disruptive than updating ash, anyway.
Fixes #974
Even though
create_surface()is typically off the hot path, we're repeatedly loading all platform-specific surface function pointers each time a new surface has to be created (sometimes for multiple windows, and on Android after every suspend-resume cycle when the app becomes invisible). In a subsequent patch these loaded extensions can be used to call their respectiveget_present_support()without reloading the entire function-pointer struct too.Additionally some users seem to be confused about object lifetimes when seeing this
Instance::new()being dropped inside the implementation. This patch only marginally addresses that and users are more than free to just drop theirSurfaceFactoryobject if no longer used (or recreate it later) without having any impact on the "parent/child relationship" mentioned in thefn create_surface()documentation.For now the choice has been made to store the passed
DisplayHandlefield with the enum variant when used increate_surface()to reduce ambiguity by not requiring it to be passed again. The user might still pass an incompatibleWindowHandlein which case this returnsERROR_EXTENSION_NOT_PRESENT.