Mesh Class Called with Default Constructor Not Working Opengl C++

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

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:

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.
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.
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?.

Error: no default constructor exists for class Mesh, attempting to pass Mesh as a parameter into a different constructor

This:

Object::Object(Transform transform, Mesh mesh, Shader shader)
{
    this->transform = transform;
    this->mesh = mesh;
    this->shader = shader;
}

Should be written like this instead:

Object::Object(const Transform& t, const Mesh& m, const Shader& s)
    : transform(t), mesh(m), shader(s)
{
}

C++ isn't like C#, and you're going to get very confused if you assume it is.

In C++, all subobjects are constructed before you enter the body of the constructor. So the subobjects of your Object class (transform, mesh, and shader) are constructed using default constructors because you didn't specify anything else. The assignments that you coded in the body of your constructor have no effect on this.

To specify different parameters for the constructors of your subobjects, use a member initializer list, like my code above shows.

Using OpenGL functions in object constructor without context loaded

Change vertices and mesh to be (smart) pointers and construct the objects in main() after you secure a GL context.

If you don't want to change your existing code (. to -> for member access) you can call them verticesPtr and meshPtr and create local references (Mesh& mesh = *meshPtr;) at the top of the functions where you reference them.

std::vector and VBOs render only the last shape

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.

OpenGL sigsegv error in class constructor

Thanks to both @much_a_chos and @vu1p3n0x for your help. Turns out much_a_chos had the right idea with the Game object initializing before the Window object, thus missing the glewInit() call altogether, resulting in the sigsegv error. The problem, however, was not in the initializer list but in the main.cpp file. I was creating a Game class object and then passing that Game object via pointer to the core class, so regardless of how I arranged the Core class, the Game object would always initialize before the Window class, and would therefore always do its glGenVertexArrays call before glewInit() is called. This is a terrible logic error on my side and I apologize for wasting your time.

Below are extracts from the fixed main.cpp file and the fixed TempCore class (please keep in mind that these are temporary fixes to illustrate how I would go about fixing my mistake):

class TempCore
{
public:
    TempCore(Window* w, Game* g) :  //take in a Window class pointer to ensure its created before the Game class constructor
        win(w), gamew(g) {}
    ~TempCore() { if(gamew) delete gamew; }

    void start();

private:
    Window* win;
    Game* gamew;
};

int WINAPI WinMain (HINSTANCE hThisInstance, HINSTANCE hPrevInstance, LPSTR lpszArgument, int nCmdShow)
{
    Window* win = new Window(800, 800, "EngineTry", false); //this way the Window constructor with the glewinit() call is called before the Game contructor
    TempCore m_core(win, new Game());
    m_core.start();

    return 0;
}

OpenGL object in C++ RAII class no longer works

All of those operations copy the C++ object. Since your class did not define a copy constructor, you get the compiler-generated copy constructor. This simply copies all of the members of the object.

Consider the first example:

vector<BufferObject> bufVec;
{
  BufferObject some_buffer;
  //Initialize some_buffer;
  bufVec.push_back(some_buffer);
}
bufVec.back(); //buffer doesn't work.

When you call push_back, it copies some_buffer into a BufferObject in the vector. So, right before we exit that scope, there are two BufferObject objects.

But what OpenGL buffer object do they store? Well, they store the same one. After all, to C++, we just copied an integer. So both C++ objects store the same integer value.

When we exit that scope, some_buffer will be destroyed. Therefore, it will call glDeleteBuffers on this OpenGL object. But the object in the vector will still have its own copy of that OpenGL object name. Which has been destroyed.

So you can't use it anymore; hence the errors.

The same thing happens with your InitBuffer function. buff will be destroyed after it is copied into the return value, which makes the returned object worthless.

This is all due to a violation of the so-called "Rule of 3/5" in C++. You created a destructor without creating copy/move constructors/assignment operators. That's bad.

To solve this, your OpenGL object wrappers should be move-only types. You should delete the copy constructor and copy assignment operator, and provide move equivalents that set the moved-from object to object 0:

class BufferObject
{
private:
  GLuint buff_;

public:
  BufferObject()
  {
    glGenBuffers(1, &buff_);
  }

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

  BufferObject(BufferObject &&other) : buff_(other.buff_)
  {
    other.buff_ = 0;
  }

  BufferObject &operator=(BufferObject &&other)
  {
    //ALWAYS check for self-assignment
    if(this != &other)
    {
      Release();
      buff_ = other.buff_;
      other.buff_ = 0;
    }

    return *this;
  }

  ~BufferObject() {Release();}

  void Release();
  {
    if(buff_)
      glDeleteBuffers(1, &buff_);
  }

//Other members.
};

There are various other techniques for making move-only RAII wrappers for OpenGL objects.

Broken Vertex Data in Mesh Class

In your Mesh::Setup you have this line at the end:

glBindBuffer(GL_ELEMENT_ARRAY_BUFFER, 0);

This unbinds your EBO from your VAO. Your implementation seems to treat undefined reads as zero, therefore all you see is the 0th vertex replicated six times.

Once you bound the EBO you don't need to unbind it.

(Note: unbinding GL_ARRAY_BUFFER, on the other hand, is OK. This is because the VBO is attached to the VAO at the time you call any of the *Pointer functions, not at the time you bind it to GL_ARRAY_BUFFER.)

(Note: Since you use the latest OpenGL version, I strongly recommend that you use the Direct State Access (DSA) functions. In this case you would bind the EBO with

glVertexArrayElementBuffer(m_VAO, m_EBO);

call, which I think would make the mistake much more obvious. See a quick reference of VAO state and the recommended functions to use to manipulate it.)

c++ & OpenGl: How do you create a mesh object instance from within a class

mesh is a pointer (Mesh* mesh;). To call Draw you must use the arrow notation: mesh->Draw();.

If you don't want to have problems later, you should also check that mesh != nullptr before calling Draw (you can simply log that you are trying to draw an uninitialized room or something like that, much easier to debug than memory corruption or segfaults).

Finally in the destructor you should have a line delete mesh; to release the memory associated and other resources a Mesh instance might be using (Or even better declare mesh using unique_ptr, like this: std::unique_ptr<Mesh>, so that it behaves like a pointer but it will be deleted automatically when you destroy your room instance.

Mesh Class Called with Default Constructor Not Working Opengl C++