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

Array function wrappers #146

Merged
merged 6 commits into from
Jun 30, 2024
Merged

Array function wrappers #146

merged 6 commits into from
Jun 30, 2024

Conversation

poconn
Copy link
Contributor

@poconn poconn commented Jun 23, 2024

Closes #145

Could probably still use a little cleanup, but I think it's getting there. Some places could be improved using string interpolation instead of calling the render*() functions, but the problem is they have some handling of casing, prefix stripping, etc. that would need to be duplicated or refactored in order to get the formatted names as a string. Shouldn't be too bad but not sure whether it's worth cluttering up the existing code for this, let me know if you want me to try it that way.

TODO:

  • fill in full list of functions
  • add proxy functions
  • update examples

Output (mostly unchanged from the issue):

pub const EnumerateInstanceExtensionPropertiesAllocError =
    EnumerateInstanceExtensionPropertiesError || Allocator.Error;
pub fn enumerateInstanceExtensionPropertiesAlloc(
    self: Self,
    p_layer_name: ?[*:0]const u8,
    allocator: Allocator,
) EnumerateInstanceExtensionPropertiesAllocError![]ExtensionProperties {
    var count: u32 = undefined;
    var data: []ExtensionProperties = &.{};
    errdefer allocator.free(data);
    var result = Result.incomplete;
    while (result == .incomplete) {
        _ = try self.enumerateInstanceExtensionProperties(p_layer_name, &count, null);
        data = try allocator.realloc(data, count);
        result = try self.enumerateInstanceExtensionProperties(p_layer_name, &count, data.ptr);
    }
    return if (count == data.len) data else allocator.realloc(data, count);
}

pub fn getPhysicalDeviceQueueFamilyPropertiesAlloc(
    self: Self,
    physical_device: PhysicalDevice,
    allocator: Allocator,
) Allocator.Error![]QueueFamilyProperties {
    var count: u32 = undefined;
    self.getPhysicalDeviceQueueFamilyProperties(physical_device, &count, null);
    const data = try allocator.alloc(QueueFamilyProperties, count);
    errdefer allocator.free(data);
    self.getPhysicalDeviceQueueFamilyProperties(physical_device, &count, data.ptr);
    return if (count == data.len) data else allocator.realloc(data, count);
}

@Snektron
Copy link
Owner

Looks pretty good!

Some places could be improved using string interpolation instead of calling the render*() functions, but the problem is they have some handling of casing, prefix stripping, etc. that would need to be duplicated or refactored in order to get the formatted names as a string.

I've been thinking, a nice refactor would be to write a custom formatter that handles some of the "casing" and namespace stripping operations.

Shouldn't be too bad but not sure whether it's worth cluttering up the existing code for this, let me know if you want me to try it that way.

I think that should be a separate effort, code looks good imo :)

@Snektron
Copy link
Owner

Please also add the Alloc versions to the Proxy wrappers

@poconn
Copy link
Contributor Author

poconn commented Jun 30, 2024

The "Alloc" in the name should probably come before the vendor suffix, right? E.g. getPhysicalDeviceSurfaceFormatsAllocKHR() and not ...KHRAlloc()? Need to fix that if so.

@poconn
Copy link
Contributor Author

poconn commented Jun 30, 2024

Turns out there are a lot of these functions - I added all of the core ones I could find and some common extensions, there are a lot more in extensions but most are pretty niche. Hopefully you're still alright with the decision to hardcode the list.

Anyway I went ahead and made the naming change mentioned above. I can revert if you don't like it but then the capitalization will still need a fix as it was formatting as ...KhrAlloc().

Those two things aside, should be just about good to go. There's probably some unnecessary duplicated code which I might have a go at later, along with some misc cleanup, but you can go ahead and merge if you're happy with it as is.

@poconn poconn marked this pull request as ready for review June 30, 2024 20:43
@Snektron
Copy link
Owner

The "Alloc" in the name should probably come before the vendor suffix, right? E.g. getPhysicalDeviceSurfaceFormatsAllocKHR() and not ...KHRAlloc()? Need to fix that if so.

I don't really mind it either way.

Turns out there are a lot of these functions - I added all of the core ones I could find and some common extensions, there are a lot more in extensions but most are pretty niche. Hopefully you're still alright with the decision to hardcode the list.

Ah, I didn't realize there are so many of these either. I guess the main problem is detecting which functions can be wrapped using such an allocator. Maybe it can just be done for all of the functions that can return VK_INCOMPLETE?

Anyway, the changes look fine. I'm fine with merging as is, though autodetecting would be nice. Its up to you if you want to do it.

@poconn
Copy link
Contributor Author

poconn commented Jun 30, 2024

Maybe it can just be done for all of the functions that can return VK_INCOMPLETE?

There's also vkGetPhysicalDeviceQueueFamilyProperties() (and possibly others) which returns void. Looks like Vulkan-Hpp checks whether one param is the array length of another:
https://github.com/KhronosGroup/Vulkan-Hpp/blob/620cf05ac20e43a729910e343e2ce8229810cc1a/VulkanHppGenerator.cpp#L3187
but I'm not sure whether that one check is enough - there's a lot going on in that file and they have a few different cases for these sorts of functions, and there are some other checks elsewhere that might be necessary to filter out false positives.

So seems like getting autodetection right will be pretty involved, and so if you're okay with the hardcoded list then IMO it's worth sticking with that, at least in the short term. Later I can dig into this some more and hopefully post a followup PR at some point.

@Snektron
Copy link
Owner

That seems like a good plan to me!

@Snektron Snektron merged commit 51c5566 into Snektron:master Jun 30, 2024
1 check passed
@poconn poconn deleted the alloc_wrappers branch June 30, 2024 22:03
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

Successfully merging this pull request may close these issues.

Wrappers for Vulkan functions returning an array
2 participants