Updated ShaderVariableVk: not creating variables for immutable samplers + code cleanup

2 files changed, 51 insertions, 46 deletions

diff --git a/Graphics/GraphicsEngineVulkan/include/ShaderVariableVk.h b/Graphics/GraphicsEngineVulkan/include/ShaderVariableVk.h
index 5f7a0628..bb555881 100644
--- a/Graphics/GraphicsEngineVulkan/include/ShaderVariableVk.h
+++ b/Graphics/GraphicsEngineVulkan/include/ShaderVariableVk.h
@@ -30,7 +30,7 @@
 //  * ShaderVariableManagerVk keeps list of variables of specific types
 //  * Every ShaderVariableVkImpl references VkResource from ShaderResourceLayoutVk
 //  * ShaderVariableManagerVk keeps pointer to ShaderResourceCacheVk
-//  * ShaderVariableManagerVk is used by ShaderVkImpl to manage static resources and by
+//  * ShaderVariableManagerVk is used by PipelineStateVkImpl to manage static resources and by
 //    ShaderResourceBindingVkImpl to manage mutable and dynamic resources
 //
 //          __________________________                   __________________________________________________________________________
@@ -83,7 +83,7 @@ public:
     ShaderVariableVkImpl* GetVariable(const Char* Name);
     ShaderVariableVkImpl* GetVariable(Uint32 Index);
 
-    void BindResources( IResourceMapping* pResourceMapping, Uint32 Flags);
+    void BindResources(IResourceMapping* pResourceMapping, Uint32 Flags);
 
     static size_t GetRequiredMemorySize(const ShaderResourceLayoutVk&          Layout, 
                                         const SHADER_RESOURCE_VARIABLE_TYPE*   AllowedVarTypes, 
@@ -98,10 +98,10 @@ private:
     Uint32 GetVariableIndex(const ShaderVariableVkImpl& Variable);
 
     IObject&                      m_Owner;
-    // Variable mgr is owned by either Shader object (in which case m_pResourceLayout points to
-    // static resource layout owned by the same shader object), or by SRB object (in which case 
+    // Variable mgr is owned by either Pipeline state object (in which case m_pResourceLayout points to
+    // static resource layout owned by the same PSO object), or by SRB object (in which case 
     // m_pResourceLayout points to corresponding layout in pipeline state). Since SRB keeps strong 
-    // reference to PSO, the layout is guaranteed be alive while SRB is alive
+    // reference to PSO, the layout is guaranteed to be alive while SRB is alive
     const ShaderResourceLayoutVk* m_pResourceLayout= nullptr;
     ShaderResourceCacheVk*        m_pResourceCache = nullptr;
 
@@ -147,7 +147,7 @@ public:
         return m_ParentManager.m_Owner.Release();
     }
 
-    void QueryInterface(const INTERFACE_ID &IID, IObject **ppInterface)override final
+    void QueryInterface(const INTERFACE_ID& IID, IObject** ppInterface)override final
     {
         if (ppInterface == nullptr)
             return;
@@ -165,7 +165,7 @@ public:
         return m_Resource.GetVariableType();
     }
 
-    virtual void Set(IDeviceObject *pObject)override final 
+    virtual void Set(IDeviceObject* pObject)override final 
     {
         VERIFY_EXPR(m_ParentManager.m_pResourceCache != nullptr);
         m_Resource.BindResource(pObject, 0, *m_ParentManager.m_pResourceCache); 
diff --git a/Graphics/GraphicsEngineVulkan/src/ShaderVariableVk.cpp b/Graphics/GraphicsEngineVulkan/src/ShaderVariableVk.cpp
index 22acaf5c..6698dd53 100644
--- a/Graphics/GraphicsEngineVulkan/src/ShaderVariableVk.cpp
+++ b/Graphics/GraphicsEngineVulkan/src/ShaderVariableVk.cpp
@@ -29,42 +29,43 @@
 namespace Diligent
 {
 
-size_t ShaderVariableManagerVk::GetRequiredMemorySize(const ShaderResourceLayoutVk& Layout, 
+size_t ShaderVariableManagerVk::GetRequiredMemorySize(const ShaderResourceLayoutVk&          Layout, 
                                                       const SHADER_RESOURCE_VARIABLE_TYPE*   AllowedVarTypes, 
-                                                      Uint32                        NumAllowedTypes,
-                                                      Uint32&                       NumVariables)
+                                                      Uint32                                 NumAllowedTypes,
+                                                      Uint32&                                NumVariables)
 {
     NumVariables = 0;
-    Uint32 AllowedTypeBits = GetAllowedTypeBits(AllowedVarTypes, NumAllowedTypes);
-    for(SHADER_RESOURCE_VARIABLE_TYPE VarType = SHADER_RESOURCE_VARIABLE_TYPE_STATIC; VarType < SHADER_RESOURCE_VARIABLE_TYPE_NUM_TYPES; VarType = static_cast<SHADER_RESOURCE_VARIABLE_TYPE>(VarType+1))
+    const Uint32 AllowedTypeBits = GetAllowedTypeBits(AllowedVarTypes, NumAllowedTypes);
+    const bool UsingSeparateSamplers = Layout.IsUsingSeparateSamplers();
+    for (SHADER_RESOURCE_VARIABLE_TYPE VarType = SHADER_RESOURCE_VARIABLE_TYPE_STATIC; VarType < SHADER_RESOURCE_VARIABLE_TYPE_NUM_TYPES; VarType = static_cast<SHADER_RESOURCE_VARIABLE_TYPE>(VarType+1))
     {
         if (IsAllowedType(VarType, AllowedTypeBits))
         {
             auto NumResources = Layout.GetResourceCount(VarType);
-            if (Layout.IsUsingSeparateSamplers())
-                NumVariables += NumResources;
-            else
+            for (Uint32 r=0; r < NumResources; ++r)
             {
-                // When using HLSL-style combined image samplers, we need to skip separate samplers
-                for( Uint32 r=0; r < NumResources; ++r )
-                {
-                    const auto& SrcRes = Layout.GetResource(VarType, r);
-                    if (SrcRes.SpirvAttribs.Type != SPIRVShaderResourceAttribs::ResourceType::SeparateSampler)
-                        ++NumVariables;
-                }
+                const auto& SrcRes = Layout.GetResource(VarType, r);
+
+                // When using HLSL-style combined image samplers, we need to skip separate samplers.
+                // Also always skip immutable separate samplers.
+                if (SrcRes.SpirvAttribs.Type == SPIRVShaderResourceAttribs::ResourceType::SeparateSampler && 
+                    (!UsingSeparateSamplers || SrcRes.IsImmutableSamplerAssigned()) )
+                    continue;
+
+                ++NumVariables;
             }
         }
     }
     
-    return NumVariables*sizeof(ShaderVariableVkImpl);
+    return NumVariables * sizeof(ShaderVariableVkImpl);
 }
 
 // Creates shader variable for every resource from SrcLayout whose type is one AllowedVarTypes
-void ShaderVariableManagerVk::Initialize(const ShaderResourceLayoutVk& SrcLayout, 
-                                         IMemoryAllocator&             Allocator,
+void ShaderVariableManagerVk::Initialize(const ShaderResourceLayoutVk&          SrcLayout, 
+                                         IMemoryAllocator&                      Allocator,
                                          const SHADER_RESOURCE_VARIABLE_TYPE*   AllowedVarTypes, 
-                                         Uint32                        NumAllowedTypes, 
-                                         ShaderResourceCacheVk&        ResourceCache)
+                                         Uint32                                 NumAllowedTypes, 
+                                         ShaderResourceCacheVk&                 ResourceCache)
 {
     m_pResourceLayout = &SrcLayout;
     m_pResourceCache  = &ResourceCache;
@@ -79,23 +80,26 @@ void ShaderVariableManagerVk::Initialize(const ShaderResourceLayoutVk& SrcLayout
     if(m_NumVariables == 0)
         return;
     
-    auto *pRawMem = ALLOCATE(Allocator, "Raw memory buffer for shader variables", MemSize);
+    auto* pRawMem = ALLOCATE(Allocator, "Raw memory buffer for shader variables", MemSize);
     m_pVariables = reinterpret_cast<ShaderVariableVkImpl*>(pRawMem);
 
     Uint32 VarInd = 0;
-    for(SHADER_RESOURCE_VARIABLE_TYPE VarType = SHADER_RESOURCE_VARIABLE_TYPE_STATIC; VarType < SHADER_RESOURCE_VARIABLE_TYPE_NUM_TYPES; VarType = static_cast<SHADER_RESOURCE_VARIABLE_TYPE>(VarType+1))
+    const bool UsingSeparateSamplers = SrcLayout.IsUsingSeparateSamplers();
+    for (SHADER_RESOURCE_VARIABLE_TYPE VarType = SHADER_RESOURCE_VARIABLE_TYPE_STATIC; VarType < SHADER_RESOURCE_VARIABLE_TYPE_NUM_TYPES; VarType = static_cast<SHADER_RESOURCE_VARIABLE_TYPE>(VarType+1))
     {
         if (!IsAllowedType(VarType, AllowedTypeBits))
             continue;
 
         Uint32 NumResources = SrcLayout.GetResourceCount(VarType);
-        for( Uint32 r=0; r < NumResources; ++r )
+        for (Uint32 r=0; r < NumResources; ++r)
         {
             const auto& SrcRes = SrcLayout.GetResource(VarType, r);
-            if (!SrcLayout.IsUsingSeparateSamplers() && SrcRes.SpirvAttribs.Type == SPIRVShaderResourceAttribs::ResourceType::SeparateSampler)
+            // Skip separate samplers when using combined HLSL-style image samplers. Also always skip immutable separate samplers.
+            if (SrcRes.SpirvAttribs.Type == SPIRVShaderResourceAttribs::ResourceType::SeparateSampler && 
+                (!UsingSeparateSamplers || SrcRes.IsImmutableSamplerAssigned()) )
                 continue;
 
-            ::new (m_pVariables + VarInd) ShaderVariableVkImpl(*this, SrcRes );
+            ::new (m_pVariables + VarInd) ShaderVariableVkImpl(*this, SrcRes);
             ++VarInd;
         }
     }
@@ -111,9 +115,9 @@ void ShaderVariableManagerVk::Destroy(IMemoryAllocator &Allocator)
 {
     VERIFY(m_pDbgAllocator == &Allocator, "Incosistent alloctor");
 
-    if(m_pVariables != nullptr)
+    if (m_pVariables != nullptr)
     {
-        for(Uint32 v=0; v < m_NumVariables; ++v)
+        for (Uint32 v=0; v < m_NumVariables; ++v)
             m_pVariables[v].~ShaderVariableVkImpl();
         Allocator.Free(m_pVariables);
         m_pVariables = nullptr;
@@ -125,7 +129,7 @@ ShaderVariableVkImpl* ShaderVariableManagerVk::GetVariable(const Char* Name)
     ShaderVariableVkImpl* pVar = nullptr;
     for (Uint32 v = 0; v < m_NumVariables; ++v)
     {
-        auto &Var = m_pVariables[v];
+        auto& Var = m_pVariables[v];
         const auto& Res = Var.m_Resource;
         if (strcmp(Res.SpirvAttribs.Name, Name) == 0)
         {
@@ -168,11 +172,11 @@ Uint32 ShaderVariableManagerVk::GetVariableIndex(const ShaderVariableVkImpl& Var
     }
 }
 
-void ShaderVariableManagerVk::BindResources( IResourceMapping* pResourceMapping, Uint32 Flags)
+void ShaderVariableManagerVk::BindResources(IResourceMapping* pResourceMapping, Uint32 Flags)
 {
     VERIFY_EXPR(m_pResourceCache != nullptr);
 
-    if( !pResourceMapping )
+    if (!pResourceMapping)
     {
         LOG_ERROR_MESSAGE( "Failed to bind resources: resource mapping is null" );
         return;
@@ -181,34 +185,35 @@ void ShaderVariableManagerVk::BindResources( IResourceMapping* pResourceMapping,
     if ( (Flags & BIND_SHADER_RESOURCES_UPDATE_ALL) == 0 )
         Flags |= BIND_SHADER_RESOURCES_UPDATE_ALL;
 
-    for(Uint32 v=0; v < m_NumVariables; ++v)
+    for (Uint32 v=0; v < m_NumVariables; ++v)
     {
-        auto &Var = m_pVariables[v];
+        auto& Var = m_pVariables[v];
         const auto& Res = Var.m_Resource;
         
-        // Skip immutable separate samplers
-        if (Res.SpirvAttribs.Type == SPIRVShaderResourceAttribs::ResourceType::SeparateSampler && Res.IsImmutableSamplerAssigned())
-            continue;
+        // There should be no immutable separate samplers
+        VERIFY(Res.SpirvAttribs.Type != SPIRVShaderResourceAttribs::ResourceType::SeparateSampler || !Res.IsImmutableSamplerAssigned(),
+               "There must be no shader resource variables for immutable separate samplers");
 
         if ( (Flags & (1 << Res.GetVariableType())) == 0 )
             continue;
 
         for (Uint32 ArrInd = 0; ArrInd < Res.SpirvAttribs.ArraySize; ++ArrInd)
         {
-            if( (Flags & BIND_SHADER_RESOURCES_KEEP_EXISTING) && Res.IsBound(ArrInd, *m_pResourceCache) )
+            if ( (Flags & BIND_SHADER_RESOURCES_KEEP_EXISTING) && Res.IsBound(ArrInd, *m_pResourceCache) )
                 continue;
 
             const auto* VarName = Res.SpirvAttribs.Name;
             RefCntAutoPtr<IDeviceObject> pObj;
             pResourceMapping->GetResource( VarName, &pObj, ArrInd );
-            if( pObj )
+            if (pObj)
             {
                 Res.BindResource(pObj, ArrInd, *m_pResourceCache);
             }
             else
             {
-                if( (Flags & BIND_SHADER_RESOURCES_VERIFY_ALL_RESOLVED) && !Res.IsBound(ArrInd, *m_pResourceCache) )
-                    LOG_ERROR_MESSAGE( "Unable to bind resource to shader variable '", Res.SpirvAttribs.GetPrintName(ArrInd), "': resource is not found in the resource mapping" );
+                if ( (Flags & BIND_SHADER_RESOURCES_VERIFY_ALL_RESOLVED) && !Res.IsBound(ArrInd, *m_pResourceCache) )
+                    LOG_ERROR_MESSAGE( "Unable to bind resource to shader variable '", Res.SpirvAttribs.GetPrintName(ArrInd), "': resource is not found in the resource mapping. "
+                                       "Do not use BIND_SHADER_RESOURCES_VERIFY_ALL_RESOLVED flag to suppress the message if this is not an issue." );
             }
         }
     }