Modern Opengl GLSL Shader Class

Question

I've finally written my own Shader class. It's a basic implementation based on a tutorial, and it works perfectly. Any suggestions on how to improve, or correct my code are welcome!

Shader.h

#pragma once
#include <GL/glew.h>
#include <map>
#include <string>
#include "LogManager.h"
#include "bindable.h"
#include "disposable.h"
#define NUM_SHADER_TYPES 4
class Shader : public Bindable, public Disposable
{
public:
 Shader();
 virtual ~Shader();
 void loadFromText(GLenum type, const std::string& src);
 void loadFromFile(GLenum type, const char* fileName);
 void loadFromPreCompiledText(GLenum type, const std::string& src){}
 void loadFromPreCompiledFile(GLenum type, const char* fileName){}
 void CreateAndLink();
 void RegisterAttribute(const char* attrib);
 void RegisterUniform(const char* uniform);
 GLuint GetProgramID() const;
 ///accesses elements : shaders/uniforms;
 GLuint GetAttribLocation(const char* attrib);
 GLuint operator[](const char* attrib);
 GLuint GetUniformLocation(const char* unif);
 GLuint operator()(const char* unif);
 virtual void Bind() const;
 virtual void UnBind() const;
 virtual void Dispose();
private:
 enum ShaderType { VERTEX_SHADER, FRAGMENT_SHADER, GEOMETRY_SHADER, PIXEL_SHADER};
 GLuint _program ;
 int _numShaders;
 GLuint _shaders[4]; /// VERTEX, FRAGMENT, GEOMETRY AND PIXEL_SHADERS !
 std::map<std::string, GLuint> _attribList;
 std::map<std::string, GLuint> _unifLocationList;
};

Shader.cpp

#include "Shader.h"
#include "LogManager.h"
#include "fstream"
Shader::Shader()
 :_program(0), _numShaders(0)
{
 _shaders[VERTEX_SHADER] = 0;
 _shaders[FRAGMENT_SHADER] = 0;
 _shaders[GEOMETRY_SHADER] = 0;
 _shaders[PIXEL_SHADER] = 0;
 _attribList.clear();
 _unifLocationList.clear();
}
Shader::~Shader(){
 _attribList.clear();
 _unifLocationList.clear();
}
void Shader::loadFromText(GLenum type, const std::string& text){
 GLuint shader = glCreateShader(type);
 const char* cstr = text.c_str();
 glShaderSource(shader, 1, &cstr, nullptr);
 ///compile + check shader load status
 GLint status;
 glCompileShader(shader);
 glGetShaderiv(shader, GL_COMPILE_STATUS, &status);
 if(status == GL_FALSE){
 GLint infoLogSize;
 glGetShaderiv(shader, GL_INFO_LOG_LENGTH, &infoLogSize);
 GLchar *infoLog = new GLchar[infoLogSize];
 glGetShaderInfoLog(shader, infoLogSize, nullptr, infoLog);
 LOG_ERROR("Shader", infoLog);
 delete [] infoLog;
 }
 _shaders[_numShaders++]=shader;
}
void Shader::CreateAndLink(){
 _program = glCreateProgram();
 if(_shaders[VERTEX_SHADER] != 0)
 glAttachShader(_program, _shaders[VERTEX_SHADER]);
 if(_shaders[FRAGMENT_SHADER] != 0)
 glAttachShader(_program, _shaders[FRAGMENT_SHADER]);
 if(_shaders[GEOMETRY_SHADER] != 0)
 glAttachShader(_program, _shaders[GEOMETRY_SHADER]);
 if(_shaders[PIXEL_SHADER] != 0)
 glAttachShader(_program, _shaders[PIXEL_SHADER]);
 ///link + check
 GLint status;
 glLinkProgram(_program);
 glGetProgramiv(_program, GL_LINK_STATUS, &status);
 if(status == GL_FALSE){
 GLint infoLogSize;
 glGetProgramiv(_program, GL_INFO_LOG_LENGTH, &infoLogSize);
 GLchar *infoLog = new GLchar[infoLogSize];
 glGetProgramInfoLog(_program, infoLogSize, nullptr, infoLog);
 delete [] infoLog;
 }
 glDetachShader(_program, _shaders[VERTEX_SHADER]);
 glDetachShader(_program, _shaders[FRAGMENT_SHADER]);
 glDetachShader(_program, _shaders[GEOMETRY_SHADER]);
 glDetachShader(_program, _shaders[PIXEL_SHADER]);
 glDeleteShader(_shaders[VERTEX_SHADER]);
 glDeleteShader(_shaders[FRAGMENT_SHADER]);
 glDeleteShader(_shaders[GEOMETRY_SHADER]);
 glDeleteShader(_shaders[PIXEL_SHADER]);
}
void Shader::Bind() const{
 glUseProgram(_program);
}
void Shader::UnBind() const{
 glUseProgram(0);
}
void Shader::RegisterAttribute(const char* attrib){
 _attribList[attrib] = glGetAttribLocation(_program, attrib);
}
void Shader::RegisterUniform(const char* unif){
 _unifLocationList[unif] = glGetUniformLocation(_program, unif);
}
GLuint Shader::GetAttribLocation(const char* attrib){
 return _attribList[attrib];
}
GLuint Shader::operator[](const char* attrib){
 return _attribList[attrib];
}
GLuint Shader::GetUniformLocation(const char* unif){
 return _unifLocationList[unif];
}
GLuint Shader::operator()(const char* unif){
 return _unifLocationList[unif];
}
GLuint Shader::GetProgramID() const{ return _program; }
void Shader::loadFromFile(GLenum which, const char* fileName){
 std::ifstream fparser;
 fparser.open(fileName, std::ios_base::in);
 if(fparser){
 ///read + load
 std::string buffer(std::istreambuf_iterator<char>(fparser), (std::istreambuf_iterator<char>()));
 loadFromText(which, buffer);
 }
 else{
 LOG_ERROR_INFO("Shader", "Invalid fileName path", fileName);
 }
}
void Shader::Dispose(){
 glDeleteProgram(_program);
 _program = -1;
}

Answer 1

Comments on design:

Inheriting from Bindable and Disposable is an interesting approach, though I don't think I'd do it myself. It will add more complexity to your code and more header/source files to maintain. You should consider that when making the decision. There aren't that many practical benefits that I can think of, besides the interface template for new classes to follow. In my opinion, a more useful base class would be a RenderResource that combines all the common properties and behavior of renderer (OpenGL) resources. Such base class could indeed make your implementation cleaner and facilitate storage of resource objects, like shaders and textures, to name a few.

Going into more specific details, I didn't like these guys:

GLuint operator[](const char* attrib);
GLuint operator()(const char* unif);

From the parameter names I can tell that the first one is a wrapper to GetAttribLocation() and the second to GetUniformLocation(). But how about the calling code? It will be impossible for an outsider to distinguish something like the following:

GLuint normal = shader["normal_vector"];
// bunch of GL calls
// ...
GLuint color = shader("diffuse_color");
// bunch of GL calls
// ...
// Which of the above was operating on a uniform var and which on a vertex attrib?

Unless one takes the trouble to further read down the code and figure out if the integer handle refers to a uniform variable or a vertex attribute, based on the GL calls, it is impossible to quickly tell the difference.

My first advice here would be to just throw away both operator overloads. Second advice is to define two small value classes: ShaderUniform and VertexAttribute, to provide type safety and context. A GLuint is used to describe any OpenGL resource (unfortunate, but understandable in a plain C API).

class ShaderUniform
{
public:
 // Not much to see here, perhaps just a
 // `getGLHandle()` to access the underlaying `GLuint`.
 // The sole purpose of this class is to provide type
 // information to differentiate from, say a `VertexAttribute`.
 // Also, keep it simple to that it can still be used as a
 // value type.
private:
 GLuint glHandle;
 // Whatever other auxiliar data you might need.
};
// Then in the Shader class, `GetUniformLocation()` would return its 
// unique type, rather then the opaque `GLuint`:
//
ShaderUniform GetUniformLocation() const { ... }

Other details in your current implementation:

• Targeting C++11, methods implemented from a base class can now be annotated with the override specifier. This new keyword will improve compiler diagnostics if you accidentally overload and shadow a base class method. It might also aid de-virtualization optimizations done by the compiler. Also note that you don't need to specify virtual again in the child class, unless that class will itself be further inherited and extended.

  void Bind() const override;
  void UnBind() const override;
  void Dispose() override;
  

• You have mixed method naming conventions. E.g.: loadFromText() and CreateAndLink(). It would be nicer to be consistent. camelCase is very commonly used in C++ code for anything that can have its memory address taken. This includes variables and functions/methods. While PascalCase is very often applied to type names (I.e.: classes, structs, enums).

• I see that both the loadFromPreCompiled*() methods are currently not implemented. This is fine but it would be a smart idea to let the user know that, in case one of the methods is called while still not implemented. A NotImplementedError exception would be a good idea. If not, at least an assert.

• Take a look at enum class (scoped enumerations, C++11). The C++ community is in general abandoning the older C-style enum.

• No apparent reason for having NUM_SHADER_TYPES as a global constant. And certainly no reason whatsoever for using a #define. constexpr is the C++11 way of declaring compile-time constants. If your compiler doesn't happen to support it yet, then const will suffice.

• You have two conflicting definitions in you ShaderType enum: FRAGMENT_SHADER and PIXEL_SHADER. This is also reflecting in all of your code. A Fragment Shader is the same thing as a Pixel Shader. The diference is that the latter term is used by Microsoft's D3D, while the former is the term used by OpenGL. Both name the same pipeline stage. The term used by OpenGL is more semantically correct, IMHO, since a fragment processed by the shader doesn't necessarily becomes a pixel on the screen. Read this discussion for more.

  Anyways, you have these several calls referring to both types when they are actually just one. So you have some cleanup there to consolidate both.

• Unneeded map clearing in the destructor:

  Shader::~Shader(){
   _attribList.clear();
   _unifLocationList.clear();
  }
  

  A std::map is an automatic container. It will be cleared without your intervention when the parent object gets destroyed. This is useless at best, repeated work in the worse case.

  What I would be calling in the destructor though, is the Dispose() method. If users of Shader forget to call it manually, you run the risk of leaking a shader program object!

• MattMatt: 1) Thanks :°) By the way, I have doubts now if my shader class disposes its shaders correctly... Do I need to worry ?
• MattMatt: 2) Yes, I agree that inheriting from Bindable and Disposable is a "werid" approach; but I'm trying to design a library, so I want the user to know that he must dispose the shader ! (otherwise, the destructor takes care of it...)
• MattMatt: 3) After playing around a bit with my class, I noticed a problem : calling LoadFromFile twice with the same GLSL file type leads to problems ! Also, calling it more then 4 times leads to BIG problems : Any ideas on how to fix it ?
• glampert: @MattMatt, It will leak the shader program object if no one calls Dispose(). I think this is an unnecessary burden on the user when you can just call it inside the destructor of Shader.
• MattMatt: OK ! I saw that you talked about spliting things into 2 structs : ShaderUniform, and VertexAttribute : can you give me an idea of its implementation