-
Notifications
You must be signed in to change notification settings - Fork 638
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
shader_debugprintf: support new VVL-DEBUG-PRINTF message and fix VVL version check for API selection #1187
base: main
Are you sure you want to change the base?
Conversation
…version check for API selection
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you very much for this PR. I do have some remarks though, mostly related to comment and code structure. I think it's important that people can easily follow understand the changes ;)
@@ -433,23 +433,30 @@ const std::vector<const char *> ShaderDebugPrintf::get_validation_layers() | |||
// This sample overrides the instance creation part of the framework to chain in additional structures | |||
std::unique_ptr<vkb::Instance> ShaderDebugPrintf::create_instance(bool headless) | |||
{ | |||
uint32_t instanceApiVersion; | |||
VK_CHECK(vkEnumerateInstanceVersion(&instanceApiVersion)); | |||
uint32_t layer_property_count; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a bit of an explanation in here as to why this is needed? That would make it easier to follow the code.
|
||
uint32_t instance_extension_count; | ||
VK_CHECK(vkEnumerateInstanceExtensionProperties(nullptr, &instance_extension_count, nullptr)); | ||
std::vector<VkExtensionProperties> available_instance_extensions(instance_extension_count); | ||
VK_CHECK(vkEnumerateInstanceExtensionProperties(nullptr, &instance_extension_count, available_instance_extensions.data())); | ||
|
||
// When VK_EXT_layer_settings is available at runtime, the debugPrintfEXT layer feature is enabled using the standard framework | ||
// For backwards compatibility with SDKs < 1.3.272 without VK_EXT_layer_settings, the remainder of this custom override is required | ||
// For this case set Vulkan API version and return via parent class, otherwise the remainder of this custom override is required | ||
if (std::any_of(available_instance_extensions.begin(), | ||
available_instance_extensions.end(), | ||
[](VkExtensionProperties const &extension) { return strcmp(extension.extensionName, VK_EXT_LAYER_SETTINGS_EXTENSION_NAME) == 0; })) | ||
{ | ||
// debugPrintfEXT layer feature requires Vulkan API 1.1, but use API 1.2 until VVL performance fix is available in SDKs > 1.3.290 | ||
// See VVL issue https://github.com/KhronosGroup/Vulkan-ValidationLayers/issues/7562 for defect and fix information |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue referenced here is closed. Does this comment still apply?
if (std::any_of(available_instance_extensions.begin(), | ||
available_instance_extensions.end(), | ||
[](VkExtensionProperties const &extension) { return strcmp(extension.extensionName, VK_EXT_LAYER_SETTINGS_EXTENSION_NAME) == 0; })) | ||
{ | ||
// debugPrintfEXT layer feature requires Vulkan API 1.1, but use API 1.2 until VVL performance fix is available in SDKs > 1.3.290 | ||
// See VVL issue https://github.com/KhronosGroup/Vulkan-ValidationLayers/issues/7562 for defect and fix information | ||
set_api_version(instanceApiVersion <= VK_MAKE_API_VERSION(0, 1, 3, 290) ? VK_API_VERSION_1_2 : VK_API_VERSION_1_1); | ||
set_api_version(vvl_properties != layer_properties.end() && vvl_properties->specVersion <= VK_MAKE_API_VERSION(0, 1, 3, 290) ? VK_API_VERSION_1_2 : VK_API_VERSION_1_1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A minor nitpick, but this one-liner is very hard to read. Could you make this easier to read by putting it outside of set_api_version and dividing and commenting it?
@@ -480,7 +487,7 @@ std::unique_ptr<vkb::Instance> ShaderDebugPrintf::create_instance(bool headless) | |||
VkApplicationInfo app_info{VK_STRUCTURE_TYPE_APPLICATION_INFO}; | |||
app_info.pApplicationName = "Shader debugprintf"; | |||
app_info.pEngineName = "Vulkan Samples"; | |||
app_info.apiVersion = instanceApiVersion <= VK_MAKE_API_VERSION(0, 1, 3, 290) ? VK_API_VERSION_1_2 : VK_API_VERSION_1_1; | |||
app_info.apiVersion = vvl_properties != layer_properties.end() && vvl_properties->specVersion <= VK_MAKE_API_VERSION(0, 1, 3, 290) ? VK_API_VERSION_1_2 : VK_API_VERSION_1_1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above, IMO that statement is too complex and should not be a one-liner on the right side of the assignment.
Thanks @SaschaWillems for the feedback. I am away on vacation this week, but will make the requested changes when I am back. |
Description
Fixes two issues that arose with Vulkan SDK 1.3.296:
VVL-DEBUG-PRINTF
callback message. Previous SDKs usedWARNING-DEBUG-PRINTF
orUNKNOWN-DEBUG-PRINTF
. Without this fix the debug data is not available in the UI Overlay.Fixes #1184.
Tested on Windows 10, Manjaro Linux, and macOS Ventura using Vulkan SDKs 1.3.290 and 1.3.296.
I hope this is the last time I have to fix this. It seems that VVL changes can easily break this sample.
General Checklist:
Please ensure the following points are checked:
Note: The Samples CI runs a number of checks including:
If this PR contains framework changes:
batch
command line argument to make sure all samples still work properlySample Checklist
If your PR contains a new or modified sample, these further checks must be carried out in addition to the General Checklist: