Skip to content

Improve descriptor set handling.#110

Merged
crud89 merged 6 commits intomainfrom
optional-default-descriptor-bindings
Dec 20, 2023
Merged

Improve descriptor set handling.#110
crud89 merged 6 commits intomainfrom
optional-default-descriptor-bindings

Conversation

@crud89
Copy link
Copy Markdown
Owner

@crud89 crud89 commented Dec 20, 2023

Describe the pull request

This PR slightly reworks descriptor set layouts and their allocation. First of all, it is now more easy to allocate descriptor sets without providing a resource to bind to it directly. The resource in this case needs to be bound before using the descriptor set later:

auto& geometryPipeline = m_device->state().pipeline("Geometry");
auto& cameraBindingLayout = geometryPipeline.layout()->descriptorSet(DescriptorSets::Constant);
auto cameraBindings = cameraBindingLayout.allocate();
// ...
auto cameraBuffer = m_device->factory().createBuffer("Camera", cameraBindingLayout, 0, BufferUsage::Resource);
cameraBindings->update(*cameraBuffer);

This is enabled by a std::monostate overload for the descriptor binding resource container, which together with the binding now being optional allows for default-constructable DescriptorBindings. This second change to the binding property allows to use pre-ordered indices for register bindings:

auto transformBindings = transformBindingLayout.allocate({
    { .resource = *transformBuffer, .firstElement = 0, .elements = 0 }, // Binds to space 0, descriptor 0
    { .resource = *transformBuffer, .firstElement = 0, .elements = 1 }, // Binds to space 0, descriptor 1
    { .resource = *transformBuffer, .firstElement = 0, .elements = 1 }  // Binds to space 0, descriptor 2
});

Note that a single DescriptorBinding refers to one descriptor within a descriptor set. For allocateMultiple this means that counting descriptor indices is reset for each element of the outer collection.

Additionally, t is now valid for the Vulkan backend to define empty descriptor sets. Earlier this would have resulted in validation errors and failures when attempting to bind the descriptor set. When creating a pipeline layout, empty descriptor sets are created for spaces that have no layout associated with them (up until the layout with the highest space index). The following shader can now be used for both backends:

ConstantBuffer<Camera> camera : register(b1, s1);
ConstantBuffer<Transform> transform : register(b0, s2);

The space indices are now validate for both backends. Within one pipeline each descriptor set layout must use distinct spaces. If two layouts share the same space, an error will be raised when attempting to create the pipeline layout.

Related issues

@crud89 crud89 added Priority: Medium A issue with normal priority. Vulkan πŸŒ‹ The issue involves the Vulkan backend. DX12 ❎ The issue involves the DX12 backend. labels Dec 20, 2023
@crud89 crud89 added this to the Alpha #04 milestone Dec 20, 2023
@crud89 crud89 self-assigned this Dec 20, 2023
@crud89 crud89 marked this pull request as ready for review December 20, 2023 14:01
@crud89 crud89 merged commit 4d55188 into main Dec 20, 2023
@crud89 crud89 deleted the optional-default-descriptor-bindings branch December 20, 2023 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

DX12 ❎ The issue involves the DX12 backend. Priority: Medium A issue with normal priority. Vulkan πŸŒ‹ The issue involves the Vulkan backend.

Projects

Status: v0.4.1

Development

Successfully merging this pull request may close these issues.

Allow descriptor set allocation without pre-existing buffers. Allow empty and undefined descriptor sets.

1 participant