OpenGL game engine

Question

I'm making a C++ game engine for entertainment and am using OpenGL to render my stuff. I made a batch renderer that was able to render 50K sprites with 300 FPS (untextured). My old setup that was able to run with 300 FPS was the following:

I had a class called Renderable2D, which held everything:

class Renderable2D
{
protected:
    vec3f m_Position;
    vec2f m_Size;
    vec4f m_Color;
    const Texture2D * m_Texture;
    std::vector<vec2f> m_TexCoords;

protected:
    Renderable2D() { }

public:
    Renderable2D(vec3f position, vec2f size, vec4f color)
        : m_Position(position), m_Size(size), m_Color(color), m_Texture(nullptr)
    {
        m_TexCoords.push_back(vec2f(0, 0));
        m_TexCoords.push_back(vec2f(0, 1));
        m_TexCoords.push_back(vec2f(1, 1));
        m_TexCoords.push_back(vec2f(1, 0));
    }

    virtual ~Renderable2D()
    { }

    inline virtual void Submit(Renderer2D * renderer) const
    {
        renderer->Submit(*this);
    }

    inline const vec3f& GetPosition() const { return m_Position; }
    inline const vec2f& GetSize() const { return m_Size; }
    inline const vec4f& GetColor() const { return m_Color; }
    inline const std::vector<vec2f>& GetTexCoords() const { return m_TexCoords; }

    inline const GLuint GetTextureID() const { return (m_Texture == nullptr ? 0 : m_Texture->GetTextureID()); }
};

Basically, this was a class that held everything: the texture coordinates (if it had any), the color, position and size. My batch renderer had a method to draw any Renderable2D based on this class:

void BatchRenderer::Submit(const Renderable2D& renderable)
{
    const vec3f& position = renderable.GetPosition();
    const vec2f& size = renderable.GetSize();
    const unsigned int color = renderable.GetColor();
    const std::vector<vec2f>& texCoords = renderable.GetTexCoords();
    const GLuint tid = renderable.GetTextureID();

    float ts = 0.0f;
    if (tid > 0)
    {
        bool found = false;
        for (unsigned int i = 0; i < m_TextureSlots.size(); i++)
        {
            if (m_TextureSlots[i] == tid)
            {
                ts = (float)(i + 1);
                found = true;
                break;
            }
        }
        if (!found)
        {
            if (m_TextureSlots.size() >= 32)
            {
                End();
                Flush();
                Begin();
            }

        m_TextureSlots.push_back(tid);
        ts = (float)(m_TextureSlots.size());
        }
    }

    Maths::vec3f _tpos = *m_TransformationBack * position;

    m_Buffer->Position = _tpos;
    m_Buffer->TexCoord = texCoords[0];
    m_Buffer->TexID = ts;
    m_Buffer->Color = color;
    m_Buffer++;

    _tpos.y += size.y;

    m_Buffer->Position = _tpos;
    m_Buffer->TexCoord = texCoords[1];
    m_Buffer->TexID = ts;
    m_Buffer->Color = color;
    m_Buffer++;

    _tpos.x += size.x;

    m_Buffer->Position = _tpos;
    m_Buffer->TexCoord = texCoords[2];
    m_Buffer->TexID = ts;
    m_Buffer->Color = color;
    m_Buffer++;

    _tpos.y -= size.y;

    m_Buffer->Position = _tpos;
    m_Buffer->TexCoord = texCoords[3];
    m_Buffer->TexID = ts;
    m_Buffer->Color = color;
    m_Buffer++;

    m_IndexCount += 6;
}

I wanted to make this a bit nicer and faster, so I created subclasses for renderable2D. Renderable2D only had it's position and size. Rectangle (inherited from Renderable2D) had color too, and Sprite had a texture and texture coords. I think this is a bit more categorized and makes a simple colored rectangle less heavily-weighted (no texture coords). Then I overrode the Renderable2D's submit method for both the rectangle and the sprite and created (which I thought were) optimized submit methods for my renderer. I've kept the original one, but overloaded the method, so I had:

void BatchRenderer::Submit(const vec2f& position, const vec2f& size, unsinged int color);
void BatchRenderer::Submit(const vec2f& position, const vec2f& size, GLuint textureID, const std::vector<vec2f>& textureCoords);

This way I didn't need to assign everything for m_Buffer and I could eliminate the if statement (if (tid > 0)) because now I knew when the renderable used a texture. I thought this would speed up the code, but it slowed it down to about half its speed (testing conditions were the same for the unoptimized and "optimized" code). Why did this refactor (which I think should speed up the code) slow it down? Less assignments, one less if statement, smaller data structures.

• m_Buffer: It's actually a vertex buffer object, it's named simply buffer because it's not only storing vertices, but colors, texture coordinates and texture IDs too. The Submit method basically writes mapped data to this buffer.

• ts: It stands for texture slot, yes it's a really bad naming, I should rename it. It's a float because OpenGL actually likes float more in a float buffer than an integer.

• m_IndexCount: The size of the index buffer. (Index of vertices)

• Renderable2D: I have a Submit method that just passes itself to the renderer. It's virtual on purpose. You don't send sprites directly to the renderer, because there are Rendereable2Ds called Groups. They override the Submit method and sends their children instead.

• m_Texture: Renderable2D sets it as nullptr for now. There's a subclass called Sprite that's basically a constructor which sets the texture to the passed parameter. And my plan is to clean up Renderable2D from the things it doesn't need like texture, because sprite needs it really.

Answer 1

A couple things I would have done differently, plus some other broader advices:

• Renderable2D seems to me is representing a single sprite, each sprite being a quadrilateral. In that case, you really don't need a variable amount of texture coordinates, but rather just 4. No need to use a std::vector in this case and force a heap allocation. Just use a plain array of vec2f or a std::array if you are open to using C++11.

• You don't need to add inline when the method is defined directly inside the class body in the header file. That serves no practical purpose and only makes the code more verbose.

• Always favor the C++-style casts. static_cast and friends will provide better compiler diagnostics if you attempt some unsafe cast.

Two bits of personal style:

• It is a common convention to name user defined types using PascalCase, like you already do, and variables and functions with camelCase, to differentiate both kinds of things. Actually, the idea is to name anything that can have its memory address take with camelCase and reserve the first letter capitalized for types.

• I find it better to place the public interface of a class first in the header file, to give it more emphasis. That's usually the part you and users of your code will be looking at more often. protected and private are implementation details, so shouldn't need to stand out as much.

Answer 2

I concur with @glampert's suggestions. I have a few questions of my own.

Naming

What is the purpose of the Submit() method? What is it submitting? (I don't see any calls to OpenGL to submit vertex attribute data, for example.) Also, it appears to update the value of an internal pointer, m_Buffer. What is it a buffer of? Its name should reflect what it represents. Is it vertex data? If so, name it m_VertexBuffer or m_SpriteBuffer or whatever would be appropriate.

Types

In the Submit() method what is the purpose of ts? It's a float but you eventually assign mBuffer->TexId to the value of ts. But texture IDs are GLuints. Also, it appears to be an index in the array of texture slots, and not an actual texture ID as OpenGL knows it. If it's not actually the ID returned by glGenTextures() or some equivalent, I would not name it TexID. Maybe TextureIndex instead? And it's not likely a float, so make ts be the correct type, whether it's GLuint or just an unsigned int.

Repetition

You have almost identical code written 4 times:

m_Buffer->Position = _tpos;
m_Buffer->TexCoord = texCoords[0];
m_Buffer->TexID = ts;
m_Buffer->Color = color;
m_Buffer++;

Why not make that into a function and call it 4 times with the appropriate arguments instead of writing it out by hand 4 times?

Miscellaneous

What does the last line of Submit() do?

m_IndexCount += 6;

The code above it appears to be adding 4 entries to the m_Buffers list. Why does the index count increase by 6? What is its purpose?

In Renderable2D, you have a Submit() method that you pass a Renderer2D to. It simply calls renderer->Submit(*this);. That seems like a pointless method. Why doesn't the caller just call renderer->Submit(*renderable); and save 1 step?

Memory

How does one set Renderable2D::m_Texture? I see how to get it, but it doesn't get set in the constructor, it's protected, and there's no accessor to set it. (It's also never freed, which might be OK if another object owns it. If so, you might consider using a std::shared_ptr if you're in C++11.)