Shader Program OpenGL

Question

I am new to OpenGL learning it on amazing website learnopengl.com I wanted to a convenient way to use shader programs thats why I created this struct, please review it.

ShaderProgram.h

#ifndef HELLO_TRIANGLE_SHADERPROGRAM_H
#define HELLO_TRIANGLE_SHADERPROGRAM_H

#include <glad/glad.h>
#include <GLFW/glfw3.h>
#include <fstream>
#include <string>

#include <variant>


struct ShaderProgram {

    void loadFromFile(const char* vertexFile, const char* fragmentFile);
    void loadFromSource(const char* vertexSource, const char* fragmentSource);
    unsigned int getProgram() const;
    bool isSuccess() const;
    const std::string_view& getErrorMessage() const;

private:
    std::variant<unsigned int, std::string_view> data;
};

#endif //HELLO_TRIANGLE_SHADERPROGRAM_H

ShaderProgram.cpp

#include "ShaderProgram.h"

void ShaderProgram::loadFromSource(const char *vertexSource, const char *fragmentSource)
{
    unsigned int vertexShaderId = glCreateShader(GL_VERTEX_SHADER);
    glShaderSource(vertexShaderId, 1, &vertexSource, nullptr);
    glCompileShader(vertexShaderId);

    int success;
    char log[512];
    glGetShaderiv(vertexShaderId, GL_COMPILE_STATUS, &success);

    if (!success)
    {
        glGetShaderInfoLog(vertexShaderId, 512, nullptr, log);
        data = log;
        return;
    }

    unsigned int fragmentShaderId = glCreateShader(GL_FRAGMENT_SHADER);
    glShaderSource(fragmentShaderId, 1, &fragmentSource, nullptr);
    glCompileShader(fragmentShaderId);

    glGetShaderiv(fragmentShaderId, GL_COMPILE_STATUS, &success);

    if (!success)
    {
        glGetShaderInfoLog(fragmentShaderId, 512, nullptr, log);
        data = log;
        return;
    }

    unsigned int program = glCreateProgram();
    glAttachShader(program, vertexShaderId);
    glAttachShader(program, fragmentShaderId);

    glLinkProgram(program);

    glGetProgramiv(program, GL_LINK_STATUS, &success);

    if (!success)
    {
        glGetProgramInfoLog(program, 512, nullptr, log);
        data = log;
        return;
    }

    data = program;
    glDeleteShader(vertexShaderId);
    glDeleteShader(fragmentShaderId);
}

void ShaderProgram::loadFromFile(const char *vertexFilePath, const char *fragmentFilePath)
{
    try
    {
        std::ifstream vertexFile(vertexFilePath);
        std::string vertexFileContent((std::istreambuf_iterator<char>(vertexFile)), std::istreambuf_iterator<char>());

        std::ifstream fragmentFile(fragmentFilePath);
        std::string fragmentFileContent((std::istreambuf_iterator<char>(fragmentFile)),
                                        std::istreambuf_iterator<char>());

        loadFromSource(vertexFileContent.c_str(), fragmentFileContent.c_str());
    } catch (std::exception &e)
    {
        data = e.what();
    }
}

unsigned int ShaderProgram::getProgram() const
{
    return std::get<unsigned int>(data);
}

bool ShaderProgram::isSuccess() const
{
    return std::holds_alternative<unsigned int>(data);
}

const std::string_view &ShaderProgram::getErrorMessage() const
{
    return std::get<std::string_view>(data);
}

Please review this and advise is there a better way to this. Thanks in advance.

UPDATE: Implementing suggestions mentioned in the answer, please review this aswell as development is a constant process. Thanks all in advance.

Shader.h

namespace shader_program
{

    enum class ShaderType
    {
        VERTEX,
        FRAGMENT
    };

    enum class SourceType
    {
        RAW,
        FILE
    };


    struct Shader
    {
        Shader(const std::string_view &source, ShaderType shaderType, SourceType sourceType);

        ~Shader();

        unsigned int getId() const;

    private:
        unsigned int shader = 0;
    };

    struct ShaderProgram
    {
        ShaderProgram(const std::string_view &vertexSource, const std::string_view &fragmentSource,
                      SourceType vertexSourceType, SourceType fragmentSourceType);

        ~ShaderProgram();

        void use() const;

    private:
        unsigned int program;
    };

} // shader_program

Shader.cpp

shader_program::Shader::Shader(const std::string_view &source, shader_program::ShaderType shaderType,
                               shader_program::SourceType sourceType)
{
    auto content = source.data(); // Assuming raw data
    if (sourceType == SourceType::FILE)
    {
        std::ifstream ifs(source.data());
        if (!ifs.good())
        {
            throw std::runtime_error("Invalid shader file");
        }
        content = std::string((std::istreambuf_iterator<char>(ifs)), std::istreambuf_iterator<char>()).data();
        if (!ifs.eof())
        {
            throw std::runtime_error("Invalid shader file");
        }
    }
    shader = glCreateShader(shaderType == ShaderType::VERTEX ? GL_VERTEX_SHADER : GL_FRAGMENT_SHADER);
    glShaderSource(shader, 1, &content, nullptr);
    glCompileShader(shader);

    int success;

    glGetShaderiv(shader, GL_COMPILE_STATUS, &success);

    if (!success)
    {
        int len;
        glGetShaderiv(shader, GL_INFO_LOG_LENGTH, &len);
        if (len == 0)
        {
            glDeleteShader(shader);
            shader = 0;
            throw std::runtime_error("Could not compile shader and could not obtain any log");
        }
        std::vector<char> log(len);
        glGetShaderInfoLog(shader, log.size(), &len, log.data());
        glDeleteShader(shader);
        shader = 0;
        throw std::runtime_error({log.begin(), log.end()});
    }

}

shader_program::Shader::~Shader()
{
    // No need to delete not created shader
    if (shader != 0)
    {
        glDeleteShader(shader);
    }
}

unsigned int shader_program::Shader::getId() const
{
    return shader;
}

shader_program::ShaderProgram::ShaderProgram(const std::string_view &vertexSource,
                                             const std::string_view &fragmentSource,
                                             shader_program::SourceType vertexSourceType,
                                             shader_program::SourceType fragmentSourceType)
{
    Shader vertexShader(vertexSource, ShaderType::VERTEX, vertexSourceType);
    Shader fragmentShader(fragmentSource, ShaderType::FRAGMENT, fragmentSourceType);

    program = glCreateProgram();

    if (program == 0)
    {
        throw std::runtime_error("Could not create a shader program");
    }

    glAttachShader(program, vertexShader.getId());

    int error = glGetError();

    if (error != GL_NO_ERROR)
    {
        std::string errorStr = "Attaching vertex shader failed with code: " + std::to_string(error);
        glDeleteProgram(program);
        program = 0;
        throw std::runtime_error(errorStr);
    }

    glAttachShader(program, fragmentShader.getId());

    error = glGetError();

    if (error != GL_NO_ERROR)
    {
        std::string errorStr = "Attaching fragment shader failed with code: " + std::to_string(error);
        glDeleteProgram(program);
        program = 0;
        throw std::runtime_error(errorStr);
    }

    glLinkProgram(program);

    int success;

    glGetProgramiv(program, GL_LINK_STATUS, &success);

    if (!success)
    {
        int len;
        glGetProgramiv(program, GL_INFO_LOG_LENGTH, &len);
        if (len == 0)
        {
            glDeleteProgram(program);
            program = 0;
            throw std::runtime_error("Could not link program and could not obtain any log");
        }
        std::vector<char> log(len);
        glGetProgramInfoLog(program, log.size(), &len, log.data());
        glDeleteProgram(program);
        program = 0;
        throw std::runtime_error({log.begin(), log.end()});
    }

}

shader_program::ShaderProgram::~ShaderProgram()
{
    // No need to delete not created program
    if (program != 0)
    {
        glDeleteProgram(program);
    }
}

void shader_program::ShaderProgram::use() const
{
    if (program == 0)
    {
        throw std::runtime_error("Cannot use not created program");
    }
    glUseProgram(program);
}

Thanks again.

Answer 1

Use of std::string_view

I see you are using std::string_view, but unfortunately in a completely incorrect way. A std::string_view is a non-owning view of a string. This means the string itself has to be stored somewhere else. When you call getShaderInfoLog(), the log message is stored into the local variable log[], and the std::string_view created from that will still point to the local variable. When loadFromSource() returns however, the variable log[] no longer exists, and the std::string_view points to memory which might no longer contain the string. The solution here is to make data contain a std::string instead of a std::string_view.

Where you should have used std::string_view instead is for the function parameters, like loadFromFile() and loadFromSource().

Error handling

I see you have thought about handling errors, but again it has not been done in a good way. You are using a try-catch block in loadFromFile(), but there is no guarantee that exceptions are thrown on all possible errors. For example, if you try to pass a non-existing filename, then that code will not throw any exception on Linux. At the very least, check that vertexFile.eof() == true after reading into vertexFileContent, and similar for fragmentFile.

Using a std::variant to store success or an error message is not the worst idea, but it has issues. Here it works because the two types are different, but what if the success type would be the same as the error type? C++23 will introduce std::expected, which is similar to a std::variant, but avoids the issue by making it very explicit what the success and error types are.

But even then, std::expected is meant to be used as a return type. By making loadFromFile return void, you are putting distance between performing an action and checking whether it succeeded. It would be much better if loadFromFile() would immediately return whether it was succesful or not, in such a way that the caller has to handle it immediately. So I would either have it return a std::expected (or a std::variant until we wait for C++23), or perhaps even better would be to throw an exception on error.

Also consider that almost everything can go wrong in a program. You did take into account that compiling and linking the shader source could go wrong, but you did not account for glGetShaderInfoLog() itself possibly returning an error. In that case, log[] might not have been written to. Consider zeroing the first character of log[] just to prevent a possible out-of-bounds read in this admittedly unlikely scenario.

Make the struct more useful

Your struct ShaderProgram implements the bare minimum for loading and compiling a shader program, and then getting the handle. But it could do much more:

• Turn loadFromFile() and loadFromSource() into constructors. This will save some lines of code for the caller.
• Add a destructor, so the code using it doesn't have to call glDeleteProgram() manually.
• Maybe add a use() member function that calls glUseProgram().

Consider that with the above, you could write:

{
    ShaderProgram program(vertexSource, fragmentSource);
    program.use();

    while (true) {
        // render frames
        ...
    }
}