3
\$\begingroup\$

So with OpenGL(and I'm assuming other GPU APIs) you create different buffers/programs/etc that live on the GPU and you are given a handle to them when created. For instance, if you want a buffer to store data, you must first generate it then you are presented with a handle to it to be used to push data to it as well as eventually clean it up.

The following is a wrapper class that holds the GPU resource handle, provides additional useful functions and cleans up the GPU resource when its time.

See edit at bottom.

GLResource.h

#pragma once

#include <functional>

namespace engine {

    template<typename T>
    class GLResource {
        std::function<void(T)> onDestroy;
    public:
        const T value;

        GLResource() = delete;
        GLResource(
            const T& value,
            const std::function<void(const T)>& onDestroy
        ) :
            value{value},
            onDestroy{onDestroy} {
        }

        ~GLResource() {
            onDestroy(value);
        }
    };

}

The above template class is just a simple mechanism for taking a generic value, and calling a custom "onDestroy" callback when the destructor is called.

UniformBuffer.h

#pragma once

#include <memory>

#include "GL.h"
#include "GLResource.h"

namespace engine {

    class UniformBuffer {
        static GLuint bindingIndexIterator;

        std::shared_ptr<GLResource<GLuint>> buffer;
    public:
        const GLuint bindingIndex;

        //must have a buffer size
        UniformBuffer() = delete;
        UniformBuffer(GLsizeiptr size);

        void bindBuffer() const;
        void pushBufferData(GLintptr offset, GLsizeiptr size, const void* data) const;
    };

}

UniformBuffer.cpp

#include "UniformBuffer.h"

#include "logger.h"

namespace engine {

    USE_LOGGER("UniformBuffer");

    GLuint UniformBuffer::bindingIndexIterator = 0;

    UniformBuffer::UniformBuffer(GLsizeiptr size) :
        bindingIndex{bindingIndexIterator++} {

        GLuint tmpBuffer;
        glGenBuffers(1, &tmpBuffer);
        glBindBuffer(GL_UNIFORM_BUFFER, tmpBuffer);
        glBufferData(GL_UNIFORM_BUFFER, size, 0, GL_DYNAMIC_DRAW);
        glBindBufferBase(GL_UNIFORM_BUFFER, bindingIndex, tmpBuffer);

        LOGGER_INFO("Created {}", tmpBuffer);
        buffer = std::make_shared<GLResource<GLuint>>(tmpBuffer, [](GLuint buffer) {
            glDeleteBuffers(1, &buffer);
            LOGGER_INFO("Deleted {}", buffer);
        });
    }

    void UniformBuffer::bindBuffer() const {
        glBindBuffer(GL_UNIFORM_BUFFER, buffer.get()->value);
    }

    void UniformBuffer::pushBufferData(GLintptr offset, GLsizeiptr size, const void* data) const {
        glBufferSubData(GL_UNIFORM_BUFFER, offset, size, data);
    }

}

The idea here is that UniformBuffer can be copied/assigned without destroying the resource held on the GPU, but when all shared instances of the UniformBuffer go out of scope/are deleted the GPU resource is then destroyed.

The use of std::shared_ptr here is to keep the classes unique GPU resource valid until it's no longer needed, the UniformBuffers destructor would normally be called when copy constructed or assigned so that's out of the question.

Anyway, I'm looking for feedback on if this is a reasonable approach, and/or if there are better ways of implementing this sort of behavior?

Edit: I have since updated a few things. Though they appear to work properly, I'm not sure if I've introduced any errors.

DestructorCallback.h (was GLResource.h)

#pragma once

#include <functional>

namespace engine {

    class DestructorCallback{
        std::function<void(void)> onDestructor;
    public:
        DestructorCallback() = delete;
        DestructorCallback(
            const std::function<void(void)>& onDestructor
        ) :
            onDestructor{onDestructor} {
        }

        ~DestructorCallback() {
            onDestructor();
        }
    };

}

UniformBuffer.cpp

...
UniformBuffer::UniformBuffer(GLsizeiptr size) :
    bindingIndex{bindingIndexIterator++} {

    glGenBuffers(1, &buffer);
    glBindBuffer(GL_UNIFORM_BUFFER, buffer);
    glBufferData(GL_UNIFORM_BUFFER, size, 0, GL_DYNAMIC_DRAW);
    glBindBufferBase(GL_UNIFORM_BUFFER, bindingIndex, buffer);

    LOGGER_INFO("Created {}", buffer);
    onDestroy = std::make_shared<DestructorCallback>([*this]() {
        LOGGER_INFO("Deleted {}", buffer);
        glDeleteBuffers(1, &buffer);
    });
}
...

Basically I moved the T value out of GLResource(as well as renamed GLResource to DestructorCallback) so it can be accessed easier without needing to go through buffer.get()->value each time. The UniformBuffers std::shared_ptr has been renamed to onDestroy.

Still looking for any feedback. (Now also curious if I should revert my changes to the previous version at the top of the post, or keep my newer version?)

Edit 2: You might realize I'm using *this in the lambda callback capture block (which seems to work). I wanted to capture by value(with [=]) but for some reason capture by value produces the wrong value for buffer when the callback is invoked. I'm not sure why this would be as to my understanding it should essentially be a callback holding a copy of the value passed at the time of creation. This makes me worry that my 2nd example may be flawed even if the problems didn't show themselves with the limited testing I did.

\$\endgroup\$
1
\$\begingroup\$

Review:

The idea here is that UniformBuffer can be copied/assigned without destroying the resource held on the GPU, but when all shared instances of the UniformBuffer go out of scope/are deleted the GPU resource is then destroyed.

You may run into problems with this approach if you need to store any other data in the UniformBuffer (or other classes containing a GLResource). Changes made to data in one instance will not be applied to the data in the other instances (unless we put it all in shared_ptrs). It would probably be safer and easier to wrap the UniformBuffer itself in a shared_ptr.

This isn't a problem with the UniformBuffer as it stands, and you'll be ok as long as you don't add any extra state into the user class (i.e. the class directs all state requests to OpenGL).

e.g. For an OpenGL Buffer object used for storing vertex data, we might want to store some state in the class to make OpenGL calls easier:

class Buffer
{
    ...

    template<class ElementT>
    void SetData(GLenum target, ElementT const* data, std::size_t elementCount, GLenum usage)
    {
        ...
        m_elementCount = elementCount;
    }

private:

    std::shared_ptr<GLResource<GLuint>> m_resource;

    std::size_t m_elementCount; // extra state: number of vertices in the buffer
};

Buffer buffer1; // create a buffer to use for vertex data
buffer1.SetData(GL_ARRAY_BUFFER, positions.data(), positions.size(), GL_DYNAMIC_DRAW); // sets m_elementCount

Buffer buffer2 = buffer1; // copy -> buffer2 is the same resource

// lets say we now have fewer vertices (positions.size() is smaller)
buffer2.SetData(GL_ARRAY_BUFFER, positions.data(), positions.size(), GL_DYNAMIC_DRAW); // OpenGL state is updated, buffer2 m_elementCount is changed, but buffer1 m_elementCount is not!

assert(buffer2.GetElementCount() == buffer1.GetElementCount()); // will fail!

I think it's reasonable here for UniformBuffer to inherit from GLResource. The UniformBuffer is a resource, and should inherit its semantics / interface.

We can support move assignment for the GLResource by making the id value non-const and private, and adding a T GetID() const public member function. This also would allow move assignment for derived resource types.


Does OpenGL use any other resource ID type than GLuint (i.e. does GLResource actually need to be templated on it)?


An alternative example:

My own OpenGL resource class from a while ago looks like this:

template<class T>
class Handle
{
public:

    Handle();

    Handle(Handle const&) = delete;
    Handle& operator=(Handle const&) = delete;

    Handle(Handle&& other);
    Handle& operator=(Handle&& other);

    GLuint GetID() const;

protected:

    ~Handle();

private:

    static GLuint Create();
    static void Destroy(GLuint id);

    GLuint m_id;
};

template<class T>
Handle<T>::Handle():
    m_id(Create())
{
    assert(m_id != 0u);
}

template<class T>
Handle<T>::~Handle()
{
    if (m_id != GLuint{ 0 })
    {
        Destroy(m_id);
        m_id = GLuint{ 0 };
    }
}

It uses the CRTP and template specialization to avoid storing a deleter (or creator). Users specialize the static Create() and Destroy() functions to generate the appropriate ID:

namespace Detail
{

    template<>
    GLuint Handle<Buffer>::Create()
    {
        auto id = GLuint{ 0 };
        glGenBuffers(1, &id);
        return id;
    }

    template<>
    void Handle<Buffer>::Destroy(GLuint id)
    {
        glDeleteBuffers(1, &id);
    }

} // Detail

class Buffer : public Detail::Handle<Buffer>
{
public:

    Buffer();

    Buffer(Buffer&&) = default;
    Buffer& operator=(Buffer&&) = default;

    ...
};

The downside to this is that certain OpenGL objects need additional parameters to the create function. When I first implemented it, the best solution I could find was to specialize the entire Detail::Handle class, e.g.:

namespace Detail
{

    template<>
    class Handle<ShaderObject>
    {
    public:

        explicit Handle(GLenum shaderType);

        Handle(Handle const&) = delete;
        Handle& operator=(Handle const&) = delete;

        Handle(Handle&& other);
        Handle& operator=(Handle&& other);

        GLuint GetID() const;

    protected:

        ~Handle();

    private:

        static GLuint Create(GLenum shaderType);
        static void Destroy(GLuint id);

        GLuint m_id;
    };

} // Detail

This could probably be avoided by using variable template arguments for the constructor and Create() functions.

\$\endgroup\$
  • \$\begingroup\$ I'll have to go through this a bit closer when I've got a bit more time, but one of the things you said "You may run into problems with this approach if you need to store any other data in the UniformBuffer ..." kinda confused me. The UniformBuffer as seen in my example doesn't hold any data, rather it purely acts like a bridge to the GL buffer. My idea there was that one or more buffers on the cpu side could update one or more buffers on the gpu side. (ie MainCamera pushes to CameraUBO for one pass, ShadowCamera pushed to CameraUBO for a different pass, etc.) Would that still be a problem? \$\endgroup\$ – Hex Feb 2 at 23:18
  • \$\begingroup\$ I've added an example. You'll be ok as long as UniformBuffer (or other classes using the GLResource don't have any state themselves). \$\endgroup\$ – user673679 Feb 3 at 9:13

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.