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

ash_window: Make enumerate_required_extensions() return all available surface extensions irrespective of a window handle #595

Closed

Conversation

Neo-Zhixing
Copy link
Contributor

@Neo-Zhixing Neo-Zhixing commented Mar 13, 2022

Currently, ash_window::enumerate_required_extensions accepts a &dyn HasRawWindowHandle. It then returns a list of &'static CStr based on the type of the RawWindowHandle. This seems fine on the surface, however it's making a few implicit assumptions:

  • The application would create the window before creating ash::Instance
  • All RawWindowHandle used during the lifetime of ash::Instance will have the same type.

The first assumption limits the usability of this function. For example, when integrating ash directly with bevy, it would be preferred to create the Instance on application startup within the AppBuilder. The window could be created a few frames later when bevy actually sends out the WindowCreated event.

The second scenario is unlikely in the real world, but there's no guarantee that a multi-window linux application wouldn't have a Xlib window and a Wayland window at the same time.

So, instead of enumerating platform-specific extensions based on the RawWindowHandler, this PR makes it so that ash_window::enumerate_required_extensions only requires a ash::Entry. It would then enumerate the supported extension list and enable all platform-specific surface extensions. In the case that no such extension exist, the API should throw ERROR_EXTENSION_NOT_PRESENT.

TODO: fix example

related: #590

@Ralith
Copy link
Collaborator

Ralith commented Mar 14, 2022

I would prefer to address this by adding const TARGET_EXTENSIONS: &[&CStr] or similar which has the complete list of WSI extensions that make sense on the target platform. That allows both use cases to be supported, and is less opinionated about handling missing extensions.

@MarijnS95
Copy link
Collaborator

This is a very real problem on Android, where window handles come and go during application runtime, and generally only become present well after the program should have initialized Vulkan.

At the same time the necessary extensions here are well-known: there's just khr::Surface and khr::AndroidSurface, leading to usually hardcoding these with a cfg! instead of calling enumerate_required_extensions.

As such, having a const TARGET_EXTENSIONS with all possibly supported extensions for the compiled platform seem like a win to me - users should do their own filtering or we could come provide a helper function in ash and/or ash-window for this (similar to what was written in this PR)?

However, all this should be additional, and IMO enumerate_required_extensions should stay in its current form returning just the extensions that are exactly required for the given window handle. Applications using multiple types of window handles can then decide for themselves if they:

  • Have all handle types present up-front, and concatenate results of enumerate_required_extensions for all handles (that's what we do in our application, even though they all use the same windowing system for now);
  • Enable all supported surface extensions for the current platform, even if some may go unused.

@Neo-Zhixing
Copy link
Contributor Author

That sounds like a reasonable approach to take here. I'll get this updated ASAP.

@MarijnS95 MarijnS95 changed the title ash_window::enumerate_required_extensions usability issue ash_window: Make enumerate_required_extensions() return all available surface extensions irrespective of a window handle May 6, 2023
@MarijnS95
Copy link
Collaborator

MarijnS95 commented May 6, 2023

Closing as stale and not a desirable approach; the recommended implementation has instead been taken up by another contributor in #611. And it's been more than a year 😉

@MarijnS95 MarijnS95 closed this May 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants