OpenGL object wrapped in a Qt Widget

I have this object wrapper class, which I will use to implement my Qt OpenGL scene. It is working, but I am looking for a way to:

• Improve the API.

• Remove extra OpenGL calls.

• Optimize it.

I know that QGL stuff is deprecated and I should switch to newer Qt5 API, but it still has some things missing.

GLObject.h:

```
#ifndef GLOBJECT_H
#define GLOBJECT_H

#ifndef GLEW_STATIC
#define GLEW_STATIC
#include
#endif

#include
#include
#include
#include
#include
#include
#include

class GLObject : public QObject
{
Q_OBJECT
public:
typedef GLfloat VectorType;
static constexpr int GLVectorType = GL_FLOAT;
typedef QVector VertexArray;
typedef QVector ColorArray;

static GLObject* getInstancePtr(const QString &name);
static GLObject& getIntanceRef(const QString &name);

explicit GLObject(const QString &name, QObject *parent = 0);
virtual ~GLObject();

void addVertexData(const VertexArray &data);
void addColorData(const ColorArray &data);
bool buildVBO();
GLuint bufferID() const;

bool bind();
void unbind();

bool addVertexShader(const QString &path);
bool addFragmentShader(const QString &path);
bool linkShaders();
QString programLog() const;
QGLShaderProgram &getProgramRef();

bool bindProgram();
void unbindProgram();

void setProjectionMatrix(const QMatrix4x4 &mat);
QMatrix4x4 &getProjectionMatrixRef();
QMatrix4x4 getProjectionMatrix() const;

void setViewMatrix(const QMatrix4x4 &mat);
QMatrix4x4 &getViewMatrixRef();
QMatrix4x4 getViewMatrix() const;

void setModelMatrix(const QMatrix4x4 &mat);
QMatrix4x4 &getModelMatrixRef();
QMatrix4x4 getModelMatrix() const;

void rebindMVPMatrix();

QString objectName() const;
virtual void draw();
private:
static void initVAO();
void * colorOffset();

static QHash sInstances;
QByteArray mVertexBuffer, mColorBuffer;
QGLBuffer mVBO;
int nVertexBufferSize, nColorBuffer

GLObject

-
You seem to be using C++11, so don't assign 0 to pointers (in QObject *parent = 0 for instance). Use nullptr.

-
GLObject has a virtual destructor. You don't seem to be inheriting from the class, so a virtual destructor wouldn't be necessary. However, you have a virtual draw() method, so this probably means you plan on extending it at some moment. The point here is to avoid virtual destructors unless you are really planning on extending a class. Don't add one "just in case".

-
Having a pair of get() and getRef() seems redundant. Following is one such instance:

void setProjectionMatrix(const QMatrix4x4 &mat);
QMatrix4x4 &getProjectionMatrixRef();
QMatrix4x4 getProjectionMatrix() const;

The Ref() overload seems unnecessary. The set() methods already allow someone to mutate the object state. A single get*() returning by value or const reference would do.

-
Minor nitpicking, but group your typedefs; that constant in the middle breaks the pattern:

typedef GLfloat VectorType;
static constexpr int GLVectorType = GL_FLOAT;
typedef QVector VertexArray;
typedef QVector ColorArray;

And by-the-way, GL_FLOAT should be assigned to a constant of type GLenum.

-
I personally wouldn't do this at the top level:

using std::memcpy;

It isn't that much more typing calling the function as std::memcpy. You did that in a source file, so it is not a big issue, but still, it is in the global scope.

Another misuse of the using directive in getIntanceRef():

GLObject &GLObject::getIntanceRef(const QString &name)
{
   using std::invalid_argument;

I'm perfectly fine with using inside a function, but you go on and reference std::invalid_argument by its namespace qualified name in the next few lines, rendering the using useless (no pun intended, honestly).

-
This is worth for both classes: I recommend consistent use of curly braces { } on all conditional statements, even the single line ones and in loops. This can prevent bugs (the infamous goto fail bug from SSL could have been detected more easily if they had followed this practice) and it also simplifies maintenance and future expansion.

-
I know that OpenGL doesn't play well with threads, but it is worth noting that the static initialization flag done inside initVAO() is not thread safe. If you ever try to use this class concurrently, that flag could cause multiple initializations.

-
Still talking about initVAO(), the way that function is implemented doesn't really make sense. You create two Vertex Array Objects, binds them and throws away the handles. That is not the way VAOs are meant to be used. They are supposed to serve as wrappers for Vertex Buffers and associated states (vertex layout, etc). Take a look at these resources: What are Vertex Array Objects?, Vertex Array Object (OGL wiki) and a tutorial on the subject. If used properly, they can reduce the number of GL calls during rendering.

-
The use of the boolean flag ok in the buildVBO method has issues:

bool ok = mVBO.create();
ok = mVBO.bind();

The return value of the second call will overwrite the first, which could hide an error. If the objective is to report the error as early as possible, check the return value of each method and bail out with false. There isn't much point in continuing with an error condition. Failing Fast is an important programming principle.

Widget

Not much to say about Widget. It looks good to me. Just a few minor things:

-
A few dead-code comments that pollute it a little. Get rid of them, since commented-out code adds nothing to readability or information about the program. You don't have to leave commented-out code for future reference either. Your Version Control history does that for you.

-
The destructor of Widget is empty, so it can be omitted.

-
exit()ing a program on error is a rather crude way of handling a failure. Consider throwing an exception and let it propagate if the error has no recovery.

-
Apparently, you have a few local constants and functions inside widget.cpp. Consider wrapping them inside an unnamed namespace to make your intentions clear:

namespace 
{
    const int WINDOW_WIDTH  = 800;
    const int WINDOW_HEIGHT = 600;

    float aspect(const int a, const int b)
    {
        ...
    }

    QGLFormat appFormat()
    {
        ...
    }
} // namespace

The constants could also be constexpr.

This was a very late answer, but I hope some of what was said here can still be of use to you. Cheers!

StackExchange Code Review Q#40877, answer score: 3