Add SpriteBatch #2166

Marioalexsan · 2022-07-17T16:55:14Z

Has this change been discussed on the forum or in an issue before?
(previously appeared in forum posts as requests, and also in an issue / PR: Add draw batching #1802)
Does the code follow the SFML Code Style Guide?
Have you provided some example/test code for your changes?
If you have additional steps which need to be performed list them as tasks!

Description

This is a draft PR for a sprite batching implementation. I have more work to do on the implementation, but I've reached a point where I have something I can show, and I'd like to hear thoughts from other people about this whole batching business.

The PR adds two classes: SpriteBatch, which implements the batching logic, and Batchable, an interface implemented by drawables that support batching. Batchable is similar in functionality with Drawable, and the classes that implement it send data to the batcher using a predefined method that takes in vertices and other state information.

The PR also adds the Batchable interface to Sprite, Text and Shape. In theory, anything that uses triangle primitives should work with the batcher, but I have only tested sprites so far.

The batcher has different modes that can be used for batching:

Deferred groups sprites that use the same texture and follow each other in the same batch, which makes the end result identical with RenderTarget. However, when rendering many sprites that use the same texture in sequence, the batcher should be more efficient with the draw calls.
TextureSort sorts the sprites by texture before grouping and rendering. This causes draw order to be lost; it's as if a "layer" is created for each group of sprites that have the same texture. This can be useful for drawing things that don't overlap, as in that case draw order is not relevant, and the batcher obtains maximum performance.
DepthSort sorts the sprites by texture and an optional "depth" parameter before grouping and rendering. This is a compromise between Deferred and TextureSort, as you can create multiple layers that have proper ordering between each other, while drawables within a single layer follow TextureSort logic. This can be useful for drawing scenes that use distinct layers, such as Background, Foreground, GUI.
There is also a fourth mode called Immediate, which just calls RenderTarget directly on every draw call.

Code that uses SpriteBatch requires calling begin and end for setting shaders and blend modes:

// Begin a batch using the given settings
spriteBatch.begin(sf::SpriteBatch::BatchMode::DepthSort, window, nullptr, sf::BlendAlpha);

// Draw some stuff
spriteBatch.draw(sprite, depth);
spriteBatch.draw(text, depth);
spriteBatch.draw(shape, depth);

// Every call to begin() needs to have a matching end()
// This is also where the data is sent for rendering
spriteBatch.end();

Tasks

How to test this PR?

You can test it by running the examples/sprite_batch demo I added. It shows the differences between regular rendering and batching, and also works as a basic benchmark for performance. It has controls for raising and lowering the sprite count (W and S), and for switching batching modes (F and G).

codecov · 2022-07-17T16:58:02Z

Codecov Report

Merging #2166 (e09cbbf) into master (5bd3722) will decrease coverage by 0.20%.
The diff coverage is 1.21%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2166      +/-   ##
==========================================
- Coverage   23.19%   22.99%   -0.20%     
==========================================
  Files         212      215       +3     
  Lines       18059    18224     +165     
  Branches     4411     4457      +46     
==========================================
+ Hits         4188     4191       +3     
- Misses      13145    13580     +435     
+ Partials      726      453     -273

Impacted Files	Coverage Δ
include/SFML/Graphics/Shape.hpp	`75.00% <0.00%> (ø)`
include/SFML/Graphics/Sprite.hpp	`0.00% <0.00%> (ø)`
include/SFML/Graphics/SpriteBatch.hpp	`0.00% <0.00%> (ø)`
include/SFML/Graphics/Text.hpp	`0.00% <0.00%> (ø)`
src/SFML/Graphics/Shape.cpp	`79.61% <0.00%> (-5.90%)`	⬇️
src/SFML/Graphics/Sprite.cpp	`0.00% <0.00%> (ø)`
src/SFML/Graphics/SpriteBatch.cpp	`0.00% <0.00%> (ø)`
src/SFML/Graphics/Text.cpp	`0.00% <0.00%> (ø)`
include/SFML/Graphics/Batchable.hpp	`100.00% <100.00%> (ø)`
include/SFML/Graphics/Transformable.hpp	`50.00% <0.00%> (-10.00%)`	⬇️
... and 54 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5bd3722...e09cbbf. Read the comment docs.

Bambo-Borris · 2022-07-17T17:25:47Z

This is a feature I have seen discussed endlessly, and seen a couple of PRs for in the past. I think of the ones I've seen I currently like this approach the most, but I need to spend a bit more time looking into this and run the examples. But I like the approach, it feels consistent with other areas of SFML (namely how sf::Drawable stuff works internally). So I like consistency, and I like the feature idea. The area I probably will drill down into later is the 3 different modes for the SpriteBatch.

There is however, a lot of other tasks for SFML3 already needing to be done so whether or not this is too big to accept into SFML3 I don't know, but really good work dude.

examples/sprite_batch/Batching.cpp

src/SFML/Graphics/SpriteBatch.cpp

ChrisThrasher · 2022-07-18T00:26:56Z

src/SFML/Graphics/SpriteBatch.cpp

+            const auto& comp = [&](const std::size_t a, const std::size_t b)
+            { return m_triangleInfos[a].texture < m_triangleInfos[b].texture; };
+
+            std::stable_sort(indices.begin(), indices.end(), comp);


Any particular reason to stable sort instead of a non-stable sort?

In my older attempts I used it to enforce that triangles with the same depth are rendered in draw call order, but it seems like the current sort logic also guarantees that. I'll switch it to std::sort after I test it a bit more.

Don't rely on std::sort happening to be stable if stable sorting is really what you need. That property is liable to change across implementations and over time since std::sort makes no stability guarantees.

[Off-topic] Arguably, this might be a case of "wrong defaults" in C++. Other languages have stable sorting as the only algorithm (e.g. Java or Python), or they explicitly name the unstable sort differently (Rust).

I agree that stable sorting is probably more intuitive in this case.

src/SFML/Graphics/SpriteBatch.cpp

Bromeon

Thanks a lot for this huge effort!
First quick feedback -- I'll need to reserve some time to look if/how this generally fits into SFML's scope.

What is missing in your description is a motivation/rationale for this feature. It may be obvious to you ("performance"), but could you elaborate more precisely where you encountered the biggest limits with current SFML, and how this solution would address those?

I like that you added quite some description to each of the 4 modes. What would the decision process for a user look like (e.g. how to choose between Deferred/Immediate/no batching)?

Could the begin() + end() method calls be replaced with an RAII guard?

{
    sf::BatchGuard guard = batch.begin();
    ...
    ...
} // automatic end()

examples/sprite_batch/Batching.cpp

Bromeon · 2022-07-18T13:11:46Z

examples/sprite_batch/Batching.cpp

+    while (tempTexture.loadFromFile(resourcesDir() / ("Tex" + std::to_string(currentTex) + ".png")))
+    {
+        textures.push_back(std::move(tempTexture));
+        tempTexture = sf::Texture();


This is probably not going to be necessary once the move constructor for sf::Texture is implemented, but I'm also fine with leaving it for explicitness.

Could also be that some compilers issue a "access to moved variable" warning otherwise.

examples/sprite_batch/Batching.cpp

Marioalexsan · 2022-07-20T21:41:39Z

@Bromeon I have to admit that I've yet to do my homework on the reasoning part. What I planned to do was to try and use the SpriteBatch inside open source SFML games to see if it would improve performance, particularly in cases where it's shy of being a consistent 60 FPS. It would also help with reducing or eliminating the manual batching inside VertexArrays that people do.

For instance, At odds seems to lag a bit on my crappy laptop. If the batcher would nudge the performance above the 60 FPS mark while being easy to use (or swap to), that would be great news. However, I still need to analyze the source code and whatnot.

Marioalexsan · 2022-07-20T22:11:44Z

@Bromeon Regarding the batch modes, first I need to mention that Immediate is the same as not using batching, since it just calls the RenderTarget directly. In hindsight, I'm going to remove it since it's not really useful.

Deferred is similar to regular drawing, since draw order is preserved (i.e. same as draw call order). This makes it easy to change RenderTarget logic to SpriteBatch logic using Deferred, and should provide performance enhancements when drawing many things in a row that use the same texture. So, an use case would be drawing all entities that use a wooden box texture, but which also have physics and can be rotated and moved around (and thus can't be inserted into a VertexArray or something).

TextureSort is useful if your application is structured into layers, such as top-down or side-scroller games. Within a layer, it's likely that objects don't overlap each other (or the order isn't very important), so if they use different textures, you can reorder them by texture to create bigger batches to send for rendering (thus you get some nice performance improvements).

DepthSort is a combination of the previous two modes, that is you can specify an explicit draw order if needed via the depth parameter, but you can also arrange your objects in multiple layers by using preset depths for each layer.

So, to summarize:

If you care for strict draw order or render many things using one texture, use Deferred
If you don't care for order and render many things using different textures, use TextureSort
If you need a partial order and render many things using different textures, use DepthSort and specify the order you need

Marioalexsan · 2022-07-20T22:18:07Z

@Bromeon Regarding the batch guard, I'm not sure if begin() and end() is something that is necessarily restricted to a scope. Cases like this could be possible:

// Set default render state in some parent method
batch.begin(/* default state */);

// Somewhere in a nested method to draw an entity
batch.end();
batch.begin(/* special state */);

/* render cool stuff */

batch.end();
batch.begin(/* previous state */);

// back to parent method
batch.end();

Bromeon · 2022-07-20T22:55:23Z

For instance, At odds seems to lag a bit on my crappy laptop. If the batcher would nudge the performance above the 60 FPS mark while being easy to use (or swap to), that would be great news. However, I still need to analyze the source code and whatnot.

Any such insights would be very interesting! I think there's a lot of potential, but since the batching API adds quite some complexity, it would be good to be sure it's beneficial in typical cases 🙂

Regarding the batch modes, first I need to mention that Immediate is the same as not using batching, since it just calls the RenderTarget directly. In hindsight, I'm going to remove it since it's not really useful.

I think it is useful, just needs to be documented that it's equivalent to using no batching. Why useful? Because it allows to quickly switch implementations and compare performances without rewriting the whole rendering code.

Also, your description following that comment is great! Do you think you could add this to the documentation?

Regarding the batch guard, I'm not sure if begin() and end() is something that is necessarily restricted to a scope. Cases like this could be possible:

I have the feeling these scenarios (having no clear owner of the batch) are precisely the situations which are hard to reason about and cause potential errors. Stuff like early returns or exceptions will immediately cause bugs that can be difficult to debug.

If batching should be "stackable", then maybe we should provide a dedicated API, which also helps restoring previous state. But one probably loses most of the advantages when switching batch anyway, no?

Marioalexsan · 2022-07-22T20:33:52Z

I pushed a modified implementation along with some changes to examples/sprite_batch. It's a bit random, but I feel like the current approach works better. I also tried to apply the suggestions given, although some are still in my backlog.

I've removed the begin() / end() approach in favor of making SpriteBatch work similar to a Drawable. Now, you set the batch mode using setBatchMode(batchMode), batch objects using batch(batchable, depth), and use the Drawable interface for rendering to RenderTarget. There is also a clear() method that removes all drawables from the batch. The current logic is like this:

// Set batch strategy
spriteBatch.setBatchMode(batchMode);

spriteBatch.batch(sprite, depth);
spriteBatch.batch(text, depth);
spriteBatch.batch(shape, depth);

// Draw the batch
// RenderStates texture is ignored here, since the batch sets the textures itself
spriteBatch.draw(window, sf::RenderStates(sf::BlendAlpha, sf::Transform::Identity, nullptr, shader));

// Optionally clear the batch for rendering on the next frame
spriteBatch.clear();

The advantage of the new implementation is that you can now keep the batch and reuse it for static scenes, which was not possible in the previous version. It also prepares the batches only once, and reuses the result if multiple calls to draw() are made.

Regarding the batch example, I've added texts, shapes and shapes with no texture for checking more drawable types. As expected, the batcher struggles with dynamic texts and shapes, since they're high vertex count drawables and kind of mini-batches by themselves. However, for static stuff, inserting into a batch and reusing said batch works almost in the same way as manually batching using VertexArray, and works pretty well if sorting is applied.

examples/sprite_batch/Batching.cpp

src/SFML/Graphics/SpriteBatch.cpp

ChrisThrasher · 2022-07-22T21:36:20Z

src/SFML/Graphics/SpriteBatch.cpp

+
+        else
+        {
+            const auto comp = [&](const std::size_t a, const std::size_t b)


Instead of using this default capture mode ([&]) is it feasible to list the things you want to capture?

Would capturing this work?

No because that's the other default capture mode. Try to explicitly list the objects you're capturing. Sometimes that is not feasible but at least try to make it work as to minimize how much stuff gets put into the scope of the lambda.

I think capturing m_triangles directly won't work, but I can do something like this:

auto& triangles = this->m_triangles; const auto comp = [triangles](const std::size_t a, const std::size_t b) { if (triangles[a].depth != triangles[b].depth) return triangles[a].depth > triangles[b].depth; else return triangles[a].texture < triangles[b].texture; };

Hmm, maybe you're right to use [this]. You can copy m_triangles into the lambda but that's wasteful. I don't hate the idea of making a local reference and using that instead. In either case, it's more explicit than using [&] which will also capture everything in the current scope.

It seems like it's possible to capture m_triangles with an initializer such as [&triangles = m_triangles]. Maybe I'll use that instead.

I think that's just making a non-const copy.

ChrisThrasher · 2022-07-22T21:36:33Z

src/SFML/Graphics/SpriteBatch.cpp

+
+        if (m_batchMode == BatchMode::TextureSort)
+        {
+            const auto comp = [&](const std::size_t a, const std::size_t b)


Instead of using this default capture mode ([&]) is it feasible to list the things you want to capture?

ChrisThrasher · 2022-07-22T21:39:00Z

src/SFML/Graphics/SpriteBatch.cpp

+                if (m_triangles[a].depth != m_triangles[b].depth)
+                    return m_triangles[a].depth > m_triangles[b].depth;
+                else
+                    return m_triangles[a].texture < m_triangles[b].texture;


Omitting the braces in this if/else helps readability. I'd recommend removing the braces for other simple loops and conditions you've added which only contain a single expression. It halves the total line count and keeps relevant code near each other. Because we have clang-format to check formatting, there is no risk of accidentally writing something like

if (condition) run1(); run2(); // Uh oh, this always get run no matter the value of condition

I agree in the case of simple loops / ifs, although in some cases I feel like the extra space given by adding braces helps readability.

ChrisThrasher · 2023-02-20T01:01:43Z

I rebased on master, fixed merge conflicts, and fixed some issues related to style changes that have happened since this PR was last updated. That includes some clang-tidy fixes.

ChrisThrasher requested changes Jul 18, 2022

View reviewed changes

Bromeon reviewed Jul 18, 2022

View reviewed changes

Bromeon added feature m:sfml-graphics s:undecided labels Jul 18, 2022

Marioalexsan force-pushed the feature/spritebatch branch 2 times, most recently from 47d1dae to 5d1c106 Compare July 22, 2022 19:24

ChrisThrasher requested changes Jul 22, 2022

View reviewed changes

Marioalexsan force-pushed the feature/spritebatch branch 4 times, most recently from c8a6f7c to c440c7d Compare July 25, 2022 19:29

Add SpriteBatch

e09cbbf

ChrisThrasher force-pushed the feature/spritebatch branch from c440c7d to e09cbbf Compare February 20, 2023 01:01

metaquarx mentioned this pull request Feb 20, 2023

Upgrade away from legacy OpenGL #2423

Open

ChrisThrasher removed the s:undecided label Mar 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add SpriteBatch #2166

Add SpriteBatch #2166

Marioalexsan commented Jul 17, 2022

codecov bot commented Jul 17, 2022 •

edited

Bambo-Borris commented Jul 17, 2022 •

edited

ChrisThrasher Jul 18, 2022

Marioalexsan Jul 20, 2022

ChrisThrasher Jul 20, 2022

Bromeon Jul 20, 2022

Bromeon left a comment •

edited

Bromeon Jul 18, 2022

Marioalexsan commented Jul 20, 2022

Marioalexsan commented Jul 20, 2022

Marioalexsan commented Jul 20, 2022

Bromeon commented Jul 20, 2022

Marioalexsan commented Jul 22, 2022

ChrisThrasher Jul 22, 2022

Marioalexsan Jul 23, 2022

ChrisThrasher Jul 23, 2022

Marioalexsan Jul 23, 2022

ChrisThrasher Jul 23, 2022

Marioalexsan Jul 23, 2022

ChrisThrasher Jul 23, 2022

ChrisThrasher Jul 22, 2022

ChrisThrasher Jul 22, 2022

Marioalexsan Jul 23, 2022

ChrisThrasher commented Feb 20, 2023 •

edited

Add SpriteBatch #2166

Are you sure you want to change the base?

Add SpriteBatch #2166

Conversation

Marioalexsan commented Jul 17, 2022

Description

Tasks

How to test this PR?

codecov bot commented Jul 17, 2022 • edited

Codecov Report

Bambo-Borris commented Jul 17, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bromeon left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Marioalexsan commented Jul 20, 2022

Marioalexsan commented Jul 20, 2022

Marioalexsan commented Jul 20, 2022

Bromeon commented Jul 20, 2022

Marioalexsan commented Jul 22, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChrisThrasher commented Feb 20, 2023 • edited

codecov bot commented Jul 17, 2022 •

edited

Bambo-Borris commented Jul 17, 2022 •

edited

Bromeon left a comment •

edited

ChrisThrasher commented Feb 20, 2023 •

edited