Skip to content
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.

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

entry: Allow querying enumerate_instance_extension_properties() by layer name #574

Merged

Conversation

pollend
Copy link
Contributor

@pollend pollend commented Feb 4, 2022

I think it would be useful to query by extension name.

When pLayerName parameter is NULL, only extensions provided by the Vulkan implementation or by implicitly enabled layers are returned. When pLayerName is the name of a layer, the instance extensions provided by that layer are returned.

the extensions not explicitly provided will be listed even if they have not been used. you would query the available extensions and then look at those extensions from a predefined list.

@MarijnS95
Copy link
Collaborator

Gah, I have to figure what this chore means some day... :(

Perhaps we should just update the enumerate_instance_extension_properties API to take an Option<&CStr>? That's a breaking change though.

Also, it looks like something went wrong in the commit title and PR title?

@pollend
Copy link
Contributor Author

pollend commented Feb 4, 2022

Gah, I have to figure what this chore means some day... :(

Perhaps we should just update the enumerate_instance_extension_properties API to take an Option<&CStr>? That's a breaking change though.

Also, it looks like something went wrong in the commit title and PR title?

I've been marking my work using this: https://www.conventionalcommits.org/en/v1.0.0/

I can change the commit when I get back home.

umm yea, I think it make sense to use another method so I don't break the orignal method. too bad you can't just have arguments with defaults.

@Ralith
Copy link
Collaborator

Ralith commented Feb 5, 2022

Perhaps we should just update the enumerate_instance_extension_properties API to take an Option<&CStr>?

+1 to this. It's breaking but not super disruptively so, and I don't think anyone expects ash to be semver-stable for long periods at the moment anyway.

@pollend pollend force-pushed the chore/allow-enumerating-extension-by-layer branch 2 times, most recently from cf92d9e to 6282747 Compare February 5, 2022 19:31
ash/src/entry.rs Outdated Show resolved Hide resolved
@pollend pollend force-pushed the chore/allow-enumerating-extension-by-layer branch from 6282747 to 86deb8c Compare February 6, 2022 01:05
@pollend pollend requested a review from Ralith February 6, 2022 01:28
@MarijnS95
Copy link
Collaborator

Gah, I have to figure what this chore means some day... :(
Perhaps we should just update the enumerate_instance_extension_properties API to take an Option<&CStr>? That's a breaking change though.
Also, it looks like something went wrong in the commit title and PR title?

I've been marking my work using this: https://www.conventionalcommits.org/en/v1.0.0/

The main point being that chore is usually a (mundane) change outside of (public) code: updating version/lock files, changing things in gitattributes/gitmodules, and the like; meta stuff if you wish. This specific prefix doesn't seem documented at that website, they refer to some "Angular" docs where it is not referenced either.

This change is not that. Perhaps it's a fix because we're adding a method that's missing? Perhaps it's a feature because of giving the new "feature" possibility of using it this way?

umm yea, I think it make sense to use another method so I don't break the orignal method. too bad you can't just have arguments with defaults.

Like @Ralith said - and you've already changed nicely - a breaking change is preferable here. I'll have to think about merge order as we don't have anything breaking right now and I'm not fond of making tons of backport branches, but we can probably come up with a reason to make another breaking release soon anyway.

ash/src/entry.rs Outdated Show resolved Hide resolved
ash/src/entry.rs Outdated Show resolved Hide resolved
pollend and others added 3 commits February 6, 2022 11:16
Co-authored-by: Marijn Suijten <marijns95@gmail.com>
Co-authored-by: Marijn Suijten <marijns95@gmail.com>
@MarijnS95 MarijnS95 changed the title chore: allow querying by entry_name by extension name Allow querying instance extension properties by layer name Feb 19, 2022
@MarijnS95
Copy link
Collaborator

@pollend I updated the title of this PR (what will show up in the patch) as I have no idea what entry_name nor "extension name" have to do with these changes. Let me know if this is correct.

Please also add this to the changelog, then we can merge your patch in time for the breaking 0.36 release somewhere in the - hopefully - next 24 hours.

@MarijnS95 MarijnS95 changed the title Allow querying instance extension properties by layer name entry: Allow querying enumerate_instance_extension_properties() by layer name Feb 19, 2022
Copy link
Collaborator

@MarijnS95 MarijnS95 left a comment

Choose a reason for hiding this comment

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

Thanks, we're all set now!

@MarijnS95 MarijnS95 merged commit 40b572f into ash-rs:master Feb 19, 2022
@pollend pollend deleted the chore/allow-enumerating-extension-by-layer branch February 19, 2022 16:47
@Narann
Copy link

Narann commented Feb 23, 2022

Hello,

Thanks for your work on ash!

In order to help everyone with this breaking change (the release note doesn't give information). Here is a code that was working before:

/// Return if Vulkan implementation has given extensions.
pub fn has_extensions(entry: &ash::Entry, ext_names: &Vec<&CStr>) -> bool {
    // We assume all extensions are supported and invalid if one is missing.
    let mut has_all = true;

    let ext_properties = entry
        .enumerate_instance_extension_properties()
        .expect("Can't get extension properties.");

    for &ext_name in ext_names {
        // Find if required extension name is present in available extension names.
        let supported = ext_properties
            .iter()
            .any(|ext| cstr_to_char_array_cmp(ext_name, &ext.extension_name));

        if !supported {
            warn!("Unsupported extension {:?}", ext_name);
            has_all = false;
        }
    }
    has_all
}

Couldn't compile with 0.36:

error[E0061]: this function takes 1 argument but 0 arguments were supplied
   --> src/vk/utils.rs:19:10
    |
19  |         .enumerate_instance_extension_properties()
    |          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^- supplied 0 arguments
    |          |
    |          expected 1 argument
    |
note: associated function defined here
   --> home/.cargo/registry/src/github.com-1ecc6299db9ec823/ash-0.36.0+1.3.206/src/entry.rs:242:12
    |
242 |     pub fn enumerate_instance_extension_properties(
    |            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

To fix it, I had to add None in the optional layer_name:

    let ext_properties = entry
        .enumerate_instance_extension_properties(None)
        .expect("Can't get extension properties.");

Thanks for your hard work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants