Shader class in OpenGL

Question

Im am currently wrapping all OpenGL ressources (which are simply a GLuint) into classes that own and manage the deletion of them, to make it possible to use OpenGL in an object oriented fassion. Since these classes manage a ressource that get's destroyed in their destructor i know the rule of five is something to think about. This is how my Shader classes look like:

Shader.h :

#pragma once
#include <glad/glad.h>
#include <string>
class Shader
{
public:
 Shader(GLuint type, const std::string& shaderPath);
 virtual ~Shader();
 Shader(const Shader& s) = delete;
 Shader& operator = (const Shader& s) = delete;
 Shader(Shader&&) = default;
 Shader& operator = (Shader&&) = default;
private:
 /*
 The Shader objects are passed to the constructor 
 of ShaderProgram that compiles the glShaderProgram -> needs id 
 */
 friend class ShaderProgram; // 
 GLuint id = 0;
};
class FragmentShader : public Shader
{
public:
 FragmentShader(const std::string& shaderPath) : Shader(GL_FRAGMENT_SHADER, shaderPath) {};
};
class VertexShader : public Shader
{
public:
 VertexShader(const std::string& shaderPath) : Shader(GL_VERTEX_SHADER, shaderPath) {};
};

Shader.cpp :

Shader::Shader(GLuint type, const std::string& shaderPath)
{
 std::ifstream fstram;
 std::stringstream sstream;
 fstram.exceptions (std::ifstream::failbit | std::ifstream::badbit); 
 fstram.open(shaderPath); 
 sstream << fstram.rdbuf();
 fstram.close(); 
 id = glCreateShader(type);
 auto data = sstream.str();
 const char* dataPtr = data.c_str();
 glShaderSource(id, 1, &dataPtr, NULL);
 glCompileShader(id);
 int result = 0;
 glGetShaderiv(id, GL_COMPILE_STATUS, &result);
 if(result == 0)
 {
 char infolog[1024];
 glGetShaderInfoLog(id, 1024, NULL, infolog);
 throw std::runtime_error(infolog);
 }
}
Shader::~Shader()
{
 glDeleteShader(id);
}

Use them like this:

try
{
 VertexShader vs("Path to file");
 FragmentShader fs("Path to file");
}
catch(const std::exception& e)
{
 std::cerr << e.what() << std::endl;
}

Because i am planning to implement classes for the ofther OpenGL ressources like buffers, textures, etc in a similar fassion i would like some feedback. Any insight on mistakes that i made or how to improve my code are very appreciated. And do i need to declare move assignment and move constructor in the derived shader classes too?

Answer 1

Is it really necessary to split vertex and fragment shaders into their own type? It causes the type to go from 4 bytes to 16 bytes on a typical 64 bit processor, just for it to store a vtable ptr for the destructor that's never going to do anything different.

They could privately inherit and not have a virtual destructor, but I would rather they just be a single class and let incorrectly assigning shaders to a program be a runtime error. There's already a potential failure point when creating the shader program, and the gl error should alert the client to what went wrong, so it's not introducing a new point of failure.

You can't use the default move functions because they won't zero-out the id, and glDeleteShader will be called twice on the same id.

Consider using a factory function to create shaders from file instead of doing file IO in the constructor. It will make it easier to test, and it's also not obvious that this is a blocking function that shouldn't be called on UI threads.

It also makes it more portable to possibly use in a context that doesn't want to use fstreams for doing file IO, for example if you had zipped shaders or hosted them on a cloud it would make more sense to pass in a byte array which is what glShaderSource will expect.

If you're going to use a path constructor, consider std::filesystem::path.