Don't replace shaders for incomplete pipeline libraries #3307
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Before, when replacing shaders, RenderDoc would treat these incomplete pipeline libraries as if they were complete pipelines, and thus attempt to create pipelines with incomplete data, producing several validation errors and on some devices crashes.
For instance, a pipeline library that only contains a vertex or fragment shader but not input assembly state would use invalid input assembly state (specifically,
VulkanShaderCache::MakeGraphicsPipelineInfo
would use the topology value fromVulkanCreationInfo::Pipeline
, which is set toVK_PRIMITIVE_TOPOLOGY_MAX_ENUM
(0x7fffffff) if the pipeline has no input assembly state byVulkanCreationInfo::Pipeline::Init
).Additionally, those pipeline libraries were treated as complete graphics pipelines, not libraries, as
VulkanShaderCache::MakeGraphicsPipelineInfo
removesVK_PIPELINE_CREATE_LIBRARY_BIT_KHR
and noVkGraphicsPipelineLibraryCreateInfoEXT
was added to thepNext
chain.RenderDoc already merges the pipeline libraries into complete graphics pipelines, so there is no reason to recreate the libraries with modified shaders.
This change removes the following validation warnings after replacing a shader that draws a teapot on the
graphics_pipeline_library
Khronos sample:The Vulkan spec is a bit ambiguous about whether a topology of
VK_PRIMITIVE_TOPOLOGY_MAX_ENUM
is ever legal. A pipeline library withoutVK_GRAPHICS_PIPELINE_LIBRARY_VERTEX_INPUT_INTERFACE_BIT_EXT
passesVUID-VkGraphicsPipelineCreateInfo-dynamicPrimitiveTopologyUnrestricted-09031
sopInputAssemblyState
can be null, but if it is not null thenVUID-VkGraphicsPipelineCreateInfo-pInputAssemblyState-09032
says it needs to be valid. But the members section says thatpInputAssemblyState
"is ignored if the pipeline includes a mesh shader stage", not just "can be null". The description section says that members being null "is optional so the application can still use a valid pointer if it needs to set thepNext
orflags
fields to specify state for other extensions".In practice, as of Vulkan SDK 1.3.280.0, it seems like the validation layers do not care about invalid topologies in
pInputAssemblyState
for a pipeline library without the vertex input interface bit or for pipelines containing mesh shaders (tested with themesh_shading
Khronos sample). I didn't try to test theextended_dynamic_state3
case as none of the Khronos samples usedynamicPrimitiveTopologyUnrestricted
. Even with this PR, renderdoc does still create a pipeline with an invalid topology when editing a shader in a pipeline with mesh shaders, but the validation layers don't care; I suspect the same thing happens withextended_dynamic_state3
.An alternative implementation of this PR that re-created pipeline libraries with the appropriate flags would also pass the validation layers, but is more awkward to implement (especially since prior to Vulkan 1.3.260,
VkGraphicsPipelineLibraryCreateInfoEXT::pNext
was a non-const
pointer, which meansconst_cast
is needed for inserting it to thepNext
chain). Another alternative option would be to only setpInputAssemblyState
if the originalpInputAssemblyState
was non-null, but that is quite messy when applied to the other fields too.