Why the destructor gets called before doing the copy?

Question

I have the following code

class Mesh
{
public:
    Mesh();
    Mesh(std::vector<Vertex> vertices, std::vector<GLuint> indices);
    ~Mesh();
    void draw(Shader& shader);

private:
    std::vector<Vertex> mVertices;
    std::vector<GLuint> mIndices;
    GLuint mVBO;
    GLuint mEBO;
};

Mesh::Mesh(std::vector<Vertex> vertices, std::vector<GLuint> indices)
{   
    mIndices = indices;
    glGenBuffers(1, &mEBO);
    glBindBuffer(GL_ELEMENT_ARRAY_BUFFER, mEBO);
    glBufferData(GL_ELEMENT_ARRAY_BUFFER, mIndices.size() * sizeof(GLuint), &mIndices[0], GL_STATIC_DRAW);

    mVertices = vertices;
    glGenBuffers(1, &mVBO);
    glBindBuffer(GL_ARRAY_BUFFER, mVBO);
    glBufferData(GL_ARRAY_BUFFER, mVertices.size() * sizeof(Vertex), &mVertices[0], GL_STATIC_DRAW);

    glBindBuffer(GL_ARRAY_BUFFER, 0);
    glBindBuffer(GL_ELEMENT_ARRAY_BUFFER, 0);

    computeBoundingBox();
}

Mesh::~Mesh()
{
    glDeleteBuffers(1, &mVBO);
    glDeleteBuffers(1, &mEBO);
}

void Mesh::draw(Shader& shader)
{   
    glBindBuffer(GL_ELEMENT_ARRAY_BUFFER, mEBO);
    glBindBuffer(GL_ARRAY_BUFFER, mVBO);

    GLuint vpos = glGetAttribLocation(shader.program(), "vPosition");
    GLuint vnor = glGetAttribLocation(shader.program(), "vNormal");

    glVertexAttribPointer(vpos, 3, GL_FLOAT, GL_FALSE, sizeof(Vertex), 0);
    glVertexAttribPointer(vnor, 3, GL_FLOAT, GL_FALSE, sizeof(Vertex), (void*)sizeof(Vector));

    shader.bind();
    glEnableVertexAttribArray(vpos);
    glEnableVertexAttribArray(vnor);
    glDrawElements(GL_TRIANGLES, mIndices.size(), GL_UNSIGNED_INT, 0);
    shader.unbind();

    glBindBuffer(GL_ELEMENT_ARRAY_BUFFER, 0);
    glBindBuffer(GL_ARRAY_BUFFER, 0);
}

void loadSquare(Mesh& mesh)
{
    std::vector<Vertex> vertices;
    vertices.push_back(Vertex(Vector(0.5f, 0.5f, 0.f), Vector(1.f, 0.f, 0.f)));
    vertices.push_back(Vertex(Vector(-0.5f, 0.5f, 0.f), Vector(0.f, 1.f, 0.f)));
    vertices.push_back(Vertex(Vector(-0.5f, -0.5f, 0.f), Vector(0.f, 0.f, 1.f)));
    vertices.push_back(Vertex(Vector(0.5f, -0.5f, 0.f), Vector(1.f, 0.f, 1.f)));

    std::vector<GLuint> indices;
    indices.push_back(0);
    indices.push_back(1);
    indices.push_back(2);
    indices.push_back(0);
    indices.push_back(2);
    indices.push_back(3);

    mesh = Mesh(vertices, indices);
}

int main(int argc, char** argv)
{
    // Create opengl context and window
    initOGL();

    // Create shaders
    Shader shader("render.vglsl", "render.fglsl");

    Mesh mesh;
    loadSquare(mesh);

    while (!glfwWindowShouldClose(window))
    {
        glClear(GL_COLOR_BUFFER_BIT | GL_DEPTH_BUFFER_BIT);

        mesh.draw(shader);

        glfwSwapBuffers(window);
        glfwPollEvents();
    }

    glfwTerminate();
    return 0;
}

If I try to run it it just displays a gray image on the window it creates.

After tracking the application with the debugger, when it hits the line mesh = Mesh(vertices, indices) it creates the buffers for OpenGL and copy the vertices and indices std::vectors to the variable mesh that was passed as parameter. However, it also calls the destructor of the object created by Mesh(vertices,indices) which in turn invalidate the buffers in the OpenGL context, so when then application reaches mesh.draw(shader) the buffers mesh points to are not valid anymore.

Does the move-constructor can help me solve my problem, i.e. avoid the call to the destructor of Mesh(vertices,indices)? Are there any other solutions?

Answer 1

From your source, your are doing the bind to vectors that are immediately destroyed.

Essentially, in your loadSquare function you are creating a mesh and binding it when you write Mesh(vertices, indices); on the right side of the assignment in the last line of loadSquare.

mesh = Mesh(vertices, indices);

Think that line like this:

  ...
  Mesh m1(vertices, indices);  // a 
  mesh= m1;                    // b
  // m1 gets destroyed here.
}

Line (a) creates and binds the mesh.

When you assign it to mesh in line b, mesh.mVertices and mesh.mIndices will get copies of the vectors and mVBO and mEBO will get copies of the bound values.

Think of that line (b) as writing

mesh.mVertices= m1.mVertices;  // mesh gets a new vector with the same values
mesh.mIndices= m1.mIndices;    // mesh gets a new vector with the same values
mesh.mVBO= m1.mVBO;
mesh.mEBO= m1.mEBO;

At the end of loadSquare() m1 will be destroyed (destructor called).

In your calling function you will end up with mesh containing mVBO and mEBO members bound to the vectors that were destroyed. It contains its own vectors, these have the same values, but these are copies in different memory locations that were never bound.

There are various ways to solve this, e.g. returning the square mesh through pointer. Or writing an assignment operator (google for shallow copy).

But my suggestion would be to create a an empty constructor and an additional fillMesh function like your current cotstructor.

Mesh::Mesh(void);            // set mVBO and mEBO to zero.
void Mesh::fillMesh(std::vector<Vertex> vertices, std::vector<GLuint> indices);  // same code as your current constructor.

Then rewrite your loadSquare function like this:

void loadSquare(Mesh& mesh)
{
 std::vector<Vertex> vertices;
 vertices.push_back(Vertex(Vector(0.5f, 0.5f, 0.f), Vector(1.f, 0.f, 0.f)));
 vertices.push_back(Vertex(Vector(-0.5f, 0.5f, 0.f), Vector(0.f, 1.f, 0.f)));
 vertices.push_back(Vertex(Vector(-0.5f, -0.5f, 0.f), Vector(0.f, 0.f, 1.f)));
 vertices.push_back(Vertex(Vector(0.5f, -0.5f, 0.f), Vector(1.f, 0.f, 1.f)));

 std::vector<GLuint> indices;
 indices.push_back(0);
 indices.push_back(1);
 indices.push_back(2);
 indices.push_back(0);
 indices.push_back(2);
 indices.push_back(3);

 mesh.fillMesh(vertices, indices);
}

Thus the loadSquare will create the vertices and indices and set them into the mesh from the calling function and bind them.

Further notes (for a clean solution):

• The destructor should probably also unbind the vector and indices from GL.
• The fillMesh function should probably check if the mesh is already bound and unbinds the old vectors before setting and binding the new ones (in case you are calling fillMesh again on an active mesh).
• You should probably still write an assignment operator that calls fillMesh: Mesh::operator=(const Mesh &other); // google shallow copy

Answer 2

The Mesh constructor gets called when you instantiate Mesh(vestices, indices). This is just a temporary object instance. The code is then calling the assignment operator to copy the temporary object instance into the mesh variable. Since you haven't defined operator= [given the code provided], it does a default assignment behavior. Once that assignment is completed the ~Mesh destructor gets called.

Answer 3

You violated rule of three (rule of five) and of course you get issue after doing assignment. On this line:

mesh = Mesh(vertices, indices);

you create a temporary object that destroyed immediately after the statement. So properly implement or prohibit copy ctor and copy assignment operator to resolve the issue. You may want to implement move ctor and move assignment operator as well, especially if you prohibit copying.