ash-rs / ash

Vulkan bindings for Rust
Apache License 2.0
1.76k stars 186 forks source link

Add method to unborrow all pointers #911

Open Rua opened 2 months ago

Rua commented 2 months ago

When calling Vulkan's functions to query information about the implementation, you often set up a chain of borrows via pNext structures and such, for Vulkan to fill them in. Then, after that is done, you want to be able to examine the information that has been filled in. However, that often gets hampered by the fact that the structures still borrow each other. There doesn't seem to be a way to release this borrow.

For every structure that holds a pointer/has a lifetime parameter, I propose adding a method unborrow_all (or whatever other name fits better). This method will set the lifetime back to 'static, and set all the pointers in the struct to null.

Ralith commented 2 months ago

Can you provide a self-contained example of code that seems to require this? I believe we did evaluate the use of extension structs in output parameters and found that it worked fine.

Rua commented 2 months ago

For example, the SurfacePresentModeCompatibilityEXT struct is set up like this before calling get_physical_device_surface_capabilities2_khr:

capabilities_present_mode_compatibility_vk.insert(
    ash::vk::SurfacePresentModeCompatibilityEXT::default()
        .present_modes(&mut capabilities_present_modes_vk),
)

After the call is complete, I'm trying to pass both capabilities_present_mode_compatibility_vk and capabilities_present_modes_vk to a function:

capabilities = capabilities.with_present_mode_compatibility(
    &capabilities_present_mode_compatibility_vk,
    &capabilities_present_modes_vk,
);

But because one mutably borrows the other, this is not possible. I would like to have a way to release the borrow so that this call is allowed.

Rua commented 2 months ago

Note: setting present_modes to an empty slice doesn't work. Because that method returns the input type with the same lifetime; it does not create a new lifetime based on the input value.

Ralith commented 2 months ago

For example [...]

That's a code fragment, not a self-contained example. I don't know what insert is, and there's no easy way to test changes. Can you provide a complete, self-contained example?

I'm trying to pass both capabilities_present_mode_compatibility_vk and capabilities_present_modes_vk to a function:

Why not pass the root structure and walk the pointer chain in the callee? If you're going to put Vulkan structs in your interface, you might as well interpret them per Vulkan idioms.

You can also erase lifetimes fairly concisely using FRU syntax if you really want to.

mahkoh commented 1 week ago

Here is a not self-contained example:

        let mut modifier_props = DrmFormatModifierPropertiesListEXT::default();
        let mut format_properties = FormatProperties2::default()
            .push_next(&mut modifier_props);
        unsafe {
            self.instance.get_physical_device_format_properties2(
                phy_dev,
                format.vk_format,
                &mut format_properties,
            );
        }
        let modifiers =
            self.load_drm_format(phy_dev, format, &format_properties, &modifier_props)?;

The last line fails because modifier_props is borrowed mutably.

This code worked fine in 0.37 and I have a few errors of this kind that are blocking me from upgrading to 0.38.

Why not pass the root structure and walk the pointer chain in the callee?

Why have type safety when you can instead walk void pointer chains and add error handling code for when the chain does not contain the expected structure lol.

The only serious solution I'm seeing is adding a transmute to 'static.

MarijnS95 commented 1 week ago

@mahkoh your example is is actually a proper demonstration of why the current system - without "unborrow function" - works. FormatProperties2 solely exists to extend FormatProperties with a p_next chain. If, as you say, you're not interested in walking the untyped p_next chain (totally understandable), but are merely interested in DrmFormatModifierPropertiesListEXT and vk::FormatProperties (pointer- and lifetime-less) just pass &format_properties.format_properties and there should be no borrowing issues.

EDIT: @mahkoh after all the only place where you seem to reference fields form vk::FormatProperties2, is to get the format_properties: vk::FormatProperties field:

https://github.com/mahkoh/jay/blob/831906df16fd299b4a840051a3381a547756ff15/src/gfx_apis/vulkan/format.rs#L145-L146


The same applies to @Rua's example of VkSurfacePresentModeCompatibilityEXT: the only fields this struct contains beside a p_next chain describe the mutable slice in capabilities_present_modes_vk that it borrows (pointer + length). @Rua can you clarify what extra info you need from capabilities_present_mode_compatibility_vk, if it is only a Vec of VkSurfacePresentModeCompatibilityEXT?

MarijnS95 commented 1 week ago

Also note, just in case, I am not saying that this isn't a problem and that we shouldn't fix it. I am however attempting to debunk both proposed "example problems" to demonstrate that there are easy and perhaps more sensible ways around it.

I'm working on a rather big Vulkan engine too, and haven't ran into a single case where this was a problem for me either. We need concrete proof that there are multiple structs now suffering from this limitation, before dedicating thousands more lines of generator code to reset all the pointers back to null() - not to mention annoyance to users when - for example - a slice was just the kind of data they need, that now becomes unset (not to forget resetting its count field back to zero, too!).

Again, if @Rua were to unborrow the pointers from VkSurfacePresentModeCompatibilityEXT, the pNext chain would be NULL, the presentModeCount field set to 0, and the pPresentModes field (array-pointer) set to null(), there is no field left besides sType. Sounds like capabilities_present_mode_compatibility_vk() shouldn't be passed to with_present_mode_compatibility() in the first place?

mahkoh commented 1 week ago

If, as you say, you're not interested in walking the untyped p_next chain (totally understandable), but are merely interested in DrmFormatModifierPropertiesListEXT and vk::FormatProperties (pointer- and lifetime-less) just pass &format_properties.format_properties and there should be no borrowing issues.

Indeed it seems that should solve my problem in this case. If FormatProperties2 itself contained a large number of fields that I would have to pass down a call chain, then I would not be happy with this. But luckily that is not the case.

MarijnS95 commented 1 week ago

For this issue we might want to modify the generator a bit to find actual structs that do contain a large number of non-pointer fields (excluding pointer-size fields) versus pointer/pNext fields and decide if this is needed/viable.