The reference counter and the destructor are called twice

advertisements

I am creating a class OpenGLManager, the idea of this class is to manage references to resources allocated in OpenGL (like Vertex Array Objects and Vertex Buffer Objects).

So what I want to do is whenever I allocate a new resource I add the reference into this class, and whenever the object that uses this resource is destroyed (destructor) the reference to this resource is decremented. One problem that I realized is that in some instances the destructor is called twice for an object, and that makes my code break. destructor called twice

What I am basically creating something like a shared_ptr, but instead of pointers I have handlers. Here is the code:

class OpenGLManager
{
public:
    static OpenGLManager& getInstance()
    {
        static OpenGLManager instance; 

        return instance;
    }
    // Increment reference counter
    void incrementVBO(GLuint vbo);
    void incrementVAO(GLuint vao);
    void incrementShaderProgram(GLuint program);
    // Decrement reference counter
    void decrementVAO(GLuint vao);
    void decrementVBO(GLuint vbo);
    void decrementShaderProgram(GLuint program);
private:
    std::unordered_map<GLuint, int> _vbos;
    std::unordered_map<GLuint, int> _vaos;
    std::unordered_map<GLuint, int> _programs;

    OpenGLManager() {};
    OpenGLManager(const OpenGLManager&) = delete;
    void operator=(OpenGLManager const&) = delete;

};
---------------------------------------------------------------
#include "opengl_manager.h"
using std::vector;

void lh::OpenGLManager::incrementShaderProgram(GLuint program)
{
    if (_programs.find(program) == _programs.end())
        _programs[program] = 1;
    else
        _programs[program]++;
}

void lh::OpenGLManager::incrementVAO(GLuint vao)
{
    if (_vaos.find(vao) == _vaos.end())
        _vaos[vao] = 1;
    else
        _vaos[vao]++;

}

void lh::OpenGLManager::incrementVBO(GLuint vbo)
{
    if (_vbos.find(vbo) == _vbos.end())
        _vbos[vbo] = 1;
    else
        _vbos[vbo]++;
}

void lh::OpenGLManager::decrementVAO(GLuint vao)
{
    if (_vaos.count(vao) == 0)
    {
        std::cerr << "Trying to decrement VAO: " << vao << ". Fatal Error." << std::endl;
        exit(EXIT_FAILURE);
    }

    _vaos[vao]--;

    if (_vaos[vao] == 0) // There are no more references
    {
        auto iter = _vaos.find(vao);
        if (iter == _vaos.end())
        {
            std::cerr << "Trying to remove inexistent hashmap (vao) key." << std::endl;
            exit(EXIT_FAILURE);
        }
        _vaos.erase(iter);

        glDeleteVertexArrays(1, &vao);
    }
}

void lh::OpenGLManager::decrementVBO(GLuint vbo)
{
    if (_vbos.count(vbo) == 0)
    {
        std::cerr << "Trying to decrement VBO: " << vbo << ". Fatal Error." << std::endl;
        exit(EXIT_FAILURE);
    }

    _vbos[vbo]--;

    if (_vbos[vbo] == 0) // There are no more references
    {
        auto iter = _vbos.find(vbo);
        if (iter == _vbos.end())
        {
            std::cerr << "Trying to remove inexistent hashmap (vbo) key." << std::endl;
            exit(EXIT_FAILURE);
        }
        _vbos.erase(iter);

        glDeleteBuffers(1, &vbo);
    }
}

void lh::OpenGLManager::decrementShaderProgram(GLuint program)
{
    if (_programs.count(program) == 0)
    {
        std::cerr << "Trying to decrement Program: " << program << ". Fatal Error." << std::endl;
        exit(EXIT_FAILURE);
    }

    _programs[program]--;

    if (_programs[program] == 0) // There are no more references
    {
        auto iter = _programs.find(program);
        if (iter == _programs.end())
        {
            std::cerr << "Trying to remove inexistent hashmap (program) key." << std::endl;
            exit(EXIT_FAILURE);
        }
        _programs.erase(iter);

        glDeleteProgram(program);
    }
}

This seems to work, except when I add objects on a std::vector with push_back, then the destructor is called twice, which decrements the reference by 2. How can I avoid this? Or is this just a bad design?

EDIT

Implementation of class that uses OpenGLManager

lh::RawModel::RawModel(vector<GLfloat> vertices, vector<GLint> indices, GLenum usage)
: _vao(-1), _indicesCount(indices.size())
{
    std::cout << "CREATING MYSELF" << std::endl;
    createAndBindVAO();
    storeVerticesDataInVBO(0, vertices, 0, usage);
    storeVerticesDataInVBO(1, vertices, VERTEX_SIZE * sizeof(GLfloat), usage);
    storeIndicesDataInEBO(indices, usage);
    unbindVAO();
}

lh::RawModel::~RawModel()
{
    // Decrement reference counters for opengl resources
    std::cout << "DESTROYING MYSELF" << std::endl;
    lh::OpenGLManager::getInstance().decrementVAO(_vao);
    lh::OpenGLManager::getInstance().decrementVBO(_vbo);
    lh::OpenGLManager::getInstance().decrementVBO(_ebo);
}

const int lh::RawModel::VERTEX_SIZE = 3;

void lh::RawModel::storeIndicesDataInEBO(vector<GLint> indices, GLenum usage)
{
    glGenBuffers(1, &_ebo);
    lh::OpenGLManager::getInstance().incrementVBO(_ebo);

    glBindBuffer(GL_ELEMENT_ARRAY_BUFFER, _ebo);
    glBufferData(GL_ELEMENT_ARRAY_BUFFER, indices.size() * sizeof(GLint), &indices[0], usage);
}

void lh::RawModel::storeVerticesDataInVBO(int iAttribute, vector<GLfloat> data, int offset, GLenum usage)
{
    glGenBuffers(1, &_vbo);
    lh::OpenGLManager::getInstance().incrementVBO(_vbo);

    glBindBuffer(GL_ARRAY_BUFFER, _vbo);
    glBufferData(GL_ARRAY_BUFFER, data.size() * sizeof(GLfloat), &data[0], usage);
    glVertexAttribPointer(iAttribute, VERTEX_SIZE, GL_FLOAT, GL_FALSE, 2 * (VERTEX_SIZE * sizeof(float)), (GLvoid*)offset);
    glEnableVertexAttribArray(iAttribute);
    glBindBuffer(GL_ARRAY_BUFFER, 0);
}

void lh::RawModel::createAndBindVAO()
{
    glGenVertexArrays(1, &_vao);
    glBindVertexArray(_vao);
    std::cout << "incrementing vao " << _vao << std::endl;
    lh::OpenGLManager::getInstance().incrementVAO(_vao);
}

int lh::RawModel::getNumberOfIndices()
{
    return _indicesCount;
}

GLuint lh::RawModel::getVAO()
{
    return _vao;
}

void lh::RawModel::unbindVAO()
{
    glBindVertexArray(0);
}

Example of a call that breaks:

models_.push_back(lh::RawModel(vertices, indices, GL_STATIC_DRAW));


This is expected behavior.

Your design unfortunately isn't safe. If you create a scoped variable, increment it, copy it to a varible outside the scope, then the scoped variable will be destroyed (call decrement) and then, when you finally destroy the other variable, it will decrement a second time, crashing the program. Something like this:

RawModel rawModel;
{
    RawModel temp;
    temp.increment() //calls increment methods.
    rawModel = temp;
} // destructor called once to clean up temp;
// destroy rawModel. destructor called twice to clean up rawModel;

And this is exactly what happens, will instantiace RawModel and then passes it to vector, which copies your variable inside, now you have two instances to call the destructor and break your program.

Options: 1. (Recommended) Use shared_ptr. 2. Define copy constructor and operator =, copying the content from one RawModel to another AND incrementing the count.

Although the second options sounds easier if you have some free time, go with option 1, learn shared_ptr, it'll help you a lot in future programs.

1.
shared_ptr<RawModel> rawModel = make_shared(new RawModel);
rawModel->increment() //increment stuff
vector.push_back(rawModel);
//now it'll work.

2.
//additional RawModel methods:
RawModel(const RawModel& rawModel) {
    // copies everything and if it is initialized increment
}

const RawModel& operator =(const RawModel& other) {
    //same as copy constructur
}

Hope this helps.