sf::Sprite as temporary view over geometry + texture

[vittorioromeo]

(Based on #3067.)

Alternative design to #3072 that:

• Solves the same lifetime issues
• Still greatly simplifies the API and implementation of sprites
• Retains SFML's OOP nature, keeping sf::Sprite as a "drawable texture"

The new design is as follows:

• A new transformable but not drawable class, sf::SpriteGeometry has been extracted from sf::Sprite
• sf::SpriteGeometry calculates and owns the vertices required to draw a textured quad
• sf::Sprite is now a lightweight view over the combination of a sf::SpriteGeometry and a sf::Texture
• sf::Sprite is still drawable, but not transformable
• Only temporary instances of sf::Sprite can be drawn on a sf::RenderTarget, preventing lifetime bugs

Here is the simplest example of using this new API:

// Load a texture from a file
const auto texture = sf::Texture::loadFromFile("texture.png").value();

// Create geometry for the sprite that will display the texture
sf::SpriteGeometry geometry(texture.getRect());

// Create and draw the textured sprite
window.draw(sf::Sprite(geometry, texture));

sf::SpriteGeometry is safe to be precalculated, stored in classes, copied, and moved around. This cleanly solves the lifetime issues encountered in #3062, in fact this PR restores part of the original design where the sprite geometry is a data member of the shader effects.

---

If you feel like the new syntax is too verbose, there is a lot of room to add "convenience" functions, e.g.,

// POSSIBLE MORE CONCISE API #1
sf::SpriteGeometry geometry(texture); // IDEA: no need for `.getRect()` call
window.draw(sf::Sprite(geometry, texture));

// POSSIBLE MORE CONCISE API #2
window.draw(sf::Sprite(texture)); // IDEA: shorthand notation for `sf::Sprite(sf::SpriteGeometry(texture), texture)`

// POSSIBLE MORE CONCISE API #3
window.draw(geometry, texture); // IDEA: shorthand notation for `sf::Sprite(geometry, texture)`

---

This API is also very suitable for a future batching extension:

std::vector<sf::SpriteGeometry> geometries;
// ...fill `geometries`...

window.draw(geometries.data(), geometries.size(), texture); // single draw call for many sprites

[Bromeon]

Maybe I misunderstand something, but with this design, wouldn't users be tempted to store sf::Sprite and thus gain back the lifetime issues -- this time not only for the texture, but also the geometry?

We can of course discourage/document that, but using the same name would likely be irritating. Also, people may wonder why this is a class in the first place (as opposed to e.g. a function taking 2 parameters) 🤔

In general, I feel the current sf::Sprite is extremely convenient, and resolving the lifetime issue at an API level is very hard to do without impacting ergonomics. Have you considered alternatives to the lifetime problem, like e.g. validity checks, centralized texture map, texture IDs instead of pointers? If performance is a concern, these could be Debug-only, to catch bugs during development, and use unchecked raw pointers in Release.

[vittorioromeo]

Maybe I misunderstand something, but with this design, wouldn't users be tempted to store sf::Sprite and thus gain back the lifetime issues -- this time not only for the texture, but also the geometry?

This design encourages the creation of sf::Sprite directly at the time of draw call. While it is possible to store an sf::Sprite, attempting to use it as an lvalue will produce a compile-time error. E.g.,

struct TotallyInnocent
{   
    sf::SpriteGeometry g;
    sf::Texture t;
    sf::Sprite s;
    
    TotallyInnocent(sf::SpriteGeometry&& xg, sf::Texture&& xt) 
        : g{std::move(xg)}, t{std::move(xt)}, s{g, t}
    {
    }
    
    void draw(sf::RenderTarget& rt)
    {
        rt.draw(s); // DOES NOT COMPILE
    }
};

It is true that the user can circumvent the compile-time error by using std::move(s), but at that point they'd already be questioning why the code does not compile, prompting them to look at the documentation and to make them realize that they might be doing something dangerous.

We can of course discourage/document that, but using the same name would likely be irritating. Also, people may wonder why this is a class in the first place (as opposed to e.g. a function taking 2 parameters) 🤔

I agree with you that a function would be the best choice here. In fact, I think that sf::RenderTarget::draw(const sf::SpriteGeometry&, const sf::Texture&) would be a great API.

Unfortunately, that's pretty much what I proposed in #3072, but pretty much everyone on GitHub hated it. (People on Twitter seemed to like the change though, so I'm at least a little bit vindicated...)

In general, I feel the current sf::Sprite is extremely convenient, and resolving the lifetime issue at an API level is very hard to do without impacting ergonomics.

I don't disagree with you. What I genuinely believe is that a small and reasonable impact on ergonomics is very much worth it to prevent subtle lifetime issue scenarios.

Have you considered alternatives to the lifetime problem, like e.g. validity checks, centralized texture map, texture IDs instead of pointers? If performance is a concern, these could be Debug-only, to catch bugs during development, and use unchecked raw pointers in Release.

I have considered all of these. I'm not sure what you exactly mean with "validity checks", but it is generally not possible to check if an object has changed address if I just have a raw pointer.

A centralized texture map or the use of handles instead of pointers would mean that now SFML imposes a resource management strategy on the user. I really like the fact that SFML gives the user the freedom to manage their own resources the way they like, and I think it's a great design choice that I'd like to keep.

Finally, having debug-only checks was one of my initial ideas. It would solve common lifetime bugs not only for sprites, but also for text, sounds, music, shapes and so on. I posted my experimental branch here #2385 (comment) and I remember discussing it on GitHub, but again no one really liked it.

I also want to reiterate that solving the lifetime issue is not the only problem I'm trying to solve with the new proposed sf::Sprite APIs, as I also think they're generally better APIs for the user, simplify the implementation of sprites, and completely decouple sf::Sprite from sf::Texture. These are advantages that should not be ignored.

[vittorioromeo]

No consensus.