ash-window: Add get_present_support() helper
#774
Conversation
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 |
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... |
|
Triage: This is still desirable, and is nearly ready. I've gone through and resolved threads which have been addressed. Remaining concerns:
|
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.