Issues trying to add instancing to graphics system

Hello. I am working on a 2D c++ game project. I am trying to improve the graphics system by reducing the number of draw calls. So I basically copied the Sprite class I had, and tweaked it into a new class: ISprite. And while I can create ISprites without any errors, they do not actually draw on the screen. Sprites and debug lines still display normally. So basically I’m trying to find out what is wrong with my instanced sprite system and would appreciate some help.

Here is some relevant code:
Vertex Shader


#version 330 core

layout (location = 0) in vec3 position;
layout (location = 1) in vec2 texCoords;
layout (location = 2) in vec3 colorSource;
layout (location = 3) in mat4 transform;

out vec2 TexCoords;
out vec4 Color;

uniform mat4 uniformView;
uniform mat4 uniformProjection;

void main()
{
  gl_Position = uniformProjection * uniformView * transform * vec4(position, 1.0f);
  TexCoords = texCoords;
  Color = vec4(colorSource, 1.0f);
}

Fragment Shader


#version 330 core
in vec2 TexCoords;
in vec4 Color;

out vec4 color;

uniform sampler2D Texture;
uniform vec4 uniformColor;

void main()
{
    vec4 texColor = texture(Texture, TexCoords) * Color;
    if(texColor.a < 0.1)
        discard;

    color = texColor;

 }

Load, does preparations required for drawing every sprite


void ISprite::Load(Shader spriteShader)
{
  GLfloat vertices[] = {
    //X    Y     Z     
    0.5f, -0.5f, 0.0f,
    -0.5f, 0.5f, 0.0f,
    -0.5f, -0.5f, 0.0f,
    0.5f, -0.5f, 0.0f,
    0.5f, 0.5f, 0.0f,
    -0.5f, 0.5f, 0.0f
  };

  glGenVertexArrays(1, &vertexArray);
  glGenBuffers(1, &positionBuffer);
  glGenBuffers(1, &texCoordsBuffer);
  glGenBuffers(1, &colorBuffer);
  glGenBuffers(1, &matrixBuffer);

  glBindVertexArray(vertexArray);

  //For vertex Position
  glEnableVertexAttribArray(0);
  glBindBuffer(GL_ARRAY_BUFFER, positionBuffer);
  glVertexAttribPointer(0, 3, GL_FLOAT, GL_FALSE, 0, (GLvoid*)0);

  //The vertex data will never change, so send that data now. 
  glBufferData(GL_ARRAY_BUFFER, sizeof(vertices), vertices, GL_STATIC_DRAW);

  //For texture coordinates
  glEnableVertexAttribArray(1);
  glBindBuffer(GL_ARRAY_BUFFER, texCoordsBuffer);
  glVertexAttribPointer(1, 2, GL_FLOAT, GL_FALSE, 0, (GLvoid*)0);

  //For Color
  glEnableVertexAttribArray(2);
  glBindBuffer(GL_ARRAY_BUFFER, colorBuffer);
  glVertexAttribPointer(2, 3, GL_FLOAT, GL_FALSE, 0, (GLvoid*)0);

  //For Transformation Matrix
  glEnableVertexAttribArray(3);
  glBindBuffer(GL_ARRAY_BUFFER, matrixBuffer);
  glVertexAttribPointer(3, 1, GL_MATRIX4_ARB, GL_FALSE, 0, (GLvoid*)0);

  //Unbind
  glBindBuffer(GL_ARRAY_BUFFER, 0);
  glBindVertexArray(0);

  //Sets up support for instancing
  glVertexAttribDivisor(0, 0); 
  glVertexAttribDivisor(1, 1); 
  glVertexAttribDivisor(2, 1);
  glVertexAttribDivisor(3, 1);

  ISprite::shader = &spriteShader;
}

And the two drawing functions:
Prepare Draw


void ISprite::prepareDraw(void)
{
  //Adds their personal data to vectors shared by class 
  transformMatrices.push_back(calculateTransorm());

  for (int i = 0; i < 6; i += 2)
  {
    texture.updateAnimation();
    ISprite::textureCoordinatesAll.push_back(texture.textureCoordinates[i]);
    ISprite::textureCoordinatesAll.push_back(texture.textureCoordinates[i + i]);
  }

  ISprite::colorValues.push_back(color.x);
  ISprite::colorValues.push_back(color.y);
  ISprite::colorValues.push_back(color.z);
}

drawSprites


void ISprite::drawSprites(Texture testTexture)
{
  shader->Use();
  glBindTexture(GL_TEXTURE_2D, testTexture.ID);

  for (std::vector<ISprite*>::iterator it = Isprites.begin(); it != Isprites.end(); ++it)
    (*it)->prepareDraw();

  glBindVertexArray(vertexArray);
  //Bind texture here if you want textures to work. if not, a single texture atlas will be bound
  glBindBuffer(GL_ARRAY_BUFFER, texCoordsBuffer);
  glBufferData(GL_ARRAY_BUFFER, textureCoordinatesAll.size() * sizeof(GLfloat),
                NULL, GL_STREAM_DRAW);  
  glBufferSubData(GL_ARRAY_BUFFER, 0, textureCoordinatesAll.size() * sizeof(GLfloat), 
                textureCoordinatesAll.data());

  glBindBuffer(GL_ARRAY_BUFFER, colorBuffer);
  glBufferData(GL_ARRAY_BUFFER, colorValues.size() * sizeof(GLfloat),
              NULL, GL_STREAM_DRAW);
  glBufferSubData(GL_ARRAY_BUFFER, 0, colorValues.size() * sizeof(GLfloat),
               colorValues.data());

  glBindBuffer(GL_ARRAY_BUFFER, matrixBuffer);
  glBufferData(GL_ARRAY_BUFFER, transformMatrices.size() * sizeof(glm::mat4),
              NULL, GL_STREAM_DRAW);
  glBufferSubData(GL_ARRAY_BUFFER, 0, transformMatrices.size() * sizeof(glm::mat4),
              transformMatrices.data());

  glDrawArraysInstanced(GL_TRIANGLES, 0, 6, Isprites.size());

  textureCoordinatesAll.clear();
  colorValues.clear();
  transformMatrices.clear();
  glBindBuffer(GL_ARRAY_BUFFER, 0);
  glBindVertexArray(0);
}

This is invalid.

Vertex attributes which are matrices are treated as multiple consecutive attributes, one for each column, i.e.:


  glVertexAttribPointer(3+0, 4, GL_FLOAT, GL_FALSE, 4*4*sizeof(GLfloat), (GLvoid*)(0*4*sizeof(GLfloat)));
  glVertexAttribPointer(3+1, 4, GL_FLOAT, GL_FALSE, 4*4*sizeof(GLfloat), (GLvoid*)(1*4*sizeof(GLfloat)));
  glVertexAttribPointer(3+2, 4, GL_FLOAT, GL_FALSE, 4*4*sizeof(GLfloat), (GLvoid*)(2*4*sizeof(GLfloat)));
  glVertexAttribPointer(3+3, 4, GL_FLOAT, GL_FALSE, 4*4*sizeof(GLfloat), (GLvoid*)(3*4*sizeof(GLfloat)));

The GL_MATRIXn_ARB enumerants are a historical relic. They are only used by the ARB_vertex_program and ARB_fragment_program extensions (which aren’t the same thing as OpenGL 2+ shader programs), where they are valid as arguments to glMatrixMode.

[QUOTE=GClements;1279485]This is invalid.

Vertex attributes which are matrices are treated as multiple consecutive attributes, one for each column, i.e.:


  glVertexAttribPointer(3+0, 4, GL_FLOAT, GL_FALSE, 4*4*sizeof(GLfloat), (GLvoid*)(0*4*sizeof(GLfloat)));
  glVertexAttribPointer(3+1, 4, GL_FLOAT, GL_FALSE, 4*4*sizeof(GLfloat), (GLvoid*)(1*4*sizeof(GLfloat)));
  glVertexAttribPointer(3+2, 4, GL_FLOAT, GL_FALSE, 4*4*sizeof(GLfloat), (GLvoid*)(2*4*sizeof(GLfloat)));
  glVertexAttribPointer(3+3, 4, GL_FLOAT, GL_FALSE, 4*4*sizeof(GLfloat), (GLvoid*)(3*4*sizeof(GLfloat)));

The GL_MATRIXn_ARB enumerants are a historical relic. They are only used by the ARB_vertex_program and ARB_fragment_program extensions (which aren’t the same thing as OpenGL 2+ shader programs), where they are valid as arguments to glMatrixMode.[/QUOTE]

Interesting. Does this change the way the shader needs to be written? Do i need to handle a layout 4, 5, and 6 and construct the matrix from it in the shader?
Does the vertexattrib divisor call need to be different?
Do buffer data calls and buffer sub data calls need to be different?

Does this change the way the shader needs to be written?

No. Matrix attributes explicitly take up a number of locations equal to their column count. These locations are allocated sequentially following the assigned location.

Your shader is fine; it’s the code interacting with it that’s wrong.

Does the vertexattrib divisor call need to be different?

Yes. Since you have 4 attribute locations, you need to assign a divisor to each location. Your code is feeding a series of vector attributes to the shader; the shader is simply automatically interpreting the group of vectors as a matrix.

All that being said, really, you shouldn’t bother with instancing for something like this. If you want to reduce the number of draw calls (to the extent that this would be impacting performance), it’s far better to just generate the vertex/color/etc data for your sprites on the CPU, shove it into a buffer, and render it in one draw command.

Instancing has a cost to it. It only tends to be worthwhile if the number of vertices per instance is non-trivially large, and if the instance count is suitably large (thousands).

So you’re suggesting that I keep a similar structure where I put each object’s data into a single container and then send that container? What changes to the system would this entail other than removing the attrib divisor calls and calling drawArrays instead of drawArraysInstanced ?

Removing instancing would move most of work of the vertex shader onto the client side. Rather than sending 4 positions, 1 pairs of texture coordinates, 1 colour and 1 transformation, you’d send 4 (transformed) positions, 4 pairs of texture coordinates and 4 colours.

Personally, I’d probably stick with instancing. Instancing will have a higher GPU load, a non-instanced approach a higher CPU load and higher memory consumption and bandwidth. For a 2D game, I doubt that you will be saturating any of those, so it doesn’t really matter which you pick. Eliminating the instancing will improve portability (instancing requires at least OpenGL 3.1, and 3.3 to be useful), but the instanced approach leads to a cleaner design (the CPU is storing and sending the data in its “natural” format, leaving the decoding to the shaders).

Hello All. I’ve made some changes as per your suggestions. But Am still not getting things to successfully draw. I’ve made some changes to some of the functions, based on the advice I’ve received from here and other sources.

No changes have been made to the shader, because people have been saying that the shader seems fine.

[u]Load[/u]


void ISprite::Load(Shader spriteShader)
{
	spriteShader.Use();

  GLfloat vertices[] = {
    //X    Y     Z     
    0.5f, -0.5f, 0.0f,
    -0.5f, 0.5f, 0.0f,
    -0.5f, -0.5f, 0.0f,
    0.5f, -0.5f, 0.0f,
    0.5f, 0.5f, 0.0f,
    -0.5f, 0.5f, 0.0f
  };

  glGenVertexArrays(1, &vertexArray);
  glGenBuffers(1, &positionBuffer);
  glGenBuffers(1, &texCoordsBuffer);
  glGenBuffers(1, &colorBuffer);
  glGenBuffers(1, &matrixBuffer);

  //The vertex data will never change, so send that data now. 
  glBindBuffer(GL_ARRAY_BUFFER, positionBuffer);
  glBufferData(GL_ARRAY_BUFFER, sizeof(vertices), vertices, GL_STATIC_DRAW);

  //For vertex Position
  glEnableVertexAttribArray(0);
  glVertexAttribPointer(0, 3, GL_FLOAT, GL_FALSE, 3*sizeof(GLfloat), (GLvoid*)0);

  //For texture coordinates
  glBindBuffer(GL_ARRAY_BUFFER, texCoordsBuffer);
  glEnableVertexAttribArray(1);
  glVertexAttribPointer(1, 2, GL_FLOAT, GL_FALSE, 2*sizeof(GLfloat), (GLvoid*)0);

  //For Color
  glBindBuffer(GL_ARRAY_BUFFER, colorBuffer);
  glEnableVertexAttribArray(2);
  glVertexAttribPointer(2, 4, GL_FLOAT, GL_FALSE, 4 * sizeof(GLfloat), (GLvoid*)0);

  //For Transformation Matrix
  glBindBuffer(GL_ARRAY_BUFFER, matrixBuffer);
  for (int i = 0; i < 4; ++i)
  {
	  glEnableVertexAttribArray(3 + i);
	  glVertexAttribPointer(3 + i, 4, GL_FLOAT, GL_FALSE, 
		  4 * 4 * sizeof(GLfloat), (GLvoid*)(4 * i * sizeof(GLfloat)));
  }

  glBindVertexArray(vertexArray);
  glBindBuffer(GL_ARRAY_BUFFER, positionBuffer);
  glBindBuffer(GL_ARRAY_BUFFER, texCoordsBuffer);
  glBindBuffer(GL_ARRAY_BUFFER, colorBuffer);
  glBindBuffer(GL_ARRAY_BUFFER, matrixBuffer);
  glBindVertexArray(0);

  glVertexAttribDivisor(positionBuffer, 0);
  glVertexAttribDivisor(texCoordsBuffer, 1);
  glVertexAttribDivisor(colorBuffer, 1);
  glVertexAttribDivisor(matrixBuffer, 1);
  glVertexAttribDivisor(matrixBuffer + 1, 1);
  glVertexAttribDivisor(matrixBuffer + 2, 1);
  glVertexAttribDivisor(matrixBuffer + 3, 1);

  ISprite::shader = &spriteShader;
}

Prepare Draw


void ISprite::prepareDraw(void)
{
  //Adds their personal data to vectors shared by class 
  glm::mat4 transform = calculateTransorm();

  for (int i = 0; i < 4; ++i)
  {
	  for (int j = 0; j < 4; ++j)
	    ISprite::transformMatrices.push_back(transform[i][j]);
  }

  texture.updateAnimation();
  for (int i = 0; i < 12; ++i)
    ISprite::textureCoordinatesAll.push_back(texture.textureCoordinates[i]);
 
  ISprite::colorValues.push_back(color.x);
  ISprite::colorValues.push_back(color.y);
  ISprite::colorValues.push_back(color.z);
  ISprite::colorValues.push_back(color.w);
}

Draw


void ISprite::drawSprites(Texture testTexture)
{
  shader->Use();

  for (std::vector<ISprite*>::iterator it = Isprites.begin(); it != Isprites.end(); ++it)
    (*it)->prepareDraw();

  glBindVertexArray(vertexArray);
  glBindTexture(GL_TEXTURE_2D, testTexture.ID);

  //Bind texture here if you want textures to work. if not, a single texture atlas will be bound
  glBindBuffer(GL_ARRAY_BUFFER, texCoordsBuffer);
  glBufferData(GL_ARRAY_BUFFER, textureCoordinatesAll.size() * sizeof(GLfloat),
	  textureCoordinatesAll.data(), GL_STREAM_DRAW);

  glBindBuffer(GL_ARRAY_BUFFER, colorBuffer);
  glBufferData(GL_ARRAY_BUFFER, colorValues.size() * sizeof(GLfloat),
	  colorValues.data(), GL_STREAM_DRAW);

  glBindBuffer(GL_ARRAY_BUFFER, matrixBuffer);
  glBufferData(GL_ARRAY_BUFFER, transformMatrices.size() * sizeof(GLfloat),
	  transformMatrices.data(), GL_STREAM_DRAW);

  glDrawArraysInstanced(GL_TRIANGLE_STRIP, 0, 6, Isprites.size());

  textureCoordinatesAll.clear();
  colorValues.clear();
  transformMatrices.clear();
  glBindBuffer(GL_ARRAY_BUFFER, 0);
  glBindTexture(GL_TEXTURE_2D, 0);
  glBindVertexArray(0);
}

At this point, the default VAO is still bound, so that’s where the attribute settings are stored. You need to bind your VAO first.

The above is pointless. The GL_ARRAY_BUFFER binding isn’t stored in a VAO; it’s global state. The buffer for each attribute (i.e. the buffer which is bound to GL_ARRAY_BUFFER at the time of a glVertexAttribPointer() call) is stored in a VAO, as is the GL_ELEMENT_ARRAY_BUFFER binding.

Again, your VAO isn’t bound at this point, so these settings are stored in the default VAO.

You’re calling glDrawArraysInstanced() with your VAO bound, but the attribute settings are all in the default VAO; your VAO is never changed from its initial state.

Ok, so it souds like the main issue is improperly binding the VAO in the load function. Does this load function look more correct? Based on your advice, I changed my function to this. but this still doesn’t draw.

Load


void ISprite::Load(Shader spriteShader)
{
  spriteShader.Use();

  GLfloat vertices[] = {
    //X    Y     Z     
    0.5f, -0.5f, 0.0f,
    -0.5f, 0.5f, 0.0f,
    -0.5f, -0.5f, 0.0f,
    0.5f, -0.5f, 0.0f,
    0.5f, 0.5f, 0.0f,
    -0.5f, 0.5f, 0.0f
  };

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

  glGenBuffers(1, &positionBuffer);
  glGenBuffers(1, &texCoordsBuffer);
  glGenBuffers(1, &colorBuffer);
  glGenBuffers(1, &matrixBuffer);

  //For vertex Position
  glBindBuffer(GL_ARRAY_BUFFER, positionBuffer);
  glEnableVertexAttribArray(0);
  glVertexAttribPointer(0, 3, GL_FLOAT, GL_FALSE, 3 * sizeof(GLfloat), (GLvoid*)0);
  //The vertex data will never change, so send that data now. 
  glBufferData(GL_ARRAY_BUFFER, sizeof(vertices), vertices, GL_STATIC_DRAW);

  //For texture coordinates
  glBindBuffer(GL_ARRAY_BUFFER, texCoordsBuffer);
  glEnableVertexAttribArray(1);
  glVertexAttribPointer(1, 2, GL_FLOAT, GL_FALSE, 2*sizeof(GLfloat), (GLvoid*)0);

  //For Color
  glBindBuffer(GL_ARRAY_BUFFER, colorBuffer);
  glEnableVertexAttribArray(2);
  glVertexAttribPointer(2, 4, GL_FLOAT, GL_FALSE, 4 * sizeof(GLfloat), (GLvoid*)0);

  //For Transformation Matrix
  glBindBuffer(GL_ARRAY_BUFFER, matrixBuffer);
  for (int i = 0; i < 4; ++i)
  {
	  glEnableVertexAttribArray(3 + i);
	  glVertexAttribPointer(3 + i, 4, GL_FLOAT, GL_FALSE, 
		  4 * 4 * sizeof(GLfloat), (GLvoid*)(4 * i * sizeof(GLfloat)));
  }

  glVertexAttribDivisor(0, 0);
  glVertexAttribDivisor(1, 1);
  glVertexAttribDivisor(2, 1);
  glVertexAttribDivisor(3, 1);
  glVertexAttribDivisor(3 + 1, 1);
  glVertexAttribDivisor(3 + 2, 1);
  glVertexAttribDivisor(3 + 3, 1);

  glBindVertexArray(0);

  ISprite::shader = &spriteShader;
}