Using a CRTP approach for loading OpenGL programs

Question

I wanted to try using CRTP (also new for me) to try making the loading of shared resources implicit. (Specifically I'm using it for loading OpenGl shader programs)

This will also help separate some of the boiler plate setup code (combining shaders into a program) from the more specific setup code (assigning locations for shader uniforms), and making sure that no one program is loaded more than once.

If it applies (pretty certain it does), what concepts/ fundamentals am I missing/ not seeing?

By using statics I understand that this isn't thread safe, however I don't think I'll be using multiple threads- but even with that said, any ideas/ tips on that matter would still be much appreciated!

The code lightly uses the Mesh class from the online tutorial series Learning Modern 3D Graphics Programming by Jason L. McKesson, and the source code for that can be found here under framework. The shaders being used are found under Tut 10/ data (but I will post them as they are very short). It also uses glm, freeglut and other libraries found in the glsdk folder of the source code.

CRTP base header, ProgramBase.h

const std::string LOCALFILEDIR = "data\\";
const int PROJECTION_BLOCK_INDEX = 2;

template<class CustomProgram> class ProgramBase
{
public:
    static GLuint program_location();
protected:
    ProgramBase();
    ~ProgramBase(){}
private:
    static GLuint LoadShader(GLenum shader_type_e, const std::string &base_shader_filename);
    static GLuint CreateProgram(const std::vector<GLuint> &shader_locations);

    static GLuint program_location_;
};

The idea is that every CustomProgram will have to define at least three members in order for the below code to work:

• vertex_shader_names_ A container of vertex shaders names to be found and linked
• fragment_shader_names_A list of fragment shaders names to be found and linked
• Init() A function specific to shaders named above that will find uniforms

template<class CustomProgram> ProgramBase<CustomProgram>::ProgramBase()
{
    if(program_location_ == -1)
    {
        std::vector<GLuint> shader_locations;
        for(const std::string& name : CustomProgram::vertex_shader_names_)
        {
            shader_locations.push_back(LoadShader(GL_VERTEX_SHADER, name));
        }
        for(const std::string& name : CustomProgram::fragment_shader_names_)
        {
            shader_locations.push_back(LoadShader(GL_FRAGMENT_SHADER, name));
        }

        program_location_ = CreateProgram(shader_locations);
        CustomProgram::Init();
    }
}

template<class CustomProgram> GLuint ProgramBase<CustomProgram>::LoadShader(
    GLenum shader_type_e, 
    const std::string &base_shader_filename)
{
    std::ifstream shader_file((LOCALFILEDIR + base_shader_filename).c_str());
    if(!shader_file.is_open())
    {
        throw std::runtime_error("Could not find the file " + base_shader_filename);
    }
    std::stringstream shader_data;
    shader_data << shader_file.rdbuf();
    shader_file.close();

    try
    {
        return glutil::CompileShader(shader_type_e, shader_data.str());
    }
    catch(std::exception &e)
    {
        fprintf(stderr, "%s\n", e.what());
        throw;
    }
}
template<class CustomProgram> GLuint ProgramBase<CustomProgram>::CreateProgram(
    const std::vector<GLuint> &shader_locations)
{
    try
    {
        GLuint prog = glutil::LinkProgram(shader_locations);
        std::for_each(shader_locations.begin(), shader_locations.end(), glDeleteShader);
        return prog;
    }
    catch(std::exception &e)
    {
        std::for_each(shader_locations.begin(), shader_locations.end(), glDeleteShader);
        fprintf(stderr, "%s\n", e.what());
        throw;
    }
}

template<class CustomProgram> GLuint ProgramBase<CustomProgram>::program_location()
{
    return program_location_;
}

template<class CustomProgram> GLuint ProgramBase<CustomProgram>::program_location_ = -1;

---

One program I incorporate is a program which simply positions the model relative to the camera, and then to clip space before drawing a single color. (this is pretty much directly from the tutorial)

PosTransform.vert

#version 330
layout(location = 0) in vec3 position;
uniform mat4 modelToCameraMatrix;
uniform Projection
{
    mat4 cameraToClipMatrix;
};
void main()
{
    gl_Position = cameraToClipMatrix * (modelToCameraMatrix * vec4(position, 1.0));
}

UniformColor.frag

#version 330
uniform vec4 objectColor;
out vec4 outputColor;
void main()
{
    outputColor = objectColor;
}

---

This is the class I've defined for it. BasicProgram.h

class BasicProgram : ProgramBase<BasicProgram>
{
    friend class ProgramBase<BasicProgram>;
public:
    BasicProgram()
        : model_matrix_(glm::mat4(1.0f)), color_vector_(glm::vec4(1.0f))
    { }

    BasicProgram(glm::mat4& model_matrix, glm::vec4& color_vector)
        : model_matrix_(model_matrix), color_vector_(color_vector)
    { }

    void Render(Framework::Mesh* mesh);
    glm::mat4 model_matrix();
    void set_model_matrix(glm::mat4 model_matrix);
    glm::vec4 color_vector();
    void set_color_vector(glm::vec4 color_vector);
private:
    static void Init();

    glm::mat4 model_matrix_;
    glm::vec4 color_vector_;

    static const std::array<std::string,1> vertex_shader_names_;
    static const std::array<std::string,1> fragment_shader_names_;

    static GLuint model_matrix_unif_;
    static GLuint color_vector_unif_;
};

And the guts

void BasicProgram::Render(Framework::Mesh* mesh)
{
    glUseProgram(program_location());
    glUniformMatrix4fv(model_matrix_unif_, 1, GL_FALSE, glm::value_ptr(model_matrix_));
    glUniform4fv(color_vector_unif_, 1,  glm::value_ptr(color_vector_));
    mesh->Render();
    glUseProgram(0);
}
void BasicProgram::Init()
{
    model_matrix_unif_  = glGetUniformLocation(program_location(), "modelToCameraMatrix");
    color_vector_unif_  = glGetUniformLocation(program_location(), "objectColor");

    GLuint projectionBlock = glGetUniformBlockIndex(program_location(), "Projection");
    glUniformBlockBinding(program_location(), projectionBlock, PROJECTION_BLOCK_INDEX);
}
const std::array<std::string,1> BasicProgram::vertex_shader_names_ = 
{
    "PosTransform.vert",
};
const std::array<std::string,1> BasicProgram::fragment_shader_names_ = 
{
    "UniformColor.frag",
};
glm::mat4 BasicProgram::model_matrix()
{
    return glm::mat4(model_matrix_);
}
void BasicProgram::set_model_matrix(glm::mat4 model_matrix)
{
    model_matrix_ = glm::mat4(model_matrix);
}
glm::vec4 BasicProgram::color_vector()
{
    return glm::vec4(color_vector_);
}
void BasicProgram::set_color_vector(glm::vec4 color_vector)
{
    color_vector_ = glm::vec4(color_vector);
}

Answer 1

Overall, I'd say it is well done. You have properly implemented CRTP. But I'm a bit skeptical about your choice. If you have a lot of shader program implementations that are included in a lot of places, your compile times will suffer a bit and the generated binary is going to be bigger. Also, any change to ProgramBase might implicate a next-to-full recompile of the project.

In this particular case, I would have resorted to simple inheritance. As far as I can tell, the only two virtual methods you would need are one to get a reference to the set of vertex_shader_names_ and one for fragment_shader_names_. Perhaps also a virtual Init() but that work can be done in the constructor of the child class. A public virtual destructor or a protected non-virtual one. That's all. Of course that the virtual calls are more expensive during runtime, but here you are only executing them once, during startup/initialization, so there is little reason to optimize and tradeoff for longer compile times and bigger binaries. Also, with simple inheritance, code would become a little simpler and you could easily get rid of the statics (not that there is a problem with threading, GL can only run from a single thread anyways).

---

Some other minor details in the code:

In the first constructor of BasicProgram, you don't need to explicitly call vec4/mat(1.0f), that's just creating a useless temporary object:

BasicProgram()
    : model_matrix_(1.0f), color_vector_(1.0f)
{ }

In the second constructor, you are making copies of the parameters. move doesn't apply here because the GLM objects are just arrays of floats. So you can continue passing the parameters by reference. But since there is no intent in modifying them, make the references const to document that:

BasicProgram(const glm::mat4& model_matrix, const glm::vec4& color_vector)
    : model_matrix_(model_matrix), color_vector_(color_vector)
{ }

Again, when returning the values, that constructor style cast in useless:

return glm::vec4(color_vector_);

color_vector_ is already a glm::vec4. Just return color_vector_.

You are using a mixed snake_case and PascalCase naming style for the methods. snake_case being used by lightweight get/set methods only. Personally, I don't appreciate that mixing of styles. A method is a method, regardless of its purpose, so I think it makes more sense naming all methods uniformly. Then identify lightweight accessors with the traditional get/Get, set/Set prefix.

Answer 2

Nothing major, but here are a few things you could change:

• Since you don't define any special constructor/operator= in ProgramBase, you don't need to explicitly define its destructor. You can let the compiler do the job for you.

• In C++, std::ifstream has a constructor that can take an std::string instead of a const char*. Therefore, you need not call c_str anymore in the following line:

  std::ifstream shader_file((LOCALFILEDIR + base_shader_filename).c_str());
  

• When opening a file, the idiomatic way t check for errors is to use operator! which may catch more errors [citation needed] than is_open:

  std::ifstream shader_file(LOCALFILEDIR + base_shader_filename);
  if(!shader_file)
  {
      throw std::runtime_error("Could not find the file " + base_shader_filename);
  }
  

• To clarify your intent, you could use an std::ostringstream instead of an std::stringstream in the method ProgramBase<CustomProgram>::LoadShader.

• This line seems a little bit C-ish:

  fprintf(stderr, "%s\n", e.what());
  

  Couldn't you simply replace it by this one?

  std::cerr << e.what();
  

• Please, only use FULL_CAPS names for macros, not for constants in general. A usual goal of the FULL_CAPS case in C/C++ is to know with a glance that there may be something fishy going on. Since macros were often used to define constants in C89, many people still write their constants in FULL_CAPS case, but pretty please, try to only use this case for macros :)

• You could use constructor delegation to slightly simplify your constructors:

  BasicProgram()
      // Delegate to the other BasicProgram constructor
      : BasicProgram(glm::mat4(1.0f), glm::vec4(1.0f))
  { }
  
  BasicProgram(glm::mat4& model_matrix, glm::vec4& color_vector)
      : model_matrix_(model_matrix), color_vector_(color_vector)
  { }