Managing shaders in OpenGL - A Shader class

Question

I've create a basic Shader class in modern OpenGL (3.1) which works perfectly, though it is not feature complete yet. It still lacks the functionality to allow the user registering uniform variables in the shader and update them by calling a method.

Suggestions on how to further advance the implementation are welcome, otherwise, any suggestions and improvements regarding the current code are also appreciated.

Shader.h:

#pragma once
#include <GL/glew.h>
#include <fstream>
#include <string>
#include "LogManager.h"
#include "Transform.h"
#include "camera.h"
namespace deprecate
{
class Shader
{
public:
 Shader(const std::string& fileName);
 virtual ~Shader();
 void Bind();
 void UnBind();
 void Update(const Transform& transform, const Camera& camera, float time);
 Shader& operator=(const Shader& shader) = delete;
 Shader(const Shader& shader) = delete;
private:
 static const unsigned int NUM_SHADERS = 2;
 enum{
 TRANSFORM_U,
 TIME_U,
 NUM_UNIFORMS
 };
 std::string LoadShader(const std::string& fileName);
 void CheckShaderError(GLuint shader, GLuint flag, bool isProgram, const char* msg);
 GLuint CreateShader(const std::string& text, GLenum type);
 GLuint m_program;
 GLuint m_shaders[NUM_SHADERS];
 GLuint m_uniforms[NUM_UNIFORMS];
};
}

Shader.cpp:

#include "Shader.h"
namespace deprecate{
Shader::Shader(const std::string& fileName){
 m_program = glCreateProgram();
 m_shaders[0] = CreateShader(LoadShader(fileName + ".vert"), GL_VERTEX_SHADER);
 m_shaders[1] = CreateShader(LoadShader(fileName + ".frag"), GL_FRAGMENT_SHADER);
 for (unsigned int i = 0; i < NUM_SHADERS; i++)
 glAttachShader(m_program, m_shaders[i]);
 glBindAttribLocation(m_program, 0, "position");
 glBindAttribLocation(m_program, 1, "texCoord");
 glBindAttribLocation(m_program, 2, "normal");
 glLinkProgram(m_program);
 CheckShaderError(m_program, GL_LINK_STATUS, true, "ERROR LNK shader program");
 glValidateProgram(m_program);
 CheckShaderError(m_program, GL_LINK_STATUS, true, "ERROR INV shader program");
 m_uniforms[TRANSFORM_U] = glGetUniformLocation(m_program, "transform");
 m_uniforms[TIME_U] = glGetUniformLocation(m_program, "time");
}
Shader::~Shader(){
 for (unsigned int i = 0; i < NUM_SHADERS; i++)
 {
 glDetachShader(m_program, m_shaders[i]);
 glDeleteShader(m_shaders[i]);
 }
 glDeleteProgram(m_program);
}
void Shader::Bind(){
 glUseProgram(m_program);
}
void Shader::UnBind(){
 glUseProgram(0);
}
void Shader::Update(const Transform& transform, const Camera& camera, float time){
 glm::mat4 model = camera.GetMVP() * transform.GetModel();
 glUniformMatrix4fv(m_uniforms[TRANSFORM_U], 1, GL_FALSE, &model[0][0]);
 glUniform1f(m_uniforms[TIME_U], time);
}
std::string Shader::LoadShader(const std::string& fileName){
 std::ifstream file;
 file.open((fileName).c_str());
 std::string output;
 std::string line;
 if (file.is_open()){
 while (file.good()){
 getline(file, line);
 output.append(line + "\n");
 }
 }
 else
 LOG_ERROR("ShaderLoader", "ERROR : Failed to load shader");
 file.close();
 return output;
}
void Shader::CheckShaderError(GLuint shader, GLuint flag, bool isProgram, const char* msg){
 GLint success = 0;
 GLchar error[1024] = { 0 };
 if (isProgram)
 glGetProgramiv(shader, flag, &success);
 else
 glGetShaderiv(shader, flag, &success);
 if (success == GL_FALSE){
 if (isProgram)
 glGetProgramInfoLog(shader, sizeof(error), NULL, error);
 else
 glGetShaderInfoLog(shader, sizeof(error), NULL, error);
 std::cout << msg << " : " << error << " " << std::endl;
 }
}
GLuint Shader::CreateShader(const std::string& text, GLenum type)
{
 GLuint shader = glCreateShader(type);
 if (shader == 0)
 LOG_ERROR("Shader", "ERROR : FAILED to create shader");
 const GLchar* shaderSource[1];
 shaderSource[0] = text.c_str();
 GLint lengths[1];
 lengths[0] = text.length();
 glShaderSource(shader, 1, shaderSource, lengths);
 glCompileShader(shader);
 CheckShaderError(shader, GL_COMPILE_STATUS, false, "ERROR : Shader compilation failed");
 return shader;
}
}

Answer 1

I assume you have access to a sensible compiler that supports C++11 or newer.

1. Don't place filesystem IO in your shader class.
2. Consider using glGetAttriblocation instead of glBindAttribLocation, but that's subjective.
3. Don't use static const unsigned int to store the length of an array, use std::array instead.
4. Your CheckShaderError is bipolar with regards to isProgram. Just create two methods.
5. If you are hard coding file extensions, don't call your variable fileName, because it is not a file's name.
6. glValidateProgram should be called just before glDrawArrays or glDrawElements. It tests whether the current 'setup' or 'context' is valid for drawing actions - in your constructor, the context is not setup. (OpenGL ES is generally more strict about this)
7. Your Update method seems useless, my own projects have many shaders with many different inputs (normal transform, shadow maps, light sources, etc).
8. Your error checking is weird. If LoadShader yields an error, you still continue executing and call CreateShader.
9. I don't believe OpenGL 3.1 is 'modern'. Version 4.3 is 'modern'.
10. If unsigned int i is used to access indices, just use size_t - it's less typing.

11. GLchar error[1024] don't assume 1024 bytes is enough. Just query for the log length, and allocate bytes based on that. This is what I did:

    GLint logLength = 0;
    glGetShaderiv(shader, GL_INFO_LOG_LENGTH, &logLength);
    if (logLength > 0) {
     auto log = std::unique_ptr<char>(new char[logLength]);
     glGetShaderInfoLog(shader, logLength, &logLength, log.get());
     printf("Shader compile log:\n%s", log.get());
    }
    

12. const GLchar* shaderSource[1]; is the same as const GLchar* shaderSource;
13. Your destructor deletes stuff, without validating whether they were created to begin with.
14. I think your Bind and UnBind methods are vague. Pretty much anything in OpenGL needs to be bound.
15. Calling your namespace deprecate is possibly confusing.
16. else LOG_ERROR("ShaderLoader", "ERROR : Failed to load shader"); worries me. Notice the missing parenthesis. What do you think will happen if you #undef your macro?
17. Having no assignment operator, default constructor, copy constructor nor a move constructor, can you use an std::vector of Shaders? This is more a broad thing about usability of your classes. I like things flexible, but that's subjective.

• MattMatt: 1) Thanks ! Yes, I have g++ 11, visual c++ 11
• MattMatt: 7) "Your Update method seems useless" : OK, but how should I update my camera, transform, time uniforms... then ?
• MattMatt: 9) I consider it has modern, compared to the other versions of opengl; there were really big changes
• MattMatt: Can you show me your shader class' update, and check error parts, so I can see what you're referring to ? ( OR if you can, the entire class would be a good idea ! )