Return to Answer

• 1

• 17.3k
• 4
• 31
• 89

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.