-
Notifications
You must be signed in to change notification settings - Fork 196
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
entry: Allow querying enumerate_instance_extension_properties()
by layer name
#574
Conversation
Gah, I have to figure what this Perhaps we should just update the 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. |
+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. |
cf92d9e
to
6282747
Compare
6282747
to
86deb8c
Compare
The main point being that 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?
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. |
Co-authored-by: Marijn Suijten <marijns95@gmail.com>
Co-authored-by: Marijn Suijten <marijns95@gmail.com>
@pollend I updated the title of this PR (what will show up in the patch) as I have no idea what 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. |
enumerate_instance_extension_properties()
by layer name
There was a problem hiding this 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!
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
To fix it, I had to add let ext_properties = entry
.enumerate_instance_extension_properties(None)
.expect("Can't get extension properties."); Thanks for your hard work! |
I think it would be useful to query by extension name.
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.