Replace the default constructor of sf::Text with one that takes an sf::Font

@ChrisThrasher

Inspired by a case of a user asking the Discord server for help in why their text was not displaying despite them using setString setStyle setFillColor setCharacterSize. They lacked setFont.

This suggests a breaking change, so I propose this be added to SFML 3. The change is trivial so doesn't take much work, but provides safety and a clearer API for the user.

Proposal

Change the default constructor of sf::Text from:
Text()
Into:
Text(const Font& font, unsigned int charachterSize = 30)

The reasoning is that allowing the user to create an sf::Text with no font is not useful, and opens up for confusion and bugs since rendering an sf::Text with no font will just display nothing. Especially with SFML's history of providing a default font, and also with many beginners not realising that a font needs to be set, this opens up a pitfall.

By removing the ability to even create an sf::Text in this state removes that class of bugs and goes along good API practices of making APIs harder to use without compromising their usefulness.

Drawback

The only obvious rebuttal I see to this are things like:

But I want to put uninitialised sf::Text in an std::vector
But I create sf::Texts in my engine that are going to reference not-yet-loaded sf::Fonts and I'll use setFont when the time comes
...similar...

These all boil down to "Sometimes it is useful to be able to construct uninitialised values for later use". This is true, and I'd argue the proper modern C++ way of doing this would be through std::optional<sf::Text> or std::unique_ptr<sf::Text> or similar, which to some might seem more verbose but this is a good type of verbose as it:

Makes clear which usages of sf::Text might be uninitialised (currently ALL usages of sf::Text could be, which is far from the default case)
Uses well tried and tested standard provided methods of creating uninitialised values that communicates intent and provides helper methods for handling it.
Makes it hard to accidentally come across uninitalised text objects, in contract to currently when ANY sf::Text opaquely might be not ready for use.

With this in mind, I personally don't see any further drawbacks with this change.

Compile errors

@ChrisThrasher did a quick test to build SFML with the default constructor removed, and there was an error in the joystick code where it seems like it uses an uninitialised instane of sf::Text. This is hopefully trivially fixed by changing it to std::optional<sf::Text>.

Summary

All in all, I think this would be a great change since it adds seatbelts against an issue that might otherwise trip up unexpecting users. It brings the API closer to being safe and modern, utilising the type system for bulletproof eradication of the described bugs. The change is seemingly easy to apply and I don't see any unaddressed drawbacks.

Interested in hearing the thoughts from the regular SFML maintainers in case I've missed something.

The compilation errors occur in the examples in island/Island.cpp, joystick/Joystick.cpp, shader/Shader.cpp, and tennis/Tennis.cpp. Some of those errors are trivial to fix but the errors in Joystick.cpp are particularly hard since many many text objects are being created in a map and lazily initialized. If we can find a good way to refactor late-initialization-heavy code to this new, stricter API that bodes well for this as good API design.

A half measure worth considering is to assert within sf::Text::draw that you shall not draw without a valid font.

void Text::draw(RenderTarget& target, const RenderStates& states) const
{
    if (m_font)
    {
        ensureGeometryUpdate();

        RenderStates statesCopy(states);

        statesCopy.transform *= getTransform();
        statesCopy.texture = &m_font->getTexture(m_characterSize);

        // Only draw the outline if there is something to draw
        if (m_outlineThickness != 0)
            target.draw(m_outlineVertices, statesCopy);

        target.draw(m_vertices, statesCopy);
    }
}

becomes

void Text::draw(RenderTarget& target, const RenderStates& states) const
{
    assert(m_font);

    ensureGeometryUpdate();

    RenderStates statesCopy(states);

    statesCopy.transform *= getTransform();
    statesCopy.texture = &m_font->getTexture(m_characterSize);

    // Only draw the outline if there is something to draw
    if (m_outlineThickness != 0)
        target.draw(m_outlineVertices, statesCopy);

    target.draw(m_vertices, statesCopy);
}

ChrisThrasher@e4e92f4

I think I have it working. The hardest part was the changes I had to make to the Joystick example but once those were in place it was easy to remove that sf::Text constructor. Hope this helps the team judge whether this is an API change we actually want.

A case we need to think about:

sf::Text text(font);
addText(std::move(text)); // give ownership away

// now `text` is like a "default-constructed" instance -- what are its semantics?

Also, sf::Font relates to sf::Text the same way that sf::Texture relates to sf::Sprite, or sf::SoundBuffer to sf::Sound. What do you think about those resources?

A case we need to think about:
sf::Text text(font);
addText(std::move(text)); // give ownership away

// now `text` is like a "default-constructed" instance -- what are its semantics?
Also, sf::Font relates to sf::Text the same way that sf::Texture relates to sf::Sprite, or sf::SoundBuffer to sf::Sound. What do you think about those resources?

How sf::Sprite currently works is that rendering without a texture produces a white square if I'm not mistaken? This could be considered valid and well defined semantics of the sf::Sprite type, so nothing would need to change with this one. However, if we do want to consider this usage of sf::Sprite an error, and that the proper way to render a while square is sf::RectangleShape, then I would be all for making the corresponding change on sf::Sprite too.
Agree that sf::Sound should have the same change - I don't think it makes much sense to ever play a sound with no soundbuffer, nor do I see the value in considering this a valid state.
As for the code snippet, I believe sf::Text should work like sf::Sprite handles textures - pass-by-reference when setting, but interally represent it as a non-owning raw pointer. That way, addText(std::move(text)) works without surprises where the added text would use the same font as the moved text was set to.

I've been maintaining my implementation of this issue here. If the SFML teams likes the idea I'll make a PR out of it.

now text is like a "default-constructed" instance -- what are its semantics?

Same as any Standard library container -- i.e. "valid but unspecified". In other words, don't touch it again :)

Any more thoughts on what I've implemented here?

Remove default `sf::Sprite` constructor

Navigation Menu

Repository navigation

Replace the default constructor of sf::Text with one that takes an sf::Font #2194

Proposal

Drawback

Compile errors

Summary

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Replace the default constructor of sf::Text with one that takes an sf::Font #2194

Description

Proposal

Drawback

Compile errors

Summary

Activity

ChrisThrasher commented on Sep 6, 2022

ChrisThrasher commented on Sep 6, 2022

Bromeon commented on Sep 6, 2022

therocode commented on Sep 6, 2022

ChrisThrasher commented on Sep 15, 2022

vittorioromeo commented on Sep 15, 2022

ChrisThrasher commented on Feb 19, 2023

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions