Adding support for Vulkan Functions to be loaded by user

bombomby / optick

C++ Profiler For Games

https://optick.dev

MIT License

2.92k stars 293 forks source link

Adding support for Vulkan Functions to be loaded by user #109

Closed Honeybunch closed 4 years ago

Honeybunch commented 4 years ago

After posting this issue: https://github.com/bombomby/optick/issues/108 I figured I'd dig in and at least see what would be needed to support vulkan meta-loaders. This is the solution I've whipped up so far. I've tested this in a personal engine and it seems to work great.

Honeybunch commented 4 years ago

Looks like some checks failed. I will work on that :)

Honeybunch commented 4 years ago

Code has changed a bit since I first opened the review. Glad I managed to fix the builds in CI. Though I'm not so happy about requiring vulkan.h to be included before optick.h. I don't like adding an include order dependency to anything but I can't think of a better way to be able to get access to the Vulkan function pointers. I would forward declare them but I don't think I can forward declare functions with unknown types (VkResult for example). Hopefully this is still okay.

bombomby commented 4 years ago

Hi Arsen, thanks for the pull request.

Though I'm not so happy about requiring vulkan.h to be included before optick.h. I don't like adding an include order dependency to anything but I can't think of a better way to be able to get access to the Vulkan function pointers. I would forward declare them but I don't think I can forward declare functions with unknown types (VkResult for example).

You should be able to replace VkResult with int32_t in forward declarations. Including vulkan.h from optick.h is a really bad idea as it will have a noticeable impact on the compilation time for big projects. I can see that you've already tried this approach in https://github.com/bombomby/optick/pull/109/commits/ef25e91739a6d0731f840b7204dd2feb863ae707. What was the problem with this approach?

Honeybunch commented 4 years ago

Thanks for the feedback! My preliminary efforts to forward declare the function pointers were thwarted by not being able to replace certain enums with compatible types. Turns out you can forward declare enums in C++ with enum VkResult; or if you want you can supply a specific size/backing type with enum VkResult : int32_t; TIL!

I'm much happier with this PR with that resolved :)

Honeybunch commented 4 years ago

Well it looks like enum VkResult; only works on MSVC and enum VkResult : int32_t will only work if the later declaration of VkResult ALSO uses the same explicit backing type. What a pain. The current solution works but does mean that including optick.h before vulkan.h will cause a problem

Honeybunch commented 4 years ago

The only way to avoid this would be to avoid trying to forward declare the enums and instead require users to cast their function pointers to the "forward declared" versions

Honeybunch commented 4 years ago

So, with forward declared enums off the table as an option. What would be your preferred solution to this?

The current manual enums that force an include order dependency
Replacing enums with int types and requiring users to manually cast function pointers
Something else that I'm missing?

bombomby commented 4 years ago

So, with forward declared enums off the table as an option. What would be your preferred solution to this?

I would go for option 2 for now: Replacing enums with int types and requiring users to manually cast function pointers. optick.h is usually included in many headers, so keeping it small and with minimum dependencies is the key.