2
\$\begingroup\$

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?

\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.