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

CreateInfo structs don't properly use OOP #33

Open
WardBenjamin opened this issue Jun 22, 2018 · 2 comments
Open

CreateInfo structs don't properly use OOP #33

WardBenjamin opened this issue Jun 22, 2018 · 2 comments

Comments

@WardBenjamin
Copy link
Contributor

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
Copy link
Contributor Author

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?

@discosultan
Copy link
Owner

discosultan commented Jun 22, 2018

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.

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

No branches or pull requests

2 participants