Modern OpenGL wrapper abstractions (shaders)

Question

I am experienced with other languages, but not so much with the latest C++ standards. I was wondering if anyone had any tips on how to improve my code or any critique on my implementation. This is not designed to be used as an external or public library and I hope my use of the initializer lists are suitable (unwrapped pointers as they have no owner and aren't copied or stored).

I'm using spdlog and the Logger typedef is a typedef std::shared_ptr<spdlog::logger> Logger; in another (common) header.

program.cpp

#include <render/program.h>

Program::Program(Logger logger) :
    logger(std::move(logger)) {}

Program::operator GLuint() const {
    return program_id;
}

void Program::create(std::initializer_list<Shader *> shaders) {
    program_id = glCreateProgram();

    for (const Shader *shader : shaders) {
        glAttachShader(program_id, (GLuint) *shader);
    }

    glLinkProgram(program_id);

    GLint link_status;
    glGetProgramiv(program_id, GL_LINK_STATUS, &link_status);

    if (!link_status) {
        logger->error("Failed to link program");

        GLint log_length;
        glGetProgramiv(program_id, GL_INFO_LOG_LENGTH, &log_length);

        if (log_length > 0) {
            GLchar info_log[log_length - 1];
            glGetProgramInfoLog(program_id, sizeof(info_log), nullptr, info_log);

            logger->error((const char *) info_log);
        }
    }

    for (const Shader *shader : shaders) {
        glDetachShader(program_id, (GLuint) *shader);
    }
}

void Program::destroy() const {
    glDeleteProgram(program_id);
}

void Program::use() const {
    glUseProgram(program_id);
}

program.h

#pragma once

#include <engine.h>
#include <render/shader.h>
#include <initializer_list>

class Program {
public:
    explicit Program(Logger logger);

    explicit operator GLuint() const;

    void create(std::initializer_list<Shader *> shaders);

    void destroy() const;

    void use() const;

private:
    const Logger logger;
    GLuint program_id;
};

shader.cpp

#include <render/shader.h>
#include <fstream>

Shader::Shader(Logger logger, std::string path) :
    logger(std::move(logger)), path(std::move(path)) {}

Shader::operator GLuint() const {
    return shader_id;
}

void Shader::create(GLenum type) {
    std::ifstream input_stream(path);

    if (!input_stream) {
        logger->error("Unable to open shader file");

        return;
    }

    std::string source;
    char buffer;

    source.reserve(120);

    while (input_stream.get(buffer)) {
        source += buffer;
    }

    shader_id = glCreateShader(type);

    const GLchar *sources[] = {source.c_str()};
    glShaderSource(shader_id, 1, sources, nullptr);
    glCompileShader(shader_id);

    GLint compile_status;
    glGetShaderiv(shader_id, GL_COMPILE_STATUS, &compile_status);

    if (!compile_status) {
        logger->error("Failed to compile shader: {}", path);

        GLint log_length;
        glGetShaderiv(shader_id, GL_INFO_LOG_LENGTH, &log_length);

        if (log_length > 0) {
            GLchar info_log[log_length - 1];
            glGetShaderInfoLog(shader_id, sizeof(info_log), nullptr, info_log);

            logger->error((const char *) info_log);
        }
    }
}

void Shader::destroy() const {
    glDeleteShader(shader_id);
}

shader.h

#pragma once

#include <engine.h>
#include <string>

class Shader {
public:
    Shader(Logger logger, std::string path);

    explicit operator GLuint() const;

    void create(GLenum type);

    void destroy() const;

private:
    const Logger logger;
    const std::string path;
    GLuint shader_id;
};

Answer 1

1. Use glDetachShader() after linking the program.

   Shaders are ref-counted on the driver side and prevented from cleanup until they have been detached from all used programs. You are hogging GPU resources for no good reason.

2. GLchar info_log[log_length - 1]; : eeep!

   On top of being dangerous (you could possibly end up allocating megabytes of memory on the stack), it's not even legal C++. Just use a std::vector<char>.

   Also, that -1 gives me the heeby-jeebies. It's just one byte, copy the null terminator.

3. Your way of loading an entire stream in a string is pretty awful and slow. You should figure out the length, make a single allocation, and read all the data at once like so:

   input_stream.seekg(0, input_stream.end);
   auto length = input_stream.tellg();
   input_stream.seekg(0, input_stream.beg);
   std::vector<char> source(length);
   input_stream.read(source.data(), length);
   

4. Holding on to loggers as member variables is pretty bad in general. I have never seen a production system that requires that kind of granularity, and the multithreaded handling of these shared_ptr<>s could come and bite you in the future. Loggers are definitely one of those things that belong in singletons/global variables in my opinion.

   Or even better: throw an exception with the error as its content. Let higher-level code handle the logging.

5. The create() functions should be invoked in the constructors (or just be the constructors). That's the whole point of RAII: bookending resource management in constructors and destructors. An additional benefit is that you won't need to keep the path and loggers around, just the GLuint.

6. On that same note, destroy() should just be the destructor, for the same reasons as above.

7. Speaking of destroy: in your current version, what would happen if destroy() is called and create() was not? Any sequence of invocations of the public interface of the class should be expected and handled (if only with an assert at worst).

8. void create(std::initializer_list<Shader *> shaders);
   

   This is pretty constraining interface-wise. It's okay to have an initializer_list-based overload, but your primary interface should be a std::vector<Shader*>, or even better: a pair of iterators (or range if your code base has them).

9. I don't like the casts to GLuint as accessor for the programs. If you are going to have a wrapper, the wrapper should be handling all interactions that require the native handle if possible, and selectively give access using friend where absolutely necessary.

10. Finally, a heads up: this is pretty bare bones. You will find that once you start adding data-driven handling of Uniform Buffers Objects (you are talking about modern GL, after all) and vertex attribute bindings, a major refactor of all this code will almost certainly be needed.

Edit elaborating here instead of the comments:

I've been told not to include too much work in constructors, and considering it's calling GL functions I figured that was considered a lot of work.

The primary role of a constructor is to establish the class invariant (the states the object is allowed to be in be in between public function calls), which you should always strive to keep as constrained as possible, because every public function should handle any and all valid invariant states. In your current code, the invariant for Shader is: The GLuint handle is either undefined, pointing to a valid shader, or pointing to a failed shader.

A better alternative would be: The GLuint handle points to a valid shader. Note that you cannot establish that invariant without using exceptions.

Limiting work done in the constructor is good, but not if it comes at the cost of a having to widen the invariant.

Similarly, keep in mind that unless there is a bug in the code, you can always count on the destructor being called, even in weird exception-handling scenarios. That's why releasing resources is always best done there. Doing the cleanup in there makes it so a user of the class can never forget to call destroy().

I've also been told that initializer_lists are lightweight, thus its use. Thoughts on any of that?

They are lightweight, but fixed in size at compilation time. What if you wanted to load a list of shader to put in a program from a file? Again, they are fine as a secondary overload, but should not be the primary interface unless you can guarantee that the length will never be data-driven.