Impl trait functions directly on EntryCustom/Instance/Device

[Rua]

Depends on #400.

A possible improvement to the code structure could be to create a core module to sit alongside extensions, containing core/v1_0.rs, core/v1_1.rs and core/v1_2.rs. This would better match the way extension code is currently structured. I think it could also be good to re-export Vulkan types from each of these.

[MaikKlein]

Sorry for the late reply.

Moving the core traits into methods seems good, I am not so sure about moving the extensions into core as well. Basically this tries to load every extensions when creating a instance/device. It is also a pretty big breaking change as now all the extension structs are gone + the extension functions have a vendor extension in their name.

I am going to request a few reviews to get more opinions.

[Ralith]

I'm in favor of dropping the traits; I've never derived any benefit from them, it's just another thing to import, and empirically a recurring source of confusion for new users.

I'm ambivalent about folding the extensions in as well. It's a bit more convenient, but also a bit more error-prone. For extensions that I don't require (e.g. debug stuff) I quite like having an Option<Extension> that makes it very easy to ensure I don't forget to handle both cases somewhere. At minimum, we shouldn't conflate that change with the removal of traits; @Rua, if you're strongly interested in advocating for it, please split it into a separate PR.

[MarijnS95]

Dropping the traits is fine by me - we're only using 1_2 and as @Ralith says it's just an extra thing to import, ash::Device is already a fat struct holding on to them all. If this struct could be more lightweight for embedded purposes we might want to revisit that, but that's more likely a case of some macro magic to define and load just the functions one needs anyway.

Moving the extensions in there should definitely be done in a separate PR, if only for cleaner history and easier review/revertability - and it's a significant breaking change that should be considered carefully while allowing "normal" PRs (introducing new features) to be released first.
This struct is already pretty large (we're holding it by value in quite a few locations in our code, something that needs to be refactored on our end) and this only makes it bigger. I don't like the idea of Option<> as that means we'd have to check whether the extension is available every time it is used through the device, instead of just once at startup.
Not that that really means anything currently since any function that fails to load is substituted with a panicking stub - I'd rather have more explicit control or at least feedback over what did and did not load - but that's also for another issue/PR.
(ie. with extensions you're AFAIK supposed to get none or all of the functions, to catch errors up-front in a Rusty way through Result without panicking).

[MaikKlein]

e. with extensions you're AFAIK supposed to get none or all of the functions, to catch errors up-front in a Rusty way through Result without panicking).

That is not fully true IIRC. Some functions from extensions might require other extensions to be enabled. So it is possible for some extension function pointers to not load.

[MarijnS95]

That is not fully true IIRC. Some functions from extensions might require other extensions to be enabled. So it is possible for some extension function pointers to not load.

Right that's true, we can't have a single Result catch-all for extensions. The XML seems to have requires= declarations for extensions as a whole but not individual functions though? Not sure how we should implement more strict function loading up-front then. Perhaps just bail if all functions are nullpointers?

[Ralith]

That is not fully true IIRC. Some functions from extensions might require other extensions to be enabled. So it is possible for some extension function pointers to not load.

Can you give a specific example of an extension that is valid without another extension loaded, but which defines additional functions only if it's present?

[Rua]

Did I misinterpret MaikKlein's reply in #408 (comment)? I thought that was giving me the go-ahead for this PR as a whole.

Anyway, I reduced this PR to only the removal of the traits. Should I implement the suggestion I originally made, to create a core subdirectory with modules for each version?

[MaikKlein]

Did I misinterpret MaikKlein's reply in #408 (comment)? I thought that was giving me the go-ahead for this PR as a whole.

Sorry I should have been clearer.

Thank you for this PR, a much needed simplification <3. @MarijnS95 can you have a quick peek?

[MarijnS95]

@MaikKlein I'll queue this up for tomorrow-morning, together with some other PRs. Will also check how it fares on our existing usecases.

[MarijnS95]

Apologies, it took some time (a full work-week, to be precise) to get to this.

Overall this is a net-win for our codebase. All we had to do was remove the trait imports and bounds, it's actually great we're now acknowledging that all (core) function pointers were stored in ash::Device anyway, and there wasn't any extra compile/runtime checking to ensure that the (mandatory) functions could be loaded in the first place.

All in all the only thing that disappears with this PR is static visibility in what core functions are actually used by a code-base. That might be a big deal. @MaikKlein Do you perhaps remember why traits were used in the first place?

@Rua You should probably remove Fixes #408 as this only removes trait bounds but does not take care of the main issue described there: unconditionally dumping all extension function pointers in ash::Device.

[MarijnS95]

@Rua I do think that you need to maintain the 1.2 - 1.1 - 1.0 ordering in device.rs like you did in instance.rs. That way the diffstat is more bearable, and the resulting diff is much easier to read and git blame is still useful.

Also, conflict resolution with master seems to have gone wrong, we replaced all redundant type names with Self and fortunately enabled the clippy::use_self warning for the entire extensions module.

[Rua]

Ok, done. cargo test didn't run clippy for me, so maybe that's something that could be changed in Ash's configuration. Or is there another command I should be running instead?

[MarijnS95]

@Rua cargo test runs application-defined tests, it doesn't perform a linter (nor formatting) pass as far as I'm aware - and there's no way to configure that either.

[MarijnS95]

Thanks! Given that all maintainers agreed this is the way forward, and the changes look clean/small enough (now after reordering the functions again), I've just merged this to unblock some other PRs 🥳

[MarijnS95]

Looking at this again, it seems Ash-Molten cannot easily override static_fn in EntryCustom anymore: https://github.com/EmbarkStudios/ash-molten/blob/main/src/lib.rs

However, there's quite a bit of duplicated code as well and can easily get away with both problems by returning a pointer to the static vkGetInstanceProcAddr from a loader closure, problem solved: EmbarkStudios/ash-molten#56