OpenGL object wrapped in a Qt Widget

Question

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 <GL/glew.h>
#endif

#include <GL/gl.h>
#include <QGLBuffer>
#include <QObject>
#include <QHash>
#include <QVector>
#include <QGLShaderProgram>
#include <QMatrix4x4>

class GLObject : public QObject
{
    Q_OBJECT
public:
    typedef GLfloat VectorType;
    static constexpr int GLVectorType = GL_FLOAT;
    typedef QVector<VectorType> VertexArray;
    typedef QVector<VectorType> 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<QString, GLObject*> sInstances;
    QByteArray mVertexBuffer, mColorBuffer;
    QGLBuffer mVBO;
    int nVertexBufferSize, nColorBufferSize;
    int mVertexCount;

    QGLShaderProgram mProgram;

    const QString mName;
    QMatrix4x4 mProjection, mView, mModel;
};


#endif // GLOBJECT_H

GLObject.cpp:

#include "GLObject.h"
QHash<QString, GLObject*> GLObject::sInstances;

#include <cstring>
using std::memcpy;

#include <exception>
#include <QDebug>

GLObject *GLObject::getInstancePtr(const QString &name)
{
    if(sInstances.contains(name))
        return sInstances[name];
    else
    {
#ifdef QT_DEBUG
        qDebug() << QString("Object with name %1 doesn't exist!").arg(name);
#endif
        return nullptr;
    }

    return sInstances[name];
}

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

    if(sInstances.contains(name))
        return *(sInstances[name]);
    else
    {
        QString errMsg = QString("Object with name %1 doesn't exist!").arg(name);
#ifdef QT_DEBUG
        qDebug() << errMsg;
#endif
        throw std::invalid_argument(errMsg.toStdString());
    }
}

GLObject::GLObject(const QString &name, QObject *parent) :
    QObject(parent), mVBO(QGLBuffer::VertexBuffer), mName(name)
{
    nVertexBufferSize = nColorBufferSize = 0;
    mVertexCount = 0;
    initVAO();
    sInstances.insert(name, this);
}

void GLObject::addVertexData(const GLObject::VertexArray &data)
{
    mVertexCount = data.size() / 3;
    mVertexBuffer.resize(data.size() * sizeof(VectorType));
    memcpy(mVertexBuffer.data(), data.constData(), mVertexBuffer.size());
    nVertexBufferSize = mVertexBuffer.size();
}

void GLObject::addColorData(const GLObject::ColorArray &data)
{
    Q_ASSERT(data.size() == mVertexCount * 3);

    mColorBuffer.resize(data.size() * sizeof(VectorType));
    memcpy(mColorBuffer.data(), data.constData(), mColorBuffer.size());
    nColorBufferSize = mColorBuffer.size();
}

bool GLObject::buildVBO()
{
    if(mVBO.isCreated())
        mVBO.destroy();

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

    const QByteArray data = mVertexBuffer + mColorBuffer;
    mVBO.allocate(data.constData(), data.size());

    mColorBuffer.clear();
    mVertexBuffer.clear();

    unbind();
    return ok;
}

GLuint GLObject::bufferID() const
{
    return mVBO.bufferId();
}

GLObject::~GLObject()
{
    unbind();
    sInstances.remove(mName);
}

bool GLObject::bind()
{
    return mVBO.bind();
}

void GLObject::unbind()
{
    glBindBuffer(GL_ARRAY_BUFFER, 0);
}

bool GLObject::addVertexShader(const QString &path)
{
    return mProgram.addShaderFromSourceFile(QGLShader::Vertex, path);
}

bool GLObject::addFragmentShader(const QString &path)
{
    return mProgram.addShaderFromSourceFile(QGLShader::Fragment, path);
}

bool GLObject::linkShaders()
{
    bool ok = mProgram.link();
    rebindMVPMatrix();
    return ok;
}

QString GLObject::programLog() const
{
    return mProgram.log();
}

QGLShaderProgram &GLObject::getProgramRef()
{
    return mProgram;
}

bool GLObject::bindProgram()
{
    return mProgram.bind();
}

void GLObject::unbindProgram()
{
    mProgram.release();
}

void GLObject::setProjectionMatrix(const QMatrix4x4 &mat)
{
    mProjection = mat;
}

QMatrix4x4 &GLObject::getProjectionMatrixRef()
{
    return mProjection;
}

QMatrix4x4 GLObject::getProjectionMatrix() const
{
    return mProjection;
}

void GLObject::setViewMatrix(const QMatrix4x4 &mat)
{
    mView = mat;
}

QMatrix4x4 &GLObject::getViewMatrixRef()
{
    return mView;
}

QMatrix4x4 GLObject::getViewMatrix() const
{
    return mView;
}

void GLObject::setModelMatrix(const QMatrix4x4 &mat)
{
    mModel = mat;
}

QMatrix4x4 &GLObject::getModelMatrixRef()
{
    return mModel;
}

QMatrix4x4 GLObject::getModelMatrix() const
{
    return mModel;
}

void GLObject::rebindMVPMatrix()
{
    const QMatrix4x4 MVP = mProjection * mView * mModel;
    bindProgram();
    mProgram.setUniformValue("MVP", MVP);
    unbindProgram();
}


QString GLObject::objectName() const
{
    return mName;
}

void GLObject::draw()
{
    if(mProgram.isLinked())
        bindProgram();
    bind();

    glVertexAttribPointer(
                0,
                3,
                GLVectorType,
                GL_FALSE,
                0,
                NULL
                );

    glVertexAttribPointer(
                1,
                3,
                GLVectorType,
                GL_FALSE,
                0,
                colorOffset()
                );

    glDrawArrays(GL_TRIANGLES, 0, mVertexCount);

    unbind();
    if(mProgram.isLinked())
        unbindProgram();
}

void GLObject::initVAO()
{
    static bool done;
    if(done)
        return;

    GLuint vao[2];
    glGenVertexArrays(2, vao);
    glBindVertexArray(vao[0]);
    glBindVertexArray(vao[1]);

    glEnableVertexAttribArray(0);
    glEnableVertexAttribArray(1);

    done = true;
}

void *GLObject::colorOffset()
{
    if(mVBO.isCreated())
        return reinterpret_cast<void*>(nVertexBufferSize);
    else
        return nullptr;
}

widget.h:

#ifndef WIDGET_H
#define WIDGET_H
#define GLEW_STATIC
#include <GL/glew.h>
#include <QGLWidget>
#include <QHash>
#include <QKeyEvent>

class Widget : public QGLWidget
{
    Q_OBJECT

public:
    Widget(QWidget *parent = 0);
    ~Widget();

protected:
    void initializeGL();
    void paintGL();
    void resizeGL(int w, int h);
    void keyPressEvent(QKeyEvent * event);
    void keyReleaseEvent(QKeyEvent * event);

    void timerEvent(QTimerEvent *);
private:
    QHash<int, bool> mPressedKeys;
};

#endif // WIDGET_H

widget.cpp:

#include "widget.h"
#include "GLObject.h"

#include <cstdlib>
using std::exit;

#include <QGLFormat>
#include <QGLContext>
#include <QDebug>

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

inline float aspect(const int a, const int b)
{
    return (static_cast<float>(a) / static_cast<float>(b));
}

QGLFormat appFormat()
{
    QGLFormat fmt;
    fmt.setVersion(4, 0);
    fmt.setProfile(QGLFormat::CoreProfile);
    //fmt.setDepth(true);
    fmt.setSampleBuffers(true);
    fmt.setSamples(8);
    //fmt.setDepthBufferSize(32);
    return fmt;
}

Widget::Widget(QWidget *parent)
    : QGLWidget(appFormat(), parent)
{
    setFixedSize(WINDOW_WIDTH, WINDOW_HEIGHT);
    //setWindowState(windowState() | Qt::WindowFullScreen);

#ifdef QT_DEBUG
    QGLFormat curFormat = format();
    if(curFormat != appFormat())
    {
        qWarning() << "Warning: using nearest format.";
        qWarning() << curFormat;
    }
#endif
}

Widget::~Widget()
{
}

void Widget::initializeGL()
{
    glewExperimental = GL_TRUE;
    GLenum ok = glewInit();
    if(ok != GLEW_OK)
    {
        qCritical() << "Error, can't initialize GLEW: " << glewGetErrorString(ok);
        exit(1);
    }

    glEnable(GL_MULTISAMPLE);

    glClearColor(0.0f, 0.0f, 0.0f, 1.0f);
    glViewport(0, 0, width(), height());

    GLObject *triangle = new GLObject("triangle", this);
    GLObject::VertexArray triangleVData = {
        -1, -1, 0,
        0, 1, 0,
        1, -1, 0,
    };
    GLObject::ColorArray triangleCData = {
        1, 0, 0,
        0, 1, 0,
        0, 0, 1
    };

    triangle->addVertexData(triangleVData);
    triangle->addColorData(triangleCData);
    triangle->buildVBO();

    triangle->addVertexShader(":/shaders/triangle.vert");
    triangle->addFragmentShader(":/shaders/triangle.frag");
    triangle->linkShaders();

    QObject::startTimer(10); // start repainting
}

void Widget::paintGL()
{
    glClear(GL_COLOR_BUFFER_BIT | GL_DEPTH_BUFFER_BIT);

    GLObject::getIntanceRef("triangle").draw();
}

void Widget::resizeGL(int w, int h)
{
    glViewport(0, 0, w, h);
}

void Widget::keyPressEvent(QKeyEvent *event)
{
    int key = event->key();
    mPressedKeys[key] = true;

    if(key == Qt::Key_Escape)
        close();

    QGLWidget::keyPressEvent(event);
}

void Widget::keyReleaseEvent(QKeyEvent *event)
{
    int key = event->key();
    mPressedKeys[key] = false;

    QGLWidget::keyReleaseEvent(event);
}

void Widget::timerEvent(QTimerEvent *)
{
    updateGL();
}

Answer 1

GLObject

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

2. 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".

3. Having a pair of get*() and get*Ref() 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.

4. 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<VectorType> VertexArray;
   typedef QVector<VectorType> ColorArray;
   

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

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

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

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

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

9. 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:

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

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

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

4. 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!