vulkano-rs / vulkano

Safe and rich Rust wrapper around the Vulkan API
Apache License 2.0
4.45k stars 435 forks source link

Handle arrays in autogen #2512

Closed 0xcaff closed 4 months ago

0xcaff commented 4 months ago

Overview

In the vk.xml added in ash 0.38, the newly added VkPhysicalDeviceHostImageCopyPropertiesEXT struct has the following xml:

<type category="struct" name="VkPhysicalDeviceHostImageCopyPropertiesEXT" structextends="VkPhysicalDeviceProperties2">
    <member values="VK_STRUCTURE_TYPE_PHYSICAL_DEVICE_HOST_IMAGE_COPY_PROPERTIES_EXT"><type>VkStructureType</type> <name>sType</name></member>
    <member optional="true"><type>void</type>*                                                        <name>pNext</name></member>
    <member optional="true" limittype="noauto"><type>uint32_t</type>                                  <name>copySrcLayoutCount</name></member>
    <member optional="true" limittype="noauto" len="copySrcLayoutCount"><type>VkImageLayout</type>*   <name>pCopySrcLayouts</name></member>
    <member optional="true" limittype="noauto"><type>uint32_t</type>                                  <name>copyDstLayoutCount</name></member>
    <member optional="true" limittype="noauto" len="copyDstLayoutCount"><type>VkImageLayout</type>*   <name>pCopyDstLayouts</name></member>
    <member optional="true" limittype="noauto"><type>uint8_t</type>                                   <name>optimalTilingLayoutUUID</name>[<enum>VK_UUID_SIZE</enum>]</member>
    <member limittype="bitmask"><type>VkBool32</type>                                                 <name>identicalMemoryTypeRequirements</name></member>
</type>

Currently, the pCopySrcLayouts and pCopyDstLayouts arrays get a type of *mut T (where T is defined by the vulkano_type function). This PR changes the autogen behavior to do the following.

  1. Interpret the *mut T as a &[T] with a length specified by the len attribute.
  2. Omit the field corresponding to the length attribute as its information is available in the array field.
  3. Rename the field containing the array from p_{field_name} to {field_name}

Merge Order

This is safe to merge today as this code adds functionality to handle the new vk.xml. The behavior for the current vk.xml will remain unchanged.

Related PRs:

Reviewing

I've broken up the changes int logical commits which have straightforward diffs to make it a bit easier to review.

Rua commented 4 months ago

I think, the way the trait is implemented now, it's not going to work. It expects the base type to implement FromVulkan on the reference, but all the existing FromVulkan implementations are on the base type directly. So it may be necessary to include the clone call after all. Alternatively, the FromVulkan trait could maybe rewritten to always take references?

0xcaff commented 4 months ago

I was thinking of having a blanket impl for references which implement Copy.

impl <U: FromVulkan<T>, T: Copy> FromVulkan<&T> for U {
    fn from_vulkan(val: &T) -> Option<Self> {
        U::from_vulkan(*val)
    }
}

For types which implement Clone but not Copy, I think its a good idea to explicitly handle the cloning. In the latest vk.xml, the only instance of a member which is an array is ImageLayout, an enum which implements Copy.

I was thinking of putting it in the next PR, given your question I've decided to add it to this one.

Rua commented 4 months ago

Since this will only be dealing with Vulkan/C types, all of them are expected to be Copy anyway.

0xcaff commented 4 months ago

I see, nice!

Anything else you want changed in this PR before it's ready to merge?