Add platform-specific extension list for creating a surface to ash_window by entropylost · Pull Request #611 · ash-rs/ash

entropylost · 2022-04-11T20:29:43Z

This pull request adds a constant TARGET_EXTENSIONS to ash_window, which allows people to create the Instance before the window by using it instead of enumerate_required_extensions. (see #595)

MarijnS95

Would you mind including the current platform name in the doc comment, so that users know it can vary across compilation targets? ~~Would be bad if they start relying on the type containing ie. an array of size 2 and trivially fail compiling on Linux(es)~~ [EDIT: Yes, better fix that with a static slice as suggested above]. At the same time I don't think we should be using cfgs to hide other options depending on the compile target, we're not doing that for platform-specific extensions either. As such, I'd rather see something like the following:

Make all of these arrays available, under the appropriate platform name. TARGET_EXTENSIONS_WINDOWS, TARGET_EXTENSIONS_UNIX/LINUX, TARGET_EXTENSIONS_ANDROID, etc. Cover it in a doc comment too;
Create a separate TARGET_EXTENSIONS_CURRENT_PLATFORM that sources from the right platform through cfg, documenting that it will obviously vary across targets on compile-time;
Reuse these slices in fn enumerate_required_extensions() because copying them is wasteful and confusing;
Nit: It seems common to write doc comments above attributes (#[cfg]), not below.

entropylost · 2022-04-11T20:50:31Z

@MarijnS95 Points 1, 2, and 4 seem nice, but actually enumerate_required_extensions doesn't give the same return value as TARGET_EXTENSIONS for linux (because of the 3 different windowing managers).

Ralith · 2022-04-11T20:55:09Z

I'm not sure I agree with point 1. What's the use case for individual constants as part of the public API? If there is no specific compelling case, we shouldn't make the API wider than necessary; the current draft is solving a concrete use case as it stands. "TARGET" also already conveys "the compile target," so that naming is odd.

MarijnS95 · 2022-04-11T21:12:34Z

Forgive the copy-paste typo, s/TARGET/WSI or something more relevant. Its main use is reuse in fn enumerate_required_extensions(), instead of "depending" on the fact that both the match statement and the enablement of these consts share the same cfg.

but actually enumerate_required_extensions doesn't give the same return value as TARGET_EXTENSIONS for linux (because of the 3 different windowing managers).

That's an exception to one out of the five supported platforms here, not the rule. That should still allow the remaining 4 platforms to only have their array/slice of required extensions defined once?

entropylost · 2022-04-12T18:51:28Z

@MarijnS95 Alright, I fixed all of your points. It should be good now.

Ralith · 2022-04-12T19:08:52Z

Its main use is reuse in fn enumerate_required_extensions(), instead of "depending" on the fact that both the match statement and the enablement of these consts share the same cfg.

This sounds good to me on its own, but they don't have to be part of the public API for that.

entropylost · 2022-04-12T19:17:26Z

This sounds good to me on its own, but they don't have to be part of the public API for that.

True, but there are some potential cases them be in the public api might be helpful, I don't really care though.

Ralith · 2022-04-12T19:20:18Z

I think we should prefer not to complicate our public API except when it is specifically known to solve a real problem someone's having.

entropylost · 2022-04-12T19:22:11Z

Alright.

MarijnS95

Looks like the doc comments still need some work, and this probably fails to compile on mac/ios now.

MarijnS95 · 2022-04-13T07:54:15Z

    }
 }

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


Suggested change

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

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

MarijnS95 · 2022-04-13T07:54:27Z

+/// 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.


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.

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.)

MarijnS95 · 2022-04-13T07:54:35Z

+    khr::XlibSurface::name().as_ptr(),
+    khr::XcbSurface::name().as_ptr(),
+];
+/// Extensions necessary for creating a surface on android.


Suggested change

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

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

MarijnS95 · 2022-04-13T07:54:59Z

+    khr::Surface::name().as_ptr(),
+    khr::AndroidSurface::name().as_ptr(),
+];
+/// Extensions necessary for creating a surface on macos.


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?

MarijnS95 · 2022-04-13T07:55:19Z

+    khr::Surface::name().as_ptr(),
+    ext::MetalSurface::name().as_ptr(),
+];
+/// Extensions necessary for creating a surface on ios.


Suggested change

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

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

MarijnS95 · 2022-04-13T08:01:44Z

 }

+/// Extensions necessary for creating a surface on windows.
+#[allow(dead_code)]


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

MarijnS95 · 2022-04-13T08:03:08Z

+
+/// 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`])
+pub const TARGET_EXTENSIONS: &'static [*const c_char] = {


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] = {

MarijnS95 · 2022-04-13T08:03:53Z

+/// 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`])


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 👎

MarijnS95 · 2022-04-13T08:14:01Z

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

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


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)

MarijnS95 · 2022-04-13T08:20:28Z

I think we should prefer not to complicate our public API except when it is specifically known to solve a real problem someone's having.

Note though that this is the separate, smaller ash-window support crate; we're not clobbering the main ash crate here.

I had some use-case in mind but I guess I'll have to PR the publicization of these when I actually come up with something and actively need it.

In any case, the allow(dead_code) seems ugly to me, rather repeat the cfg() so that things are not defined when not used. And it's unfortunate that nice doc comments don't show up in the final documentation either, even though they're still relevant.

MarijnS95 · 2023-05-06T16:34:13Z

@iMplode-nZ do you still intend to push this forward after a year? Otherwise I'll redo this with all of my requested changes applied.

entropylost · 2023-05-06T20:06:55Z

Sorry, I forgot about it; I'd appreciate if you did it according to your designs though.

MarijnS95 · 2023-05-06T20:11:59Z

@iMplode-nZ no worries, it happens. I'll see what I can do, and ping @Ralith on some of my review comments to make sure my view/opinion makes sense before making any massive changes, but would like to keep your PR for context and authorship as base.

entropylost added 2 commits April 11, 2022 12:57

Added TARGET_EXTENSIONS to ash-window.

494eb34

Added documentation.

929adc5

Ralith reviewed Apr 11, 2022

View reviewed changes

Comment thread ash-window/src/lib.rs

MarijnS95 requested changes Apr 11, 2022

View reviewed changes

Chhanged from arrays to slices.

31782b1

Updated documentation.

cfb320f

MarijnS95 reviewed Apr 11, 2022

View reviewed changes

Comment thread ash-window/src/lib.rs Outdated

Removed duplicate code and publicized the per-platform extensions.

9cca727

Ralith reviewed Apr 12, 2022

View reviewed changes

Comment thread ash-window/src/lib.rs Outdated

Removed unnecessary &s.

4fdeb03

Made extensions private.

4359a06

MarijnS95 requested changes Apr 13, 2022

View reviewed changes

MarijnS95 mentioned this pull request May 6, 2023

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

Closed

entropylost closed this May 6, 2023

MarijnS95 reopened this May 6, 2023

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

		/// 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.

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

		/// 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`])

Conversation

entropylost commented Apr 11, 2022

Uh oh!

Uh oh!

MarijnS95 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

entropylost commented Apr 11, 2022

Uh oh!

Ralith commented Apr 11, 2022

Uh oh!

MarijnS95 commented Apr 11, 2022

Uh oh!

Uh oh!

entropylost commented Apr 12, 2022

Uh oh!

Ralith commented Apr 12, 2022

Uh oh!

Uh oh!

entropylost commented Apr 12, 2022

Uh oh!

Ralith commented Apr 12, 2022

Uh oh!

entropylost commented Apr 12, 2022

Uh oh!

MarijnS95 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MarijnS95 commented Apr 13, 2022

Uh oh!

MarijnS95 commented May 6, 2023

Uh oh!

entropylost commented May 6, 2023

Uh oh!

MarijnS95 commented May 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

MarijnS95 left a comment •

edited

Loading

MarijnS95 commented May 6, 2023 •

edited

Loading