OpenGL shader abstraction class

Question

I have implemented a class to abstract the process of building a shader program in OpenGL (For now it does not deal with uniforms).

I would like some feedback on the coding style, and more specifically the structure. I have implemented a bunch of small and private methods in the class to handle various aspects of compilation - but I am uncertain of when/where to create small functions, and when to just do it all in once place.

Shader.hpp

#pragma once

#include <GL/glew.h>

#include <iostream>
#include <fstream>
#include <string>
#include <vector>

class Shader
{
public:
    enum Type : unsigned int
    {
        Vertex = GL_VERTEX_SHADER,
        Fragment = GL_FRAGMENT_SHADER
    };

public:
    Shader(std::string vertex_shader_path, std::string fragment_shader_path);
    ~Shader();

    void use();

private:
    unsigned int program;
    unsigned int vertex_shader;
    unsigned int fragment_shader;

private:
    std::string get_contents(std::string file_path);

    unsigned int create_shader(const char* shader_code, Type shader_type);
    unsigned int create_program();

    bool check_shader_compilation_status(unsigned int shader);
    bool check_program_linking_status(unsigned int program);

    void print_error(std::vector<char> error_message, std::string info);
};

Shader.cpp

#include "Shader.hpp"

Shader::Shader(std::string vertex_shader_path, std::string fragment_shader_path)
{
    vertex_shader = create_shader(get_contents(vertex_shader_path).c_str(), Type::Vertex);
    fragment_shader = create_shader(get_contents(fragment_shader_path).c_str(), Type::Fragment);
    program = create_program();
}


Shader::~Shader()
{
    glDeleteShader(vertex_shader);
    glDeleteShader(fragment_shader);
    glDeleteProgram(program);
}


void Shader::use()
{
    glUseProgram(program);
}


std::string Shader::get_contents(std::string file_path)
{
    std::ifstream file(file_path);
    return std::string(std::istreambuf_iterator<char>(file), std::istreambuf_iterator<char>());
}


unsigned int Shader::create_shader(const char* shader_code, Type shader_type)
{
    unsigned int shader = glCreateShader(shader_type);
    glShaderSource(shader, 1, &shader_code, nullptr);
    glCompileShader(shader);

    if (!check_shader_compilation_status(shader))
    {
        glDeleteShader(shader);
        return 0;
    }
    else
    {
        return shader;
    }
}


unsigned int Shader::create_program()
{
    unsigned int program = glCreateProgram();
    glAttachShader(program, vertex_shader);
    glAttachShader(program, fragment_shader);
    glLinkProgram(program);

    if (!check_program_linking_status(program))
    {
        glDeleteProgram(program);
        return 0;
    }
    else
    {
        return program;
    }
}


bool Shader::check_shader_compilation_status(unsigned int shader)
{
    int is_compiled = 0;
    glGetShaderiv(shader, GL_COMPILE_STATUS, &is_compiled);

    if (!is_compiled)
    {
        int max_length = 0;
        glGetShaderiv(shader, GL_INFO_LOG_LENGTH, &max_length);

        std::vector<char> error_log(max_length);
        glGetShaderInfoLog(shader, max_length, &max_length, &error_log[0]);

        print_error(error_log, "Shader compalation failed");
        return false;
    }
    else
    {
        return true;
    }
}


bool Shader::check_program_linking_status(unsigned int program)
{
    int is_compiled = 0;
    glGetProgramiv(program, GL_LINK_STATUS, &is_compiled);

    if (!is_compiled)
    {
        int max_length = 0;
        glGetProgramiv(program, GL_INFO_LOG_LENGTH, &max_length);

        std::vector<char> error_log(max_length);
        glGetShaderInfoLog(program, max_length, &max_length, &error_log[0]);

        print_error(error_log, "Program linking failed");
        return false;
    }
    else
    {
        return true;
    }
}

void Shader::print_error(std::vector<char> error_message, std::string info)
{
    std::cout << info << ": ";

    for (char letter : error_message)
    {
        std::cout << letter;
    }
}

Im primairly concered with simply bettering my coding skills - so any suggestions are appreciated.

Answer 1

std::strings can be passed by const& (or as a std::string_view) rather than by value if we don't need to make a copy, e.g.:

Shader(std::string const& vertex_shader_path, std::string const& fragment_shader_path)

std::string get_contents(std::string const& file_path);

void Shader::print_error(std::vector<char> const& error_message, std::string const& info)

Incidentally, using a std::string instead of a std::vector<char> for the error message would make printing easier.

---

Member functions that don't change member data should be const.

void use() const;

Member functions that don't require access to member data should be static.

static std::string get_contents(std::string const& file_path);
... and all of the others!

---

There are quite a few error cases we have to handle:

• File opening fails.
• Reading from the file fails.
• Shader compilation fails.
• Program linking fails.

Currently the code continues attempting to create the shader program when an earlier step fails, even though it won't succeed. This adds complexity, since we have to check that everything will "work" (in this case fail gracefully) with our invalid state.

While it might be helpful to show the compilation errors for every shader object, we probably don't want to try linking the program - we'll just be generating an OpenGL error, as well as extra noise from the linking failure in our logs.

Similarly, if we fail to read from a file, we should output an appropriate error message, and not try to compile a shader object or link the program.

---

I'd suggest using the specified OpenGL types for interacting with OpenGL. e.g. GLuint for shader object / program ids, GLint for compile status, etc.

This is safer and more portable, and also makes the purpose of each variable more obvious.

---

We don't need to immediately delete shaders that fail to compile (or programs that fail to link). The Shader class destructor will still do that for us (it might even be useful to keep the ID around for debugging). So we can simpify a bit:

unsigned int Shader::create_shader(const char* shader_code, Type shader_type)
{
    GLuint shader = glCreateShader(shader_type);
    glShaderSource(shader, 1, &shader_code, nullptr);
    glCompileShader(shader);

    check_shader_compilation_status(shader);

    return shader;
}

---

This looks fine for a simple shader class, but you might run into a few issues in future:

• A shader object can be composed from multiple shader source strings / files (which is very useful to avoid unnecessary duplication of shader code).

• There are other types of shader object (tessellation control / evaluation, geometry, compute) that may or may not be present in the shader program.

It would be more flexible to load the shader sources from their files outside of the Shader class and pass them in. Or even to add a separate ShaderObject class, and create the Shader from a std::vector<ShaderObject>.

But maybe that's more than you need right now.

---

Technically, we should write error messages to stderr, not stdout, which means using std::cerr or std::clog instead of std::cout. It doesn't really matter for a graphical program though.

---

if (!is_compiled)
{
    // ...
    return false;
}
else
{
    return true;
}

We don't need the else statement here, since we return from the other branch. We can just return true and avoid the brackets and the extra indent. (This is more of an issue when there's more code in the else branch).