Mesh class called with default constructor not working OpenGL C++

Question

I created a Mesh class for OpenGL 3.3, it works fine when I create the class with a non-default constructor, when I create the vertices when I create the object.
However, I now want to have multiple objects that I can create dynamically by putting them in a vector, so I had to add in a default constructor I use the same functions for setting up the buffer data as with the other constructor... but it doesn't work. It's as far as I can tell not because of the fact it's in the vector but it's something to do with the constructor or something with the fact the buffer data is created later. I'm really not quite sure.

Here are my classes. ( When I create a mesh that works I call the constructor with parameters and when it doesn't work I construct a mesh with no parameters and call the "changeMesh" function)

mesh.h

#ifndef MESH_H
#define MESH_H

#include <iostream>
#include <vector>
#include <GL/glew.h>
#include <glm/glm.hpp>
#include <glm/gtc/matrix_transform.hpp>
#include <glm/gtc/type_ptr.hpp>

class mesh
{
    public:
        mesh();
        mesh(std::vector<GLfloat> vertices, std::vector<GLuint> triangles, GLuint shaderProgram);
        ~mesh();
        void changeMesh(std::vector<GLfloat> vertices, std::vector<GLuint> triangles, GLuint shaderProgram);
        void render();
        void Translate(glm::vec3 addVector);
        void Rotate(glm::vec3 rotVector, GLfloat angle);
    protected:
    private:
        GLuint vertexArrayObject, vertexBuffer, elementBuffer, shaderProgram;
        std::vector<GLfloat> vertices;
        std::vector<GLuint> indices;
        glm::mat4 transform;
        void setUpMesh();
        void bindVertices();
};

#endif // MESH_H

mesh.cpp

    #include "../include/mesh.h"

mesh::mesh(std::vector<GLfloat> vertices, std::vector<GLuint> indices, GLuint shaderProgram)
{
    this->shaderProgram = shaderProgram;
    this->vertices = vertices;
    this->indices = indices;
    setUpMesh();
}

mesh::mesh(){
    glGenVertexArrays(1, &vertexArrayObject);
    glBindVertexArray(vertexArrayObject);

    glGenBuffers(1, &vertexBuffer);
    glGenBuffers(1, &elementBuffer);
}

mesh::~mesh()
{
    glDeleteBuffers(1, &elementBuffer);
    glDeleteBuffers(1, &vertexBuffer);

    glDeleteVertexArrays(1, &vertexArrayObject);
}

void mesh::changeMesh(std::vector<GLfloat> vertices, std::vector<GLuint> triangles, GLuint shaderProgram){
    this->shaderProgram = shaderProgram;
    this->vertices = vertices;
    this->indices = indices;
    bindVertices();

}

void mesh::setUpMesh(){
    glGenVertexArrays(1, &vertexArrayObject);
    glBindVertexArray(vertexArrayObject);

    glGenBuffers(1, &vertexBuffer);
    glGenBuffers(1, &elementBuffer);

    bindVertices();
    glBindVertexArray(0);

}

void mesh::bindVertices(){
    glBindVertexArray(vertexArrayObject);

    glBindBuffer(GL_ARRAY_BUFFER, vertexBuffer);
    glBufferData(GL_ARRAY_BUFFER, this->vertices.size() * sizeof(GLfloat), this->vertices.data(), GL_STATIC_DRAW);

    glBindBuffer(GL_ELEMENT_ARRAY_BUFFER, elementBuffer);
    glBufferData(GL_ELEMENT_ARRAY_BUFFER, this->indices.size() * sizeof(GLuint), this->indices.data(), GL_STATIC_DRAW);


    GLint amountDataPerVert = 5;

    glEnableVertexAttribArray(0);
    glVertexAttribPointer(0, 3, GL_FLOAT, GL_FALSE, amountDataPerVert*sizeof(GLfloat), 0);

    glEnableVertexAttribArray(1);
    glVertexAttribPointer(1, 2, GL_FLOAT, GL_FALSE, amountDataPerVert*sizeof(GLfloat), (void*)(3*sizeof(GLfloat)));


    glBindVertexArray(0);

}
void mesh::render(){
    glBindVertexArray(vertexArrayObject);
    glUniformMatrix4fv(glGetUniformLocation(shaderProgram, "transform"), 1, GL_FALSE, glm::value_ptr(transform));

    glDrawElements(GL_TRIANGLES, indices.size(), GL_UNSIGNED_INT, 0);
    glBindVertexArray(0);
}

void mesh::Translate(glm::vec3 addVector){
    transform = glm::translate(transform, addVector);
}

void mesh::Rotate(glm::vec3 rotVector, GLfloat angle){
    transform = glm::rotate(transform, glm::radians(angle), rotVector);
}

Answer 1

While you believe that the problem has nothing to do with storing the objects in a vector, I have a strong feeling that it probably does. The way you are encapsulating OpenGL objects in C++ wrappers is a recipe for pain, and you're probably finding out like many did before you.

The typical problems are caused by the combination of what happens when objects are copied and destructed. The OpenGL objects owned by the C++ wrapper are deleted in the destructor:

mesh::~mesh()
{
    glDeleteBuffers(1, &elementBuffer);
    glDeleteBuffers(1, &vertexBuffer);

    glDeleteVertexArrays(1, &vertexArrayObject);
}

To illustrate the problem with this, let's look at a typical sequence. Let's say you have a vector of mesh objects, and a method to add a new mesh to this vector (points annotated for later reference):

std::vector<mesh> m_meshes;

void createMesh(...) {
    mesh newMesh;  // point 1
    newMesh.changeMesh(...);
    m_meshes.push_back(newMesh);  // point 2
}  // point 3

Looks harmless? It's not at all. Bad things happened here:

• Point 1: New object is created. The constructor creates the OpenGL objects, and stores their names in member variables.
• Point 2: A copy of the mesh object is added to the vector, where the copy is created with the default copy constructor. This means that the member variables, which contain the OpenGL object names, are copied.
• Point 3: The mesh object goes out of scope. The destructor is invoked, which deletes the OpenGL objects.

What you have after all of this is a mesh object stored in the vector, with OpenGL object names stored in its member variables, while the actual OpenGL objects have been deleted. This means that the object names stored in this mesh object are now invalid.

The root problem is that your class does not have proper copy constructors and assignment operators. And unfortunately, it is not easily possible to implement them when storing OpenGL object names in member variables, and generating/deleting the object names in constructor/destructor.

There are a number of ways to handle this. None of them are perfectly pretty:

1. Do not generate/delete the OpenGL objects in constructor/destructor. Instead, use some form of init()/cleanup() methods that you invoke explicitly. The downside is that you have to be careful to invoke these methods correctly. For example, if you have a vector of objects, and want to delete the vector, you have to invoke cleanup() on all members of the vector manually.

2. Always reference the objects with pointers. Instead of having a vector of mesh objects, use a vector of mesh object pointers. This way, the objects are not copied. You also have to be careful to manage the lifetime of the objects correctly, and not leak them. This is easiest if you use some form of smart pointer instead of naked pointers.

3. Use some form of hybrid, where you still use actual C++ objects, but they store the names of the underlying OpenGL objects in a nested object that is reference counted. This way, they can implement proper copy/assign semantics.

I think the easiest and cleanest approach is option 2 with using smart pointers. Newer versions of C++ have smart pointers in the standard library, so there isn't anything you need to implement. For example in C++11, you can use the type std::shared_ptr<mesh> to reference your mesh objects. The code fragment above would then look like this:

std::vector<std::shared_ptr<mesh> > m_meshes;

void createMesh(...) {
    std::shared_ptr<mesh> newMesh = std::make_shared<mesh>();
    newMesh->changeMesh(...);
    m_meshes.push_back(newMesh);
}

To be sure that you don't accidentally copy the objects anyway, it's also a good idea to declare unimplemented (private) copy constructors and assignment operators for the class. This topic explains how to do that best in C++11: With explicitly deleted member functions in C++11, is it still worthwhile to inherit from a noncopyable base class?.