Draw multiple objects

Question

The OpenGL tutorial that I was following only used a single model to demonstrate various points. I wanted to draw more than one model. After some research, I came to the conclusion that I needed to create a vertex array object, once for each model, and than I can use it to access buffers and attributes for each model. In order to do that, I am using this game object struct.

(I'm not including the whole code because it is quite long.)

typedef struct GameObject {
 GLuint VertexArray;
 GLuint NumVertices;
 kmMat4 ModelMatrix;
} GameObject;

For each object I want to draw, I am using loader function, such as the following:

#include "GameObject.h"
static const GLfloat triangle_vertices[] = {
 0, 1, 0,
 -1, -1, 0,
 1, -1, 0
};
static const GLfloat triangle_colors[] = {
 0.2, 0.2, 0.2,
 0.5, 0.5, 0.5,
 0.8, 0.8, 0.8
};
GameObject *newTriangle()
{
 GameObject *triangle = malloc(sizeof(GameObject));
 GLuint VertexBuffer;
 GLuint ColorBuffer;
 // 1. Generate VertexArray for object
 glGenVertexArrays(1, &(triangle->VertexArray));
 // Bind it
 glBindVertexArray(triangle->VertexArray);
 // Create and fill buffers vertex and color buffers
 glGenBuffers(1, &VertexBuffer);
 glBindBuffer(GL_ARRAY_BUFFER, VertexBuffer);
 glBufferData(GL_ARRAY_BUFFER, sizeof(triangle_vertices), triangle_vertices, GL_STATIC_DRAW);
 glEnableVertexAttribArray(0);
 glVertexAttribPointer(
 0, 
 3, 
 GL_FLOAT, 
 GL_FALSE, 
 0, 
 (void*)0
 );
 glGenBuffers(1, &ColorBuffer);
 glBindBuffer(GL_ARRAY_BUFFER, ColorBuffer);
 glBufferData(GL_ARRAY_BUFFER, sizeof(triangle_colors), triangle_colors, GL_STATIC_DRAW);
 glEnableVertexAttribArray(1);
 glVertexAttribPointer(
 1, 
 3, 
 GL_FLOAT, 
 GL_FALSE, 
 0, 
 (void*)0
 );
 // unbind vertex array
 glBindVertexArray(0);
 triangle->NumVertices = (sizeof(triangle_vertices)/sizeof(GLfloat));
 kmMat4Identity(&(triangle->ModelMatrix));
 return triangle;
}

And I am using this function to draw each game object:

#include "GameObject.h"
void Draw(GameObject *go, kmMat4 *VP, GLuint MatrixID)
{
 kmMat4 MVP;
 kmMat4Multiply(&MVP,VP,&go->ModelMatrix);
 // Bind vertex array
 glBindVertexArray(go->VertexArray);
 // set mvp matrix
 glUniformMatrix4fv(MatrixID, 1, GL_FALSE, MVP.mat);
 // Draw object
 glDrawArrays(GL_TRIANGLES, 0, go->NumVertices);
 // Unbind VertexArray
 glBindVertexArray(0);
}

And this is my main loop:

while (!glfwWindowShouldClose(window))
{
 kmMat4 current_translation;
 GLfloat translatex = 0;
 GLfloat translatez = 0;
 double current_time = glfwGetTime();
 double delta_time = current_time - last_time;
 last_time = current_time;
 if (glfwGetKey( window, GLFW_KEY_RIGHT ) == GLFW_PRESS){
 translatex += 2 * delta_time;
 }
 if (glfwGetKey( window, GLFW_KEY_LEFT ) == GLFW_PRESS){
 translatex -= 2 * delta_time;
 }
 if (glfwGetKey( window, GLFW_KEY_UP ) == GLFW_PRESS){
 translatez -= 2 * delta_time;
 }
 if (glfwGetKey( window, GLFW_KEY_DOWN ) == GLFW_PRESS){
 translatez += 2 * delta_time;
 }
 kmMat4Translation(&current_translation, translatex, 0, translatez);
 kmMat4Multiply(&square->ModelMatrix, &current_translation, &square->ModelMatrix);
 glClear(GL_COLOR_BUFFER_BIT | GL_DEPTH_BUFFER_BIT);
 Draw(triangle, &vp, MatrixID);
 Draw(square, &vp, MatrixID);
 glfwSwapBuffers(window);
 glfwPollEvents();
}

Answer 1

You are leaking the Vertex Buffer handles!

In your create function you allocated two Vertex Buffer Objects (VBOs), one for colors and one for positions, but you never store them anywhere, so your code will leak that memory, since those handles are no longer accessible after newTriangle returns. You should augment your GameObject structure to save those buffer handles and free them at a later time (with glDeleteBuffers). Since there's an allocation function, there should also be some sort of cleanup function in your code (e.g.: some freeGameObject or similar).

One VBO for just one triangle?

This is very inefficient. You should try to group stuff into the same VBO. Normally, you would put an entire 3D model into a buffer. You can very easily write a generic helper function that wraps the OpenGL calls for you. I'd also suggest declaring a helper structure that defines an interleaved vertex (more on this is a moment), so you can make efficient use of graphics memory by packing stuff into as little buffers as possible. Example:

typedef struct 
{
 float px, py, pz; // vertex position
 float nx, ny, nz; // vertex normal for lighting
 float r, g, b; // RGB vertex color
 float u, v; // texture coordinates
 // anything else you may need
} my_gl_vertex;
// Then a helper to allocate the GL buffers for you, 
// taking your generic vertex type:
void create_gl_buffers(const my_gl_vertex * verts_in, int vert_count, 
 GLuint * vao_out, GLuint * vbo_out);

So, what was that about interleaved vertexes again?

Just a fancy name to say that we are packing all the attributes of a model vertex into a structure, thus, we only allocate one VBO and place everything in it, rather than allocating one VBO for positions, one for colors, one for normals, etc. You can find examples of how to set this up in here. You should care about using interleaved data because it performs much better in the rendering, since all relevant data for a vertex will be tightly packed in memory, rather than scattered across different buffers. This will also allocate a single buffer, which might also reduce allocation overhead in the graphics driver side.

Move to indexed drawing

Last basic improvement you can add is to support indexed drawing, or indexed vertex specification. If you try, for instance, to draw a rectangle using two triangles, you'll see that you have to repeat two vertexes. This repetition can add up for larger and more complex 3D models, so if you instead supply a set of unique vertexes and use a separate set of integer indexes into this set of verts to describe the model, you get rid of all vertex duplication! The indexes will repeat, but they consist of small integer numbers, which should be a lot smaller than the average 3D vertex with color, position, whatnot. That's indexed drawing in a nutshell. You can check this tutorial for more details, or just look around in the web and you should find several more.

Answer 2

In addition to what @glampert said, I have one more suggestion:

Use a structure

You should make structures for data that has a structure. For example, this structure shouldn't be an array of floats:

static const GLfloat triangle_vertices[] = {
 0, 1, 0,
 -1, -1, 0,
 1, -1, 0
};

Your vertices are a structure. They can be 2, 3, or 4 component vectors. You should define a structure for your vertices (and colors) that looks something like this:

typedef struct vec3 {
 GLfloat x;
 GLfloat y;
 GLfloat z;
} vec3;

Otherwise your code becomes very hard to read and much harder to maintain. You can still initialize it inline via:

static const vec3 triangle_vertices[] = {
 { 0, 1, 0 },
 { -1, -1, 0 },
 { 1, -1, 0 }
};

And if you want to do something in a loop, you can write something like:

for (int i = 0; i < MAX_VERTICES; i++)
{
 triangle_vertices [ i ].x = <whatever>;
 triangle_vertices [ i ].y = <whatever>;
 triangle_vertices [ i ].z = <whatever>;
}

instead of this other common technique:

for (int i = 0; i < MAX_VERTICES; i++)
{
 triangle_vertices [ i * 3 ] = <whatever>;
 triangle_vertices [ i * 3 + 1 ] = <whatever>;
 triangle_vertices [ i * 3 + 2 ] = <whatever>;
}