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

Add RequiresAllOf, automatically enable required extensions and features #2233

Merged
merged 5 commits into from
Jun 25, 2023

Conversation

Rua
Copy link
Contributor

@Rua Rua commented Jun 18, 2023

Changelog:

### Additions
- When creating an instance or device, you only need to specify the extensions and features you actually care about. Any extensions and features that are required by the extensions that you specified are now automatically enabled too.

This adds the ability to specify multiple requirements that must be enabled together, using RequiresAllOf. This is needed for some parts of the API, such as vkAcquireNextImage2KHR and vkGetPhysicalDevicePresentRectanglesKHR. RequiresOneOf now contains a slice of RequiresAllOf, which in turns contains a slice of Requires enums. In other words, requirements now follow a DNF structure.

Additionally, I added the automatic enabling of required extensions and features. This is purely a convenience for the user, but hopefully one that makes dealing with extensions less frustrating, by letting the user focus on what they actually need.

@marc0246
Copy link
Contributor

Why did you revert #2205?

@Rua
Copy link
Contributor Author

Rua commented Jun 24, 2023

I was confused that Debug seemed to print the same thing as Display, when the norm is for Debug to show the internal structure of the object. What do you think the output should say?

@marc0246
Copy link
Contributor

I think it should work like it did before, with Debug printing a nicely-formatted message. See the PR for an example, and/or try it for yourself. Nothing is saying Debug must be derived.

@Rua
Copy link
Contributor Author

Rua commented Jun 24, 2023

But how should it be formatted, now that there's multiple layers of requirements?

@Rua
Copy link
Contributor Author

Rua commented Jun 24, 2023

Actually looking at the code, I'm even more confused. There is a custom Debug implementation for ValidationError already.

@marc0246
Copy link
Contributor

If you want I can do the formatting, I have a clear picture in mind. There's some more serious issues I think I found here that you should address.

@Rua
Copy link
Contributor Author

Rua commented Jun 24, 2023

Yes please.

@marc0246
Copy link
Contributor

I was trying to debug it but you'll know better than me. If you look at the build output of DeviceExtensions::check_requirements it's all borked.

@Rua
Copy link
Contributor Author

Rua commented Jun 24, 2023

Do you get a compile error? It looks normal on my end:

pub(super) fn check_requirements(
        &self,
        supported: &DeviceExtensions,
        api_version: crate::Version,
        instance_extensions: &crate::instance::InstanceExtensions,
    ) -> Result<(), crate::ValidationError> {
        if self.khr_16bit_storage {
            if !supported.khr_16bit_storage {
                return Err (crate :: ValidationError { problem : "contains `khr_16bit_storage`, but this extension is not supported by the physical device" . into () , .. Default :: default () }) ;
            }
            if !(api_version >= crate::Version::V1_1
                || instance_extensions.khr_get_physical_device_properties2)
            {
                return Err(crate::ValidationError {
                    problem: "contains `khr_16bit_storage`".into(),
                    requires_one_of: crate::RequiresOneOf(&[
                        crate::RequiresAllOf(&[crate::Requires::APIVersion(crate::Version::V1_1)]),
                        crate::RequiresAllOf(&[crate::Requires::InstanceExtension(
                            "khr_get_physical_device_properties2",
                        )]),
                    ]),
                    ..Default::default()
                });
            }
            if !(api_version >= crate::Version::V1_1) {
                return Err(crate::ValidationError {
                    problem: "contains `khr_16bit_storage`".into(),
                    requires_one_of: crate::RequiresOneOf(&[crate::RequiresAllOf(&[
                        crate::Requires::APIVersion(crate::Version::V1_1),
                    ])]),
                    ..Default::default()
                });
            }
        }

etc.

@marc0246
Copy link
Contributor

No there's no compile error, the checks it does are not correct.

@marc0246
Copy link
Contributor

Will you mind if I push the formatting changes directly to your branch? I could also open a PR to your branch but lazy >.>

@Rua
Copy link
Contributor Author

Rua commented Jun 24, 2023

Yes, go ahead.

Are you talking about the device extensions required by other device extensions? Those checks are gone because they're automatically enabled now.

@Rua
Copy link
Contributor Author

Rua commented Jun 24, 2023

Ah never mind, I think I see what's going on.

@Rua
Copy link
Contributor Author

Rua commented Jun 24, 2023

I made a fix. Whenever a device extension was one of the alternatives, since we're always able to enable that extension, we can treat that alternative as true, making the whole test true. So now, the check is only emitted when there is no device extension among the alternatives.

@marc0246
Copy link
Contributor

I added the Debug impl back.

Unwrapping a VulkanError looks like this:

called `Result::unwrap()` on an `Err` value: a validation error occurred

Caused by:
    something: is wrong

Requires one of:
    feature `feat1` + device extension `ext1` + device extension `ext2`
    feature `feat1` + feature `feat2`

Vulkan VUIDs:
    VUID-something-12345

I checked the Display implementation as well, using anyhow, and it looks like this:

a validation error occurred

Caused by:
    something: is wrong -- requires one of: (feature `feat1` + device extension `ext1` + device extension `ext2`) or (feature `feat1` + feature `feat2`) (Vulkan VUIDs: VUID-something-12345)

@Rua
Copy link
Contributor Author

Rua commented Jun 25, 2023

Is there anything else I need to do?

@marc0246
Copy link
Contributor

Do you approve of the commit I added?

@Rua
Copy link
Contributor Author

Rua commented Jun 25, 2023

Yeah, it's fine.

@marc0246
Copy link
Contributor

Alrighty, then it looks to me like this is good to go. Thank you for the work!

@marc0246 marc0246 merged commit 0427482 into vulkano-rs:master Jun 25, 2023
Rua added a commit that referenced this pull request Jun 25, 2023
@Rua Rua deleted the requires-one-of branch October 25, 2023 14:25
hakolao pushed a commit to hakolao/vulkano that referenced this pull request Feb 20, 2024
…tures (vulkano-rs#2233)

* Add `RequiresAllOf`, automatically enable required extensions and features

* Add more missing backticks and backslashes to error messages

* Use updated VUIDs that require `acceleration_structure`

* Fix `check_requirements`

* Cooperation go brr

---------

Co-authored-by: marc0246 <40955683+marc0246@users.noreply.github.com>
hakolao pushed a commit to hakolao/vulkano that referenced this pull request Feb 20, 2024
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.

2 participants