Completed shader compilation refactoring (fixed https://github.com/DiligentGraphics/DiligentCore/issues/160)

6 files changed, 107 insertions, 90 deletions

diff --git a/Graphics/GraphicsEngineD3DBase/src/ShaderD3DBase.cpp b/Graphics/GraphicsEngineD3DBase/src/ShaderD3DBase.cpp
index 39684476..4ac72599 100644
--- a/Graphics/GraphicsEngineD3DBase/src/ShaderD3DBase.cpp
+++ b/Graphics/GraphicsEngineD3DBase/src/ShaderD3DBase.cpp
@@ -63,7 +63,7 @@ public:
             return E_FAIL;
         }
 
-        RefCntAutoPtr<IDataBlob> pFileData(MakeNewRCObj<DataBlobImpl>()(0));
+        RefCntAutoPtr<IDataBlob> pFileData(MakeNewRCObj<DataBlobImpl>{}(0));
         pSourceStream->ReadBlob(pFileData);
         *ppData = pFileData->GetDataPtr();
         *pBytes = static_cast<UINT>(pFileData->GetSize());
@@ -115,17 +115,27 @@ ShaderD3DBase::ShaderD3DBase(const ShaderCreateInfo& ShaderCI, const ShaderVersi
     {
         DEV_CHECK_ERR(ShaderCI.ByteCode == nullptr, "'ByteCode' must be null when shader is created from the source code or a file");
         DEV_CHECK_ERR(ShaderCI.ByteCodeSize == 0, "'ByteCodeSize' must be 0 when shader is created from the source code or a file");
+        DEV_CHECK_ERR(ShaderCI.EntryPoint != nullptr, "Entry point must not be null");
 
         bool UseDXC = false;
 
         // validate compiler type
         switch (ShaderCI.ShaderCompiler)
         {
-            // clang-format off
-            case SHADER_COMPILER_DEFAULT: UseDXC = false;                                            break;
-            case SHADER_COMPILER_DXC:     UseDXC = DxCompiler != nullptr && DxCompiler->IsLoaded();  break;
-            case SHADER_COMPILER_FXC:     UseDXC = false;                                            break;
-            // clang-format on
+            case SHADER_COMPILER_DEFAULT:
+                UseDXC = false;
+                break;
+
+            case SHADER_COMPILER_DXC:
+                UseDXC = DxCompiler != nullptr && DxCompiler->IsLoaded();
+                if (!UseDXC)
+                    LOG_WARNING_MESSAGE("DXC compiler is not available. Using default shader compiler");
+                break;
+
+            case SHADER_COMPILER_FXC:
+                UseDXC = false;
+                break;
+
             default: UNEXPECTED("Unsupported shader compiler");
         }
 
@@ -140,37 +150,10 @@ ShaderD3DBase::ShaderD3DBase(const ShaderCreateInfo& ShaderCI, const ShaderVersi
 
             String ShaderSource = BuildHLSLSourceString(ShaderCI);
 
-            DEV_CHECK_ERR(ShaderCI.EntryPoint != nullptr, "Entry point must not be null");
-
-            CComPtr<ID3DBlob> errors;
-
-            auto hr = CompileShader(ShaderSource.c_str(), ShaderSource.length(), ShaderCI, strShaderProfile.c_str(), &m_pShaderByteCode, &errors);
-
-            const size_t CompilerMsgLen = errors ? errors->GetBufferSize() : 0;
-            const char*  CompilerMsg    = CompilerMsgLen > 0 ? static_cast<const char*>(errors->GetBufferPointer()) : nullptr;
-
-            if (CompilerMsg != nullptr && ShaderCI.ppCompilerOutput != nullptr)
-            {
-                auto* pOutputDataBlob = MakeNewRCObj<DataBlobImpl>()(CompilerMsgLen + 1 + ShaderSource.length() + 1);
-                char* DataPtr         = static_cast<char*>(pOutputDataBlob->GetDataPtr());
-                memcpy(DataPtr, CompilerMsg, CompilerMsgLen);
-                DataPtr[CompilerMsgLen] = 0; // Set null terminator
-                memcpy(DataPtr + CompilerMsgLen + 1, ShaderSource.data(), ShaderSource.length() + 1);
-                pOutputDataBlob->QueryInterface(IID_DataBlob, reinterpret_cast<IObject**>(ShaderCI.ppCompilerOutput));
-            }
-
-            if (FAILED(hr))
-            {
-                ComErrorDesc ErrDesc(hr);
-                if (ShaderCI.ppCompilerOutput != nullptr)
-                {
-                    LOG_ERROR_AND_THROW("Failed to compile D3D shader \"", (ShaderCI.Desc.Name != nullptr ? ShaderCI.Desc.Name : ""), "\" (", ErrDesc.Get(), ").");
-                }
-                else
-                {
-                    LOG_ERROR_AND_THROW("Failed to compile D3D shader \"", (ShaderCI.Desc.Name != nullptr ? ShaderCI.Desc.Name : ""), "\" (", ErrDesc.Get(), "):\n", (CompilerMsg != nullptr ? CompilerMsg : "<no compiler log available>"));
-                }
-            }
+            CComPtr<ID3DBlob> CompilerOutput;
+
+            auto hr = CompileShader(ShaderSource.c_str(), ShaderSource.length(), ShaderCI, strShaderProfile.c_str(), &m_pShaderByteCode, &CompilerOutput);
+            HandleHLSLCompilerResult(SUCCEEDED(hr), CompilerOutput.p, ShaderSource, ShaderCI.Desc.Name, ShaderCI.ppCompilerOutput);
         }
     }
     else if (ShaderCI.ByteCode)
diff --git a/Graphics/GraphicsEngineOpenGL/include/ShaderGLImpl.hpp b/Graphics/GraphicsEngineOpenGL/include/ShaderGLImpl.hpp
index 374fe0c2..bc0f32e6 100644
--- a/Graphics/GraphicsEngineOpenGL/include/ShaderGLImpl.hpp
+++ b/Graphics/GraphicsEngineOpenGL/include/ShaderGLImpl.hpp
@@ -80,7 +80,7 @@ public:
 
     ShaderGLImpl(IReferenceCounters*     pRefCounters,
                  RenderDeviceGLImpl*     pDeviceGL,
-                 const ShaderCreateInfo& ShaderCreateInfo,
+                 const ShaderCreateInfo& ShaderCI,
                  bool                    bIsDeviceInternal = false);
     ~ShaderGLImpl();
 
diff --git a/Graphics/GraphicsEngineOpenGL/src/ShaderGLImpl.cpp b/Graphics/GraphicsEngineOpenGL/src/ShaderGLImpl.cpp
index c83905f8..f2707fd4 100644
--- a/Graphics/GraphicsEngineOpenGL/src/ShaderGLImpl.cpp
+++ b/Graphics/GraphicsEngineOpenGL/src/ShaderGLImpl.cpp
@@ -40,26 +40,26 @@ namespace Diligent
 
 ShaderGLImpl::ShaderGLImpl(IReferenceCounters*     pRefCounters,
                            RenderDeviceGLImpl*     pDeviceGL,
-                           const ShaderCreateInfo& CreationAttribs,
+                           const ShaderCreateInfo& ShaderCI,
                            bool                    bIsDeviceInternal) :
     // clang-format off
     TShaderBase
     {
         pRefCounters,
         pDeviceGL,
-        CreationAttribs.Desc,
+        ShaderCI.Desc,
         bIsDeviceInternal
     },
     m_GLShaderObj{true, GLObjectWrappers::GLShaderObjCreateReleaseHelper{GetGLShaderType(m_Desc.ShaderType)}}
 // clang-format on
 {
-    DEV_CHECK_ERR(CreationAttribs.ByteCode == nullptr, "'ByteCode' must be null when shader is created from the source code or a file");
-    DEV_CHECK_ERR(CreationAttribs.ByteCodeSize == 0, "'ByteCodeSize' must be 0 when shader is created from the source code or a file");
-    DEV_CHECK_ERR(CreationAttribs.ShaderCompiler == SHADER_COMPILER_DEFAULT, "only default compiler is supported in OpenGL");
+    DEV_CHECK_ERR(ShaderCI.ByteCode == nullptr, "'ByteCode' must be null when shader is created from the source code or a file");
+    DEV_CHECK_ERR(ShaderCI.ByteCodeSize == 0, "'ByteCodeSize' must be 0 when shader is created from the source code or a file");
+    DEV_CHECK_ERR(ShaderCI.ShaderCompiler == SHADER_COMPILER_DEFAULT, "only default compiler is supported in OpenGL");
 
     const auto& deviceCaps = pDeviceGL->GetDeviceCaps();
 
-    auto GLSLSource = BuildGLSLSourceString(CreationAttribs, deviceCaps, TargetGLSLCompiler::driver);
+    auto GLSLSource = BuildGLSLSourceString(ShaderCI, deviceCaps, TargetGLSLCompiler::driver);
 
     // Note: there is a simpler way to create the program:
     //m_uiShaderSeparateProg = glCreateShaderProgramv(GL_VERTEX_SHADER, _countof(ShaderStrings), ShaderStrings);
@@ -92,7 +92,7 @@ ShaderGLImpl::ShaderGLImpl(IReferenceCounters*     pRefCounters,
             FullSource.append(str);
 
         std::stringstream ErrorMsgSS;
-        ErrorMsgSS << "Failed to compile shader file '" << (CreationAttribs.Desc.Name != nullptr ? CreationAttribs.Desc.Name : "") << '\'' << std::endl;
+        ErrorMsgSS << "Failed to compile shader file '" << (ShaderCI.Desc.Name != nullptr ? ShaderCI.Desc.Name : "") << '\'' << std::endl;
         int infoLogLen = 0;
         // The function glGetShaderiv() tells how many bytes to allocate; the length includes the NULL terminator.
         glGetShaderiv(m_GLShaderObj, GL_INFO_LOG_LENGTH, &infoLogLen);
@@ -110,7 +110,7 @@ ShaderGLImpl::ShaderGLImpl(IReferenceCounters*     pRefCounters,
                        << infoLog.data() << std::endl;
         }
 
-        if (CreationAttribs.ppCompilerOutput != nullptr)
+        if (ShaderCI.ppCompilerOutput != nullptr)
         {
             // infoLogLen accounts for null terminator
             auto* pOutputDataBlob = MakeNewRCObj<DataBlobImpl>()(infoLogLen + FullSource.length() + 1);
@@ -118,7 +118,7 @@ ShaderGLImpl::ShaderGLImpl(IReferenceCounters*     pRefCounters,
             if (infoLogLen > 0)
                 memcpy(DataPtr, infoLog.data(), infoLogLen);
             memcpy(DataPtr + infoLogLen, FullSource.data(), FullSource.length() + 1);
-            pOutputDataBlob->QueryInterface(IID_DataBlob, reinterpret_cast<IObject**>(CreationAttribs.ppCompilerOutput));
+            pOutputDataBlob->QueryInterface(IID_DataBlob, reinterpret_cast<IObject**>(ShaderCI.ppCompilerOutput));
         }
         else
         {
diff --git a/Graphics/GraphicsEngineVulkan/src/ShaderVkImpl.cpp b/Graphics/GraphicsEngineVulkan/src/ShaderVkImpl.cpp
index 033f711a..c16010d8 100644
--- a/Graphics/GraphicsEngineVulkan/src/ShaderVkImpl.cpp
+++ b/Graphics/GraphicsEngineVulkan/src/ShaderVkImpl.cpp
@@ -64,19 +64,24 @@ ShaderVkImpl::ShaderVkImpl(IReferenceCounters*     pRefCounters,
             "#   define VULKAN 1\n"
             "#endif\n";
 
-        switch (CreationAttribs.ShaderCompiler)
+        auto ShaderCompiler = CreationAttribs.ShaderCompiler;
+        if (ShaderCompiler == SHADER_COMPILER_DXC)
+        {
+            auto* pDXComiler = pRenderDeviceVk->GetDxCompiler();
+            if (pDXComiler == nullptr || !pDXComiler->IsLoaded())
+            {
+                LOG_WARNING_MESSAGE("DX Compiler is not loaded. Using default shader compiler");
+                ShaderCompiler = SHADER_COMPILER_DEFAULT;
+            }
+        }
+
+        switch (ShaderCompiler)
         {
             case SHADER_COMPILER_DXC:
             {
                 auto* pDXComiler = pRenderDeviceVk->GetDxCompiler();
-                if (pDXComiler != nullptr && pDXComiler->IsLoaded())
-                {
-                    pDXComiler->Compile(CreationAttribs, VulkanDefine, nullptr, &m_SPIRV, CreationAttribs.ppCompilerOutput);
-                }
-                else
-                {
-                    LOG_ERROR_AND_THROW("DX Compiler is not loaded");
-                }
+                VERIFY_EXPR(pDXComiler != nullptr && pDXComiler->IsLoaded());
+                pDXComiler->Compile(CreationAttribs, VulkanDefine, nullptr, &m_SPIRV, CreationAttribs.ppCompilerOutput);
             }
             break;
 
@@ -84,7 +89,7 @@ ShaderVkImpl::ShaderVkImpl(IReferenceCounters*     pRefCounters,
             case SHADER_COMPILER_GLSLANG:
             {
 #if DILIGENT_NO_GLSLANG
-                LOG_ERROR_AND_THROW("Diligent engine was not linked with glslang, use DXIL compiler or precompiled SPIRV bytecode.");
+                LOG_ERROR_AND_THROW("Diligent engine was not linked with glslang, use DXC or precompiled SPIRV bytecode.");
 #else
                 if (CreationAttribs.SourceLanguage == SHADER_SOURCE_LANGUAGE_HLSL)
                 {
diff --git a/Graphics/ShaderTools/include/HLSLUtils.hpp b/Graphics/ShaderTools/include/HLSLUtils.hpp
index 9abd1e7d..0e36c7b4 100644
--- a/Graphics/ShaderTools/include/HLSLUtils.hpp
+++ b/Graphics/ShaderTools/include/HLSLUtils.hpp
@@ -27,6 +27,8 @@
 
 #pragma once
 
+#include <sstream>
+
 #include "BasicTypes.h"
 #include "GraphicsTypes.h"
 #include "Shader.h"
@@ -39,4 +41,55 @@ String BuildHLSLSourceString(const ShaderCreateInfo& ShaderCI,
 
 String GetHLSLProfileString(SHADER_TYPE ShaderType, ShaderVersion ShaderModel);
 
+template <typename BlobType>
+void HandleHLSLCompilerResult(bool               CompilationSucceeded,
+                              BlobType*          pCompilerMsgBlob,
+                              const std::string& ShaderSource,
+                              const char*        ShaderName,
+                              IDataBlob**        ppOutputLog) noexcept(false)
+{
+    const char*  CompilerMsg    = pCompilerMsgBlob ? static_cast<const char*>(pCompilerMsgBlob->GetBufferPointer()) : nullptr;
+    const size_t CompilerMsgLen = CompilerMsg ? pCompilerMsgBlob->GetBufferSize() : 0;
+
+    if (ppOutputLog != nullptr)
+    {
+        const auto ShaderSourceLen = ShaderSource.length();
+        auto*      pOutputLogBlob  = MakeNewRCObj<DataBlobImpl>{}(ShaderSourceLen + 1 + CompilerMsgLen + 1);
+
+        auto* log = static_cast<char*>(pOutputLogBlob->GetDataPtr());
+
+        if (CompilerMsg != nullptr)
+            memcpy(log, CompilerMsg, CompilerMsgLen);
+        log[CompilerMsgLen] = 0; // Explicitly set null terminator
+        log += CompilerMsgLen + 1;
+
+        memcpy(log, ShaderSource.data(), ShaderSourceLen);
+        log[ShaderSourceLen] = 0;
+
+        pOutputLogBlob->QueryInterface(IID_DataBlob, reinterpret_cast<IObject**>(ppOutputLog));
+    }
+
+    if (!CompilationSucceeded || CompilerMsgLen != 0)
+    {
+        std::stringstream ss;
+        ss << (CompilationSucceeded ? "Compiler output for shader '" : "Failed to compile shader '")
+           << (ShaderName != nullptr ? ShaderName : "<unknown>")
+           << "'";
+        if (CompilerMsg != nullptr && CompilerMsgLen != 0)
+        {
+            ss << ":" << std::endl
+               << CompilerMsg;
+        }
+        else if (!CompilationSucceeded)
+        {
+            ss << " (no shader log available).";
+        }
+
+        if (CompilationSucceeded)
+            LOG_INFO_MESSAGE(ss.str());
+        else
+            LOG_ERROR_AND_THROW(ss.str());
+    }
+}
+
 } // namespace Diligent
diff --git a/Graphics/ShaderTools/src/DXCompiler.cpp b/Graphics/ShaderTools/src/DXCompiler.cpp
index 3ce6066f..ad7a0023 100644
--- a/Graphics/ShaderTools/src/DXCompiler.cpp
+++ b/Graphics/ShaderTools/src/DXCompiler.cpp
@@ -459,8 +459,8 @@ void DXCompilerImpl::Compile(const ShaderCreateInfo& ShaderCI,
 #endif
         };
 
-    CComPtr<IDxcBlob> compiled;
-    CComPtr<IDxcBlob> errors;
+    CComPtr<IDxcBlob> pCompiledShader;
+    CComPtr<IDxcBlob> pDxcLog;
 
     IDXCompiler::CompileAttribs CA;
 
@@ -475,44 +475,20 @@ void DXCompilerImpl::Compile(const ShaderCreateInfo& ShaderCI,
     CA.pArgs                      = m_Target == DXCompilerTarget::Direct3D12 ? pDxbcArgs : pSpirvArgs;
     CA.ArgsCount                  = m_Target == DXCompilerTarget::Direct3D12 ? _countof(pDxbcArgs) : _countof(pSpirvArgs);
     CA.pShaderSourceStreamFactory = ShaderCI.pShaderSourceStreamFactory;
-    CA.ppBlobOut                  = &compiled;
-    CA.ppCompilerOutput           = &errors;
+    CA.ppBlobOut                  = &pCompiledShader;
+    CA.ppCompilerOutput           = &pDxcLog;
 
     auto result = Compile(CA);
+    HandleHLSLCompilerResult(result, pDxcLog.p, Source, ShaderCI.Desc.Name, ppCompilerOutput);
 
-    const size_t CompilerMsgLen = errors ? errors->GetBufferSize() : 0;
-    const char*  CompilerMsg    = CompilerMsgLen > 0 ? static_cast<const char*>(errors->GetBufferPointer()) : nullptr;
-
-    if (CompilerMsg != nullptr && ppCompilerOutput != nullptr)
-    {
-        auto* pOutputDataBlob = MakeNewRCObj<DataBlobImpl>()(Source.length() + 1 + CompilerMsgLen + 1);
-        char* DataPtr         = static_cast<char*>(pOutputDataBlob->GetDataPtr());
-        memcpy(DataPtr, CompilerMsg, CompilerMsgLen);
-        DataPtr[CompilerMsgLen] = 0; // Set null terminator as CompilerMsgLen may not account for it
-        memcpy(DataPtr + CompilerMsgLen + 1, Source.data(), Source.length() + 1);
-        pOutputDataBlob->QueryInterface(IID_DataBlob, reinterpret_cast<IObject**>(ppCompilerOutput));
-    }
-
-    if (!result)
-    {
-        if (ppCompilerOutput != nullptr)
-        {
-            LOG_ERROR_AND_THROW("Failed to compile Vulkan shader \"", (ShaderCI.Desc.Name != nullptr ? ShaderCI.Desc.Name : ""), "\".");
-        }
-        else
-        {
-            LOG_ERROR_AND_THROW("Failed to compile Vukan shader \"", (ShaderCI.Desc.Name != nullptr ? ShaderCI.Desc.Name : ""), "\":\n", (CompilerMsg != nullptr ? std::string(CompilerMsg, CompilerMsgLen) : "<no compiler log available>"));
-        }
-    }
-
-    if (result && compiled && compiled->GetBufferSize() > 0)
+    if (result && pCompiledShader && pCompiledShader->GetBufferSize() > 0)
     {
         if (pByteCode != nullptr)
-            pByteCode->assign(static_cast<uint32_t*>(compiled->GetBufferPointer()),
-                              static_cast<uint32_t*>(compiled->GetBufferPointer()) + compiled->GetBufferSize() / sizeof(uint32_t));
+            pByteCode->assign(static_cast<uint32_t*>(pCompiledShader->GetBufferPointer()),
+                              static_cast<uint32_t*>(pCompiledShader->GetBufferPointer()) + pCompiledShader->GetBufferSize() / sizeof(uint32_t));
 
         if (ppByteCodeBlob != nullptr)
-            *ppByteCodeBlob = compiled.Detach();
+            *ppByteCodeBlob = pCompiledShader.Detach();
     }
 }