Yet another OpenGL Shader: the Builder pattern and lifetime management

Question

I've noticed that whenever a shader program class is implemented (from some questions on this site, as well as more reputable implementations), that:

1. The class tries to do too much. Binding to the current context, setting attribute variable values, and much more.

2. Use the JavaBeans pattern in one form or another, allowing the shader program to be in an invalid or incomplete state. For example, in between addShaderFromSource calls, the program is only partially built, and not usable.

My ideal shader program class would do much less; its purpose would just be to manage the lifetime of a shader program object, while maintaining a valid state throughout the object's lifetime (either a zero program or a valid, linked program), and the solution I came up with to achieve this uses the builder pattern.

void deleteAttachedShaders(GLuint programId) {
    constexpr GLsizei MAX_SHADER_COUNT = 5;
    GLuint shaderIds[MAX_SHADER_COUNT];
    GLsizei shaderActualCount;
    glGetAttachedShaders(programId, MAX_SHADER_COUNT, &shaderActualCount, shaderIds);

    for(int i = 0; i < shaderActualCount; ++i) {
        glDetachShader(programId, shaderIds[i]);
        glDeleteShader(shaderIds[i]);
    }
}

class GLShaderProgram
{
public:

    class Builder {
    public:
        Builder() : linked_(false) {
            programId_ = glCreateProgram();
        }

        Builder &addShaderFromSource(std::string const &shaderSource, GLenum shaderType) {
            char const *shaderCStr = shaderSource.c_str();
            GLuint shaderId = glCreateShader(shaderType);
            glShaderSource(shaderId, 1, &shaderCStr, nullptr);
            glCompileShader(shaderId);
            glAttachShader(programId_, shaderId);
            return *this;
        }

        Builder &addShaderFromFile(std::string const &shaderFile, GLenum shaderType) {
            std::ifstream fileName(shaderFile);
            std::istreambuf_iterator<char> fileBegin(fileName), fileEnd;
            std::string fileContents(fileBegin, fileEnd);
            return addShaderFromSource(fileContents, shaderType);
        }

        GLShaderProgram linkProgram() {
            glLinkProgram(programId_);
            GLShaderProgram theProgram(programId_);
            deleteAttachedShaders(programId_);
            linked_ = true;
            return theProgram;
        }

        ~Builder() {
             if(!linked_) {
                 glDeleteProgram(programId_);
             }
         }
    private:
        GLuint programId_;
        bool linked_;
    };

    GLShaderProgram() : programId_(0) { }

    GLShaderProgram(GLShaderProgram &&other) {
        *this = std::move(other);
    }

    GLShaderProgram &operator=(GLShaderProgram &&other)
    {
        programId_ = other.programId_;
        other.programId_ = 0;

        if (other.programId_ != 0) {
            glDeleteProgram(other.programId_);
        }

        return *this;
    }

    ~GLShaderProgram() {
        glDeleteShader(programId_);
    }

    GLuint programId() const { return programId_; }

    GLShaderProgram(GLShaderProgram const &other) = delete;
    GLShaderProgram &operator=(GLShaderProgram const &other) = delete;
private:
    GLShaderProgram(GLuint programId) : programId_(programId) { }
    GLuint programId_;
};

Example usage:

auto shaderProgram = GLShaderProgram::Builder()
    .addShaderFromFile("vertex.glsl", GL_VERTEX_SHADER)
    .addShaderFromFile("fragment.glsl", GL_FRAGMENT_SHADER)
    .linkProgram();

glUseProgram(shaderProgram.programId());

Any comments on the code's safety/quality would be appreciated.

Answer 1

Your implementation is very OOPy and I'd call it a good rendering of the builder pattern. Your usage example makes for a nice "fluent" API, BUT, I would personally not buy it.

The reason why 99% of the implementations out there are just reinventing OpenGL in a OOP way is because everyone takes the sorter path of a bottom up approach. They look at the API and see: Humm, so it looks like we have this glShaderSource thingy, so my class will need an addShaderSource method...

If they'd take a top down approach instead, they would first consider if exposing an addShaderSource method of sorts is even necessary.

• First basic constraint of a GL shader program is that it requires at least two stages: Vertex and Fragment. So it doesn't even make sense for a shader program to exist without these two stages.

• Linking the program is also a low-level implementation detail that I'd avoid exposing to the interface whenever possible. Again, a shader that fails to link is useless and an unlinked object should not be allowed to propagate.

Take all that into account and what should come to you is the good ole factory function. It receives all the required inputs and return a valid object or fails in your favourite way (return null, exception, error code, etc) without leaking intermediate incomplete objects.

// 
// Creates and links the shader or returns a null to indicate failure.
// You could also do a lot more for error handling, like throwing and exception
// with the compiler info log or returning the info log as an optional output parameter.
// I usually integrate this kind of stuff with a log system, so that's where my shader info log goes.
//
std::unique_ptr<GLShaderProgram> createGLShaderProgram(const std::string & vertexFilename, const std::string & fragmentFilename);

Now suppose you'd like to support more shader stages rather then the minimal Vertex+Fragment pair, like Geometry shader or Compute shaders. There are many ways to do it, but sticking to our previous example, I'd personally introduce a helper structure in this case:

struct ProgramStage
{
    enum Type
    {
        Vertex,
        Fragment,
        Geometry,
        // ... what have you ...
    };

    Type type;
    std::string filename; // This could also be a source code string instead if it suits you.
};

std::unique_ptr<GLShaderProgram> createGLShaderProgram(const std::vector<ProgramStage> & stages);

I hope the above snippets made the idea clear to you. The point is not to just try to wrap OpenGL in a class, but take a step back and actually consider what is really needed, which is avoiding the propagation of incomplete objects (I'd also add to that a simple and clean interface!). Your builder pattern doesn't do that. If the user forgets to add a mandatory stage, BAM!, incomplete state.

---

Now lets look at the rest of the code.

Move operator is broken:

GLShaderProgram &operator=(GLShaderProgram &&other)
{
    programId_ = other.programId_;
    other.programId_ = 0;

    if (other.programId_ != 0) {
        glDeleteProgram(other.programId_);
    }

    return *this;
}

Look closely to it in isolation for a few seconds. Do you realize the bug? other.programId_ is set to zero, then you test if it is non-zero in the next line to delete it. That if check is always false. But there is another less obvious error there. programId_ from this object gets overwritten, so you are officially leaking that handle if it contained a valid object.

You'd better just scrap that operator and rewrite a new one using the copy and swap idiom.

glDeleteShader != glDeleteProgram:

~GLShaderProgram() {
    glDeleteShader(programId_);
}

I'm fairly confident that the member programId_ is a shader program, not a shader object/stage, so it is being deleted by the wrong function. This is undefined behavior at best. You should be using glDeleteProgram, that was probably a typo, since in other places you use the correct function. Blame OpenGL for using goddamned integers for everything!

Make deleteAttachedShaders a member of something?

If that function is not useful elsewhere, then it should be made a static member of one of your classes.

Answer 2

I'd like to add a few things.

   GLShaderProgram linkProgram() {
        glLinkProgram(programId_);
        GLShaderProgram theProgram(programId_);
        deleteAttachedShaders(programId_);
        linked_ = true;
        return theProgram;
    }

Called twice, this might crash, might produce some undefined or bad gl-state or might just produce an opengl-error and run just fine. You have to check linked_ before you link your program again. Either you just throw out if it is already linked or you return a std::shared_ptr<GLShaderProgram> and save this in the builder, on the second call just return that ptr again.

Of course, you'd never call this method twice, but this doesn't mean you can just ignore the proper error-handling. The reason I don't need to check for *xyz* because it can't/won't happen is probably the #1 reason for bugs you'll need hours to debug.

~GLShaderProgram() {
    glDeleteShader(programId_);
}

If you've moved out of this shader, you delete the program 0. Not sure how opengl handles this.

And the worst thing you could ever do: You do absolutely no error handling for compile-errors and link-errors. If you have a bug in your shader-code, you won't notice anything. (Only that the graphics are weird..)