10
\$\begingroup\$

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.

\$\endgroup\$
7
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Good eye on the typos. I made an edit to the move constructor (if that's okay), because indeed, the intent was to handle the program object that would be potentially leaked. Could you please comment on the new move constructor? (I've kept the typo in the destructor) \$\endgroup\$ – nasser-sh Jul 18 '15 at 9:17
  • \$\begingroup\$ Also, I like your idea of the factory function, but I'm not sure I buy the unique_ptr return type. Could you point out what are the advantages of using a unique_ptr over using a movable GLShaderProgram? \$\endgroup\$ – nasser-sh Jul 18 '15 at 9:20
  • \$\begingroup\$ I meant, I edited the move-assignment, not move-constructor \$\endgroup\$ – nasser-sh Jul 18 '15 at 9:40
  • \$\begingroup\$ @Nasser, check the comment in your post. After applying any changes, it is better to post a follow-up question if you're interested on new feedback. That's our convention here to avoid having questions and answers out of sync. \$\endgroup\$ – glampert Jul 18 '15 at 17:56
  • \$\begingroup\$ @Nasser, having a value type is arguably easier to use, so both options are pretty good (pointer and movable). I tend to use pointers for this kind of resources because they are shareable (in that case it would be a shared_ptr, but I wanted to keep things simple in the example). If you see yourself sharing the same shader program with several objects (very frequent for materials), then most likely it will have to be a pointer. Otherwise, then probably go with the movable value type. With the movable you'd have to use some other error mechanism since it can return null. \$\endgroup\$ – glampert Jul 18 '15 at 18:02
4
\$\begingroup\$

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..)

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.