ash-rs / ash

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

Merge function pointer table struct with high level wrapper struct #876

Closed Friz64 closed 6 months ago

Friz64 commented 6 months ago

-- Using VK_KHR_swapchain as an example:

I think it's pretty confusing to have both ash::extensions::khr::Swapchain and ash::vk::KhrSwapchainFn. Is there a good justification for why the wrapper functions shouldn't be directly defined on the struct that also holds the function pointers?

I originally alluded to this in this comment: https://github.com/ash-rs/ash/pull/734#issuecomment-2001024938.

The Device handle would be a bit awkwardly grouped with the function pointers, but overall it results in an easier to grok API, especially after the changes in #734.

Ralith commented 6 months ago

I'm weakly in favor of this, since very few users will care about the *Fn structs, I've seen them cause confusion/impair discoverability in the wild, and I can't think of a good reason to want a *Fn struct that isn't also satisfied by a unified struct.

One case that might call for a raw function table struct is storing wrapped functions in the implementation of a layer, in which storing the handle is (usually?) going to be redundant to the passed-in handle -- but even there, having access to the ergonomic wrappers is more valuable than saving a few bytes.

Ralith commented 6 months ago

https://github.com/ash-rs/ash/pull/894 unifies the module hierarchies. It does not unify the structs, but since they're now clearly documented and immediately adjacent, confusion from those should be at least lessened. Once it's merged, making the function pointer tables themselves private, if we want that, would be a trivial addition.

Rua commented 6 months ago

I hope the function pointer tables, and everything else in the vk module, remain available for those who just want to use the raw Vulkan API without any additional wrappers (e.g. Vulkano). I think in the past, someone proposed separating out the vk module into its own crate, containing only autogenerated code?

MarijnS95 commented 6 months ago

@Rua the proposed PR that will close this issue does exactly that: it moves the original types around to sit inside the same module, but otherwise keeps their original definition the same. You are still able to use the function pointer loaders without the Instance/Device wrappers.