VulkanCore icon indicating copy to clipboard operation
VulkanCore copied to clipboard

CreateInfo structs don't properly use OOP

Open WardBenjamin opened this issue 7 years ago • 2 comments

Every once in a while, one of the CreateInfo structs takes a direct handle (IntPtr or long) instead of being able to pass in the actual object - here's a few examples:

FramebufferCreateInfo.Attachments should be type ImageView[] RenderPassBeginInfo.RenderPass should take RenderPass PipelineShaderStageCreateInfo.Module should take ShaderModule

I'm sure there's more of these scattered throughout the code; this should be a very simple fix but will break the current API.

WardBenjamin avatar Jun 22 '18 02:06 WardBenjamin

Looking through the source, I noticed that these CreateInfo structs do actually take the proper types in their constructors but convert them immediately to handles - so maybe this isn't an issue at all. However, that prevents the creation of these structs via initializer list without resorting to direct access of the Handle by the end library user.

Can we do better?

WardBenjamin avatar Jun 22 '18 02:06 WardBenjamin

You raise a very valid point.

From the top of my head, I think it was the case across all the structs that they never hold references to actual Handle types but the underlying handle values themselves. There are two reasons for this:

  1. By using underlying handle values directly, some structs are now blittable and don't require a Native version to represent their memory layout. Unfortunately, this wouldn't be possible by using the actual reference type. To keep things consistent, all the structs follow the same handle representation pattern.

  2. If the same struct is passed to multiple Vulkan commands, there wouldn't be duplicate copying of data from ImageView[] to long[] as is the case for FramebufferCreateInfo. Regarding the other two samples, RenderPassBeginInfo and PipelineShaderStageCreateInfo, there is no issue with copying to use Handle types directly.


When considering only the second point, for most of the cases, replacing the handle type will just work. It's slightly trickier in other cases like FramebufferCreateInfo, because we need to turn

internal Framebuffer(Device parent, RenderPass renderPass, ref FramebufferCreateInfo createInfo,
    ref AllocationCallbacks? allocator)
{
    Parent = parent;
    RenderPass = renderPass;
    Allocator = allocator;

    fixed (long* attachmentsPtr = createInfo.Attachments)
    {
        createInfo.ToNative(out FramebufferCreateInfo.Native nativeCreateInfo, attachmentsPtr, renderPass);
        long handle;
        Result result = vkCreateFramebuffer(Parent, &nativeCreateInfo, NativeAllocator, &handle);
        VulkanException.ThrowForInvalidResult(result);
        Handle = handle;
    }
}

to this

internal Framebuffer(Device parent, RenderPass renderPass, ref FramebufferCreateInfo createInfo,
    ref AllocationCallbacks? allocator)
{
    Parent = parent;
    RenderPass = renderPass;
    Allocator = allocator;

    int count = createInfo.Attachments?.Length ?? 0;
    long* attachmentsPtr = stackalloc long[count];
    for (int i = 0; i < count; i++)
        attachmentsPtr[i] = createInfo.Attachments[i];

    createInfo.ToNative(out FramebufferCreateInfo.Native nativeCreateInfo, attachmentsPtr, renderPass);
    long handle;
    Result result = vkCreateFramebuffer(Parent, &nativeCreateInfo, NativeAllocator, &handle);
    VulkanException.ThrowForInvalidResult(result);
    Handle = handle;
}

to avoid any allocations.

discosultan avatar Jun 22 '18 18:06 discosultan