Allocated refactor

[jherico]

Description

This PR is currently built on top of #907 and #901 and should not be merged until they're approved and merged.

Migrate the code common to the objects using the VMA library to a single location, mostly the new allocated.h header and template types. The four classes, vkb::core::Buffer, vkb::core::Image, vkb::core::HPPBuffer and ``vkb::core::HPPImage` are now all implemented in terms of common base classes.

The new classes prefer the use of a builder pattern for construction, with reasonable defaults for some values set in the image builder types, reducing the cognitive load of using them as well as the complexity of the image constructors. In a future PR the existing ctors will be marked deprecated and all the samples referencing them will be updated to use the builder pattern.

The buffer types have new static methods intended to be used to create short-lived staging buffers containing data. These static methods include convenience signatures to make it easy to upload containers full of data... so for instance, this code

core::Buffer stage_buffer{device,
                          vertex_data.size() * sizeof(Vertex),
                          VK_BUFFER_USAGE_TRANSFER_SRC_BIT,
                          VMA_MEMORY_USAGE_CPU_ONLY};

stage_buffer.update(vertex_data.data(), vertex_data.size() * sizeof(Vertex));

reduces to

core::Buffer stage_buffer = core::Buffer::create_staging_buffer(device, aligned_vertex_data);

Fixes #900

I'm struggling to 100% test the samples as fragment_shading_rate_dynamic currently crashes on my desktop, even on the main branch and the hlsl shader examples both produce validation warnings.

General Checklist:

Please ensure the following points are checked:

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

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

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

[SaschaWillems]

This sadly doesn't build for me, so I can't test it:

I'm on Windows with VS2022 17.8.7

[jherico]

@SaschaWillems @gary-sweet should be ready for testing and review.

[jherico]

@SaschaWillems I've fixed the ray tracing examples to use the builder pattern but retain the usage flags used in the old code.

Regarding the buffers in question raygen_shader_binding_table, miss_shader_binding_table, hit_shader_binding_table, they're currently created, and populated in a way that requires them to be host visible, but they're only written to once. The current logic uses the deprecated VMA usage enum of VMA_MEMORY_USAGE_CPU_TO_GPU, which I believe tries to allocate memory that is device local but host visible if the card provides such a type, but will revert to host visible otherwise. That's typical for things like uniform buffers that get written every frame, but seems strange here.

I guess my question is, since these buffers are only written once, why not just use straight up DEVICE_LOCAL memory and a staging buffer to populate them?

Probably need to to test all samples and also on different platforms to make sure we don't break anything with this.

I've now tested all samples and verified that they all produce output and no new validation errors as far as I can tell (the HLSL examples produce validation errors on main, and the fragment_shader_rate_dynamic sample crashes on main). However, I only really have my main system of Windows 10 and an nVidia card to test on.

[gary-sweet]

This all seems to work ok on my platform now, but I notice there is a clang format check failure so I can't approve it yet.

[SaschaWillems]

And while I'm in general a fan of the builder pattern, I'm kinda skeptical. Afaik we don't use it elsewhere and it kinda feels odd to introduce (yet) another pattern for an already convoluted and hard to maintain framework. Not sure on this, what do the other reviewers think of that approach?

[jherico]

@SaschaWillems I have removed all use of the builder pattern from the code (except the internal usage in the image and buffer classes to simplify constructors. I have left, and expanded the use of convenience functions for creating staging buffers, as well as noticed and corrected some minor bugs in populating staging buffers where they fail to flush before unmapping.

[SaschaWillems]

Any idea why the profiles sample cpp is displayed as completely changed (all lines) in this PR? It only looks like the copyright has changed, but I'm not able to see any other change due to the whole file being displayed as different.

[SaschaWillems]

It's the same for a lot of other files too, which makes it nigh impossible to review these changes:

[jherico]

@SaschaWillems It looks like some of the files got converted from LF to CRLF. Most of the files in the repository are LF, but a few are CRLF (prior to this change). I've updated the commit to revert the line ending changes. I must have misconfigured something in git, because I thought I had my system configured to checkout and commit LF, always (autocrlf=false). I'll review my settings and try to figure out how this happened.

@tomadamatkinson might be worthwhile to add to the pre-commit hook something that would verify that none of the line-endings have changed for existing files. These are the only 5 files on main that are currently crlf:

i/crlf  w/crlf  attr/                   framework/scene_graph/components/orthographic_camera.cpp
i/crlf  w/crlf  attr/                   framework/vulkan_sample.h
i/crlf  w/crlf  attr/                   samples/performance/multithreading_render_passes/CMakeLists.txt
i/crlf  w/crlf  attr/                   samples/performance/multithreading_render_passes/multithreading_render_passes.cpp
i/crlf  w/crlf  attr/                   samples/performance/multithreading_render_passes/multithreading_render_passes.h

[kanavin]

@jherico I'm a bit baffled, but this seems to have regressed 32 bit builds (both arm and x86), where 64 bit build without issues:

| In file included from /srv/storage/alex/yocto/build-arm-32-alt/tmp/work/cortexa15t2hf-neon-poky-linux-gnueabi/vulkan-samples/git/git/framework/core/allocated.h:22,
|                  from /srv/storage/alex/yocto/build-arm-32-alt/tmp/work/cortexa15t2hf-neon-poky-linux-gnueabi/vulkan-samples/git/git/framework/core/buffer.h:22,
|                  from /srv/storage/alex/yocto/build-arm-32-alt/tmp/work/cortexa15t2hf-neon-poky-linux-gnueabi/vulkan-samples/git/git/framework/api_vulkan_sample.h:36,
|                  from /srv/storage/alex/yocto/build-arm-32-alt/tmp/work/cortexa15t2hf-neon-poky-linux-gnueabi/vulkan-samples/git/git/framework/gltf_loader.cpp:32:
| /srv/storage/alex/yocto/build-arm-32-alt/tmp/work/cortexa15t2hf-neon-poky-linux-gnueabi/vulkan-samples/git/git/framework/core/vulkan_resource.h:50:24: error: redefinition of 'constexpr VkObjectType vkb::core::get_object_type(const HandleType&) [with HandleType = long long unsigned int; VkObjectType = VkObjectType]'
|    50 | constexpr VkObjectType get_object_type(const VkImageView &handle)
|       |                        ^~~~~~~~~~~~~~~
| /srv/storage/alex/yocto/build-arm-32-alt/tmp/work/cortexa15t2hf-neon-poky-linux-gnueabi/vulkan-samples/git/git/framework/core/vulkan_resource.h:44:24: note: 'constexpr VkObjectType vkb::core::get_object_type(const HandleType&) [with HandleType = long long unsigned int; VkObjectType = VkObjectType]' previously declared here
|    44 | constexpr VkObjectType get_object_type(const VkImage &handle)
|       |                        ^~~~~~~~~~~~~~~
| /srv/storage/alex/yocto/build-arm-32-alt/tmp/work/cortexa15t2hf-neon-poky-linux-gnueabi/vulkan-samples/git/git/framework/core/vulkan_resource.h:56:24: error: redefinition of 'constexpr VkObjectType vkb::core::get_object_type(const HandleType&) [with HandleType = long long unsigned int; VkObjectType = VkObjectType]'
|    56 | constexpr VkObjectType get_object_type(const VkRenderPass &handle)
|       |                        ^~~~~~~~~~~~~~~
| /srv/storage/alex/yocto/build-arm-32-alt/tmp/work/cortexa15t2hf-neon-poky-linux-gnueabi/vulkan-samples/git/git/framework/core/vulkan_resource.h:44:24: note: 'constexpr VkObjectType vkb::core::get_object_type(const HandleType&) [with HandleType = long long unsigned int; VkObjectType = VkObjectType]' previously declared here
|    44 | constexpr VkObjectType get_object_type(const VkImage &handle)
|       |                        ^~~~~~~~~~~~~~~
| /srv/storage/alex/yocto/build-arm-32-alt/tmp/work/cortexa15t2hf-neon-poky-linux-gnueabi/vulkan-samples/git/git/framework/core/vulkan_resource.h:62:24: error: redefinition of 'constexpr VkObjectType vkb::core::get_object_type(const HandleType&) [with HandleType = long long unsigned int; VkObjectType = VkObjectType]'
|    62 | constexpr VkObjectType get_object_type(const VkSampler &handle)
|       |                        ^~~~~~~~~~~~~~~
| /srv/storage/alex/yocto/build-arm-32-alt/tmp/work/cortexa15t2hf-neon-poky-linux-gnueabi/vulkan-samples/git/git/framework/core/vulkan_resource.h:44:24: note: 'constexpr VkObjectType vkb::core::get_object_type(const HandleType&) [with HandleType = long long unsigned int; VkObjectType = VkObjectType]' previously declared here
|    44 | constexpr VkObjectType get_object_type(const VkImage &handle)
|       |                        ^~~~~~~~~~~~~~~
| /srv/storage/alex/yocto/build-arm-32-alt/tmp/work/cortexa15t2hf-neon-poky-linux-gnueabi/vulkan-samples/git/git/framework/core/vulkan_resource.h:68:24: error: redefinition of 'constexpr VkObjectType vkb::core::get_object_type(const HandleType&) [with HandleType = long long unsigned int; VkObjectType = VkObjectType]'
|    68 | constexpr VkObjectType get_object_type(const VkBuffer &handle)
|       |                        ^~~~~~~~~~~~~~~
| /srv/storage/alex/yocto/build-arm-32-alt/tmp/work/cortexa15t2hf-neon-poky-linux-gnueabi/vulkan-samples/git/git/framework/core/vulkan_resource.h:44:24: note: 'constexpr VkObjectType vkb::core::get_object_type(const HandleType&) [with HandleType = long long unsigned int; VkObjectType = VkObjectType]' previously declared here
|    44 | constexpr VkObjectType get_object_type(const VkImage &handle)
|       |                        ^~~~~~~~~~~~~~~
| ninja: build stopped: subcommand failed.

[jherico]

@kanavin I'll try to locally reproduce and find a fix today.

[jherico]

ok, so the problem here is that on 32 bit architectures the handles still need to be 64 bit. So while on 64 bit architectures VkImage resolves to struct objectVkImage_T *, on 32 bit platforms it resolves to uint64_t.

#ifndef VK_DEFINE_NON_DISPATCHABLE_HANDLE
    #if (VK_USE_64_BIT_PTR_DEFINES==1)
        #define VK_DEFINE_NON_DISPATCHABLE_HANDLE(object) typedef struct object##_T *object;
    #else
        #define VK_DEFINE_NON_DISPATCHABLE_HANDLE(object) typedef uint64_t object;
    #endif
#endif

This means that all the template specializations like this:

template <>
constexpr VkObjectType get_object_type(const VkImage &handle)
{ ... }
template <>
constexpr VkObjectType get_object_type(const VkImageView &handle)
{ ... }

have the exact same signature and can't be differentiated by the compiler the way they can in 64-bit land.

@tomadamatkinson should a 32-bit build be added to the CI configs? Two distinct PR's of mine appear to have caused regressions for cmake -A Win32.

[jherico]

So, when I run cmake -A Win32 and build the framework I get a LOT of errors, many of which appear way outside the scope of the changes in this PR and in the ASTC PR. For instance, I get a lot of errors building KTX, which hasn't been touched in 3 years.

Looking at the readme, I'm unclear on if 32-bit builds are supported since all of the instructions seem to explicitly specify 64-bit arch.

I'm working on a branch that resolves some of the 32-bit issues I've seen so far, but I'm not touching the third-party build issues other than for astc.

[kanavin]

So, when I run cmake -A Win32 and build the framework I get a LOT of errors, many of which appear way outside the scope of the changes in this PR and in the ASTC PR. For instance, I get a lot of errors building KTX, which hasn't been touched in 3 years.

Looking at the readme, I'm unclear on if 32-bit builds are supported since all of the instructions seem to explicitly specify 64-bit arch.

I'm working on a branch that resolves some of the 32-bit issues I've seen so far, but I'm not touching the third-party build issues other than for astc.

I was worried that in the absence of 32 bit CI that gates all changes, fixing one issue will uncover another (with no clear end in sight). Please do fix what's in your 'jurisdiction' and I'll see what can be done about the rest (my interest is primarily 32 bit linux, especially arm and risc-v which are still used to ship products).