Designing a shader class

Question

Since I have started learning OpenGL, I thought I would as well write a small C++ framework (for myself) to avoid the nausea that the excessive use of C-ish code is apparently causing. :) Since I am intending to stick with Qt, the framework uses some Qt classes.

The first thing I really needed was an easy way to use shaders and programs. Here's my idea of the shader class.

class Shader
{
public:
    //create a shader with no source code 
    explicit Shader(GLenum shaderType);
    //create a shader with source code and compile it
    Shader(GLenum shaderType, const QString& sourceCode);
    //create a shader from source file and compile it
    Shader(GLenum shaderType, QFile& sourceFile);
    ~Shader();

    //change the source code and recompile
    void Source(QFile& sourceFile);
    void Source(const QString& sourceCode);

    GLuint get() const; //get the handle

private:
    //common part for creation in different constructors
    void createShader(GLenum shaderType); 
    //compile
    void compile();

private:
    GLuint handle;
};

It must be pretty obvious what the different functions are doing. Each is calling the relevant OpenGL routines, checks for errors and throws exceptions in case of any failure. The constructor calls glCreateShader. Now the tricky part. The destructor needs to call glDeleteShader(handle); but in this case I have a dilemma:

Option 1: Disable assignment and copying. This has the upside of avoiding reference counting and the downside of being forced to use shared_pointers to put these in vectors and passing around in general.

Option 2: Enable reference counting. This has the obvious upside of enabling copying, and therefore storing in containers(which I will need to later pass a range of shaders to a program). The downside is the following:

Shader s1(GL_VERTEX_SHADER, QFile("MyVertexShader.vp"));
Shader s2(s1);
s2.Source(QFile("MyOtherVertexShader.vp"));

As you see, I changed the source of s1 via s2, because they share the same internal shader handle. To be honest, I don't see a big problem here. I wrote the class, so I know its copy-semantics are like this and I'm OK with it. The problem is I am not sure this kind of design is ever acceptable. All this could be achieved with Option1 + shared pointers, with the only difference that I don't want to have a shared pointer every time I create a shader (not for performance reasons - just for syntactic convenience).

Q1: Please comment on the options and optionally the whole idea.¹
Q2: If I were to choose option 2, do I have to implement it myself or there's a ready class in boost or Qt which I could derive from or have a member of and I would get a free reference counting?
Q3: Do you agree that making Shader an abstract class and having three derived classes VertexShader, FragmentShader, and GeometryShader would be overkill?

¹ _{If you should refer me to an existing C++ OpenGL framework, that's very good(since I haven't actually found one) but that should really be a side note than an answer to my questions. Also note that I have seen a QGLShader class somewhere in the docs, but it is apparently not present in my version of Qt and I have my reasons to avoid upgrading right now.}

UPDATE

Thanks for the answers. I eventually decided to make my shader class immutable by removing the source functions. The shader gets compiled at creation and has no non-const member-functions. Thus a simple reference counting solves all my problems at once.

Answer 1

I say use option 1: it can do everything option 2 can (via smart pointers), whereas option 2 makes you pay the indirection cost even when you don't need it. On top of that, it's arguably easier to write.

Similarly I've once considered using handle-body/PIMPL when wrapping over a C API, to allow returning objects from functions (the C handle type wasn't guaranteed copyable, so the indirection was necessary for that). I decided against it since std::unique_ptr<T> does the non-movable -> movable transformation (much as shared_ptr<T> makes T copyable). Since then I design my classes to have the 'tightest' move/copy semantics.

You do have a point when it comes to syntactical noise however! Things like Boost.Phoenix and lambdas tend to help. If/when they are not an option, I'd say writing a separate shared_shader or whatever wrapper (wrapper wrapper?) makes sense, at least for library-level code (which I believe is the case here). I don't know of any utility to help with the tediousness of writing the forwarding functions.

I also don't know much when it comes to shaders so I'm not sure I can answer your last question. I think making a class hierarchy would make sense if the number of different shaders were liable to change often. I don't think that's the case; I also think even if that were the case since the implementation at your level is wrapping a preexisting API it's not too much of a hassle to revisit the code to forward to that API if/when a new shader is added.

---

Since you're asking for an example of Phoenix niceness.

What I want to do assuming I don't have to dereference:

std::transform(begin, end, functor);

Instead:

std::for_each(begin, end, *arg1 = ref(functor)(*arg1));

It's possible to still use std::transform (desirable for clarity) using some Phoenix facilities (construct IIRC) but that would cost an allocation.

Answer 2

I've already evaluated these options, and I've implemented a shader class is a different way.

The first point is that CreateShader and DeleteShader need a current context, which is not always true. All functions return errors and latter one can cause leaks. So, I will introduce a Create and Delete routine which really call CreateShader and Delete Shader. In this way, it is possible to destroy an object even in in a separate thread (the shader itself will be destroyed later when the context will be current.

The second point is that the shader object, once linked to a shader program, can be relinked in another shader program, without recompiling (except in the case the source depends on preprocessor symbols). So, i would collect a collection of commoly used shader objects, to be reused during program creations.

The last point is that the ShaderObject class assignment is fine, as long you don't leak a created shader object. In the case of the source, I think tehere two options: source can be changed, and the shader become invalid, or shader become dirty and indeed requires a compilation.

Because a shader source can be compiled for different shader stages, I would suggest to avoid Vertex, Fragment etc...derivations. Optionally, you can have a default and set it before creation. Of course, it is possible by defining the create method.

Another point is that a shader object do not need to exists once the program is linked. So, in combination with a common pre-compiled shader database, the reference counting is used only by shaders programs (especially not linked ones) to indicate that threy need that shader object for linking. Always in this case, there should be a shader program database, in order to avoid redundant program creations; in this scenario, assignment and copy becomes a very rare operation, which I would avoid to expose; instead, define a friend method and use it in your framework.