Add support to define descriptor buffer offset and range

[ilkkasaa]

Descriptor buffers can now be bound with byte offset and range via DESCRIPTOR_OFFSET and DESCRIPTOR_RANGE parameters.

Descriptor array elements can now point to same buffer.

Fixes problem with dynamic uniform/storage buffers: when a dynamic offset is > 0, VK_WHOLE_SIZE must not be used as buffer range.

Fixes #945

[paulthomson]

This looks really good, especially all the tests!

However, you have changed a few vectors into unordered_maps, and we iterate over these. We almost never want to iterate over maps because the order is nondeterministic. This can cause errors to become (even more) nondeterministic, when they don't need to be. Thus, we ideally always want to have a vector (containing the keys) available for the cases where we iterate, and possibly a map alongside if needed (or another vector, but maps/sets are good for preventing duplicates). An ordered map is not ideal either; the order can still be nondeterministic (e.g. if storing pointers), and it is more intuitive to iterate in the order that items were added, not some other ordering. Please can you find a way to remove the iteration over maps? Thanks.

[ilkkasaa]

Yeah, that's a good point! So, now it doesn't loop over maps anymore. The transfer images/buffers are still stored in maps for easier inserts and amber_buffer->transfer_buffer lookups, but when looping, the amber_buffers vector is used to get the keys or an ordered vector of amber_buffer->transfer_buffer pairs.

[ilkkasaa]

While doing this PR, I was thinking, that now we can share a buffer between descriptors within a descriptor array, but the same doesn't apply to separate bindings or descriptor sets. e.g. following Amber code is valid, but it will actually create two buffers with same contents:

BIND BUFFER buf0 AS uniform DESCRIPTOR_SET 0 BINDING 0 
BIND BUFFER buf0 AS uniform DESCRIPTOR_SET 0 BINDING 1

The real problem is with buffers that the shaders can update, like SSBOs, e.g. this doesn't even work:

BIND BUFFER buf0 AS storage DESCRIPTOR_SET 0 BINDING 0
BIND BUFFER buf0 AS storage DESCRIPTOR_SET 0 BINDING 1

I think that Amber needs some completely different way of handling descriptor buffers to resolve this problem, but that's probably out of this PR's scope.

[paulthomson]

[arm64-v8a] Compile++      : amber <= buffer_descriptor.cc
../src/vulkan/buffer_descriptor.cc:158:19: error: 'auto' not allowed in lambda parameter
        [&](const auto& buffer) { return buffer.first == amber_buffer; });
                  ^~~~
In file included from ../src/vulkan/buffer_descriptor.cc:15:
In file included from ../src/vulkan/buffer_descriptor.h:19:
In file included from /tmpfs/src/android-ndk-r21/sources/cxx-stl/llvm-libc++/include/unordered_map:409:
In file included from /tmpfs/src/android-ndk-r21/sources/cxx-stl/llvm-libc++/include/__hash_table:17:
/tmpfs/src/android-ndk-r21/sources/cxx-stl/llvm-libc++/include/algorithm:933:13: error: no matching function for call to object of type '(lambda at ../src/vulkan/buffer_descriptor.cc:158:9)'
        if (__pred(*__first))
            ^~~~~~
../src/vulkan/buffer_descriptor.cc:156:30: note: in instantiation of function template specialization 'std::__ndk1::find_if<std::__ndk1::__wrap_iter<std::__ndk1::pair<amber::Buffer *, amber::vulkan::Resource *> *>, (lambda at ../src/vulkan/buffer_descriptor.cc:158:9)>' requested here
    const auto& image = std::find_if(
                             ^
../src/vulkan/buffer_descriptor.cc:158:9: note: candidate function not viable: no known conversion from 'std::__ndk1::pair<amber::Buffer *, amber::vulkan::Resource *>' to 'int &' for 1st argument
        [&](const auto& buffer) { return buffer.first == amber_buffer; });
        ^
2 errors generated.
make: *** [/tmpfs/src/android-ndk-r21/build/core/build-binary.mk:478: /tmpfs/src/github/amber/build/app/local/arm64-v8a/objs/amber/src/vulkan/buffer_descriptor.o] Error 1

[ilkkasaa]

Thanks, I'll fix that.

[paulthomson]

Nice work! Left a few minor comments.

[paulthomson]

The ternary expression above suggests that amber_buffer can be nullptr. But then we use it here anyway. You haven't actually changed this. But it seems odd. Perhaps we should just remove the ternary expression?

[ilkkasaa]

Yeah, that seems odd. amber_buffer is actually used also before the ternary expression:

if (amber_buffer->ValuePtr()->empty())
      continue;

I removed the ternary expression.

[paulthomson]

While doing this PR, I was thinking, that now we can share a buffer between descriptors within a descriptor array, but the same doesn't apply to separate bindings or descriptor sets. e.g. following Amber code is valid, but it will actually create two buffers with same contents:

BIND BUFFER buf0 AS uniform DESCRIPTOR_SET 0 BINDING 0 
BIND BUFFER buf0 AS uniform DESCRIPTOR_SET 0 BINDING 1

The real problem is with buffers that the shaders can update, like SSBOs, e.g. this doesn't even work:

BIND BUFFER buf0 AS storage DESCRIPTOR_SET 0 BINDING 0
BIND BUFFER buf0 AS storage DESCRIPTOR_SET 0 BINDING 1

I think that Amber needs some completely different way of handling descriptor buffers to resolve this problem, but that's probably out of this PR's scope.

Oh really!? That is a shame. I think we should fix this, if you can think of a way. But, as you say, not necessarily in this PR.

[ilkkasaa]

While doing this PR, I was thinking, that now we can share a buffer between descriptors within a descriptor array, but the same doesn't apply to separate bindings or descriptor sets. e.g. following Amber code is valid, but it will actually create two buffers with same contents:

BIND BUFFER buf0 AS uniform DESCRIPTOR_SET 0 BINDING 0 
BIND BUFFER buf0 AS uniform DESCRIPTOR_SET 0 BINDING 1

The real problem is with buffers that the shaders can update, like SSBOs, e.g. this doesn't even work:

BIND BUFFER buf0 AS storage DESCRIPTOR_SET 0 BINDING 0
BIND BUFFER buf0 AS storage DESCRIPTOR_SET 0 BINDING 1

I think that Amber needs some completely different way of handling descriptor buffers to resolve this problem, but that's probably out of this PR's scope.

Oh really!? That is a shame. I think we should fix this, if you can think of a way. But, as you say, not necessarily in this PR.

Yeah, I have some ideas. I can look into that after this PR.