fix(shader): recover gracefully from GLSL compile/link failures instead of crashing

OloEngine/src/Platform/OpenGL/OpenGLShader.cpp

@@ -227,9 +227,14 @@
         const Timer timer;
 
         OLO_CORE_INFO("Compiling shader '{}' from '{}'", m_Name, filepath);
-        CompileOrGetVulkanBinaries(shaderSources);
-
-        if (Utils::IsAmdGpu())
+        if (!CompileOrGetVulkanBinaries(shaderSources))
+        {
+            // A user-authored GLSL syntax/compile error is not an engineering
+            // invariant violation — surface it as a Failed shader instead of
+            // crashing the editor (issue #568). Already logged via OLO_CORE_CRITICAL.
+            m_CompilationStatus = ShaderCompilationStatus::Failed;
+        }
+        else if (Utils::IsAmdGpu())
         {
             std::string fullVersion(reinterpret_cast<const char*>(glGetString(GL_VERSION)));
 
@@ -249,22 +254,29 @@
                 {
                     CreateProgramForAmd();
                 }
-                else
+                else if (CompileOrGetOpenGLBinaries())
                 {
-                    CompileOrGetOpenGLBinaries();
                     CreateProgram();
                 }
+                else
+                {
+                    m_CompilationStatus = ShaderCompilationStatus::Failed;
+                }
             }
             else
             {
                 OLO_CORE_ERROR("Could not find driver version in string: '{0}'", fullVersion);
+                m_CompilationStatus = ShaderCompilationStatus::Failed;
             }
         }
-        else
+        else if (CompileOrGetOpenGLBinaries())
         {
-            CompileOrGetOpenGLBinaries();
             CreateProgram();
         }
+        else
+        {
+            m_CompilationStatus = ShaderCompilationStatus::Failed;
+        }
         const f64 compilationTime = timer.ElapsedMillis();
 
         // If the shader is still Compiling (async path), report that; otherwise it's Ready/Failed
@@ -307,15 +319,23 @@
         {
             // AMD path: compile GLSL source strings directly (no SPIR-V)
             m_OriginalSourceCode = sources;
-            CompileOrGetVulkanBinaries(sources);
-            CreateProgramForAmd();
+            if (CompileOrGetVulkanBinaries(sources))
+            {
+                CreateProgramForAmd();
+            }
+            else
+            {
+                m_CompilationStatus = ShaderCompilationStatus::Failed;
+            }
         }
-        else
+        else if (CompileOrGetVulkanBinaries(sources) && CompileOrGetOpenGLBinaries())
         {
-            CompileOrGetVulkanBinaries(sources);
-            CompileOrGetOpenGLBinaries();
             CreateProgram();
         }
+        else
+        {
+            m_CompilationStatus = ShaderCompilationStatus::Failed;
+        }
 
         // Source-string shaders (boot/fallback) must be ready immediately —
         // the boot shader is needed to render the warmup progress bar, and
@@ -634,10 +654,8 @@
         return shaderSources;
     }
 
-    void OpenGLShader::CompileOrGetVulkanBinaries(const std::unordered_map<GLenum, std::string>& shaderSources)
+    bool OpenGLShader::CompileOrGetVulkanBinaries(const std::unordered_map<GLenum, std::string>& shaderSources)
     {
-        glCreateProgram();
-
         // Store original preprocessed source code for debugging
         m_OriginalSourceCode = shaderSources;
 
@@ -676,13 +694,18 @@
             static_cast<i32>(stageSourcePairs.size()),
             [this, &hasError, &stageSourcePairs, &results, &cacheDirectory, &disableCache](i32 index)
             {
-                if (hasError.load(std::memory_order_relaxed))
-                    return; // Early exit if another stage failed
-
+                // Record the stage before the early-out below so the collect loop can
+                // still identify (and log) this result even if this task never runs —
+                // the default-constructed CompilationResult::Stage is 0, which isn't a
+                // valid GLenum and would otherwise hit GLShaderStageToString's own
+                // switch-default assert when logging the failure.
                 const auto& [stage, source] = stageSourcePairs[index];
                 CompilationResult& result = results[index];
                 result.Stage = stage;
 
+                if (hasError.load(std::memory_order_relaxed))
+                    return; // Early exit if another stage failed
+
                 const std::filesystem::path shaderFilePath = m_FilePath;
                 const std::filesystem::path cachedPath = cacheDirectory / (shaderFilePath.filename().string() + Utils::GLShaderStageCachedVulkanFileExtension(stage));
                 result.CachePath = cachedPath;
@@ -740,14 +763,21 @@
                 result.NeedsCache = !disableCache;
             });
 
-        // Collect results and write cache (sequential to avoid map race conditions)
+        // Collect results and write cache (sequential to avoid map race conditions).
+        // This loop itself is single-threaded (ParallelFor above already joined all
+        // workers), so accumulating into a plain local bool is race-free — the
+        // subtlety is only that a later *successful* stage must not make the overall
+        // result look like a success after an earlier stage failed.
+        bool anyStageFailed = false;
         for (const auto& result : results)
         {
             if (!result.Success)
             {
+                // A stage that never ran because an earlier stage already tripped
+                // |hasError| carries an empty ErrorMessage — still a real failure.
                 OLO_CORE_CRITICAL("[OpenGL] SPIR-V compilation failed for '{}' (stage {}): {}",
                                   m_FilePath, Utils::GLShaderStageToString(result.Stage), result.ErrorMessage);
-                OLO_CORE_VERIFY(false, "Shader SPIR-V compilation failure");
+                anyStageFailed = true;
                 continue;
             }
 
@@ -767,13 +797,18 @@
             }
         }
 
+        if (anyStageFailed)
+            return false;
+
         for (auto&& [stage, data] : shaderData)
         {
             Reflect(stage, data);
         }
+
+        return true;
     }
 
-    void OpenGLShader::CompileOrGetOpenGLBinaries()
+    bool OpenGLShader::CompileOrGetOpenGLBinaries()
     {
         auto& shaderData = m_OpenGLSPIRV;
 
@@ -812,13 +847,15 @@
             static_cast<i32>(stageSpirvPairs.size()),
             [this, &hasError, &stageSpirvPairs, &results, &cacheDirectory, &disableCache](i32 index)
             {
-                if (hasError.load(std::memory_order_relaxed))
-                    return;
-
+                // Record the stage before the early-out below — see the matching
+                // comment in CompileOrGetVulkanBinaries's ParallelFor lambda.
                 const auto& [stage, vulkanSpirv] = stageSpirvPairs[index];
                 OpenGLCompilationResult& result = results[index];
                 result.Stage = stage;
 
+                if (hasError.load(std::memory_order_relaxed))
+                    return;
+
                 const std::filesystem::path shaderFilePath = m_FilePath;
                 const std::filesystem::path cachedPath = cacheDirectory / (shaderFilePath.filename().string() + Utils::GLShaderStageCachedOpenGLFileExtension(stage));
                 result.CachePath = cachedPath;
@@ -917,14 +954,16 @@
                 result.NeedsCache = !disableCache;
             });
 
-        // Collect results and write cache (sequential)
+        // Collect results and write cache (sequential). Single-threaded here too
+        // (ParallelFor above already joined), so this bool accumulation is race-free.
+        bool anyStageFailed = false;
         for (const auto& result : results)
         {
             if (!result.Success)
             {
                 OLO_CORE_CRITICAL("[OpenGL] SPIR-V cross-compilation failed for '{}' (stage {}): {}",
                                   m_FilePath, Utils::GLShaderStageToString(result.Stage), result.ErrorMessage);
-                OLO_CORE_VERIFY(false, "Shader SPIR-V cross-compilation failure");
+                anyStageFailed = true;
                 continue;
             }
 
@@ -947,6 +986,8 @@
                 }
             }
         }
+
+        return !anyStageFailed;
     }
 
     void OpenGLShader::FinalizeProgram(GLenum const& program, const std::unordered_map<GLenum, std::vector<u32>>& spirvMap)
@@ -1062,7 +1103,6 @@
             std::vector<GLchar> infoLog(maxLength);
             glGetProgramInfoLog(program, maxLength, &maxLength, infoLog.data());
             OLO_CORE_CRITICAL("[OpenGL] Shader linking failed for '{}':\n{}", m_FilePath, infoLog.data());
-            OLO_CORE_VERIFY(false, "Shader program linking failure");
 
             glDeleteProgram(program);
 
@@ -1294,7 +1334,6 @@
             glGetProgramInfoLog(program, maxLength, &maxLength, &infoLog[0]);
 
             OLO_CORE_CRITICAL("[OpenGL] Shader link failure for '{}': {}", filePath, infoLog.data());
-            OLO_CORE_VERIFY(false, "Shader program linking failure");
             return false;
         }
         return true;
@@ -1326,7 +1365,14 @@
         program = glCreateProgram();
 
         std::array<u32, 2> glShadersIDs{};
-        CompileOpenGLBinariesForAmd(program, glShadersIDs);
+        if (!CompileOpenGLBinariesForAmd(program, glShadersIDs))
+        {
+            // CompileOpenGLBinariesForAmd already cleaned up any shaders it attached
+            // before failing; |program| itself is still ours to delete.
+            glDeleteProgram(program);
+            m_CompilationStatus = ShaderCompilationStatus::Failed;
+            return;
+        }
         // Required by spec for glGetProgramBinary; prevents Mesa/radeonsi crash.
         glProgramParameteri(program, GL_PROGRAM_BINARY_RETRIEVABLE_HINT, GL_TRUE);
         glLinkProgram(program);
@@ -1374,7 +1420,7 @@
         m_CompilationStatus = ShaderCompilationStatus::Ready;
     }
 
-    void OpenGLShader::CompileOpenGLBinariesForAmd(GLenum const& program, std::array<u32, 2>& glShadersIDs) const
+    bool OpenGLShader::CompileOpenGLBinariesForAmd(GLenum const& program, std::array<u32, 2>& glShadersIDs) const
     {
         int glShaderIDIndex = 0;
         for (auto&& [stage, spirv] : m_VulkanSPIRV)
@@ -1425,12 +1471,22 @@
 
                 OLO_CORE_CRITICAL("[OpenGL] Shader compilation failed for '{}' (stage {}): {}",
                                   m_FilePath, Utils::GLShaderStageToString(stage), infoLog.data());
-                OLO_CORE_VERIFY(false, "Shader source compilation failure");
-                return;
+
+                // Clean up any stages already compiled+attached during this call so the
+                // caller is left with a clean, empty |program| rather than a partially
+                // attached one it doesn't know about.
+                for (int i = 0; i < glShaderIDIndex; ++i)
+                {
+                    glDetachShader(program, glShadersIDs[i]);
+                    glDeleteShader(glShadersIDs[i]);
+                }
+                glShadersIDs.fill(0);
+                return false;
             }
             glAttachShader(program, shader);
             glShadersIDs[glShaderIDIndex++] = shader;
         }
+        return true;
     }
 
     void OpenGLShader::Reflect(const GLenum stage, const std::vector<u32>& shaderData)
@@ -1532,16 +1588,25 @@
 
         try
         {
-            CompileOrGetVulkanBinaries(shaderSources);
-            if (Utils::IsAmdGpu())
+            if (!CompileOrGetVulkanBinaries(shaderSources))
+            {
+                // Broken GLSL in the edited file — already logged via
+                // OLO_CORE_CRITICAL. Fall through to Failed; the restore-old-program
+                // block below keeps the previously-working shader bound (issue #568).
+                m_CompilationStatus = ShaderCompilationStatus::Failed;
+            }
+            else if (Utils::IsAmdGpu())
             {
                 CreateProgramForAmd();
             }
-            else
+            else if (CompileOrGetOpenGLBinaries())
             {
-                CompileOrGetOpenGLBinaries();
                 CreateProgram();
             }
+            else
+            {
+                m_CompilationStatus = ShaderCompilationStatus::Failed;
+            }
 
             // For hot-reload we want synchronous completion so the new shader
             // is immediately usable — force-finish any async link.

OloEngine/src/Platform/OpenGL/OpenGLShader.h

@@ -152,11 +152,17 @@
         static std::string ProcessIncludesInternal(const std::string& source, const std::string& directory, std::unordered_set<std::string>& includedFiles);
         std::unordered_map<GLenum, std::string> PreProcess(std::string_view source);
 
-        void CompileOrGetVulkanBinaries(const std::unordered_map<GLenum, std::string>& shaderSources);
-        void CompileOrGetOpenGLBinaries();
+        // Returns false if any shader stage failed to compile (already logged via
+        // OLO_CORE_CRITICAL). Callers must not proceed to link/finalize on failure —
+        // set m_CompilationStatus to Failed and bail instead (see issue #568).
+        [[nodiscard]] bool CompileOrGetVulkanBinaries(const std::unordered_map<GLenum, std::string>& shaderSources);
+        [[nodiscard]] bool CompileOrGetOpenGLBinaries();
         void CreateProgram();
 
-        void CompileOpenGLBinariesForAmd(GLenum const& program, std::array<u32, 2>& glShadersIDs) const;
+        // Returns false if any stage failed to compile; any shader objects already
+        // attached to |program| during this call are detached/deleted before
+        // returning so the caller is left with a clean, empty program.
+        [[nodiscard]] bool CompileOpenGLBinariesForAmd(GLenum const& program, std::array<u32, 2>& glShadersIDs) const;
         void CreateProgramForAmd();
 
         void Reflect(GLenum stage, const std::vector<u32>& shaderData);

OloEngine/tests/CMakeLists.txt

@@ -194,6 +194,7 @@ add_executable(OloEngine-Tests
 		Rendering/PropertyTests/DeferredOverlayPassTests.cpp
 		Rendering/PropertyTests/OITPropertyTests.cpp
 		Rendering/PropertyTests/ShaderCompilationTest.cpp
+		Rendering/PropertyTests/ShaderCompileFailureRecoveryTest.cpp
 		Rendering/PropertyTests/AssetPreviewRendererTest.cpp
 		Rendering/PropertyTests/RendererAttachedTest.cpp
 		Rendering/PropertyTests/RendererAttachedSmokeTest.cpp