Skip to content
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

Vk ext shader object optional layer #780

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

gpx1000
Copy link
Collaborator

@gpx1000 gpx1000 commented Aug 17, 2023

Description

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

Coleman Jonas and others added 9 commits August 15, 2023 10:26
…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 SaschaWillems self-requested a review August 18, 2023 16:16
SaschaWillems
SaschaWillems previously approved these changes Aug 18, 2023
Copy link
Collaborator

@SaschaWillems SaschaWillems left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@asuessenbach asuessenbach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
@@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
@@ -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,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
}

if (!found)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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; }))

}
return true;
}

bool validate_layers(const std::vector<const char *> &required,
const std::vector<vk::LayerProperties> &available)
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

@@ -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);
Copy link
Contributor

@asuessenbach asuessenbach Jun 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

@@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same comments as for hpp_instance.cpp hold here.

@@ -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
Copy link
Contributor

@asuessenbach asuessenbach Jun 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no ShaderObject::get_validation_layers anymore.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants