std::vector and VBOs render only the last shape

Question

I'm running through a weird problem. Basically I have Mesh class depending on a flag, I can draw a point, a line, or a triangle. For example, if I want to draw two lines, I can do the following

    Vertex vertices1[] = {
        Vertex(glm::vec3(-.5, -.5, 0)),
        Vertex(glm::vec3(  0,  .5, 0))
    };

    Vertex vertices2[] = {
        Vertex(glm::vec3(  .5, -.5, 0)),
        Vertex(glm::vec3( -.5,  .5, 0))
    };

    Mesh mesh1(vertices1, sizeof(vertices1)/sizeof(vertices1[0]), 'L');
    Mesh mesh2(vertices2, sizeof(vertices2)/sizeof(vertices2[0]), 'L');

// Rendering Loop:
while( Window.isOpen() ){
        ...
        //================( Rendering )=========================
        ourShader.Use();
        mesh1.draw();
        mesh2.draw();
        //======================================================
        ...
    }

The result is

Now I would like to use std::vector<Mesh> and loop through meshes. My attempt is as follows

std::vector<Mesh> meshes;
meshes.push_back(mesh1);
meshes.push_back(mesh2);

while( Window.isOpen() ){
    ...
    //================( Rendering )=========================
    ourShader.Use();
    for ( int i(0); i < meshes.size(); ++i )
        meshes[i].draw();
    //======================================================
    ...
}

With the preceding approach, only the last line is drawn and this is the result

Moreover, once I use .push_back() even if I don't loop through the vector, the last line is drawn. I don't understand why using std::vector deteriorates the rendering. I even tried meshes[0].draw() but with no luck. Any suggestions?

Edit: This is the constructor of Mesh class

#include <iostream>
#include <vector>
#include <glm/glm.hpp>
#include <GL/glew.h>
#include <GLFW/glfw3.h>
#include "display.h"
#include "keyboard.h"
#include "shader.h"

class Vertex
{
public:
    Vertex(const glm::vec3& p) : m_position(p)
    {}

private:
    glm::vec3 m_position;

};

class Mesh
{
public:
    Mesh(Vertex* vertices, unsigned int numVertices, const char& flag);
    ~Mesh();
    void draw();
private:
    enum{
        POSITION_VB,
        NUM_BUFFERS
    };

    GLuint m_vertexArrayObject;
    GLuint m_vertexArrayBuffers[NUM_BUFFERS];
    unsigned int m_drawCount;

    char m_flag;
};


Mesh::Mesh(Vertex* vertices, unsigned int numVertices, const char& flag) : m_flag(flag), m_drawCount(numVertices)
{

    glGenVertexArrays(1, &m_vertexArrayObject);
    glBindVertexArray(m_vertexArrayObject);

    glGenBuffers(NUM_BUFFERS, m_vertexArrayBuffers);
    glBindBuffer(GL_ARRAY_BUFFER, m_vertexArrayBuffers[POSITION_VB]);
    glBufferData(GL_ARRAY_BUFFER, numVertices*sizeof(vertices[0]), vertices, GL_STATIC_DRAW);

    glEnableVertexAttribArray(0);

    glVertexAttribPointer(0, 3, GL_FLOAT, GL_FALSE, 0, 0);

    glBindVertexArray(0);
}

Mesh::~Mesh()
{
    glDeleteVertexArrays(1, &m_vertexArrayObject);
    glDeleteBuffers(1, m_vertexArrayBuffers);
}

void Mesh::draw()
{
    switch(m_flag)
    {
        case 'P':
            glBindVertexArray(m_vertexArrayObject);
            glDrawArrays(GL_POINTS, 0, m_drawCount);
            glBindVertexArray(0);
        break;
        case 'L':
            glBindVertexArray(m_vertexArrayObject);
            glDrawArrays(GL_LINES, 0, m_drawCount);
            glBindVertexArray(0);
        break;
        case 'T':
            glBindVertexArray(m_vertexArrayObject);
            glDrawArrays(GL_TRIANGLES, 0, m_drawCount);
            glBindVertexArray(0);
        break;
    }

}



int main(void)
{
    Display Window(800, 600, "OpenGL Window");
    Keyboard myKeyboard( Window.getWindowPointer() );

    Vertex vertices1[] = {
        Vertex(glm::vec3(-.5, -.5, 0)),
        Vertex(glm::vec3(  0,  .5, 0))
    };

    Vertex vertices2[] = {
        Vertex(glm::vec3(  .5, -.5, 0)),
        Vertex(glm::vec3( -.5,  .5, 0))
    };

    Mesh mesh1(vertices1, sizeof(vertices1)/sizeof(vertices1[0]), 'L');
    Mesh mesh2(vertices2, sizeof(vertices2)/sizeof(vertices2[0]), 'L');

    std::vector<Mesh> meshes;
    meshes.emplace_back(mesh1);
    meshes.emplace_back(mesh2);
    std::cout << meshes.size() << std::endl;
    //*****************( SHADER )************************
    Shader ourShader("shader.vs", "shader.frag");

    glEnable(GL_PROGRAM_POINT_SIZE);


    while( Window.isOpen() ){
        Window.PollEvents();
        Window.clear();

        //================( Rendering )=========================
        ourShader.Use();
        //mesh1.draw();
        //mesh2.draw();
        for ( int i(0); i < meshes.size(); ++i )
            meshes[i].draw();
        //meshes[0].draw();
        //meshes[1].draw();
        //======================================================

        Window.SwapBuffers();
    }



    glfwTerminate();
    return 0;
}

Shaders

#version 330 core                      

out vec4 color;                        
void main()                            
{                                      
    color = vec4(1.0f,0.5f,0.2f,1.0f); 
}   

#version 330 core                                                

layout (location = 0) in vec3 position; 

void main()                                                      
{         
    gl_PointSize = 10.0;
    gl_Position = vec4(position, 1.0); 
}

You code looks OK. It's hard to tell what is the problem without `Mesh` code. Does it contain VAO descriptor? Maybe the problem is in it's assignment operator or copy constructor? What will happen if you don't create `mesh1` and `mesh2` objects, but use `std::vector::emplace` with appropriate arguments instead? — Sergey, Nov 29 '16 at 03:50
@Sergey, yes it does contain VAO. Please see the update. I've tried `std::vector::emplace` with the same issue. — CroCo, Nov 29 '16 at 04:01
If you iterate through your vector in reverse order...is only the first one drawn? Is it possible that 'draw' is erasing the scene? — Arunas, Nov 29 '16 at 05:38
Try changing ++i to i++ in the for loop, seems to me that the variable currently increments and then runs in the for loop, leading to only the second mesh being drawn. — SporreKing, Nov 29 '16 at 07:45
@SporreKing,as I said in the post even if I explicitly call this `meshes[0].draw()`, the problem persists. — CroCo, Nov 29 '16 at 07:50
It's probably a copy-constructor issue. Without seeing a [MCVE](http://stackoverflow.com/help/mcve) it's hard solve your problem remotely. — Yakov Galka, Nov 29 '16 at 08:23
Along the lines of what @ybungalobill was already hinting at: Is this the complete `Mesh` code? Or does the class also have a destructor? — Reto Koradi, Nov 30 '16 at 03:39
@RetoKoradi, please see MCVE in the update. For your question, yes it does has destructor. — CroCo, Nov 30 '16 at 03:46
@RetoKoradi, it seems the problem from the destructor. It is being called. When I comment out `glDeleteVertexArrays(1, &m_vertexArrayObject);` the code works. — CroCo, Nov 30 '16 at 03:58

score 2 · Accepted Answer · edited May 23 '17 at 12:24

As I suspected, the problem is with the (lack of) copy constructor. The default one just copies all the members. As a result your VAOs and buffers get deleted multiple times, even before you manage to draw anything (vectors move during reallocation, and if they can't move they copy). As a rule of thumb: if you have a non-default destructor, you must implement also a copy constructor and an assignment operator, or explicitly delete them if your class is not meant to be copyable.

For your concrete case the solutions are:

Quick solution: store pointers to meshes in the vector:

std::vector<Mesh*> meshes;
meshes.emplace_back(&mesh1);
meshes.emplace_back(&mesh2);

Correct solution: use proper RAII for resource management. Using the unique_ptr technique from here your MCVE code becomes:

class Mesh
{
public:
    Mesh(Vertex* vertices, unsigned int numVertices, const char& flag);
    void draw();
private:
    //...
    GLvertexarray m_vertexArrayObject;
    GLbuffer m_vertexArrayBuffers[NUM_BUFFERS];
    unsigned int m_drawCount;
    char m_flag;
};


Mesh::Mesh(Vertex* vertices, unsigned int numVertices, const char& flag) : m_flag(flag), m_drawCount(numVertices)
{
    GLuint id;
    glGenVertexArrays(1, &id);
    glBindVertexArray(id);
    m_vertexArrayObject.reset(id);
    for(int i = 0; i < NUM_BUFFERS; ++i)
    {
        glGenBuffers(1, &id);
        glBindBuffer(GL_ARRAY_BUFFER, id);
        m_vertexArrayBuffers[i].reset(id);
        glBufferData(GL_ARRAY_BUFFER, numVertices*sizeof(vertices[0]), vertices, GL_STATIC_DRAW);
    }

    glEnableVertexAttribArray(0);
    glVertexAttribPointer(0, 3, GL_FLOAT, GL_FALSE, 0, 0);
    glBindVertexArray(0);
}

void Mesh::draw()
{
    switch(m_flag)
    {
        case 'P':
            glBindVertexArray(m_vertexArrayObject.get());
            glDrawArrays(GL_POINTS, 0, m_drawCount);
            glBindVertexArray(0);
        break;
        case 'L':
            glBindVertexArray(m_vertexArrayObject.get());
            glDrawArrays(GL_LINES, 0, m_drawCount);
            glBindVertexArray(0);
        break;
        case 'T':
            glBindVertexArray(m_vertexArrayObject.get());
            glDrawArrays(GL_TRIANGLES, 0, m_drawCount);
            glBindVertexArray(0);
        break;
    }

}

int main()
{
    //...

    Mesh mesh1(vertices1, sizeof(vertices1)/sizeof(vertices1[0]), 'L');
    Mesh mesh2(vertices2, sizeof(vertices2)/sizeof(vertices2[0]), 'L');

    std::vector<Mesh> meshes;
    meshes.emplace_back(std::move(mesh1));
    meshes.emplace_back(std::move(mesh2));

    // ...

    return 0;
}

Notice how there is no more need for defining a destructor, and your class automatically becomes movable but not copyable. Furthermore, if you have OpenGL 4.5 or ARB_direct_state_access then things get even simpler.

score 1 · Answer 2 · edited May 23 '17 at 11:52

EDIT

The main problem is, that the destructor is called when you add the Mesh objects to the vector, therefore the underlying data gets cleaned up. Further reading: Why does my class's destructor get called when I add instances to a vector? | What is The Rule of Three?

I'd personally create separate init_buffers and free_buffers methods to my Mesh class and use them appropriately. (Initialize buffers after the OpenGL context is obtained, free the buffers when the window is closed.) This way you can start building meshes (and add them to the scene) before actually having the OpenGL context.

I've implemented the missing code parts and tried your code using GLFW using CLion.

It works. See code / CLion project here: OpenGLSandbox/main.cpp

The only code I've added are basically these, so it's your turn to figure out the difference / error.

// Constants
const size_t NUM_BUFFERS = 1;
const size_t POSITION_VB = 0;

// Vertex class
class Vertex {
private:
    glm::vec3 mCoords;
public:
    Vertex(glm::vec3 coords) : mCoords(coords) {};
};

// Mesh class
class Mesh {
private:
    GLuint m_vertexArrayObject;
    char m_flag;
    unsigned int m_drawCount;
    GLuint m_vertexArrayBuffers[NUM_BUFFERS];
public:
    /* your ctor and draw method */
}

Thank you for the answer. I will probably compile the same code in my Mac to see if this makes any difference. — CroCo, Nov 30 '16 at 03:48

std::vector and VBOs render only the last shape

2 Answers2