Many quads, not plenty fps despite VBO

Question

I got some time and I've read about VBO and that's what I got :

http://img64.imageshack.us/img64/5733/fps8.jpg

Ok, it's much better than before. It's compiled on Release. I uses VBO (probably, if everthing's OK) and glDrawArrays to draw.

Here's drawing code. Please give me advice how to optimize it. I wanted with terrain ... uh few thousand FPS, is it real ?

void DrawVBO (void)
{
    int i;

    CVert*      m_pVertices; 
    CTexCoord*  m_pTexCoords;

    unsigned int    m_nTextureId;

    m_pVertices = NULL;
    m_pTexCoords = NULL;
    m_nVertexCount = 0;
    m_nVBOVertices = m_nVBOTexCoords = m_nTextureId = 0;


    if( IsExtensionSupported( "GL_ARB_vertex_buffer_object" ) )
    {
        // Pobierz wskaźniki na funkcje OpenGL 
        glGenBuffersARB = (PFNGLGENBUFFERSARBPROC) wglGetProcAddress("glGenBuffersARB");
        glBindBufferARB = (PFNGLBINDBUFFERARBPROC) wglGetProcAddress("glBindBufferARB");
        glBufferDataARB = (PFNGLBUFFERDATAARBPROC) wglGetProcAddress("glBufferDataARB");
        glDeleteBuffersARB = (PFNGLDELETEBUFFERSARBPROC) 
        wglGetProcAddress("glDeleteBuffersARB"); 
    }

    todrawquads=0;
    nIndex=0;


// my function counting how many quads I will draw


    for (i=0;i<MAX_CHUNKS_LOADED;i++)
    {
        if (chunks_loaded[i].created==1)
        {
            countquads(i);
        }
    }

    m_nVertexCount=4*todrawquads;

    m_pVertices = new CVec[m_nVertexCount];
    m_pTexCoords = new CTexCoord[m_nVertexCount];

// another my function adding every quad which i'm going to draw (its verticles) to array

    for (i=0;i<MAX_CHUNKS_LOADED;i++)
    {
        if (chunks_loaded[i].created==1)
        {
            addchunktodraw(i,m_pVertices,m_pTexCoords);
        }
    }


    glClearColor (1,1,1, 0.0);
    glColor3f(1,1,1);

    glClear (GL_COLOR_BUFFER_BIT | GL_DEPTH_BUFFER_BIT);        // Clear Screen And Depth Buffer
    glLoadIdentity ();                                          // Reset The Modelview Matrix

    fps++;

    // Camera settings.

    //gluLookAt (zom, zom, zom, 0.0, 0.0, 0.0, 0, 0, 1);

    gluLookAt (zoom, zoom, zoom, 0.0, 0.0, 0.0, 0, 0, 1);
    glRotatef((rot_x / 180 * 3.141592654f),1,0,0);  
    glRotatef((rot_y / 180 * 3.141592654f),0,1,0);
    glRotatef((rot_z / 180 * 3.141592654f),0,0,1);

    //m_nTextureId = t_terrain;

    // Generate And Bind The Vertex Buffer
    glGenBuffersARB( 1, &m_nVBOVertices );                          // Get A Valid Name
    glBindBufferARB( GL_ARRAY_BUFFER_ARB, m_nVBOVertices );         // Bind The Buffer
    // Load The Data
    glBufferDataARB( GL_ARRAY_BUFFER_ARB, m_nVertexCount*3*sizeof(float), m_pVertices, GL_STATIC_DRAW_ARB );

    // Generate And Bind The Texture Coordinate Buffer
    glGenBuffersARB( 1, &m_nVBOTexCoords );                         // Get A Valid Name
    glBindBufferARB( GL_ARRAY_BUFFER_ARB, m_nVBOTexCoords );        // Bind The Buffer
    // Load The Data
    glBufferDataARB( GL_ARRAY_BUFFER_ARB, m_nVertexCount*2*sizeof(float), m_pTexCoords, GL_STATIC_DRAW_ARB );

    // Enable Pointers
    glEnableClientState( GL_VERTEX_ARRAY );                     // Enable Vertex Arrays
    glEnableClientState( GL_TEXTURE_COORD_ARRAY );              // Enable Texture Coord Arrays

    glBindBufferARB( GL_ARRAY_BUFFER_ARB, m_nVBOVertices );
    glVertexPointer( 3, GL_FLOAT, 0, (char *) NULL );       // Set The Vertex Pointer To The Vertex Buffer
    glBindBufferARB( GL_ARRAY_BUFFER_ARB, m_nVBOTexCoords );
    glTexCoordPointer( 2, GL_FLOAT, 0, (char *) NULL );     // Set The TexCoord Pointer To The TexCoord Buffe

    glDrawArrays( GL_QUADS, 0, m_nVertexCount); // Draw All Of The Triangles At Once

    glDisableClientState( GL_VERTEX_ARRAY );                    // Disable Vertex Arrays
    glDisableClientState( GL_TEXTURE_COORD_ARRAY );             // Disable Texture Coord Arrays

    liniergb();

    glutSwapBuffers();

    delete [] m_pVertices; m_pVertices = NULL;
    delete [] m_pTexCoords; m_pTexCoords = NULL;
}

So what I can do with it ? (Code above is main draw function)

edit

I've moved this :

if( IsExtensionSupported( "GL_ARB_vertex_buffer_object" ) )
{
    // Pobierz wskaźniki na funkcje OpenGL 
    glGenBuffersARB = (PFNGLGENBUFFERSARBPROC) wglGetProcAddress("glGenBuffersARB");
    glBindBufferARB = (PFNGLBINDBUFFERARBPROC) wglGetProcAddress("glBindBufferARB");
    glBufferDataARB = (PFNGLBUFFERDATAARBPROC) wglGetProcAddress("glBufferDataARB");
    glDeleteBuffersARB = (PFNGLDELETEBUFFERSARBPROC) 
    wglGetProcAddress("glDeleteBuffersARB"); 
}

to my main function. Probably no improvement.

edit2

Now , also I can see that it's eating memory. Every few seconds program memory usage is rising up more and more ... What's wrong ? What I'm not deleting ?

edit3

Ok , thanks sooooo much. I've moved some code outside draw function and ... much more fps ! Thanks so much !

http://img197.imageshack.us/img197/5193/fpsfinal.jpg

It's 640x640 blocks (so 40 times bigger) map with 650'000 quads (about 70 times more) and still ~170 fps. Great ! And no memory leaks. Thanks again !

Answer 1

Your DrawVBO function should contain only:

glClearColor (1,1,1, 0.0);
glColor3f(1,1,1);

glClear (GL_COLOR_BUFFER_BIT | GL_DEPTH_BUFFER_BIT);        // Clear Screen And Depth Buffer
glLoadIdentity ();                                          // Reset The Modelview Matrix

fps++;

// Camera settings.

//gluLookAt (zom, zom, zom, 0.0, 0.0, 0.0, 0, 0, 1);

gluLookAt (zoom, zoom, zoom, 0.0, 0.0, 0.0, 0, 0, 1);
glRotatef((rot_x / 180 * 3.141592654f),1,0,0);  
glRotatef((rot_y / 180 * 3.141592654f),0,1,0);
glRotatef((rot_z / 180 * 3.141592654f),0,0,1);

// Enable Pointers
glEnableClientState( GL_VERTEX_ARRAY );                     // Enable Vertex Arrays
glEnableClientState( GL_TEXTURE_COORD_ARRAY );              // Enable Texture Coord Arrays

glBindBufferARB( GL_ARRAY_BUFFER_ARB, m_nVBOVertices );
glVertexPointer( 3, GL_FLOAT, 0, (char *) NULL );       // Set The Vertex Pointer To The Vertex Buffer
glBindBufferARB( GL_ARRAY_BUFFER_ARB, m_nVBOTexCoords );
glTexCoordPointer( 2, GL_FLOAT, 0, (char *) NULL );     // Set The TexCoord Pointer To The TexCoord Buffe

glDrawArrays( GL_QUADS, 0, m_nVertexCount); // Draw All Of The Triangles At Once

glDisableClientState( GL_VERTEX_ARRAY );                    // Disable Vertex Arrays
glDisableClientState( GL_TEXTURE_COORD_ARRAY );             // Disable Texture Coord Arrays

liniergb();

glutSwapBuffers();

You need to move the rest to separate function called only once at start-up (or when the terrain changes).

Answer 2

You're reloading all your buffers and freeing them again in every single frame? Stop doing that and your frame rate will go up.

Note that your current code will eventually run out of VBO identifiers, since you never delete the VBOs you create.

Linking extension functions definitely doesn't need to be done every frame either.

Answer 3

A few things stand out:

In your drawing function you're allocating memory for your geometry data

    m_pVertices = new CVec[m_nVertexCount];
    m_pTexCoords = new CTexCoord[m_nVertexCount];

Memory allocation is a extreme expensive operation, this is one of those things that should be done only once. OpenGL is not meant to be "initialized" – but the data structures you're going to pass to it are!

Here you're copying the newly allocated buffers to OpenGL, again and again with each frame. This is exactly the opposite of what to do.

// Generate And Bind The Vertex Buffer
glGenBuffersARB( 1, &m_nVBOVertices );                          // Get A Valid Name
glBindBufferARB( GL_ARRAY_BUFFER_ARB, m_nVBOVertices );         // Bind The Buffer
// Load The Data
glBufferDataARB( GL_ARRAY_BUFFER_ARB, m_nVertexCount*3*sizeof(float), m_pVertices, GL_STATIC_DRAW_ARB );

// Generate And Bind The Texture Coordinate Buffer
glGenBuffersARB( 1, &m_nVBOTexCoords );                         // Get A Valid Name
glBindBufferARB( GL_ARRAY_BUFFER_ARB, m_nVBOTexCoords );        // Bind The Buffer
// Load The Data
glBufferDataARB( GL_ARRAY_BUFFER_ARB, m_nVertexCount*2*sizeof(float), m_pTexCoords, GL_STATIC_DRAW_ARB );

The whole idea of VBOs is, to load the data only once, copy it to OpenGL and then never reallocate it again. Note that this is not OpenGL initialization, it's data initialization, something totally reasonable. I see that you have named your variables m_pVertices and m_pTexCoords indicating that those are class member variables. Then the solution is simple: Move that whole initialization code into some loader function. Also instead of naked C++ arrays I strongly suggest using std::vector.

So let's fix this:

// Load Extensions only once. Well, once per context actually, however
// why don't you just use an extension wrapper and forget about those
// gritty details? Google GLEW or GLee
void init_extensions()
{
    if( IsExtensionSupported( "GL_ARB_vertex_buffer_object" ) )
    {
    // Pobierz wska\u017aniki na funkcje OpenGL 
    glGenBuffersARB = (PFNGLGENBUFFERSARBPROC) wglGetProcAddress("glGenBuffersARB");
    glBindBufferARB = (PFNGLBINDBUFFERARBPROC) wglGetProcAddress("glBindBufferARB");
    glBufferDataARB = (PFNGLBUFFERDATAARBPROC) wglGetProcAddress("glBufferDataARB");
    glDeleteBuffersARB = (PFNGLDELETEBUFFERSARBPROC) wglGetProcAddress("glDeleteBuffersARB"); 
    }
}

class Block0r
{
protected:
    GLuint m_nTextureId;
    GLuint m_nVBOVertices;
    GLuint m_nVBOTexCoords;
    GLuint m_nVertexCount;

    // Call this one time to load the data    
    void LoadVBO()
    {
        std::vector<CVert> vertices;
        std::vector<CTexCoord> texCoords;
        // my function counting how many quads I will draw

        todrawquads = 0;

        for(int i=0; i < MAX_CHUNKS_LOADED; i++) {
            if( chunks_loaded[i].created == 1 ) {
                countquads(i);
            }
        }

        m_nVertexCount = 4*todrawquads;

        vertices.resize(vertexcount);
        texcoords.resize(vertexcount);


        for (i=0;i<MAX_CHUNKS_LOADED;i++) {
            if (chunks_loaded[i].created==1) {
                addchunktodraw(i, &vertices[0], &texcoords[0]);
            }
        }

        glGenBuffersARB( 1, &m_nVBOVertices );
            glBindBufferARB( GL_ARRAY_BUFFER_ARB, m_nVBOVertices );
            glBufferDataARB( GL_ARRAY_BUFFER_ARB, vertices.size()*sizeof(CVert), &vertices[0], GL_STATIC_DRAW_ARB );

        glGenBuffersARB( 1, &m_nVBOTexCoords );
            glBindBufferARB( GL_ARRAY_BUFFER_ARB, m_nVBOTexCoords );
        glBufferDataARB( GL_ARRAY_BUFFER_ARB, texCoords.size()*sizeof(CTexCoord), &texCoords[0], GL_STATIC_DRAW_ARB );
    }

    void DrawVBO()
    {
        glClearColor (1,1,1, 0.0);
        glColor3f(1,1,1);

        glClear(GL_COLOR_BUFFER_BIT | GL_DEPTH_BUFFER_BIT);

        glMatrixMode(GL_PROJECTION);
            setup_projection(); // this should really be done in the drawing handler

        glMatrixMode(GL_MODELVIEW); // don't asssume a certain matrix being active!
        glLoadIdentity();

        fps++;

        gluLookAt (zoom, zoom, zoom, 0.0, 0.0, 0.0, 0, 0, 1);
        glRotatef((rot_x / 180 * 3.141592654f),1,0,0);  
        glRotatef((rot_y / 180 * 3.141592654f),0,1,0);
        glRotatef((rot_z / 180 * 3.141592654f),0,0,1);

        glEnableClientState( GL_VERTEX_ARRAY );
        glEnableClientState( GL_TEXTURE_COORD_ARRAY );

        glBindBufferARB( GL_ARRAY_BUFFER_ARB, m_nVBOVertices );
        glVertexPointer( 3, GL_FLOAT, 0, (GLvoid *) 0 );

        glBindBufferARB( GL_ARRAY_BUFFER_ARB, m_nVBOTexCoords );
        glTexCoordPointer( 2, GL_FLOAT, 0, (GLvoid *) 0 );

        glDrawArrays( GL_QUADS, 0, m_nVertexCount);
        glDisableClientState( GL_VERTEX_ARRAY );
        glDisableClientState( GL_TEXTURE_COORD_ARRAY );
        liniergb();

        glutSwapBuffers();
    }
}

On a side note: Comments like // Generate And Bind The Vertex Buffer or // Set The Vertex Pointer To The Vertex Buffer are not very usefull. Those comments just redundantly tell, what one can read from the code anyway. Comments should be added to code, which inner workings are not immediately understandably, or if you had to do some kind of hack to fix something and this hack would puzzle someone else, or yourself in a few months time.