From 3aa6720f3c2f04338ef7c7831a1f4c4b0340712f Mon Sep 17 00:00:00 2001 From: AnyOldName3 Date: Fri, 4 Apr 2025 19:52:20 +0100 Subject: [PATCH 1/5] Use partial pipeline layout compatibility for inherited state Otherwise you can't have nice things like the same view descriptor set for all shadersets. --- include/vsg/utils/ShaderSet.h | 5 +- .../utils/GraphicsPipelineConfigurator.cpp | 6 +-- src/vsg/utils/ShaderSet.cpp | 46 +++++++++++++++++++ 3 files changed, 53 insertions(+), 4 deletions(-) diff --git a/include/vsg/utils/ShaderSet.h b/include/vsg/utils/ShaderSet.h index e3bf15ac4d..5cba36a42d 100644 --- a/include/vsg/utils/ShaderSet.h +++ b/include/vsg/utils/ShaderSet.h @@ -177,9 +177,12 @@ namespace vsg /// create the descriptor set layout. virtual ref_ptr createDescriptorSetLayout(const std::set& defines, uint32_t set) const; - /// return true of specified pipeline layout is compatible with what is required for this ShaderSet + /// return true if specified pipeline layout is compatible with what is required for this ShaderSet virtual bool compatiblePipelineLayout(const PipelineLayout& layout, const std::set& defines) const; + /// return true if specified pipeline layout is partially compatible with what is required for this ShaderSet + virtual bool partiallyCompatiblePipelineLayout(const PipelineLayout& layout, const std::set& defines, bool onlyPushConstants, uint32_t descriptorSet) const; + /// create the pipeline layout for all descriptor sets enabled by specified defines or required by default. inline ref_ptr createPipelineLayout(const std::set& defines) { return createPipelineLayout(defines, descriptorSetRange()); } diff --git a/src/vsg/utils/GraphicsPipelineConfigurator.cpp b/src/vsg/utils/GraphicsPipelineConfigurator.cpp index 698139c635..34ea0328b2 100644 --- a/src/vsg/utils/GraphicsPipelineConfigurator.cpp +++ b/src/vsg/utils/GraphicsPipelineConfigurator.cpp @@ -554,7 +554,7 @@ void GraphicsPipelineConfigurator::_assignInheritedSets() { if (!bds.descriptorSet || !bds.descriptorSet->setLayout || !gpc.descriptorConfigurator) return; - if (gpc.shaderSet->compatiblePipelineLayout(*bds.layout, gpc.shaderHints->defines)) + if (gpc.shaderSet->partiallyCompatiblePipelineLayout(*bds.layout, gpc.shaderHints->defines, true, bds.firstSet)) { gpc.inheritedSets.insert(bds.firstSet); } @@ -564,7 +564,7 @@ void GraphicsPipelineConfigurator::_assignInheritedSets() { if (!gpc.descriptorConfigurator) return; - if (gpc.shaderSet->compatiblePipelineLayout(*bds.layout, gpc.shaderHints->defines)) + if (gpc.shaderSet->partiallyCompatiblePipelineLayout(*bds.layout, gpc.shaderHints->defines, true, bds.firstSet + bds.descriptorSets.size() - 1)) { for (size_t i = 0; i < bds.descriptorSets.size(); ++i) { @@ -575,7 +575,7 @@ void GraphicsPipelineConfigurator::_assignInheritedSets() void apply(const BindViewDescriptorSets& bvds) override { - if (!gpc.shaderSet->compatiblePipelineLayout(*bvds.layout, gpc.shaderHints->defines)) + if (!gpc.shaderSet->partiallyCompatiblePipelineLayout(*bvds.layout, gpc.shaderHints->defines, true, bvds.firstSet)) { return; } diff --git a/src/vsg/utils/ShaderSet.cpp b/src/vsg/utils/ShaderSet.cpp index 6441aa5b04..bc17d231c9 100644 --- a/src/vsg/utils/ShaderSet.cpp +++ b/src/vsg/utils/ShaderSet.cpp @@ -561,6 +561,13 @@ bool ShaderSet::compatiblePipelineLayout(const PipelineLayout& layout, const std ++set; } +#ifdef VK_EXT_graphics_pipeline_library + if (layout.flags & VK_PIPELINE_LAYOUT_CREATE_INDEPENDENT_SETS_BIT_EXT) + { + return false; + } +#endif + PushConstantRanges ranges; for (auto& pcr : pushConstantRanges) { @@ -578,6 +585,45 @@ bool ShaderSet::compatiblePipelineLayout(const PipelineLayout& layout, const std return true; } +bool vsg::ShaderSet::partiallyCompatiblePipelineLayout(const PipelineLayout& layout, const std::set& defines, bool onlyPushConstants, uint32_t descriptorSet) const +{ + PushConstantRanges ranges; + for (auto& pcr : pushConstantRanges) + { + if (pcr.define.empty() || defines.count(pcr.define) == 1) + { + ranges.push_back(pcr.range); + } + } + + if (compare_value_container(layout.pushConstantRanges, ranges) != 0) + { + return false; + } + + if (onlyPushConstants) + { + return true; + } + +#ifdef VK_EXT_graphics_pipeline_library + if (layout.flags & VK_PIPELINE_LAYOUT_CREATE_INDEPENDENT_SETS_BIT_EXT) + { + return false; + } +#endif + + for (uint32_t set = 0; set <= std::min(layout.setLayouts.size() - 1, size_t(descriptorSet)); ++set) + { + if (layout.setLayouts[set] && !compatibleDescriptorSetLayout(*layout.setLayouts[set], defines, set)) + { + return false; + } + } + + return true; +} + ref_ptr ShaderSet::createPipelineLayout(const std::set& defines, std::pair range) const { DescriptorSetLayouts descriptorSetLayouts; From 1a6ec2ca5fcb3c0fab58e679a678a6a4e3b0a466 Mon Sep 17 00:00:00 2001 From: AnyOldName3 Date: Tue, 8 Apr 2025 18:36:26 +0100 Subject: [PATCH 2/5] Fix incorrect parameter and narrowing conversion warning --- src/vsg/utils/GraphicsPipelineConfigurator.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/vsg/utils/GraphicsPipelineConfigurator.cpp b/src/vsg/utils/GraphicsPipelineConfigurator.cpp index 34ea0328b2..8e30fde887 100644 --- a/src/vsg/utils/GraphicsPipelineConfigurator.cpp +++ b/src/vsg/utils/GraphicsPipelineConfigurator.cpp @@ -554,7 +554,7 @@ void GraphicsPipelineConfigurator::_assignInheritedSets() { if (!bds.descriptorSet || !bds.descriptorSet->setLayout || !gpc.descriptorConfigurator) return; - if (gpc.shaderSet->partiallyCompatiblePipelineLayout(*bds.layout, gpc.shaderHints->defines, true, bds.firstSet)) + if (gpc.shaderSet->partiallyCompatiblePipelineLayout(*bds.layout, gpc.shaderHints->defines, false, bds.firstSet)) { gpc.inheritedSets.insert(bds.firstSet); } @@ -564,7 +564,7 @@ void GraphicsPipelineConfigurator::_assignInheritedSets() { if (!gpc.descriptorConfigurator) return; - if (gpc.shaderSet->partiallyCompatiblePipelineLayout(*bds.layout, gpc.shaderHints->defines, true, bds.firstSet + bds.descriptorSets.size() - 1)) + if (gpc.shaderSet->partiallyCompatiblePipelineLayout(*bds.layout, gpc.shaderHints->defines, false, bds.firstSet + static_cast(bds.descriptorSets.size()) - 1)) { for (size_t i = 0; i < bds.descriptorSets.size(); ++i) { @@ -575,7 +575,7 @@ void GraphicsPipelineConfigurator::_assignInheritedSets() void apply(const BindViewDescriptorSets& bvds) override { - if (!gpc.shaderSet->partiallyCompatiblePipelineLayout(*bvds.layout, gpc.shaderHints->defines, true, bvds.firstSet)) + if (!gpc.shaderSet->partiallyCompatiblePipelineLayout(*bvds.layout, gpc.shaderHints->defines, false, bvds.firstSet)) { return; } From 8e575d75d5cabd50d349abadc9983260ca5bd8d6 Mon Sep 17 00:00:00 2001 From: AnyOldName3 Date: Wed, 9 Apr 2025 16:06:34 +0100 Subject: [PATCH 3/5] Make null checks match the new requirements --- src/vsg/utils/GraphicsPipelineConfigurator.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/vsg/utils/GraphicsPipelineConfigurator.cpp b/src/vsg/utils/GraphicsPipelineConfigurator.cpp index 8e30fde887..f886e9f028 100644 --- a/src/vsg/utils/GraphicsPipelineConfigurator.cpp +++ b/src/vsg/utils/GraphicsPipelineConfigurator.cpp @@ -552,7 +552,7 @@ void GraphicsPipelineConfigurator::_assignInheritedSets() void apply(const BindDescriptorSet& bds) override { - if (!bds.descriptorSet || !bds.descriptorSet->setLayout || !gpc.descriptorConfigurator) return; + if (!bds.layout || !gpc.descriptorConfigurator) return; if (gpc.shaderSet->partiallyCompatiblePipelineLayout(*bds.layout, gpc.shaderHints->defines, false, bds.firstSet)) { @@ -562,7 +562,7 @@ void GraphicsPipelineConfigurator::_assignInheritedSets() void apply(const BindDescriptorSets& bds) override { - if (!gpc.descriptorConfigurator) return; + if (!bds.layout || !gpc.descriptorConfigurator) return; if (gpc.shaderSet->partiallyCompatiblePipelineLayout(*bds.layout, gpc.shaderHints->defines, false, bds.firstSet + static_cast(bds.descriptorSets.size()) - 1)) { From 1a45b1af6a079a418e76da2ae51f31fb00771687 Mon Sep 17 00:00:00 2001 From: AnyOldName3 Date: Wed, 7 May 2025 18:48:40 +0100 Subject: [PATCH 4/5] Avoid binding descriptor sets for layouts without that set Avoids disturbing necessary descriptor sets while still avoiding the need for full layout compatibility checks. This is achieved with a basic vector saying which sets a pipeline layout has and doesn't have, so layouts it doesn't have won't be bound. This could mean different sets are bound than if a full compatibility check was done, but that should only happen when the scenegraph requested invalid API usage - there should only be one BindDescriptorSet for each slot at the top of the state stacks, and if it's for a slot the current pipeline layout needs, it must be compatible. Therefore, we only need to worry about compatibility checks for slots the current pipeline layout doesn't use. --- include/vsg/state/PipelineLayout.h | 1 + include/vsg/vk/CommandBuffer.h | 2 ++ src/vsg/state/BindDescriptorSet.cpp | 18 ++++++++++++++---- src/vsg/state/PipelineLayout.cpp | 7 ++++++- src/vsg/state/ViewDependentState.cpp | 2 +- src/vsg/vk/CommandBuffer.cpp | 3 +++ 6 files changed, 27 insertions(+), 6 deletions(-) diff --git a/include/vsg/state/PipelineLayout.h b/include/vsg/state/PipelineLayout.h index 2d5554f993..5634cc2b68 100644 --- a/include/vsg/state/PipelineLayout.h +++ b/include/vsg/state/PipelineLayout.h @@ -34,6 +34,7 @@ namespace vsg VkPipelineLayoutCreateFlags flags = 0; DescriptorSetLayouts setLayouts; PushConstantRanges pushConstantRanges; + std::vector descriptorSetSlots; /// Vulkan VkPipelineLayout handle VkPipelineLayout vk(uint32_t deviceID) const { return _implementation[deviceID]->_pipelineLayout; } diff --git a/include/vsg/vk/CommandBuffer.h b/include/vsg/vk/CommandBuffer.h index 966c055943..9ec0d0a50e 100644 --- a/include/vsg/vk/CommandBuffer.h +++ b/include/vsg/vk/CommandBuffer.h @@ -56,6 +56,7 @@ namespace vsg void setCurrentPipelineLayout(const PipelineLayout* pipelineLayout); VkPipelineLayout getCurrentPipelineLayout() const { return _currentPipelineLayout; } + const std::vector& getCurrentDescriptorSetSlots() const { return _currentDescriptorSetSlots; } VkShaderStageFlags getCurrentPushConstantStageFlags() const { return _currentPushConstantStageFlags; } ref_ptr scratchMemory; @@ -73,6 +74,7 @@ namespace vsg ref_ptr _device; ref_ptr _commandPool; VkPipelineLayout _currentPipelineLayout; + std::vector _currentDescriptorSetSlots; VkShaderStageFlags _currentPushConstantStageFlags; }; VSG_type_name(vsg::CommandBuffer); diff --git a/src/vsg/state/BindDescriptorSet.cpp b/src/vsg/state/BindDescriptorSet.cpp index 4af20a8f3b..4f60ea87e1 100644 --- a/src/vsg/state/BindDescriptorSet.cpp +++ b/src/vsg/state/BindDescriptorSet.cpp @@ -113,6 +113,13 @@ void BindDescriptorSets::compile(Context& context) void BindDescriptorSets::record(CommandBuffer& commandBuffer) const { //info("BindDescriptorSets::record() ", dynamicOffsets.size(), ", ", dynamicOffsets.data()); + if (commandBuffer.getCurrentDescriptorSetSlots().size() < firstSet + descriptorSets.size()) + return; + for (size_t slot = firstSet; slot < firstSet + descriptorSets.size(); ++slot) + { + if (!commandBuffer.getCurrentDescriptorSetSlots()[slot]) + return; + } auto& vkd = _vulkanData[commandBuffer.deviceID]; vkCmdBindDescriptorSets(commandBuffer, pipelineBindPoint, vkd._vkPipelineLayout, firstSet, static_cast(vkd._vkDescriptorSets.size()), vkd._vkDescriptorSets.data(), @@ -209,8 +216,11 @@ void BindDescriptorSet::compile(Context& context) void BindDescriptorSet::record(CommandBuffer& commandBuffer) const { //info("BindDescriptorSet::record() ", dynamicOffsets.size(), ", ", dynamicOffsets.data()); - auto& vkd = _vulkanData[commandBuffer.deviceID]; - vkCmdBindDescriptorSets(commandBuffer, pipelineBindPoint, vkd._vkPipelineLayout, firstSet, - 1, &(vkd._vkDescriptorSet), - static_cast(dynamicOffsets.size()), dynamicOffsets.data()); + if (commandBuffer.getCurrentDescriptorSetSlots().size() > firstSet && commandBuffer.getCurrentDescriptorSetSlots()[firstSet]) + { + auto& vkd = _vulkanData[commandBuffer.deviceID]; + vkCmdBindDescriptorSets(commandBuffer, pipelineBindPoint, vkd._vkPipelineLayout, firstSet, + 1, &(vkd._vkDescriptorSet), + static_cast(dynamicOffsets.size()), dynamicOffsets.data()); + } } diff --git a/src/vsg/state/PipelineLayout.cpp b/src/vsg/state/PipelineLayout.cpp index 5fab02a848..b7769bd950 100644 --- a/src/vsg/state/PipelineLayout.cpp +++ b/src/vsg/state/PipelineLayout.cpp @@ -30,7 +30,8 @@ PipelineLayout::PipelineLayout(const PipelineLayout& rhs, const CopyOp& copyop) Inherit(rhs, copyop), flags(rhs.flags), setLayouts(rhs.setLayouts), - pushConstantRanges(rhs.pushConstantRanges) + pushConstantRanges(rhs.pushConstantRanges), + descriptorSetSlots(rhs.descriptorSetSlots) { } @@ -123,9 +124,13 @@ void PipelineLayout::compile(Context& context) { if (!_implementation[context.deviceID]) { + descriptorSetSlots.clear(); + descriptorSetSlots.reserve(setLayouts.size()); + for (auto dsl : setLayouts) { if (dsl) dsl->compile(context); + descriptorSetSlots.push_back(dsl != nullptr); } _implementation[context.deviceID] = PipelineLayout::Implementation::create(context.device, setLayouts, pushConstantRanges, flags); } diff --git a/src/vsg/state/ViewDependentState.cpp b/src/vsg/state/ViewDependentState.cpp index baef9fd173..34ac4b3a74 100644 --- a/src/vsg/state/ViewDependentState.cpp +++ b/src/vsg/state/ViewDependentState.cpp @@ -151,7 +151,7 @@ void BindViewDescriptorSets::compile(Context& context) void BindViewDescriptorSets::record(CommandBuffer& commandBuffer) const { - if (commandBuffer.viewDependentState) + if (commandBuffer.viewDependentState && commandBuffer.getCurrentDescriptorSetSlots().size() > firstSet && commandBuffer.getCurrentDescriptorSetSlots()[firstSet]) { commandBuffer.viewDependentState->bindDescriptorSets(commandBuffer, pipelineBindPoint, layout->vk(commandBuffer.deviceID), firstSet); } diff --git a/src/vsg/vk/CommandBuffer.cpp b/src/vsg/vk/CommandBuffer.cpp index 314e340752..89f10a82cd 100644 --- a/src/vsg/vk/CommandBuffer.cpp +++ b/src/vsg/vk/CommandBuffer.cpp @@ -30,6 +30,7 @@ CommandBuffer::CommandBuffer(CommandPool* commandPool, VkCommandBuffer commandBu _device(commandPool->getDevice()), _commandPool(commandPool), _currentPipelineLayout(VK_NULL_HANDLE), + _currentDescriptorSetSlots(), _currentPushConstantStageFlags(0) { } @@ -45,6 +46,7 @@ CommandBuffer::~CommandBuffer() void CommandBuffer::reset() { _currentPipelineLayout = VK_NULL_HANDLE; + _currentDescriptorSetSlots.clear(); _currentPushConstantStageFlags = 0; _commandPool->reset(); @@ -59,6 +61,7 @@ void CommandBuffer::setCurrentPipelineLayout(const PipelineLayout* pipelineLayou state->dirtyStateStacks(); _currentPipelineLayout = newLayout; + _currentDescriptorSetSlots = pipelineLayout->descriptorSetSlots; if (pipelineLayout->pushConstantRanges.empty()) _currentPushConstantStageFlags = 0; else From 1e582d004f1d438af3a1717cb80f54f5d0b7d9c0 Mon Sep 17 00:00:00 2001 From: AnyOldName3 Date: Tue, 13 May 2025 22:18:39 +0100 Subject: [PATCH 5/5] Handle empty descriptor sets in pipeline layout The VSG generates layouts like this when descriptor bindings are dependent on defines, and doesn't generate matching empty BindDescriptorSet instances to go with them. That disrupts the fix in the previous commit, so they must be handled as if they don't exist. Even though the pipeline layout says these sets exist, there's no need to bind anything to them as they inherently can't be statically used by the pipeline. --- include/vsg/state/DescriptorSetLayout.h | 2 ++ include/vsg/state/ViewDependentState.h | 2 ++ src/vsg/state/PipelineLayout.cpp | 2 +- 3 files changed, 5 insertions(+), 1 deletion(-) diff --git a/include/vsg/state/DescriptorSetLayout.h b/include/vsg/state/DescriptorSetLayout.h index c9e31f49b3..ffadb396bb 100644 --- a/include/vsg/state/DescriptorSetLayout.h +++ b/include/vsg/state/DescriptorSetLayout.h @@ -37,6 +37,8 @@ namespace vsg /// VkDescriptorSetLayoutCreateInfo settings DescriptorSetLayoutBindings bindings; + virtual bool empty() const { return bindings.empty(); } + /// map the descriptor bindings to the descriptor pool sizes that will be required to represent them. void getDescriptorPoolSizes(DescriptorPoolSizes& descriptorPoolSizes); diff --git a/include/vsg/state/ViewDependentState.h b/include/vsg/state/ViewDependentState.h index 56adc361df..7f57239c60 100644 --- a/include/vsg/state/ViewDependentState.h +++ b/include/vsg/state/ViewDependentState.h @@ -33,6 +33,8 @@ namespace vsg public: ViewDescriptorSetLayout(); + bool empty() const override { return false; } + VkDescriptorSetLayout vk(uint32_t deviceID) const override { return _viewDescriptorSetLayout ? _viewDescriptorSetLayout->vk(deviceID) : 0; } int compare(const Object& rhs_object) const override; diff --git a/src/vsg/state/PipelineLayout.cpp b/src/vsg/state/PipelineLayout.cpp index b7769bd950..fc243ed541 100644 --- a/src/vsg/state/PipelineLayout.cpp +++ b/src/vsg/state/PipelineLayout.cpp @@ -130,7 +130,7 @@ void PipelineLayout::compile(Context& context) for (auto dsl : setLayouts) { if (dsl) dsl->compile(context); - descriptorSetSlots.push_back(dsl != nullptr); + descriptorSetSlots.push_back(dsl != nullptr && !dsl->empty()); } _implementation[context.deviceID] = PipelineLayout::Implementation::create(context.device, setLayouts, pushConstantRanges, flags); }