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 InstanceExtension and DeviceExtension traits #912

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Neo-Zhixing
Copy link
Contributor

@Neo-Zhixing Neo-Zhixing commented Apr 24, 2024

Create new marker traits InstanceExtension and DeviceExtension as defined below:

pub enum PromotionStatus {
    None,
    PromotedToCore(u32),
    PromotedToExtension(&'static std::ffi::CStr),
}
pub trait InstanceExtension: Send + Sync + Sized + Clone + 'static {
    fn new(entry: &crate::Entry, instance: &crate::Instance) -> Self;
    const NAME: &'static std::ffi::CStr;
    const SPEC_VERSION: u32;

    type Fp;
    fn fp(&self) -> &Self::Fp;
    fn instance(&self) -> vk::Instance;
    const PROMOTION_STATUS: PromotionStatus;
}

pub trait DeviceExtension: Send + Sync + Sized + Clone + 'static {
    fn new(instance: &crate::Instance, device: &crate::Device) -> Self;
    const NAME: &'static std::ffi::CStr;
    const SPEC_VERSION: u32;

    type Fp;
    fn fp(&self) -> &Self::Fp;
    fn device(&self) -> vk::Device;
    const PROMOTION_STATUS: PromotionStatus;
}

These traits are automatically implemented for all extension fp structs.

Traits like these make it easier to manage extensions in higher level abstractions. Marker traits like these are not unprecedented: we already have TaggedStructure and Extends* traits, and the changes proposed in this PR are similar in spirit.

cc #734

@Ralith
Copy link
Collaborator

Ralith commented Apr 25, 2024

TaggedStructure and Extends* exist to support functionality within ash, that being match_{in,out}_struct macros and push_next setters respectively. No ash functionality benefits from these traits, so they're not well precedented.

What's the use case for these traits that justifies the added maintenance load and possible build time cost?

@Neo-Zhixing
Copy link
Contributor Author

Neo-Zhixing commented Apr 25, 2024

In my case I manage extensions by storing all of them in a BTreeMap<&'static CStr, Box<dyn DeviceExtension>> indexed by extension names. Users can enable extensions during application startup by calling device.enable_device_extension::<T: DeviceExtension>(). This will check the extension promotion status, construct the extension fp struct if needed, and add it to the BTreeMap. At runtime, the user can retrieve the extension functions by calling device.device_extension<AccelerationStructure>.xxxxxx() which will panic if the required extension wasn't enabled.

This proposal requires minimal changes to the generator and it will not add any additional maintenance burden or build time cost. We're not creating new impls; we're just converting existing inherent impls into trait impls.

impl Device {
  fn new(instance: &crate::Instance, device: &crate::Device) -> Self { .. }
  fn fp(&self) -> &DeviceFn { .. }
  fn device(&self) -> crate::vk::Device { .. }
}

to

impl DeviceExtension for Device {
  fn new(instance: &crate::Instance, device: &crate::Device) -> Self { .. }
  fn fp(&self) -> &DeviceFn { .. }
  fn device(&self) -> crate::vk::Device { .. }
}

For convenience, we also added the extension name, spec version, and promotion status to the trait definitions.

In general, I believe that this change is well justified to be in ash. For a third party library to provide similar functionality, trait implementation will need to be added for each extension alongside their name, spec version and promotion status by hand. Meanwhile, this information is readily available for ash and can be provided to the application with the addition of just 2 new trait definitions.

No ash functionality benefits from these traits, so they're not well precedented.

TaggedStructure was only referenced in utility macros match_out_struct and match_in_struct. Extends* traits exist to support the push_next utility method. Strictly speaking, none of those added new functionality: without TaggedStructure the user will just need to do the matching manually, and without Extends* traits the user will have to ensure that the extension structs they provided do indeed extend the struct in question. They're quality-of-life improvements allowing the user to write safer code and less code.

Yes, #614 added match_*_struct macros, but even without match_*_struct macros, TaggedStructure is still useful by itself. For example, thanks to the TaggedStructure trait, I can easily implement abstractions for managing physical device features.

To summarize, similar to how the TaggedStructure trait enabled the users to create abstractions on structs, DeviceExtension and InstanceExtension offered new functionality by enabling the users to create abstractions on extensions.

@MarijnS95
Copy link
Collaborator

TaggedStructure was only referenced in utility macros match_out_struct and match_in_struct

Not true, these are also referenced inside default() constructors, while also providing users direct access to information from vk.xml.


Note that #734 made a big effort to create per-prefix and per-extension modules, where all associated constants live. In part so that users aren't unnecessarily confused whether khr::swapchain::Device::NAME is something different than khr::swapchain::Instance::NAME. This PR is reverting all that by having the associated constants directly on Instance/Device, and doesn't even go to lengths to make sure PromotionStatus is available on the root module.

I think PromotionStatus is still useful on its own and should have an isolated PR that adds it to the mod root, which is something we can approve and accept swiftly.

Separate from that, we can discuss how to best model such "extension implementation introspection" on Instance and Device without causing all kinds of unnecessary duplication and confusing repetition. Besides, your example won't work as written as the provided trait requires Sized (and has associated consts and types) which makes it explicitly not object-safe and disallows creating trait objects (Box<dyn DeviceExtension>) from it.

@MarijnS95
Copy link
Collaborator

This, in hindsight, makes me wonder if it was ever designed/desired for the *Extends trait to be object-safe and dispatchable. Requiring : Sized on those traits would have also opted us out of dynamic dispatch, just like the existing StructureType thanks to its associated constant. It would have disallowed use-cases like those proposed in #855, though.

@Neo-Zhixing
Copy link
Contributor Author

Not true, these are also referenced inside default() constructors, while also providing users direct access to information from vk.xml.

Ok sure, but same idea here - TaggedStructure traits are not required here to offer this functionality.

This PR is reverting all that by having the associated constants directly on Instance/Device, and doesn't even go to lengths to make sure PromotionStatus is available on the root module.

The key issue here is that we cannot implement traits for modules, so NAME and PROMOTION_STATUS need somewhere else to go other than the module root. PROMOTION_STATUS wouldn't be very useful on the module root, because typing all of that out would be overly verbose:

app.enable_extension(
   ash::khr::acceleration_structure::NAME,
   ash::khr::acceleration_structure::PROMOTION_STATUS,
   ash::khr::acceleration_structure::Device::new,
)

compared to

app.enable_extension::<ash::khr::acceleration_structure::Device>()

In #913 I added a new type, Meta on module root and implemented the trait for the Meta type. I hope you like that approach better.

Besides, your example won't work as written as the provided trait requires Sized (and has associated consts and types) which makes it explicitly not object-safe and disallows creating trait objects (Box) from it.

My bad, should've been BTreeMap<&'static CStr, Box<dyn Any>>

@Neo-Zhixing
Copy link
Contributor Author

I will keep this PR open for now. If we end up deciding to adopt #913 we can close this.

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.

None yet

3 participants