Allow building Entry/Instance/Device from handle+fns

[pac85]

Adds a constructor to build a device from a handle + functions.
Helps with interoperability with other vulkan wrappers where the user might have loaded the functions already.

[Ralith]

Another wrapper will have its own function pointer table structs, and converting those to ash's would be incredibly tedious and fragile. Why not just let ash re-load the function pointers as usual?

[pac85]

Another wrapper will have its own function pointer table structs, and converting those to ash's would be incredibly tedious and fragile. Why not just let ash re-load the function pointers as usual?

Vulkano uses ash under the hood and therefore the same function pointer table structs. I don't know if there are other examples. Loading twice would obviously work but doesn't seem like it should be the only choice to me.

[MarijnS95]

Then Vulkano has ash::Device already, or should start using it and take out the function pointers via fp_v1_x functions on ash::Device? Otherwise, is there an upstream issue for Vulkano why they are owning the function-pointer tables themselves, and why this change on the ash side is needed to accommodate for something?

[pac85]

Then Vulkano has ash::Device already, or should start using it and take out the function pointers via fp_v1_x functions on ash::Device? Otherwise, is there an upstream issue for Vulkano why they are owning the function-pointer tables themselves, and why this change on the ash side is needed to accommodate for something?

They don't want to use ash::Device because they don't like that it takes slices from what I understand.
I don't think that's something that is going to change.

EDIT: I misunderstood what you meant, so you say they should use ash::Device and then use the function pointers from it. I suppose this is feasible. But either way I was just providing an use case. I think adding this constructor just makes ash more versatile.

[pac85]

Then Vulkano has ash::Device already, or should start using it and take out the function pointers via fp_v1_x functions on ash::Device? Otherwise, is there an upstream issue for Vulkano why they are owning the function-pointer tables themselves, and why this change on the ash side is needed to accommodate for something?

Also the request for this functionality is not coming from them, it's coming from me, I need to use both crates because vulkano is removing some unsafe wrappers which I use.

Thinking this is something one might generally be interested in doing and also something that doesn't add maintenance burden I opened this PR.

[pac85]

To put it another way, it seems like it makes sense that loading the driver shouldn't be the only way of using the wrappers.

[Ralith]

They don't want to use ash::Device because they don't like that it takes slices from what I understand.

Not liking the high level wrappers does not preclude using Device to load and represent the function pointer tables.

Loading twice would obviously work but doesn't seem like it should be the only choice to me.

It has significant upsides. For example, it does not force you to use the same version of ash as Vulkano.

That said, so long as we expose DeviceFn*::load, it makes sense to allow someone who uses those interfaces to put them together into a Device, so I'm okay with adding this API.

[Ralith]

Thoughts:

• Entry::from_static_fn already exists and is spiritually similar
• If we're offering this for Device, we should also add it for Instance.

[pac85]

Thoughts:

* [`Entry::from_static_fn`](https://docs.rs/ash/0.37.2+1.3.238/ash/struct.Entry.html#method.from_static_fn) already exists and is spiritually similar

* If we're offering this for `Device`, we should also add it for `Instance`.

As suggested I added an analogous constructor to Instance

[Ralith]

Hmm, one other consideration: these might force semver breakage when we support new minor versions of Vulkan. Do we otherwise necessarily break on such updates? If not, that makes this more costly. We could mitigate that by having a separate constructor for every minor version, in which case this should probably be renamed, though that feels rather ugly.

[pac85]

Hmm, one other consideration: these might force semver breakage when we support new minor versions of Vulkan. Do we otherwise necessarily break on such updates? If not, that makes this more costly. We could mitigate that by having a separate constructor for every minor version, in which case this should probably be renamed, though that feels rather ugly.

That's a good point, so would it be sufficient if I appended _1_3 to the current function? Or maybe, even though there is no real need to do so, should I add previous versions as well? (I suppose for consistency?)

[Ralith]

would it be sufficient if I appended _1_3 to the current function?

I think that solves the problem. I'm interested in @MarijnS95's thoughts on this too.

Or maybe, even though there is no real need to do so, should I add previous versions as well?

Let's not; we can always add them in the future. It could confuse users targeting earlier Vulkan versions, but it's an obscure enough interface that I'm not too bothered.

[MarijnS95]

Vulkano doesn't use ash for external interoperability but (at least initially) to not have to manually generate and keep up with low-level sys-like bindings and the function tables.

I was under the impression that Vulkano might not expose their internals nor ash types to more strictly guarantee the extra safety that they aim to provide around Vulkan by utilizing Rust semantics. But it seems to give you access to these (all function pointers including extensions) after all.

Perhaps we could ask @Rua to replace ash::vk::Device with ash::Device after all if this is intended.

---

Sure, we could provide this helper to be more lenient in how ash high-level wrappers are put together and used. After all, a user could pass a custom loader function with a massive match table that forwards already-loaded pointers to achieve the same, which is only cumbersome.

If you want to be consistent and provide it for Device and Instance, we should also provide it for Entry. Entry::from_static_fn() likewise only takes a loader callback which is used to load a few enumeration functions in vk::EntryFnV1_0 and vk::EntryFnV1_0.

---

Then the semver-breakage: this is tricky. We cannot easily version the function as all fields must be initialized (they are not Option<>al nor have a Default). The only thing we can do is:

1. Add newer (versioned) from_parts constructors (or version them from the get-go), marking any non-latest fn as #[deprecated] which is not breaking;
2. Initialize every newer version that isn't passed as argument (i.e. from_parts_v1_3 for a hypothetical new vk::DeviceFnV1_4) with a no-op loader function that returns NULL for every function.

Alternatively we can preemptively make this function return an Option/Result and utilize that, which would be an API break on newer versions but not an ABI break. The former is more likable though.

---

@Ralith what's your opinion on the from_parts naming?

[Ralith]

I think from_parts is an idiomatic name for this.

The only thing we can do is ... version them from the get-go

That's what I had in mind, though I don't know if it's useful to deprecate older ones, since they'll continue to be reasonable to use when targeting older versions. Deprecating does let us remove them when we break for other reasons, which might be nice to limit growth, though I'm not sure if Vulkan is going to get that many minor versions, and we can always deprecate in the future.

[pac85]

Added a 1_3 postfix

[MarijnS95]

That's what I had in mind, though I don't know if it's useful to deprecate older ones, since they'll continue to be reasonable to use when targeting older versions. Deprecating does let us remove them when we break for other reasons, which might be nice to limit growth, though I'm not sure if Vulkan is going to get that many minor versions, and we can always deprecate in the future.

I'd #[deprecate] them to point out that certain functions are never going to be initialized and will always panic, even if it is now clear from the function name and arguments. It's a discussion point we have globally anyway for the current loader functions being infallible, even if it might be helpful to make them infallible to have a clearer overview of what is available (on a side-note: this is problematic when functions within the same extension have different requirements: #635).

[MarijnS95]

Thanks @pac85. Looks like a few things remain before this is ready:

• Add the function to Entry as well (suggested above);
• List these additions in the changelog (copy-paste the title, with (#748) suffix) at the bottom of the ### Added list;
• Mark these functions as #[inline].

[pac85]

Thanks @pac85. Looks like a few things remain before this is ready:

* [ ]  Add the function to `Entry` as well (suggested above);

* [ ]  List these additions in the changelog (copy-paste the title, with `(#748)` suffix) at the bottom of the `### Added` list;

* [ ]  Mark these functions as `#[inline]`.

Np!
Those things should now be done and I've also rebased.
Let me know if there is something else I need to do or if you don't like the commit messages.

[MarijnS95]

Looks good, thanks! I've been wondering if it makes sense to let e.g. Entry::from_static_fn() use the new function, but other than that I'll merge and backport this shortly.

[MarijnS95]

Let me know if there is something else I need to do or if you don't like the commit messages.

I generally try to (but not always) stick to a single commit, especially if all the messages would be the same, since it gets squash-merged in the end. Having a separate commit per entry/instance/device doesn't really add value here. But don't worry about squashing them, I can fix that up very easily while applying.

Sorry for the conflicts, we just removed a bunch of fields (with empty structs) in #752 :)

[pac85]

Now constructors use the from_parts frunctions and I've solved the conflicts

[MarijnS95]

Thanks, looks like it still wouldn't compile though, but let me allow the CI workflow to be sure.

[pac85]

Thanks, looks like it still wouldn't compile though, but let me allow the CI workflow to be sure.

compilation fixed now

[Ralith]

Thanks!

[MarijnS95]

Thanks, looks like we're all set!

[pac85]

Fantastic! Thanks everyone!