search

google / amber

Amber is a multi-API shader test framework
Apache License 2.0
193 stars 65 forks source link

Backwards compatibility in Amber #986

Closed rayanht closed 2 years ago

rayanht commented 2 years ago

It seems like Amber imports features from Vulkan release without first checking if the specific Vulkan release is actually available on the machine. This means that we have no backwards compatibility whenever we introduce new Vulkan features in Amber (for example with this commit).

VkPhysicalDeviceVulkan11Features* vulkan11_ptrs = nullptr;
VkPhysicalDeviceVulkan12Features* vulkan12_ptrs = nullptr;
VkPhysicalDeviceVulkan13Features* vulkan13_ptrs = nullptr;

This will not compile on a machine with Vulkan 1.2 because VkPhysicalDeviceVulkan13Features is unknown.

Could we safely wrap these behaviours in guards that check the installed Vulkan version? Alternatively, we could introduce semantic versioning to make it clear that new release of Amber is not backwards-compatible.

paulthomson commented 2 years ago

It sounds like you mean that it is not possible to build Amber with old Vulkan headers. I think you are correct, but we are OK with this. Amber can be built without a Vulkan driver and without a Vulkan SDK being installed in the system (which is how it is built in the CI). The binary produced should be backwards compatible with all versions of Vulkan. Just pass -DAMBER_USE_LOCAL_VULKAN=1 to your initial cmake command.

rayanht commented 2 years ago

I might be missing something painfully obvious here but wouldn't the linker still fail to find VkPhysicalDeviceVulkan13Features in the case you describe if I provide a Vulkan 1.2 SDK?

paulthomson commented 2 years ago

I believe that VkPhysicalDeviceVulkan13Features should be defined as a typedef struct in header file vulkan_core.h (included via #include "vulkan/vulkan.h", specially third_party/vulkan-headers/include/vulkan/vulkan.h), so the linker should not be involved.