Return to Answer

I don't usually code with Java, but I often play with OpenGL and shaders, so I'll focus more on that in this review.

• I'm uncertain about this line:

    GLES20.glBufferData(GLES20.GL_ARRAY_BUFFER, count * (Float.SIZE / 8), vertexBatch, GLES20.GL_STATIC_DRAW);
                                                        ^^^^^^^^^^^^^^^^
  

glBufferData takes the size in bytes of the buffer as its second parameter. So logically, shouldn't it be count * Float.SIZE instead? Is count the number of floats in vertexBatch, or something else? You should look into this. Try drawing a single sprite and see if the two triangles draw properly. You might have missed this if you were drawing a lot of sprites. Most of them would probably still show even with a wrong buffer size.

I don't usually code with Java, but I often play with OpenGL and shaders, so I'll focus more on that for this review.

• The magic number 48: If I guess it correctly, this is the number of floats in a single sprite quadrilateral in the vertex batch. So why not name it properly like VERTEXES_IN_A_QUAD. It might also be nice to provide a method to test if the batch is full, so a test would look like: if (vertexBatchIsFull()) { ... }.

• Second issue is the unnecessary nesting of the else part, which is very big. Since the first if is a terminal condition to the method, just return and avoid nesting the rest of the code inside the else:

    if (vertexBatchIsFull())
    {
        Log.e(TAG, "Vertex batch too small !");
        return;
    }
    // The previous code that was inside the 'else' part.
  

    int count = vertexBatch.position();
    if(count <= 0)
    {
        return;
    }

glBufferData takes the size in bytes of the buffer as its second parameter. So logically, shouldn't it be count * Float.SIZE instead? Is count the number of floats in vertexBatch, or something else? You should look into this. Try drawing a single sprite and see is the two triangles draw properly. You might have missed this if you were drawing a lot of sprites. Most of them would probably still show even with a wrong buffer size.

• Prefer qualifying decimal literals with the dot and a zero. Some lame mobile drivers might complain that you are trying to assign an integer constant to a float/vector type. So this line should be:

    gl_Position = un_Transform * vec4(ve_Position.xy, 0.0, 1.0);
    //                                                ^^^~~^^^~~~ Notice the explicit '.0' here
  

• The naming convention you've adopted is unusual. I understand the logic though, ve_ is for Vertex Shader and fr_ is for Fragment Shader. A more usual notation is instead to append a_ for vertex attributes and v_ for varyings, u_ for uniforms and s_ for samplers. But since there is no universal agreement in the matter, this is left to personal choice. I for one have stopped using these prefixes as of late, since I couldn't really see any benefits towards readability, while it adds maintenance overhead.

• The magic number 48: If I guess it correctly, this is the number of floats in a single sprite quadrilateral in the vertex batch. So why not name it properly like NUM_FLOATS_IN_A_SPRITE. It might also be nice to provide a method to test if the batch is full, so a test could instead look like: if (vertexBatchIsFull()) { ... }.

• Second issue is the unnecessary nesting of the else part, which is very big. Since the first if is a terminal condition to the method, just return and avoid nesting the rest of the code inside the else:

    if (vertexBatchIsFull())
    {
        Log.e(TAG, "Vertex batch too small !");
        return;
    }
    // The previous code that was inside the 'else' part.
  

    int count = vertexBatch.position();
    if (count <= 0)
    {
        return;
    }

glBufferData takes the size in bytes of the buffer as its second parameter. So logically, shouldn't it be count * Float.SIZE instead? Is count the number of floats in vertexBatch, or something else? You should look into this. Try drawing a single sprite and see if the two triangles draw properly. You might have missed this if you were drawing a lot of sprites. Most of them would probably still show even with a wrong buffer size.

• Prefer qualifying decimal literals with the dot and zero. Some lame mobile drivers might complain that you are trying to assign an integer constant to a float/vector type. So this line should be:

    gl_Position = un_Transform * vec4(ve_Position.xy, 0.0, 1.0);
    //                                                ^^^~~^^^~~~ Notice the explicit '.0' here
  

• The naming convention you've adopted is unusual. I understand the logic though, ve_ is for Vertex Shader and fr_ is for Fragment Shader. A more usual notation is instead to append a_ for vertex attributes and v_ for varyings, u_ for uniforms and s_ for samplers. But since there is no universal agreement in the matter, this is left to personal choice. I for one have stopped using these prefixes as of late, since I couldn't really see any benefits towards readability, while it adds maintenance overhead.

• I'd suggest using GL_DYNAMIC_DRAW or even GL_STREAM_DRAW as the last parameter of glBufferData. GL_STATIC_DRAW is for buffers that are created once and rarely or never get updated. DYNAMIC is for buffers updated once per frame or less. STREAM is for buffer that are updated more than once per frame. In your case, if you only have a single endTextureRendering() call per frame, then GL_DYNAMIC_DRAW should be the best match.

• In the glVertexAttribPointer calls, you have 8 * (Float.SIZE / 8). That's just the same as Float.SIZE.

• Disabling the attribute indexes after drawing is not strictly necessary, the same way you don't unbind the textures or shader. They will get overwritten the next time you draw something. Those lines could go away:

• Prefer qualifying decimal literals with the dot and a zero. Some lame mobile drivers might complain that you are trying to assign an integer constant to a float/vector type. So this line should be:

    gl_Position = un_Transform * vec4(ve_Position.xy, 0.0, 1.0);
    //                                                ^^^~~^^^~~~ Notice the explicit '.0' here
  

• The naming convention you've adopted is unusual. In understand the logic though, ve_ is for Vertex Shader and fr_ is for Fragment Shader. A more usual notation is instead to append a_ for vertex attributes and v_ for varyings, u_ for uniforms and s_ for samplers. But since there is no universal agreement in the matter, this is left to personal choice. I for one have stopped using these prefixes as of late, since I couldn't really see any benefit towards readability.

• I'd suggest using GL_DYNAMIC_DRAW or even GL_STREAM_DRAW as the last parameter of glBufferData. GL_STATIC_DRAW is for buffers that are created once and rarely or never get updated. DYNAMIC is for buffers updated once per frame or less. STREAM is for buffers that are updated more than once per frame. In your case, if you only have a single endTextureRendering() call per frame, then GL_DYNAMIC_DRAW should be the best match.

• In the glVertexAttribPointer calls, you have 8 * (Float.SIZE / 8). That's just the same as Float.SIZE.

• Disabling the attribute indexes after drawing is not strictly necessary, the same way you don't unbind the textures or shader. They will get overwritten the next time you draw something. Those lines could go away:

• Prefer qualifying decimal literals with the dot and a zero. Some lame mobile drivers might complain that you are trying to assign an integer constant to a float/vector type. So this line should be:

    gl_Position = un_Transform * vec4(ve_Position.xy, 0.0, 1.0);
    //                                                ^^^~~^^^~~~ Notice the explicit '.0' here
  

• The naming convention you've adopted is unusual. I understand the logic though, ve_ is for Vertex Shader and fr_ is for Fragment Shader. A more usual notation is instead to append a_ for vertex attributes and v_ for varyings, u_ for uniforms and s_ for samplers. But since there is no universal agreement in the matter, this is left to personal choice. I for one have stopped using these prefixes as of late, since I couldn't really see any benefits towards readability, while it adds maintenance overhead.