Refactor instance creation

[asuessenbach]

Description

This is a suggestion on how the instance creation could be simplified.
The main goal was to move all the application specific logic out of vkb::core::Instance into vkb::VulkanSample and the sample specific code.

This means that the vkb::core::Instance constructor now only receives the data that is to be used for the actual instantiation and checks its validity. As the structures chained into the vk::InstanceCreateInfo via pNext might depend on the actually enabled layers or extensions, it also takes a function to determine those structures.

In vkb::VulkanSample::create_instance, a couple of virtual functions are called (get_name, get_api_version, request_layers, request_instance_extension) or provided (get_instance_create_info_extensions) to actually get the data needed to instantiate that vkb::core::Instance. The default implementations in vkb::VulkanSample fit to the needs of most samples, and some samples override some of them for special needs. Those virtual functions replace some calls previously done in the samples constructor or prepare function.

To summarize, this MR makes more clear when and where data needed for vkb::core::Instance creation is provided by the samples.

Build tested on Win11 with VS2022. Run tested on Win10 with NVidia GPU.

General Checklist:

Please ensure the following points are checked:

• My code follows the coding style
• I have reviewed file licenses
• I have commented any added functions (in line with Doxygen)
• I have commented any code that could be hard to understand
• My changes do not add any new compiler warnings
• My changes do not add any new validation layer errors or warnings
• I have used existing framework/helper functions where possible
• My changes do not add any regressions
• I have tested every sample to ensure everything runs correctly
• This PR describes the scope and expected impact of the changes I am making

Note: The Samples CI runs a number of checks including:

• I have updated the header Copyright to reflect the current year (CI build will fail if Copyright is out of date)
• My changes build on Windows, Linux, macOS and Android. Otherwise I have documented any exceptions

If this PR contains framework changes:

• I did a full batch run using the batch command line argument to make sure all samples still work properly

Sample Checklist

If your PR contains a new or modified sample, these further checks must be carried out in addition to the General Checklist:

• I have tested the sample on at least one compliant Vulkan implementation
• If the sample is vendor-specific, I have tagged it appropriately
• I have stated on what implementation the sample has been tested so that others can test on different implementations and platforms
• Any dependent assets have been merged and published in downstream modules
• For new samples, I have added a paragraph with a summary to the appropriate chapter in the readme of the folder that the sample belongs to e.g. api samples readme
• For new samples, I have added a tutorial README.md file to guide users through what they need to know to implement code using this feature. For example, see conditional_rendering
• For new samples, I have added a link to the Antora navigation so that the sample will be listed at the Vulkan documentation site

[SaschaWillems]

Not sure about this :/

Probably a personal preference, but I prefer the old way over this. This is kinda odd to read (and write) and having instance and device creation getting setup in two very different ways feels somewhat unnatural.

[asuessenbach]

having instance and device creation getting setup in two very different ways feels somewhat unnatural.

Sure. I had planned to adjust the device creation accordingly, if this approach would be approved.

This is kinda odd to read (and write)

Could you elaborate a bit more on this? What is odd?

[SaschaWillems]

As for the odd part, I personally find this:

add_instance_extension(VK_KHR_EXTERNAL_MEMORY_CAPABILITIES_EXTENSION_NAME);
add_instance_extension(VK_KHR_EXTERNAL_SEMAPHORE_CAPABILITIES_EXTENSION_NAME);
add_instance_extension(VK_KHR_EXTERNAL_FENCE_CAPABILITIES_EXTENSION_NAME);

Easier to write, easier to read and easier to remember than this:

void OpenCLInterop::request_instance_extensions(std::unordered_map<std::string, vkb::RequestMode> &requested_extensions) const
{
	ApiVulkanSample::request_instance_extensions(requested_extensions);
	requested_extensions[VK_KHR_EXTERNAL_MEMORY_CAPABILITIES_EXTENSION_NAME]    = vkb::RequestMode::Required;
	requested_extensions[VK_KHR_EXTERNAL_SEMAPHORE_CAPABILITIES_EXTENSION_NAME] = vkb::RequestMode::Required;
	requested_extensions[VK_KHR_EXTERNAL_FENCE_CAPABILITIES_EXTENSION_NAME]     = vkb::RequestMode::Required;
}

Interestingly I've been undoing similar constructs like these in my samples to make them easier to maintain and to follow.

Maybe that's because I'm a mostly pragmatic programmer trying to keep code as simple as possible, esp. when writing code that people should learn from ;)

[asuessenbach]

I personally find it more clear and easier to understand, if there's one function (e.g. request_instance_extensions) called by the framework to resolve one task (here: list all the requested extensions). It's clear what to do and when.

But if it's consensus to keep the add/set functions and to not introduce those virtual functions, I could revert that part of this PR.

[asuessenbach]

Any additional opinion anyone?

[gpx1000]

I've been thinking about this. I'm good with both ways of doing it. I know that's kind of a cop out. Sascha's preference and the current way we do it has the potential problem that it could be called from anywhere at anytime as the "customer" of the API (framework) has control over when those calls are made. In Andreas' version, the Framework has control, it is mandatory how it's called and when in the setup. There's no ambiguity about it.
However, we're developing the framework (API) for maintainers of the Sample, and the goal of the framework is to be a black box. So, while I normally would side with the better architecture approach, Andreas' version, the fact that it's what we already have and we should be able to trust ourselves to consume our framework product about as well as we trust ourselves to write the framework. That's where my ability to go with both versions comes from.

However, if we're telling people that the samples are the best practices that people should try to replicate; then it absolutely is a good idea to demonstrate good architecture design of the API we choose to use ourselves. Thus, I think I'd slightly favor refactoring. Even though that argument does fall flat in the face of there's plenty of other instances that are much more egregious to proper architecture than this lone example and thus, we shouldn't really worry too much about it.

[gpx1000]

I'm fine with these changes as noted in my earlier review. I think this is best practice architecture; however, I'm still okay if we decide to not take this. However, mechanically, this is a PR that is proper to receive a thumb up from me.

[gary-sweet]

This doesn't run for me, batch mode immediately spits out this for example:

[error] 307460652 - VUID-vkCreateDevice-ppEnabledExtensionNames-01387: vkCreateDevice(): pCreateInfo->ppEnabledExtensionNames[5] Missing extension required by the device extension VK_KHR_swapchain: VK_KHR_surface.
The Vulkan spec states: All required device extensions for each extension in the VkDeviceCreateInfo::ppEnabledExtensionNames list must also be present in that list. (https://docs.vulkan.org/spec/latest/chapters/devsandqueues.html#VUID-vkCreateDevice-ppEnabledExtensionNames-01387)
vulkan_samples: Vulkan-Samples/third_party/vulkan/include/vulkan/vulkan_funcs.hpp:9090: vk::ResultValueType<unsigned int>::type vk::PhysicalDevice::getSurfaceSupportKHR(uint32_t, vk::SurfaceKHR, const Dispatch&) const [with Dispatch = vk::detail::DispatchLoaderDynamic; typename std::enable_if<vk::detail::isDispatchLoader<Type>::value, bool>::type <anonymous> = true; vk::ResultValueType<unsigned int>::type = unsigned int; uint32_t = unsigned int]: Assertion `d.vkGetPhysicalDeviceSurfaceSupportKHR && "Function <vkGetPhysicalDeviceSurfaceSupportKHR> requires <VK_KHR_surface>"' failed.
Aborted

[asuessenbach]

Great catch!
You probably don't run the samples with glfw, which already requests VK_KHR_SURFACE_EXTENSION_NAME.

[SaschaWillems]

As discussed on the last call I'm okay with this.

Did a full batch run on Win11 + RTX 4070 with the latest developer driver and saw no crashes or validation errors outside of those already known and not caused by this PR.

[gary-sweet]

Great catch! You probably don't run the samples with glfw, which already requests VK_KHR_SURFACE_EXTENSION_NAME.

Correct, no GLFW on our platform. The samples work fine now though. Thanks

[marty-johnson59]

3 approvals - merging