Open Neo-Zhixing opened 5 months ago
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?
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.
TaggedStructure
was only referenced in utility macrosmatch_out_struct
andmatch_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 mod
ules, 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 const
s and type
s) which makes it explicitly not object-safe and disallows creating trait objects (Box<dyn DeviceExtension>
) from it.
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.
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>>
I will keep this PR open for now. If we end up deciding to adopt #913 we can close this.
Create new marker traits
InstanceExtension
andDeviceExtension
as defined below: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
andExtends*
traits, and the changes proposed in this PR are similar in spirit.cc #734