C++ OpenGLBuffer class, wrapper around the raw OpenGL api

Question

I am trying to code a thin wrapper around the OpenGL so I can use it more convenient. This class is not 100% done yet, but I don't want to do all typing for nothing. And I think the design is clear. I would hope some review anything they see with my C++ code or with the way I use OpenGL.

.hpp file

#pragma once

#include <GL\glew.h>

enum class BufferType   : GLenum
{
    VertexBuffer = GL_ARRAY_BUFFER,
    IndexBuffer = GL_ELEMENT_ARRAY_BUFFER,
    // Rest will be added when I need it

    None
};

enum class AccessType // Used for mapping later
{
    Read,
    Write,
    ReadWrite
};

enum class UsageType : GLenum
{
    StreamDraw, // Fill these in when I need it
    StreamRead,
    StreamCopy,
    StaticDraw  = GL_STATIC_DRAW,
    StaticRead  = GL_STATIC_READ,
    StaticCopy  = GL_STATIC_COPY,
    DynamicDraw,
    DynamicRead,
    DynamicCopy,

    None
};

class OpenGLBuffer
{
public:
    OpenGLBuffer();
    OpenGLBuffer(BufferType Buffer, UsageType Usage);
    ~OpenGLBuffer();

    bool Create(unsigned int Size);
    bool Create();

    //Maybe also make a Read/Write func that handles a vector ( bool WriteData(unsigned int Offset, unsgiedn int SizeOfObject, std::vector<void*>& data);
    bool WriteData(unsigned int Offset, unsigned int Size, void* data);
    bool ReadData(unsigned int Offset, unsigned int Size, void* data);

    void Bind();
    void Unbind();

    bool Grow(unsigned int Size, bool Copy = true);

    //Mapping functions, when I need those

    bool IsCreated();
    bool IsAutoResize();

    void SetBufferType(BufferType Type);
    void SetUsageType(UsageType Type);
    void SetAutoResize(bool AutoResize, unsigned int Size = 100);

    BufferType GetBufferType();
    UsageType GetUsageType();
    GLuint GetId();
    unsigned int GetSize();
    unsigned int GetDataUsed();

private:
    GLuint m_Id = 0;
    BufferType m_BufferType;
    UsageType m_UsageType;
    unsigned int m_Size = 0;
    unsigned int m_DataUsed = 0;
    bool m_IsCreated = false;
    bool m_AutoResize = false;
    unsigned int m_ResizeSize = 100; // Random, is there a better thing?
};

And this is the .cpp file:

#include "OpenGLBuffer.hpp"

OpenGLBuffer::OpenGLBuffer()
{
    OpenGLBuffer(BufferType::None, UsageType::None);
}

OpenGLBuffer::OpenGLBuffer(BufferType Buffer, UsageType Usage)
{
    m_BufferType = Buffer;
    m_UsageType = Usage;
}

OpenGLBuffer::~OpenGLBuffer()
{
    // Bind and delete it
}

bool OpenGLBuffer::Create(unsigned int Size)
{
    if (m_BufferType == BufferType::None || m_UsageType == UsageType::None)
        return false;

    glGenBuffers(1, &m_Id);
    Bind();
    glBufferData(static_cast<GLenum>(m_BufferType), Size, 0, static_cast<GLenum>(m_UsageType));

    m_IsCreated = true;
    m_Size = Size;

    return true;

}

bool OpenGLBuffer::Create()
{
    return Create(m_ResizeSize);
}

bool OpenGLBuffer::WriteData(unsigned int Offset, unsigned int Size, void* data)
{
    if (!m_IsCreated)
        return false;

    if (m_DataUsed + Size > m_Size)
    {
        if (m_AutoResize)
        {
            if (m_ResizeSize < m_DataUsed + Size)
                if (!Grow(Size - m_DataUsed + m_ResizeSize))
                    return false;
                else
                    if (!Grow(m_ResizeSize, true))
                        return false;
        }
        else
            return false;
    }

    Bind(); // Is this a good idea?

    glBufferSubData(static_cast<GLenum>(m_BufferType), m_DataUsed, Size, data);
    //Have some checks
    Unbind();

    m_DataUsed += Size;

    return true;
}

bool OpenGLBuffer::ReadData(unsigned int Offset, unsigned int Size, void* data)
{
    if (!m_IsCreated)
        return false;

    // First ask if this a good approach
    return false;
}

void OpenGLBuffer::Bind()
{
    if(m_IsCreated)
        glBindBuffer(static_cast<GLenum>(m_BufferType), m_Id);
}

void OpenGLBuffer::Unbind()
{
    if(m_IsCreated)
        glBindBuffer(static_cast<GLenum>(m_BufferType), 0);
}

bool OpenGLBuffer::Grow(unsigned int Size, bool Copy )
{
    if (!m_IsCreated)
        return false;

    glBindBuffer(GL_COPY_READ_BUFFER, m_Id);

    GLuint NewId;
    glGenBuffers(1, &NewId);
    glBindBuffer(GL_COPY_WRITE_BUFFER, NewId);
    glBufferData(GL_COPY_WRITE_BUFFER, m_DataUsed + Size, 0, static_cast<GLenum>(m_UsageType));

    glCopyBufferSubData(GL_COPY_READ_BUFFER, GL_COPY_WRITE_BUFFER, 0, 0, m_DataUsed);
    //Have some checks
    //Delete old buffer ID

    m_Id = NewId;
    m_Size = Size + m_DataUsed;

    return true;
}

bool OpenGLBuffer::IsCreated()
{
    return m_IsCreated;
}

bool OpenGLBuffer::IsAutoResize()
{
    return m_AutoResize;
}

void OpenGLBuffer::SetBufferType(BufferType Type)
{
    m_BufferType = Type;
}

void OpenGLBuffer::SetUsageType(UsageType Type)
{
    m_UsageType = Type;
}

void OpenGLBuffer::SetAutoResize(bool AutoResize, unsigned int Size)
{
    m_AutoResize = AutoResize;
    m_ResizeSize = Size;
}

BufferType OpenGLBuffer::GetBufferType()
{
    return m_BufferType;
}

UsageType OpenGLBuffer::GetUsageType()
{
    return m_UsageType;
}

GLuint OpenGLBuffer::GetId()
{
    return m_Id;
}

unsigned int OpenGLBuffer::GetSize()
{
    return m_Size;
}

unsigned int OpenGLBuffer::GetDataUsed()
{
    return m_DataUsed;
}

Answer 1

I will assume that you do not intend to support immutable buffers with this code. I will also assume that you do not intend to support direct state access functions.

There are still issues with your wrapper class.

First, you simply do not expose the full capabilities of OpenGL. I am not talking about capabilities you have not exercised like mapping or more recent features I talked about. You have missed a very basic aspect of buffer objects.

In your API, buffer objects are typed. The user, upon construction, passes in a BufferType, which Bind and Unbind are based on. FYI: it's bad form to have a None enumerator for a parameter unless that's actually a legal parameter. It's better to prevent errors at compile time, rather than checking at runtime.

In OpenGL, buffer objects do not have a type. Unlike textures, buffer objects can be bound for any usage. You can take a buffer that you use for transform feedback, then bind it for use as vertex arrays, then bind part of it for uniform data. That's all 100% legal in OpenGL.

Therefore, your Bind and Unbind functions really need to take a BufferType.

Next, there is the question of performance. Your API makes promises to the user. That the buffer can grow arbitrarily, being the most important.

The problem is that, because your object makes these basic buffer object functionality, you give the illusion to the user that these are actually fast operations.

They are actually quite the opposite. Grow or your m_AutoResize functionality is basically the kiss of death as far as performance is concerned. While it can be useful to have some kind of object that can handle more flexible memory allocation, such things should not be in the lowest level of your abstraction. By putting them there, you make them seem like basic functionality when they very much are not.

The lowest-level wrapper needs to be exactly and only OpenGL's functionality. That way, users can tell what is provided by the wrapped API and what is being provided by your library. You can later build on that low-level wrapper by higher-level classes. But users of your class ought to be able to get at the root stuff without the higher-level abstractions promising them reasonably fast Grow operations and the like.

Also, having such a separation makes it clear when users are using those higher-level features. If a user creates a ResizableBufferObject, then its abundantly clear what they're doing.

I also see two-stage construction. The constructor doesn't actually make any OpenGL calls. I know that some users, particularly OpenGL beginners, absolutely love to make global variables of such types. So there's a tendency to use two-stage construction to keep them from doing the wrong thing (making OpenGL calls before OpenGL has been initialized). Even so, I would encourage you to resist that temptation.

Oh, and whether you keep the two-stage construction or not, whichever one actually calls glBufferData also needs to take a pointer, so that the user can create and initialize the buffer. There's really no point in separating the steps if the user has the data right there.