Vk ext shader object optional layer #780

gpx1000 · 2023-08-17T17:49:17Z

Description

This fixes the remaining issues in the newly accepted/merged VK_EXT_shader_object PR.

…yer, and it's not found, continue to load the application instead of erroring out. Fix the validation layer discovered issues in shader object. The render_pass gets created twice (once in the super class prepare and once after the child class recreates the frame buffer. Destroying the first created renderpass will solve this.). Also the heightmap_texture and terrain_array_texture both get their sampler's default created and then explicitly recreated. So adding the destroy sampler just before recreation solves the second and third validation issue.

…it can be downloaded after the run and applied as a patch.

SaschaWillems

LGTM

asuessenbach

This PR works.
But instead of improving the somewhat hacky usage of VulkanSample::get_validation_layers, I would completely remove that function, and instead introduce some VulkanSample::add_instance_layer (or maybe just VulkanSample::add_layer, as there's nothing like a device layer), similar to VulkanSample::add_instance_extension, and handle it analogously.

.github/workflows/check.yml

asuessenbach · 2023-08-21T09:48:55Z

samples/extensions/shader_object/shader_object.cpp

@@ -230,6 +230,11 @@ void ShaderObject::setup_framebuffer()
 // Create render pass for UI drawing
 void ShaderObject::setup_render_pass()
 {
+	// delete existing render pass
+	if (render_pass != VK_NULL_HANDLE)


Is setup_render_pass called multiple times?
If so, you would need to initialize render_pass with VK_NULL_HANDLE to make this check work in debug mode. If not, you don't need to destroy the render pass here.

Hi @asuessenbach , per discussion today, can you open an issue so we don't lose the context? We're going to merge this one soon w/o capturing this - but want to circle back later

Sorry, I missed the meeting where this was discussed...
Why should this issue not be resolved right here?

The idea is to fix this after this PR has been merged. The PR is kinda urgent, and the line you highlighted is not new to the PR but already present in the base sample.

@SaschaWillems so it seems, you know about the issue to be resolved later. Would you be so kind and file that issue, instead of me guessing what's meant to be done here?

Sorry, I was under the impression that this change was already present beforehand. But as this code is new to this PR you're right to mention this and it indeed needs fixing in this PR.

I did this change because renderpass was being created by the vulkansample in the framework automatically. I can take this change out, but then validation layers correctly identifies a render_pass object not getting released. The rest of the changes are updated to conform with your requests.

That makes sense. We should keep that change then :)

…_shader_object_optional_layer # Conflicts: # framework/core/hpp_instance.h # framework/core/instance.h # framework/hpp_vulkan_sample.cpp # framework/hpp_vulkan_sample.h # framework/vulkan_sample.cpp # samples/extensions/shader_object/README.adoc # third_party/volk # third_party/vulkan

Related functions and variables have been updated to accommodate the change. This ensures consistency and optimizes the performance for layer searches and validations.

asuessenbach · 2024-06-11T12:51:37Z

framework/core/hpp_instance.cpp

@@ -74,6 +74,41 @@ static VKAPI_ATTR VkBool32 VKAPI_CALL debug_callback(VkDebugReportFlagsEXT flags
 }
 #endif

+bool validate_layers(std::unordered_map<const char *, bool> &required,


The required layers here actually are not required (they're allowed to be optional), but requested. Maybe rename the argument?

asuessenbach · 2024-06-11T13:07:27Z

framework/core/hpp_instance.cpp

+			}
+		}
+
+		if (!found)


Maybe we can replace the loop over the availables with this check:
if (std::none_of(available.begin(), available.end(), [&layer](vk::LayerProperties const &lp) { return lp.layerName == layer.first; }))

asuessenbach · 2024-06-11T14:58:20Z

framework/core/hpp_instance.cpp

+	}
+	return true;
+}
+
 bool validate_layers(const std::vector<const char *>        &required,
                     const std::vector<vk::LayerProperties> &available)
 {


In case, you want to change a little more, this version of validate_layers could look like this:

auto requiredButNotAvailableIt = std::find_if(required.begin(), required.end(), [&available](char const *layer) { return std::none_of(available.begin(), available.end(), [&layer](vk::LayerProperties const &lp) { return lp.layerName == layer; }); }); if (requiredButNotAvailableIt != required.end()) { LOGE("Validation Layer {} not found", *requiredButNotAvailableIt); return false; } return true;

asuessenbach · 2024-06-11T15:49:11Z

framework/core/hpp_instance.cpp

@@ -299,6 +335,20 @@ HPPInstance::HPPInstance(const std::string                            &applicati
 		throw std::runtime_error("Required validation layers are missing.");
 	}

+	std::unordered_map<const char *, bool> layers = (std::unordered_map<const char *, bool>) (requested_layers);


No.
Never cast the const away.
If you would handle the requested_layers the same way as the required_extensions (which should be named requested_extensions!), you would not even need the new validate_layers version.
And, by the way, the layers are never used here!

asuessenbach · 2024-06-11T15:52:04Z

framework/core/instance.cpp

@@ -76,6 +76,41 @@ static VKAPI_ATTR VkBool32 VKAPI_CALL debug_callback(VkDebugReportFlagsEXT flags
 }
 #endif

+bool validate_layers(std::unordered_map<const char *, bool> &required,
+                     const std::vector<VkLayerProperties>   &available)


The same comments as for hpp_instance.cpp hold here.

asuessenbach · 2024-06-11T15:55:39Z

samples/extensions/shader_object/README.adoc

@@ -436,13 +436,15 @@ The layer can be shipped with your application, and it will disable itself if a
 The emulation layer can be enabled by adding `VK_LAYER_KHRONOS_shader_object` to `ppEnabledLayerNames` in `VkDeviceCreateInfo`.

 The sample framework already has an existing abstraction normally used for enabling the validation layer.
-This sample repurposes this mechanism to instead load the emulation layer.
+This


Newline here?

Besides that, is that description still valid? We now have some add_layer and get_layers. None of them explicitly mentions validation layers.

asuessenbach · 2024-06-11T15:59:45Z

samples/extensions/shader_object/README.adoc


 [,CPP]
 ----
-const std::vector<const char *> ShaderObject::get_validation_layers()
+const std::unordered_map<const char *, bool> ShaderObject::get_validation_layers()


There's no ShaderObject::get_validation_layers anymore.

Coleman Jonas and others added 9 commits August 15, 2023 10:26

Add VK_EXT_shader_object sample

89d6fe8

Merge remote-tracking branch 'upstream/main'

2b0356d

Merge branch 'main' into VK_EXT_shader_object_optional_layer

53482ab

fix the formatting and copyright breaks.

a86f7d2

get full_screen_exclusive to build.

fb01dcf

clang format locally wants it formatted differently than CI.

33bdc15

if there's an error in clang-format then save off the diff output so …

ad8e78c

…it can be downloaded after the run and applied as a patch.

attempt to fix clang-format

9290b5f

SaschaWillems self-requested a review August 18, 2023 16:16

SaschaWillems previously approved these changes Aug 18, 2023

View reviewed changes

asuessenbach reviewed Aug 21, 2023

View reviewed changes

gpx1000 added 2 commits May 15, 2024 17:55

Refactor validation layers to use vector instead of map

892f289

Related functions and variables have been updated to accommodate the change. This ensures consistency and optimizes the performance for layer searches and validations.

gpx1000 dismissed SaschaWillems’s stale review via 892f289 May 16, 2024 23:46

gpx1000 added 6 commits May 16, 2024 16:48

revert the change in check.yml

48d8b14

fix the copyright and formating and doxygen problems.

e92bdb0

revert vulkan and volk submodule changes.

3b73db8

clang format

d2b132c

update the clang-format

27e35ba

revert full_screen_exclusive.cpp and hello_triangle.cpp

709d7de

SaschaWillems approved these changes May 28, 2024

View reviewed changes

asuessenbach reviewed Jun 11, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Vk ext shader object optional layer #780

Vk ext shader object optional layer #780

gpx1000 commented Aug 17, 2023

SaschaWillems left a comment

asuessenbach left a comment

asuessenbach Aug 21, 2023

marty-johnson59 Sep 25, 2023

asuessenbach Sep 26, 2023

SaschaWillems Oct 5, 2023

asuessenbach Oct 5, 2023

SaschaWillems Oct 5, 2023

gpx1000 May 16, 2024

SaschaWillems May 17, 2024

asuessenbach Jun 11, 2024

asuessenbach Jun 11, 2024

asuessenbach Jun 11, 2024

asuessenbach Jun 11, 2024 •

edited

asuessenbach Jun 11, 2024

asuessenbach Jun 11, 2024 •

edited

asuessenbach Jun 11, 2024

Vk ext shader object optional layer #780

Are you sure you want to change the base?

Vk ext shader object optional layer #780

Conversation

gpx1000 commented Aug 17, 2023

Description

SaschaWillems left a comment

Choose a reason for hiding this comment

asuessenbach left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asuessenbach Jun 11, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asuessenbach Jun 11, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asuessenbach Jun 11, 2024 •

edited

asuessenbach Jun 11, 2024 •

edited