Drawing a colored rectangle on-screen

I have made my first program that draws a colored rectangle on-screen. I want to know the best practices of making OpenGL and GLSL programs.

```
#include
#include
#include
#include
#include
#include
#include

#define GLSL(src) "#version 400 core\n" #src
#define GETERROR() getError(__FILE__,__LINE__)

namespace
{
const unsigned CurrentWidth = 800;
const unsigned CurrentHeight = 600;
}

void getError(const char *file, int line)
{
GLenum error(glGetError());

while (error != GL_NO_ERROR)
{
std::string errorString;

switch (error)
{
case GL_INVALID_OPERATION: errorString = "INVALID_OPERATION"; break;
case GL_INVALID_ENUM: errorString = "INVALID_ENUM"; break;
case GL_INVALID_VALUE: errorString = "INVALID_VALUE"; break;
case GL_OUT_OF_MEMORY: errorString = "OUT_OF_MEMORY"; break;
case GL_INVALID_FRAMEBUFFER_OPERATION: errorString = "INVALID_FRAMEBUFFER_OPERATION"; break;
}

throw std::runtime_error("GL_" + errorString + " - " + std::string(file) + ":" + std::to_string(line));
error = glGetError();
}
}

class Tutorial
{
public:
Tutorial();
~Tutorial();
void run();

private:
void processEvent();
void render();

void createVBO();
void destroyVBO();
void createShaders();
void destroyShaders();

private:
sf::Window mWindow;

GLuint mVertexShaderID;
GLuint mFragmentShaderID;
GLuint mProgramID;
GLuint mVAOID;
GLuint mVBOID;
GLuint mColorBufferID;

const std::vector mVertexShader =
{
{
GLSL(
layout(location = 0) in vec4 in_Position;
layout(location = 1) in vec4 in_Color;

out vec4 ex_Color;

void main(void)
{
gl_Position = in_Position;
ex_Color = in_Color;
}
)
}
};
const std:

A few things, roughly order of appearance in the code:

Miscellaneous:

-
I don't think CurrentWidth/CurrentHeight describe the intent of those constants very well. They are the initial dimensions of the application window, so something in the lines of InitWindowWidth, InitWindowHeight would make more sense.

-
In the getError() function, you have used a while loop to query the GL errors, which is correct, since glGetError pops the top of the error stack, so there might be more than one. However, you throw an exception right at the first iteration of the loop, so it is just the same as no loop at all. If you just care about the latest error, replace the while loop with an if. If you'd like to clear the error stack instead, change your function to append the error list to a string and move the throw to the end of the function. By the way, if getError() is a local function, it can also be placed inside the unnamed namespace.

-
Make sure to use const on local variables whenever practical to ensure single-assignment. In createVBO(), the local data arrays can and should be constants.

Shader source code management:

I see that you are using the slightly hackish stringification macro to make the shader source code look like normal C code. This is cool because it enables syntax highlighting in your editor and removes the need to manually quote each line to make it a string, but like all hackish solutions, it breaks very easily. In particular, it will break if you have a comma (,) in the shader source, because the preprocessor will think it is actually a comma separating the parameters of a function-like macro.

So this solution is far from ideal and not very scalable either. Having it as a literal string avoids the macros issues, but it is even worse to maintain. You are also forced to recompile the program whenever the shader source is updated, which shouldn't be the case, since it is not part of the program, but rather, a resource or asset in the form of a text string.

For any usage more complex than a couple lines long shader program, you should definitely move the code to an external text file and load it at startup into a string. This will decouple the C++ program from the shader code, allowing you to freely change the shaders without recompiling the C++ program. You can also take this further and allow for hot reloading of shaders while the main program is running.

Reading a text file into a string in C++ is as simple as this:

std::string readTextFile(const std::string & filename)
{
    std::ifstream ifs(filename);
    if (ifs)
    {
        return std::string(std::istreambuf_iterator(ifs), 
                           std::istreambuf_iterator());
    }
    return std::string(); // Or throw and error, return a default string, etc...
}

Also note that you can use a std::string for the source code. No need to use a vector of GLchar. OpenGL/GLSL only accepts ASCII strings, so GLchar is a typedef to char. I think this typedef is only provided for consistently with the rest of the API (like GLvoid for instance, which must be void), since it is very unlikely that it will ever change to some other char type, due to the big legacy codebase of GL.

StackExchange Code Review Q#98900, answer score: 2