Suggestion: Refactor (HPP)Image & (HPP)Buffer to improve VMA usage and reduce duplicated code.

[jherico]

The Buffer and Image classes (along with their corresponding HPP versions) have a few things that might benefit from refactoring.

• They share a significant amount of duplicated code.
• The Buffer classes have some odd choices for the defaults in their constructors
  • The Buffer classes don't have any default for the VmaMemoryUsage but the do default VmaAllocationCreateFlags to VMA_ALLOCATION_CREATE_MAPPED_BIT as mentioned. This is actually inverted. The VmaAllocationCreateFlags should probably NOT have a default while the VmaMemoryUsage should almost always default to VMA_MEMORY_USAGE_AUTO.
  • This may be the underlying cause of #848
• The Image constructors don't expose the VmaAllocationCreateFlags but are already unwieldy in terms of the number of arguments
• Maybe Images that act have attachment usage in their usage flags should get the VMA_ALLOCATION_CREATE_DEDICATED_MEMORY_BIT flag automatically?

I propose a change.

First, create a shared base class for anything that uses VmaAllocation for storage. Since VMA doesn't have a C++ set of bindings, the base class could be shared by both the C++ and C classes (the class would include a template argument to specify whether it derived from HPPVulkanResource or VulkanResource). All of the VMA specific members could move into this base class, including all the mapping, unmapping and flushing related code.

Second, fixing the Buffer constructor to reverse the arguments to give one a new default and take the other away would create a large burden for a single change, and adding any arguments to the Image ctors would probably be a mistake.

As an alternative, I suggest that a builder pattern be added. Instead a long list of ctor arguments, a set of reasonable defaults inside a builder object could be created, and then customized as much or as little as desired in use cases. The Image and Buffer classes could then accept as their primary constructor a builder object.

Again, a common base class could be utilized, so that you'd have builder functionality that was VMA specific centralized, looking something like this

template <typename BuilderType>
struct AllocatedBuilder
{
    VmaAllocationCreateInfo alloc_create_info;

    AllocatedBuilder()
    {
        alloc_create_info       = {};
        alloc_create_info.usage = VMA_MEMORY_USAGE_AUTO;
    };

    BuilderType &set_vma_flags(VmaAllocationCreateFlags flags)
    {
        alloc_create_info.flags = flags;
        return *static_cast<BuilderType *>(this);
    }
        ...

And then for each subclass you'd have an individual builder like this for for example for the HPPImage

struct HPPImageBuilder : public AllocatedBuilder<HPPImageBuilder>
{
    vk::ImageCreateInfo create_info;

    HPPImageBuilder(vk::Extent3D const &extent, vk::Format format = vk::Format::eR8G8B8A8Unorm)
    {
        create_info.extent = extent;
        create_info.format = format;
        // Better reasonable defaults than vk::ImageCreateInfo default ctor
        create_info.mipLevels   = 1;
        create_info.arrayLayers = 1;
        create_info.imageType   = vk::ImageType::e2D;
    }

    HPPImageBuilder(vk::Extent2D const &extent, vk::Format format = vk::Format::eR8G8B8A8Unorm) :
        HPPImageBuilder(vk::Extent3D{extent.width, extent.height, 1}, format)
    {
    }

    HPPImageBuilder &set_image_type(vk::ImageType type)
    {
        create_info.imageType = type;
        return *this;
    }

    HPPImageBuilder &set_array_layers(uint32_t layers)
    {
        create_info.arrayLayers = layers;
        return *this;
    }
    ...

The main HPPImage constructor could then look like HPPImage(HPPDevice &device, HPPImageBuilder const & builder);

A given use case for the object might change from this...

        auto color_image = vkb::core::HPPImage{device,
                                               vk::Extent3D{surface_extent.width, surface_extent.height, 1},
                                               DEFAULT_VK_FORMAT,        // We can use any format here that we like
                                               vk::ImageUsageFlagBits::eColorAttachment | vk::ImageUsageFlagBits::eTransferSrc,
                                               VMA_MEMORY_USAGE_GPU_ONLY};

to this

        // We can use any format here that we like
        core::HPPImageBuilder builder(surface_extent, DEFAULT_VK_FORMAT);
        builder.set_usage(vk::ImageUsageFlagBits::eColorAttachment | vk::ImageUsageFlagBits::eTransferSrc);
        auto color_image = vkb::core::HPPImage{device, builder};

In this case I've included a core::HPPImageBuilder ctor that accepts an Extent2D and automatically extends it to the Extent3D inside the builder class.

[jherico]

Note in this case I've explicitly set up the builder ctor to require any params without which the resulting object would make no sense. In the case of Images that's the extent and the format, although even the format is defaulted in the ctor. In the case of buffers it's just the size.

[tomadamatkinson]

Hey @jherico, great ideas!

I agree that we are a little bloated in this area. There is an ongoing initiative to simplify the framework. I think i have a branch with a pattern like this on. Just hard to get in as nearly every place in the framework uses the existing interface so there are a lot of places that need changing.

I agree that a builder pattern could simplify how we create images

[jherico]

I've got a branch that does this partially, built on top of the VMA version update. I'll see if I can whip it into shape and put it up.

[tomadamatkinson]

Feel free to request my review when your ready, I'll try be prompt

[jherico]

@tomadamatkinson it's #906

The current PR just sets up the new builder ctors without removing the old ones, but it does change a few uses inside the framework.

[SaschaWillems]

This sounds similar to what @asuessenbach is working on in #880. We should talk about how this fits into our framework roadmap.