Skip to content

Add platform-specific extension list for creating a surface to ash_window #611

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.

Sign up for GitHub

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

Open
entropylost wants to merge 7 commits into
base: master
Choose a base branch
from
+61 −32
Open

Add platform-specific extension list for creating a surface to ash_window #611

Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
494eb34
Added TARGET_EXTENSIONS to ash-window.
entropylost Apr 11, 2022
929adc5
Added documentation.
entropylost Apr 11, 2022
31782b1
Chhanged from arrays to slices.
entropylost Apr 11, 2022
cfb320f
Updated documentation.
entropylost Apr 11, 2022
9cca727
Removed duplicate code and publicized the per-platform extensions.
entropylost Apr 12, 2022
4fdeb03
Removed unnecessary &s.
entropylost Apr 12, 2022
4359a06
Made extensions private.
entropylost Apr 12, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
93 changes: 61 additions & 32 deletions ash-window/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

use std::os::raw::c_char;

use ash::{extensions::khr, prelude::*, vk, Entry, Instance};
use ash::{extensions::ext, extensions::khr, prelude::*, vk, Entry, Instance};

@MarijnS95 MarijnS95 Apr 13, 2022

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then remove:

#[cfg(any(target_os = "macos", target_os = "ios"))]
use ash::extensions::ext; // portability extensions

You can't import the same thing in scope twice.

(But I rather see cfg() on the per-platform consts then)

use raw_window_handle::{HasRawWindowHandle, RawWindowHandle};

#[cfg(any(target_os = "macos", target_os = "ios"))]
Expand Down Expand Up @@ -118,6 +118,59 @@ pub unsafe fn create_surface(
}
}

/// Extensions necessary for creating a surface on windows.

@MarijnS95 MarijnS95 Apr 13, 2022

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// Extensions necessary for creating a surface on windows.
/// Extensions necessary for creating a surface on Windows.

#[allow(dead_code)]

@MarijnS95 MarijnS95 Apr 13, 2022

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If not making them public it seems cleaner to just repeat the cfg()?

const WINDOWS_SURFACE_EXTENSIONS: &'static [*const c_char] = &[
khr::Surface::name().as_ptr(),
khr::Win32Surface::name().as_ptr(),
];
/// Extensions necessary for creating a surface on unix.
/// Note that this is not equal to the return value of [`enumerate_required_extensions`] on Unix due to the multiple window types.
Comment on lines +127 to +128

@MarijnS95 MarijnS95 Apr 13, 2022

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// Extensions necessary for creating a surface on unix.
/// Note that this is not equal to the return value of [`enumerate_required_extensions`] on Unix due to the multiple window types.
/// Extensions necessary for creating a surface on Unix.
/// Note that this is not equal to the return value of [`enumerate_required_extensions()`] on Unix due to the multiple window types.

@MarijnS95 MarijnS95 Apr 13, 2022

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about elaborating this to return all possible extensions on Unix, and explaining how enumerate_required_extensions() differs in that situation?

Suggested change
/// Extensions necessary for creating a surface on unix.
/// Note that this is not equal to the return value of [`enumerate_required_extensions`] on Unix due to the multiple window types.
/// _All_ possible extensions for creating a surface on Unix.
///
/// Defer to [`enumerate_required_extensions()`] for a subset of this list applicable to the display server protocol specific to the given [`HasRawWindowHandle`].

Also add this to TARGET_SURFACE_EXTENSIONS since UNIX_SURFACE_EXTENSIONS is not public 😞

(If you intend these to be separate paragraphs, add an empty /// in between. Otherwise the two sentences get concatenated into one paragraph.)

#[allow(dead_code)]
const UNIX_SURFACE_EXTENSIONS: &'static [*const c_char] = &[
khr::Surface::name().as_ptr(),
khr::WaylandSurface::name().as_ptr(),
khr::XlibSurface::name().as_ptr(),
khr::XcbSurface::name().as_ptr(),
];
/// Extensions necessary for creating a surface on android.

@MarijnS95 MarijnS95 Apr 13, 2022

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// Extensions necessary for creating a surface on android.
/// Extensions necessary for creating a surface on Android.

#[allow(dead_code)]
const ANDROID_SURFACE_EXTENSIONS: &'static [*const c_char] = &[
khr::Surface::name().as_ptr(),
khr::AndroidSurface::name().as_ptr(),
];
/// Extensions necessary for creating a surface on macos.

@MarijnS95 MarijnS95 Apr 13, 2022

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// Extensions necessary for creating a surface on macos.
/// Extensions necessary for creating a surface on macOS.

At least I think this is the right capitalization?

#[allow(dead_code)]
const MACOS_SURFACE_EXTENSIONS: &'static [*const c_char] = &[
khr::Surface::name().as_ptr(),
ext::MetalSurface::name().as_ptr(),
];
/// Extensions necessary for creating a surface on ios.

@MarijnS95 MarijnS95 Apr 13, 2022

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// Extensions necessary for creating a surface on ios.
/// Extensions necessary for creating a surface on iOS.

#[allow(dead_code)]
const IOS_SURFACE_EXTENSIONS: &'static [*const c_char] = MACOS_SURFACE_EXTENSIONS;

/// Extensions necessary for creating a surface on the current target platform.
/// (Note that on Unix, this is not equal to the return value of [`enumerate_required_extensions`])
Comment on lines +152 to +153

@MarijnS95 MarijnS95 Apr 13, 2022

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same "create a separate paragraph for this":

Suggested change
/// Extensions necessary for creating a surface on the current target platform.
/// (Note that on Unix, this is not equal to the return value of [`enumerate_required_extensions`])
/// Extensions necessary for creating a surface on the current target platform.
///
/// On Unix, contains _all_ possible extensions for creating a surface. Defer to [`enumerate_required_extensions()`] for a subset of this list applicable to the display server protocol specific to the given [`HasRawWindowHandle`].

Same comment applies here though: elaborate how it differs . After all, with the removal of pub readers won't see what doc comment is written on UNIX_SURFACE_EXTENSIONS 👎

pub const TARGET_EXTENSIONS: &'static [*const c_char] = {

@MarijnS95 MarijnS95 Apr 13, 2022

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since TARGET here refers to the current target_os, are we perhaps missing _SURFACE_ in between? So:

Suggested change
pub const TARGET_EXTENSIONS: &'static [*const c_char] = {
pub const TARGET_SURFACE_EXTENSIONS: &'static [*const c_char] = {

#[cfg(target_os = "windows")]
let out = WINDOWS_SURFACE_EXTENSIONS;
#[cfg(any(
target_os = "linux",
target_os = "dragonfly",
target_os = "freebsd",
target_os = "netbsd",
target_os = "openbsd"
))]
let out = UNIX_SURFACE_EXTENSIONS;
#[cfg(target_os = "android")]
let out = ANDROID_SURFACE_EXTENSIONS;
#[cfg(target_os = "macos")]
let out = MACOS_SURFACE_EXTENSIONS;
#[cfg(target_os = "ios")]
let out = IOS_SURFACE_EXTENSIONS;
out
};

Comment thread
entropylost marked this conversation as resolved.
/// Query the required instance extensions for creating a surface from a window handle.
///
/// The returned extensions will include all extension dependencies.
Expand All @@ -126,13 +179,7 @@ pub fn enumerate_required_extensions(
) -> VkResult<&'static [*const c_char]> {
let extensions = match window_handle.raw_window_handle() {
#[cfg(target_os = "windows")]
RawWindowHandle::Windows(_) => {
const WINDOWS_EXTS: [*const c_char; 2] = [
khr::Surface::name().as_ptr(),
khr::Win32Surface::name().as_ptr(),
];
&WINDOWS_EXTS
}
RawWindowHandle::Windows(_) => WINDOWS_SURFACE_EXTENSIONS,

#[cfg(any(
target_os = "linux",
Expand Down Expand Up @@ -179,32 +226,14 @@ pub fn enumerate_required_extensions(
&XCB_EXTS
}

#[cfg(any(target_os = "android"))]
RawWindowHandle::Android(_) => {
const ANDROID_EXTS: [*const c_char; 2] = [
khr::Surface::name().as_ptr(),
khr::AndroidSurface::name().as_ptr(),
];
&ANDROID_EXTS
}
#[cfg(target_os = "android")]
RawWindowHandle::Android(_) => ANDROID_SURFACE_EXTENSIONS,

#[cfg(any(target_os = "macos"))]
RawWindowHandle::MacOS(_) => {
const MACOS_EXTS: [*const c_char; 2] = [
khr::Surface::name().as_ptr(),
ext::MetalSurface::name().as_ptr(),
];
&MACOS_EXTS
}
#[cfg(target_os = "macos")]
RawWindowHandle::MacOS(_) => METAL_SURFACE_EXTENSIONS,

#[cfg(any(target_os = "ios"))]
RawWindowHandle::IOS(_) => {
const IOS_EXTS: [*const c_char; 2] = [
khr::Surface::name().as_ptr(),
ext::MetalSurface::name().as_ptr(),
];
&IOS_EXTS
}
#[cfg(target_os = "ios")]
RawWindowHandle::IOS(_) => IOS_SURFACE_EXTENSIONS,

_ => return Err(vk::Result::ERROR_EXTENSION_NOT_PRESENT),
};
Expand Down