ash-window: Add `get_present_support()` helper

[Rob2309]

Introduce get_present_support() to ash-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.

[MarijnS95]

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 Display connection? I'm not sure how much of a difference it makes beyond allowing the Vulkan driver to connect to the WSI though (perhaps for Xorg Display handles that could be networked?).

[Rob2309]

I think I addressed everything above in this new commit. I added x11 and x11-dl as optional dependencies, x11-dl being enabled by default. There does not seem to be a xcb-dl lib so I added xcb as a mandatory dependency for now. The xlib code path is working on my wsl2 installation, as for xcb I am not sure, winit seems to not actually support xcb as far as I can tell. I also changed the parameters of get_physical_device_xcb_presentation_support and get_physical_device_wayland_presentation_support to be pointers instead of references.

[Rob2309]

I just noticed that --no-default-features seems to be ignored for running the winit example, I am not sure if I made a mistake or this is a cargo quirk.

[MarijnS95]

I just noticed that --no-default-features seems to be ignored for running the winit example, I am not sure if I made a mistake or this is a cargo quirk.

How so? If I run this on your branch, which disables both x11 and x11-dl (which I do not care about when using Wayland), the creates are disables as expected:

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

[Rob2309]

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.

[Rob2309]

I just noticed that --no-default-features seems to be ignored for running the winit example, I am not sure if I made a mistake or this is a cargo quirk.

How so? If I run this on your branch, which disables both x11 and x11-dl (which I do not care about when using Wayland), the creates are disables as expected:

$ 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 just found out that cargo seems to ignore --no-default-features when not explicitly specifying a package. So cargo run --example winit --no-default-features still uses default features, but cargo run -p ash-window --example winit --no-default-features does not... Weird cargo quirk I guess

[MarijnS95]

I just found out that cargo seems to ignore --no-default-features when not explicitly specifying a package. So cargo run --example winit --no-default-features still uses default features, but cargo run -p ash-window --example winit --no-default-features does not... Weird cargo quirk I guess

I didn't expect cargo run --example to collect all examples from all members in the workspace automatically. TIL :)

EDIT: But that sounds buggy nonetheless.

[MarijnS95]

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 get_ prefixes; can we come up with a better name? has_present_support(for_display_handle)() perhaps?

[Rob2309]

And on that note, Rust naming says to not use get_ prefixes; can we come up with a better name? has_present_support(for_display_handle)() perhaps?

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

[MarijnS95]

winit seems to not actually support xcb as far as I can tell.

I just ran into https://github.com/rust-windowing/winit/issues/3198

[Rob2309]

I added a simple loop to the example that prints info about every physical device.

[Rob2309]

winit seems to not actually support xcb as far as I can tell.

I just ran into rust-windowing/winit#3198

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

[Rob2309]

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

[MarijnS95]

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

No need to, it's supposed to a simple winit example. We can wait for them to implement xcb.

[Rob2309]

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

No need to, it's supposed to a simple winit example. We can wait for them to implement xcb.

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.

[MarijnS95]

Hmm you have already written it, but it also adds some extra maintenance burden to keep it up-to-date and functional.

[Rob2309]

Looks like I forgot to submit this outstanding review. Don't forget to address https://github.com/ash-rs/ash/pull/774/files#r1378560481 :)

I can add this to the changelog, I was just hesitant because of the separate PR point.

[Rob2309]

The forced dev-dependency is now removed, it seems to just work with the regular optional dependency. I also added entries to both the ash-window and ash changelog reflecting the changes here. Is there any special marking that the breaking changes should have?

[Rob2309]

Is there anything left to do here? I must admit I am not quite sure about the status of this PR anymore...

[Ralith]

Triage: This is still desirable, and is nearly ready. I've gone through and resolved threads which have been addressed.

Remaining concerns:

• enabling xcb by default on Unix platforms may be a poor experience, especially on e.g. macos. Also possibly not desired in Wayland-only environments.
• Needs a rebase.