Refactor shader program creation into a factory (#19)

* Refactor shader program creation into a factory Centralizes complex construction logic and makes debugging easier. Will also enable further abstraction down the road for HLSL, or Vulkan shaders. * Update c-cpp.yml * Update codeql-analysis.yml

Author: htmlboss

.github/workflows/c-cpp.yml

@@ -13,6 +13,8 @@ jobs:
 
  steps:
  - uses: actions/checkout@v2
+ - name: apt update
+ run: sudo apt update
  - name: install deps
  run: bash ./install-deps-ubuntu.sh
  - name: make

.github/workflows/codeql-analysis.yml

@@ -61,6 +61,7 @@ jobs:
  # uses a compiled language
 
  - run: |
+ sudo apt update
  bash ./install-deps-ubuntu.sh
  make all
 

MP-APS/Core/RenderSystem.cpp

@@ -1,6 +1,6 @@
 #include "RenderSystem.h"
 
-#include "../Graphics/GLShader.h"
+#include "../Graphics/GLShaderProgramFactory.h"
 #include "../Camera.h"
 
 #include "../Input.h"
@@ -17,8 +17,25 @@
 #define GPU_MEMORY_INFO_TOTAL_AVAILABLE_MEMORY_NVX 0x9048
 #define GPU_MEMORY_INFO_CURRENT_AVAILABLE_VIDMEM_NVX 0x9049
 
+/***********************************************************************************/
+void GLAPIENTRY
+MessageCallback( GLenum source,
+ GLenum type,
+ GLuint id,
+ GLenum severity,
+ GLsizei length,
+ const GLchar* message,
+ const void* userParam )
+{
+ std::fprintf( stderr, "GL CALLBACK: %s type = 0x%x, severity = 0x%x, message = %s\n",
+ ( type == GL_DEBUG_TYPE_ERROR ? "** GL ERROR **" : "" ),
+ type, severity, message );
+}
+
 /***********************************************************************************/
 void RenderSystem::Init(const pugi::xml_node& rendererNode) {
+
+m_rendererNode = rendererNode;
 
 if (!gladLoadGLLoader(reinterpret_cast<GLADloadproc>(glfwGetProcAddress))) {
 std::cerr << "Failed to start GLAD.";
@@ -29,7 +46,7 @@ void RenderSystem::Init(const pugi::xml_node& rendererNode) {
 
 std::cout << m_caps << '\n';
 
-#ifdef _DEBUG
+#ifndef NDEBUG
 std::cout << "OpenGL Version: " << glGetString(GL_VERSION) << '\n';
 std::cout << "GLSL Version: " << glGetString(GL_SHADING_LANGUAGE_VERSION) << '\n';
 std::cout << "OpenGL Driver Vendor: " << glGetString(GL_VENDOR) << '\n';
@@ -46,32 +63,16 @@ void RenderSystem::Init(const pugi::xml_node& rendererNode) {
 m_hdrFBO.Init("HDR FBO");
 m_skybox.Init("Data/hdri/barcelona.hdr", 2048);
 
-// Compile all shader programs in config.xml
-for (auto program = rendererNode.child("Program"); program; program = program.next_sibling("Program")) {
-
-// Get all shader files that make up the program
-std::vector<GLShader> shaders;
-for (auto shader = program.child("Shader"); shader; shader = shader.next_sibling("Shader")) {
-shaders.emplace_back(shader.attribute("path").as_string(), shader.attribute("type").as_string());
-}
-
-//GLShaderProgram prgrm;
-//prgrm.Init(program.attribute("name").as_string(), shaders);
-
-// Compile and cache shader program
-//m_shaderCache.try_emplace(program.attribute("name").as_string(), prgrm);
-m_shaderCache.try_emplace(program.attribute("name").as_string(), program.attribute("name").as_string(), shaders);
-}
+compileShaders();
 
 setupScreenquad();
 setupTextureSamplers();
 setupShadowMap();
 setupPostProcessing();
 
-#ifdef _DEBUG
 glEnable(GL_DEBUG_OUTPUT);
 glEnable(GL_DEBUG_OUTPUT_SYNCHRONOUS);
-#endif
+//glDebugMessageCallback(MessageCallback, nullptr);
 setDefaultState();
 glEnable(GL_MULTISAMPLE);
 
@@ -213,6 +214,27 @@ void RenderSystem::UpdateView(const Camera& camera) {
 glBufferSubData(GL_UNIFORM_BUFFER, 0, sizeof(glm::mat4), glm::value_ptr(m_projMatrix));
 }
 
+/***********************************************************************************/
+void RenderSystem::compileShaders() {
+m_shaderCache.clear();
+for (auto program = m_rendererNode.child("Program"); program; program = program.next_sibling("Program")) {
+
+// Get all shader files that make up the program
+std::vector<Graphics::ShaderStage> stages;
+for (auto shader = program.child("Shader"); shader; shader = shader.next_sibling("Shader")) {
+stages.emplace_back(shader.attribute("path").as_string(), shader.attribute("type").as_string());
+}
+
+std::string name{ program.attribute("name").as_string() };
+
+auto shaderProgram{ Graphics::GLShaderProgramFactory::createShaderProgram(name, stages) };
+
+if (shaderProgram) {
+m_shaderCache.try_emplace(name, std::move(shaderProgram.value())); // value_or for default and remove if-check?
+}
+}
+}
+
 /***********************************************************************************/
 void RenderSystem::queryHardwareCaps() {
 // Anisotropic filtering

MP-APS/Core/RenderSystem.h

@@ -7,6 +7,8 @@
 #include "../Graphics/GLShaderProgram.h"
 #include "../Graphics/HardwareCaps.h"
 
+#include <pugixml.hpp>
+
 #include <unordered_map>
 #include <vector>
 
@@ -18,9 +20,6 @@
 class Camera;
 class SceneBase;
 class GLShaderProgram;
-namespace pugi {
-class xml_node;
-}
 
 /***********************************************************************************/
 class RenderSystem {
@@ -48,6 +47,8 @@ class RenderSystem {
 private:
 // Helper functions
 
+//
+void compileShaders();
 //
 void queryHardwareCaps();
 // Sets the default state required for rendering
@@ -71,6 +72,8 @@ class RenderSystem {
 // Sets projection matrix variable and updates UBO
 void setProjectionMatrix(const Camera& camera);
 
+pugi::xml_node m_rendererNode;
+
 Graphics::HardwareCaps m_caps;
 
 // Screen dimensions

MP-APS/Graphics/GLShader.cpp

@@ -1,28 +1,13 @@
 #include "GLShaderProgram.h"
 
-#include "GLShader.h"
-
+#include <cassert>
 #include <iostream>
-#include <string_view>
-#include <string>
 
 /***********************************************************************************/
-GLShaderProgram::GLShaderProgram(const std::string_view programName, const std::vector<GLShader>& shaders) : m_programName(programName) {
-m_programID = glCreateProgram();
-
-for (const auto& shader : shaders) {
-shader.AttachShader(m_programID);
-}
-
-if (linkAndValidate()) {
-getUniforms();
-}
-
-// Cleanup
-for (const auto& shader : shaders) {
-shader.DetachShader(m_programID);
-shader.DeleteShader();
-}
+GLShaderProgram::GLShaderProgram(const std::string& programName, const GLuint programID) :
+m_programName(programName), m_programID(programID)
+{
+getUniforms();
 }
 
 /***********************************************************************************/
@@ -32,12 +17,15 @@ GLShaderProgram::~GLShaderProgram() {
 
 /***********************************************************************************/
 void GLShaderProgram::Bind() const {
+assert(m_programID != 0);
+
 glUseProgram(m_programID);
 }
 
 /***********************************************************************************/
 void GLShaderProgram::DeleteProgram() const {
 if (m_programID != 0) {
+std::cout << "Deleting program: " << m_programName << '\n';
 glDeleteProgram(m_programID);
 }
 }
@@ -98,28 +86,6 @@ GLShaderProgram& GLShaderProgram::SetUniform(const std::string& uniformName, con
 return *this;
 }
 
-/***********************************************************************************/
-bool GLShaderProgram::linkAndValidate() {
-
-glLinkProgram(m_programID);
-glGetProgramiv(m_programID, GL_LINK_STATUS, &m_success);
-if (!m_success) {
-glGetProgramInfoLog(m_programID, m_infoLog.size(), nullptr, m_infoLog.data());
-std::cerr << "Shader Program Linking Error: " << m_infoLog.data() << std::endl;
-return false;
-}
-
-glValidateProgram(m_programID);
-glGetProgramiv(m_programID, GL_VALIDATE_STATUS, &m_success);
-if (!m_success) {
-glGetProgramInfoLog(m_programID, m_infoLog.size(), nullptr, m_infoLog.data());
-std::cerr << "Shader Program Validation Error: " << m_infoLog.data() << std::endl;
-return false;
-}
-
-return true;
-}
-
 /***********************************************************************************/
 void GLShaderProgram::getUniforms() {
 
@@ -137,4 +103,4 @@ void GLShaderProgram::getUniforms() {
 // TODO: Filter out uniform block members using glGetActiveUniformsiv
 m_uniforms.try_emplace(nameStr, glGetUniformLocation(m_programID, name));
 }
-}
+}