a simple sprite batch class feedback needed

[leftnoodle]

This is my first time using SFML and C++ so would really appreciate any feedback. Just want to make sure I am not heading down the wrong path..

For my sprite batch I decided I wanted to keep it as simple as possible, so all draw calls will reference the texture that was passed into sprite batch begin. Currently there is only one draw method, it draws the full texture. I will be adding more to handle sub frames, scaling, rotations etc.

The alternative would have been to identify when a different texture was passed to a draw call and flush the batch but this is added complexity and I didn't feel I needed it. I know what sprites will be crammed into my atlases and with some care, I can select the appropriate texture.

Here is my sprite batch class:

class SpriteBatch : public sf::Drawable, public sf::Transformable
{
public:

        SpriteBatch(size_t batchSize = 1024) : m_vertices(new sf::Vertex[batchSize * 4])
        {
                m_vertexCount = 0;
                m_vertexLimit = batchSize * 4;
                m_active = false;
        }

        void begin(sf::RenderWindow& target, sf::Texture& texture)
        {
                m_target = ⌖
                m_texture = texture;
                m_vertexCount = 0;
                m_active = true;
        }

        void draw(float x, float y)
        {
                if (!m_active)
                {
                        printf("ERROR: SpriteBatch - draw() called on inactive batch!");
                        return;
                }

                if (m_vertexCount >= m_vertexLimit)
                        flush();

                sf::Vector2u size = m_texture.getSize();
                float minX = x;
                float minY = y;
                float maxX = x + float(size.x);
                float maxY = y + float(size.y);

                push_vertex(minX, minY, 0.0f, 0.0f, sf::Color::White);
                push_vertex(maxX, minY, float(size.x), 0.0f, sf::Color::White);
                push_vertex(maxX, maxY, float(size.x), float(size.y), sf::Color::White);
                push_vertex(minX, maxY, 0.0f, float(size.y), sf::Color::White);
        }

        void end()
        {
                if (!m_active)
                {
                        printf("ERROR: SpriteBatch - end() called on inactive batch!");
                        return;
                }

                if (m_vertexCount > 0)
                        flush();

                m_active = false;
        }

private:

        void push_vertex(float x, float y, float u, float v, sf::Color colour)
        {
                sf::Vertex* vertex = &m_vertices[m_vertexCount];
                ++m_vertexCount;

                vertex->position.x = x;
                vertex->position.y = y;
                vertex->texCoords.x = u;
                vertex->texCoords.y = v;
                vertex->color = colour;
        }

        void flush()
        {
                (*m_target).draw(*this);
                m_vertexCount = 0;
        }

        virtual void draw(sf::RenderTarget& target, sf::RenderStates states) const
        {
                states.transform *= getTransform();
                states.texture = &m_texture;

                target.draw(&m_vertices[0], m_vertexCount, sf::PrimitiveType::Quads, states);
        }

        sf::RenderTarget* m_target;
        sf::Texture m_texture;

        std::unique_ptr<sf::Vertex[]> m_vertices;
        size_t m_vertexCount;
        size_t m_vertexLimit;

        bool m_active;
};
 

And then to use it:

batch.begin(window, atlas);

batch.draw(32, 32);
batch.draw(48, 16);

batch.end();
 

Thanks for any advice and guidance! Also if the code isn't terrible, feel free to use it

[eXpl0it3r]

I recommend to use constructor and destructor (if necessary) to handle initialization and destruction. That way it's enforced by the compiler that your object can only be used when it's fully initialized and you don't need runtme m_active checks.

I recommend using C++ style casts with static_cast<float>(var) and if it's about a whole sf::Vector2<T> type conversion, you could also call the constructor that allows explicit conversions sf::Vector2f(size).

For a sprite batching, I'd assume, that I could individually position sprites with different texture coordinates, so the  x, y parameter for draw() seems a bit limited.

[leftnoodle]

I recommend to use constructor and destructor (if necessary) to handle initialization and destruction. That way it's enforced by the compiler that your object can only be used when it's fully initialized and you don't need runtme m_active checks.

I have decided to remove the .Begin and .End methods, I was copying an API and the more I thought about it, the less I liked that approach. The batch now builds up render op lists, detecting changes that would require a new batch to be started and instead pushes a new list. Nothing gets drawn until flush is called, all lists are then drawn in order.

I recommend using C++ style casts with static_cast<float>(var) and if it's about a whole sf::Vector2<T> type conversion, you could also call the constructor that allows explicit conversions sf::Vector2f(size).

Thank you, I've made the changes you suggested.

For a sprite batching, I'd assume, that I could individually position sprites with different texture coordinates, so the  x, y parameter for draw() seems a bit limited.

Yes that draw method is very limited, only allowing you to draw the full texture at x,y position. That was purely for testing and to keep the posted code as concise as possible. This has been overloaded to allow sub rects, rotation, scaling etc.

Thank you for you help