ash-window: Add get_present_support() helper
#774
Conversation
ash-window/src/lib.rs
Outdated
| physical_device, | ||
| queue_family_index, | ||
| &mut *h.connection, | ||
| h.screen as _, |
There was a problem hiding this comment.
This is supposed to be a visual, not a screen.
ash-window/src/lib.rs
Outdated
| physical_device, | ||
| queue_family_index, | ||
| h.display, | ||
| h.screen as _, |
There was a problem hiding this comment.
You're right, I think a visualID can be somehow derived from the screen, but I think this would require linking to some X library. As an alternative, the get_present_support function could require a window handle, but this would defeat the main use case.
There was a problem hiding this comment.
Yeah. We could pull in x11-dl and use it to make the necessary call (XDefaultVisualOfScreen I guess?). Bonus points for adding a cargo feature to use x11 instead, as dynamic linking is a bit tidier than runtime loading on Linux, but it can be nice to avoid a buildtime dep on a foreign lib.
Alternatively, maybe a more convenient approach would be adding a visual_id member to to raw_window_handle::XlibDisplayHandle upstream (and making sure winit populates it). This is the same motivation behind its inclusion in the window handle struct, though I'm not enough of an X11 expert to judge how weird that might be.
There was a problem hiding this comment.
Bonus points for adding a cargo feature to use
x11instead, as dynamic linking is a bit tidier than runtime loading on Linux, but it can be nice to avoid a buildtime dep on a foreign lib.
Quickly scrolling through the changes, it seems that this feature is now forcibly requiring either x11 or x11-dl, or a compiler error will occur on all of unix. Same for xcb now being forced on all consumers of ash-window.
The cfg guards on those RawDisplayHandles should be refactored to contain the optional libraries as features, otherwise get_present_support() won't provide support for x11/xcb but the other functions are not affected.
Then, the module documentation and function documentation should be extended to explain these features.
ash-window/src/lib.rs
Outdated
| /// [`VkGetPhysicalDeviceSurfaceSupportKHR`](https://registry.khronos.org/vulkan/specs/1.3-extensions/html/chap34.html#vkGetPhysicalDeviceSurfaceSupportKHR), | ||
| /// which requires having an actual surface available before choosing a physical device. | ||
| /// | ||
| /// For more information see [the vulkan spec on WSI integration](https://registry.khronos.org/vulkan/specs/1.3-extensions/html/chap34.html#_querying_for_wsi_support). |
There was a problem hiding this comment.
| /// For more information see [the vulkan spec on WSI integration](https://registry.khronos.org/vulkan/specs/1.3-extensions/html/chap34.html#_querying_for_wsi_support). | |
| /// For more information see [the Vulkan spec on WSI integration][_querying_for_wsi_support]. | |
| /// | |
| /// [_querying_for_wsi_support]: https://registry.khronos.org/vulkan/specs/1.3-extensions/html/chap34.html#_querying_for_wsi_support |
ash-window/src/lib.rs
Outdated
| h.screen as _, | ||
| )) | ||
| }, | ||
| // All other platforms mentioned in the vulkan spec are not supported by ash. |
There was a problem hiding this comment.
| // All other platforms mentioned in the vulkan spec are not supported by ash. | |
| // All other platforms mentioned in the Vulkan spec are not supported by ash-window. |
Because ash maps the entire Vulkan API (== supports everything), even though we might not have written a slightly-higher-level Fn wrapper for it yet the lower-level API is still available.
Though I would reword that to ... don't currently have an implementation in ash-window, as we can always expand.
MarijnS95
changed the title
get_present_support() helper
|
While reviewing and refactoring the PR message, should we also mention (in the PR/commit message and the doc-comment) that it checks for support on the given |
|
I think I addressed everything above in this new commit. |
|
I just noticed that |
| @@ -43,7 +43,7 @@ impl WaylandSurface { | |||
| &self, | |||
| physical_device: vk::PhysicalDevice, | |||
| queue_family_index: u32, | |||
| wl_display: &mut vk::wl_display, | |||
| wl_display: *mut vk::wl_display, | |||
There was a problem hiding this comment.
We should write these down as breaking changes in the CHANGELOG (maybe via a separate PR so that it shows up properly in the commit history), though if someone passes exactly a &mut vk::wl_display it'll automatically coerce to the raw pointer.
ash-window/src/lib.rs
Outdated
| /// surface that might be created. This function can be used to find a suitable | ||
| /// [`vk::PhysicalDevice`] and queue family for rendering before a single surface is created. | ||
| /// | ||
| /// This function can be a more useful alternative for [`vkGetPhysicalDeviceSurfaceSupportKHR`], |
There was a problem hiding this comment.
I meant to just have:
| /// This function can be a more useful alternative for [`vkGetPhysicalDeviceSurfaceSupportKHR`], | |
| /// This function can be a more useful alternative for [`khr::Surface::get_physical_device_surface_support()`], |
We don't generally use Vulkan symbol naming in Ash.
ash-window/src/lib.rs
Outdated
| physical_device, | ||
| queue_family_index, | ||
| h.display, | ||
| h.screen as _, |
There was a problem hiding this comment.
Bonus points for adding a cargo feature to use
x11instead, as dynamic linking is a bit tidier than runtime loading on Linux, but it can be nice to avoid a buildtime dep on a foreign lib.
Quickly scrolling through the changes, it seems that this feature is now forcibly requiring either x11 or x11-dl, or a compiler error will occur on all of unix. Same for xcb now being forced on all consumers of ash-window.
The cfg guards on those RawDisplayHandles should be refactored to contain the optional libraries as features, otherwise get_present_support() won't provide support for x11/xcb but the other functions are not affected.
Then, the module documentation and function documentation should be extended to explain these features.
How so? If I run this on your branch, which disables both $ cargo r -p ash-window --example winit --no-default-features
...
error: Exactly one of x11-dl or x11 must be enabled on unix
--> ash-window/src/lib.rs:244:13
|
244 | compile_error!("Exactly one of x11-dl or x11 must be enabled on unix");
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^Though as said above, I don't think we should make this a compile error. |
|
I now made every feature optional, but made x11-dl and xcb the default, so that every platform just works without enabling any features manually. A possible problem I see is that when x11/x11-dl is not enabled, the function will just return a generic EXTENSION_NOT_PRESENT error, which might be confusing. The same goes for xcb. |
I just found out that cargo seems to ignore |
I didn't expect EDIT: But that sounds buggy nonetheless. |
|
Cool stuff! Can we extend the example to showcase this API? I hacked in this chunk to see the type of window handle and print the result, but we should probably come up with something a bit nicer. dbg!(window.raw_display_handle());
let &pdev = instance.enumerate_physical_devices()?.first().unwrap();
let support = ash_window::get_present_support(
&entry,
&instance,
pdev,
0,
window.raw_display_handle(),
);
dbg!(support);And on that note, Rust naming says to not use |
I would probably prefer keeping the get_prefix, as all the platform specific functions used also start with a get prefix. Ash itself also leaves the get_ prefix in place for many functions. But that is just my opinion |
I just ran into rust-windowing/winit#3198 |
|
I added a simple loop to the example that prints info about every physical device. |
Since I can't get winit to use xcb, I might test the xcb code by creating a window with the xcb crate directly. Might take some time to get it right though... |
|
I now added a basic xcb example, the xcb code seems to be working just fine. Not sure if the example should be kept though... |
No need to, it's supposed to a simple |
In that case, should I remove the example again? I used it to verify that the xcb code works, but I don't think it adds any value that the winit example does not. |
|
Hmm you have already written it, but it also adds some extra maintenance burden to keep it up-to-date and functional. |
| xcb = { version = "1.2.2" } | ||
|
|
||
| [features] | ||
| default = ["x11-dl", "xcb"] |
There was a problem hiding this comment.
Requiring xcb by default means we need libxcb present at buildtime, right? While probably most environments will have this, disabling it will make it easier for downstream users to get CI working in minimal VMs and such.
There was a problem hiding this comment.
The reason for defaulting this is to have the least surprising behavior by default. I dont think the default should be to not support a very broadly used platform.
There was a problem hiding this comment.
We know winit doesn't use xcb. Where in the raw-window-handle ecosystem is xcb broadly used?
There was a problem hiding this comment.
Good point, however if winit at some point decides to switch to xcb, it should be defaulted again...
There was a problem hiding this comment.
I think winit will want to avoid adding foreign build-time dependencies for the same reason.
There was a problem hiding this comment.
Looks like I forgot to submit this outstanding review. Don't forget to address https://github.com/ash-rs/ash/pull/774/files#r1378560481 :)
| for dev in devices { | ||
| let props = instance.get_physical_device_properties(dev); | ||
| let queue_families = instance.get_physical_device_queue_family_properties(dev); | ||
| let dev_name = CStr::from_ptr(props.device_name.as_ptr()).to_str().unwrap(); |
There was a problem hiding this comment.
Note to self: I should replace this in #746.
ash-window/Cargo.toml
Outdated
|
|
||
| [[example]] | ||
| name = "xcb" | ||
| required-features = ["ash/linked"] |
There was a problem hiding this comment.
This could also require the xcb feature, though always enabling it in dev-dependencies might be fine? Not sure yet.
There was a problem hiding this comment.
I am not quite sure whether normal dependencies are available in examples. Will test this and change if possible.
I can add this to the changelog, I was just hesitant because of the separate PR point. |
|
The forced dev-dependency is now removed, it seems to just work with the regular optional dependency. |
|
Is there anything left to do here? I must admit I am not quite sure about the status of this PR anymore... |
Introduce
get_present_support()toash-window, which is a higher-level wrapper around the various platform-specific functions for querying whether a physical device queue family supports presenting to surfaces (see the Vulkan spec).This allows choosing a physical device before creating a surface.