From 6d1bcbd9ebd9902b241af6e2272f93cd4011355a Mon Sep 17 00:00:00 2001 From: Chris Thrasher Date: Tue, 5 Sep 2023 19:41:22 -0600 Subject: [PATCH] Add `std::shared_ptr` overloads for assigning resources while maintaining lifetime safety --- include/SFML/Graphics/Sprite.hpp | 46 +++++++++++++++++++-- include/SFML/Graphics/Text.hpp | 31 +++++++++++++- src/SFML/Graphics/Sprite.cpp | 48 +++++++++++++++++++++- src/SFML/Graphics/Text.cpp | 69 ++++++++++++++++++++++++-------- test/Graphics/Sprite.test.cpp | 68 +++++++++++++++++++++++-------- test/Graphics/Text.test.cpp | 66 +++++++++++++++++++++--------- 6 files changed, 271 insertions(+), 57 deletions(-) diff --git a/include/SFML/Graphics/Sprite.hpp b/include/SFML/Graphics/Sprite.hpp index 69687d0af..727e8e0fa 100644 --- a/include/SFML/Graphics/Sprite.hpp +++ b/include/SFML/Graphics/Sprite.hpp @@ -34,6 +34,9 @@ #include #include +#include +#include + namespace sf { @@ -57,6 +60,16 @@ public: //////////////////////////////////////////////////////////// explicit Sprite(const Texture& texture); + //////////////////////////////////////////////////////////// + /// \brief Construct the sprite from a source texture + /// + /// \param texture Source texture + /// + /// \see setTexture + /// + //////////////////////////////////////////////////////////// + explicit Sprite(std::shared_ptr texture); + //////////////////////////////////////////////////////////// /// \brief Disallow construction from a temporary texture /// @@ -74,6 +87,17 @@ public: //////////////////////////////////////////////////////////// Sprite(const Texture& texture, const IntRect& rectangle); + //////////////////////////////////////////////////////////// + /// \brief Construct the sprite from a sub-rectangle of a source texture + /// + /// \param texture Source texture + /// \param rectangle Sub-rectangle of the texture to assign to the sprite + /// + /// \see setTexture, setTextureRect + /// + //////////////////////////////////////////////////////////// + Sprite(std::shared_ptr texture, const IntRect& rectangle); + //////////////////////////////////////////////////////////// /// \brief Disallow construction from a temporary texture /// @@ -101,6 +125,21 @@ public: //////////////////////////////////////////////////////////// void setTexture(const Texture& texture, bool resetRect = false); + //////////////////////////////////////////////////////////// + /// \brief Change the source texture of the sprite + /// + /// Same as above but using a shared pointer to a texture to + /// which guarantees that the texture remains alive for the + /// entire lifetime of the sprite. + /// + /// \param texture New texture + /// \param resetRect Should the texture rect be reset to the size of the new texture? + /// + /// \see getTexture, setTextureRect + /// + //////////////////////////////////////////////////////////// + void setTexture(std::shared_ptr texture, bool resetRect = false); + //////////////////////////////////////////////////////////// /// \brief Disallow setting from a temporary texture /// @@ -222,9 +261,10 @@ private: //////////////////////////////////////////////////////////// // Member data //////////////////////////////////////////////////////////// - Vertex m_vertices[4]; //!< Vertices defining the sprite's geometry - const Texture* m_texture{}; //!< Texture of the sprite - IntRect m_textureRect; //!< Rectangle defining the area of the source texture to display + using TexturePtr = std::variant>; + Vertex m_vertices[4]; //!< Vertices defining the sprite's geometry + TexturePtr m_texture; //!< Texture of the sprite + IntRect m_textureRect; //!< Rectangle defining the area of the source texture to display }; } // namespace sf diff --git a/include/SFML/Graphics/Text.hpp b/include/SFML/Graphics/Text.hpp index 17d8cb0e2..34cabd25f 100644 --- a/include/SFML/Graphics/Text.hpp +++ b/include/SFML/Graphics/Text.hpp @@ -36,6 +36,8 @@ #include +#include +#include #include @@ -80,6 +82,20 @@ public: //////////////////////////////////////////////////////////// Text(const Font& font, String string = "", unsigned int characterSize = 30); + //////////////////////////////////////////////////////////// + /// \brief Construct the text from a string, font and size + /// + /// Construct as above but using a shared pointer to a font + /// which guarantees the font remains alive for the lifetime + /// of the text object. + /// + /// \param string Text assigned to the string + /// \param font Font used to draw the string + /// \param characterSize Base size of characters, in pixels + /// + //////////////////////////////////////////////////////////// + Text(std::shared_ptr font, String string = "", unsigned int characterSize = 30); + //////////////////////////////////////////////////////////// /// \brief Disallow construction from a temporary font /// @@ -124,6 +140,18 @@ public: //////////////////////////////////////////////////////////// void setFont(const Font& font); + //////////////////////////////////////////////////////////// + /// \brief Set the text's font + /// + /// Same as above but using a shared pointer to a font + /// which guarantees the font remains alive for the lifetime + /// of the text object. + /// + /// \see getFont + /// + //////////////////////////////////////////////////////////// + void setFont(std::shared_ptr font); + //////////////////////////////////////////////////////////// /// \brief Disallow setting from a temporary font /// @@ -406,8 +434,9 @@ private: //////////////////////////////////////////////////////////// // Member data //////////////////////////////////////////////////////////// + using FontPtr = std::variant>; String m_string; //!< String to display - const Font* m_font{}; //!< Font used to display the string + FontPtr m_font; //!< Font used to display the string unsigned int m_characterSize{30}; //!< Base size of characters, in pixels float m_letterSpacingFactor{1.f}; //!< Spacing factor between letters float m_lineSpacingFactor{1.f}; //!< Spacing factor between lines diff --git a/src/SFML/Graphics/Sprite.cpp b/src/SFML/Graphics/Sprite.cpp index 4c22276d0..7e20d41bc 100644 --- a/src/SFML/Graphics/Sprite.cpp +++ b/src/SFML/Graphics/Sprite.cpp @@ -42,6 +42,13 @@ Sprite::Sprite(const Texture& texture) } +//////////////////////////////////////////////////////////// +Sprite::Sprite(std::shared_ptr texture) +{ + setTexture(std::move(texture), true); +} + + //////////////////////////////////////////////////////////// Sprite::Sprite(const Texture& texture, const IntRect& rectangle) { @@ -52,6 +59,16 @@ Sprite::Sprite(const Texture& texture, const IntRect& rectangle) } +//////////////////////////////////////////////////////////// +Sprite::Sprite(std::shared_ptr texture, const IntRect& rectangle) +{ + // Compute the texture area + setTextureRect(rectangle); + // Assign texture + setTexture(std::move(texture), false); +} + + //////////////////////////////////////////////////////////// void Sprite::setTexture(const Texture& texture, bool resetRect) { @@ -66,6 +83,22 @@ void Sprite::setTexture(const Texture& texture, bool resetRect) } +//////////////////////////////////////////////////////////// +void Sprite::setTexture(std::shared_ptr texture, bool resetRect) +{ + assert(texture && "Sprite::setTexture() Texture cannot be null"); + + // Recompute the texture area if requested + if (resetRect) + { + setTextureRect(IntRect({0, 0}, Vector2i(texture->getSize()))); + } + + // Assign the new texture + m_texture = std::move(texture); +} + + //////////////////////////////////////////////////////////// void Sprite::setTextureRect(const IntRect& rectangle) { @@ -92,7 +125,18 @@ void Sprite::setColor(const Color& color) //////////////////////////////////////////////////////////// const Texture& Sprite::getTexture() const { - return *m_texture; + static const struct Visitor + { + const sf::Texture* operator()(const sf::Texture* texture) const + { + return texture; + } + const sf::Texture* operator()(const std::shared_ptr& texture) const + { + return texture.get(); + } + } visitor; + return *std::visit(visitor, m_texture); } @@ -133,7 +177,7 @@ void Sprite::draw(RenderTarget& target, const RenderStates& states) const RenderStates statesCopy(states); statesCopy.transform *= getTransform(); - statesCopy.texture = m_texture; + statesCopy.texture = &getTexture(); target.draw(m_vertices, 4, PrimitiveType::TriangleStrip, statesCopy); } diff --git a/src/SFML/Graphics/Text.cpp b/src/SFML/Graphics/Text.cpp index 2feab6429..1539f71c2 100644 --- a/src/SFML/Graphics/Text.cpp +++ b/src/SFML/Graphics/Text.cpp @@ -33,6 +33,7 @@ #include #include +#include #include @@ -104,6 +105,16 @@ m_characterSize(characterSize) } +//////////////////////////////////////////////////////////// +Text::Text(std::shared_ptr font, String string, unsigned int characterSize) : +m_string(std::move(string)), +m_characterSize(characterSize) +{ + assert(font && "Text::Text() Font cannot be null"); + m_font = std::move(font); +} + + //////////////////////////////////////////////////////////// void Text::setString(const String& string) { @@ -118,7 +129,7 @@ void Text::setString(const String& string) //////////////////////////////////////////////////////////// void Text::setFont(const Font& font) { - if (m_font != &font) + if (&getFont() != &font) { m_font = &font; m_geometryNeedUpdate = true; @@ -126,6 +137,19 @@ void Text::setFont(const Font& font) } +//////////////////////////////////////////////////////////// +void Text::setFont(std::shared_ptr font) +{ + assert(font && "Text::setFont() Font cannot be null"); + + if (&getFont() != font.get()) + { + m_font = std::move(font); + m_geometryNeedUpdate = true; + } +} + + //////////////////////////////////////////////////////////// void Text::setCharacterSize(unsigned int size) { @@ -227,7 +251,18 @@ const String& Text::getString() const //////////////////////////////////////////////////////////// const Font& Text::getFont() const { - return *m_font; + static const struct Visitor + { + const sf::Font* operator()(const sf::Font* font) const + { + return font; + } + const sf::Font* operator()(const std::shared_ptr& font) const + { + return font.get(); + } + } visitor; + return *std::visit(visitor, m_font); } @@ -289,10 +324,10 @@ Vector2f Text::findCharacterPos(std::size_t index) const // Precompute the variables needed by the algorithm const bool isBold = m_style & Bold; - float whitespaceWidth = m_font->getGlyph(U' ', m_characterSize, isBold).advance; + float whitespaceWidth = getFont().getGlyph(U' ', m_characterSize, isBold).advance; const float letterSpacing = (whitespaceWidth / 3.f) * (m_letterSpacingFactor - 1.f); whitespaceWidth += letterSpacing; - const float lineSpacing = m_font->getLineSpacing(m_characterSize) * m_lineSpacingFactor; + const float lineSpacing = getFont().getLineSpacing(m_characterSize) * m_lineSpacingFactor; // Compute the position Vector2f position; @@ -302,7 +337,7 @@ Vector2f Text::findCharacterPos(std::size_t index) const const std::uint32_t curChar = m_string[i]; // Apply the kerning offset - position.x += m_font->getKerning(prevChar, curChar, m_characterSize, isBold); + position.x += getFont().getKerning(prevChar, curChar, m_characterSize, isBold); prevChar = curChar; // Handle special characters @@ -321,7 +356,7 @@ Vector2f Text::findCharacterPos(std::size_t index) const } // For regular characters, add the advance offset of the glyph - position.x += m_font->getGlyph(curChar, m_characterSize, isBold).advance + letterSpacing; + position.x += getFont().getGlyph(curChar, m_characterSize, isBold).advance + letterSpacing; } // Transform the position to global coordinates @@ -355,7 +390,7 @@ void Text::draw(RenderTarget& target, const RenderStates& states) const RenderStates statesCopy(states); statesCopy.transform *= getTransform(); - statesCopy.texture = &m_font->getTexture(m_characterSize); + statesCopy.texture = &getFont().getTexture(m_characterSize); // Only draw the outline if there is something to draw if (m_outlineThickness != 0) @@ -369,11 +404,11 @@ void Text::draw(RenderTarget& target, const RenderStates& states) const void Text::ensureGeometryUpdate() const { // Do nothing, if geometry has not changed and the font texture has not changed - if (!m_geometryNeedUpdate && m_font->getTexture(m_characterSize).m_cacheId == m_fontTextureId) + if (!m_geometryNeedUpdate && getFont().getTexture(m_characterSize).m_cacheId == m_fontTextureId) return; // Save the current fonts texture id - m_fontTextureId = m_font->getTexture(m_characterSize).m_cacheId; + m_fontTextureId = getFont().getTexture(m_characterSize).m_cacheId; // Mark geometry as updated m_geometryNeedUpdate = false; @@ -392,20 +427,20 @@ void Text::ensureGeometryUpdate() const const bool isUnderlined = m_style & Underlined; const bool isStrikeThrough = m_style & StrikeThrough; const float italicShear = (m_style & Italic) ? sf::degrees(12).asRadians() : 0.f; - const float underlineOffset = m_font->getUnderlinePosition(m_characterSize); - const float underlineThickness = m_font->getUnderlineThickness(m_characterSize); + const float underlineOffset = getFont().getUnderlinePosition(m_characterSize); + const float underlineThickness = getFont().getUnderlineThickness(m_characterSize); // Compute the location of the strike through dynamically // We use the center point of the lowercase 'x' glyph as the reference // We reuse the underline thickness as the thickness of the strike through as well - const FloatRect xBounds = m_font->getGlyph(U'x', m_characterSize, isBold).bounds; + const FloatRect xBounds = getFont().getGlyph(U'x', m_characterSize, isBold).bounds; const float strikeThroughOffset = xBounds.top + xBounds.height / 2.f; // Precompute the variables needed by the algorithm - float whitespaceWidth = m_font->getGlyph(U' ', m_characterSize, isBold).advance; + float whitespaceWidth = getFont().getGlyph(U' ', m_characterSize, isBold).advance; const float letterSpacing = (whitespaceWidth / 3.f) * (m_letterSpacingFactor - 1.f); whitespaceWidth += letterSpacing; - const float lineSpacing = m_font->getLineSpacing(m_characterSize) * m_lineSpacingFactor; + const float lineSpacing = getFont().getLineSpacing(m_characterSize) * m_lineSpacingFactor; float x = 0.f; auto y = static_cast(m_characterSize); @@ -424,7 +459,7 @@ void Text::ensureGeometryUpdate() const continue; // Apply the kerning offset - x += m_font->getKerning(prevChar, curChar, m_characterSize, isBold); + x += getFont().getKerning(prevChar, curChar, m_characterSize, isBold); // If we're using the underlined style and there's a new line, draw a line if (isUnderlined && (curChar == U'\n' && prevChar != U'\n')) @@ -478,14 +513,14 @@ void Text::ensureGeometryUpdate() const // Apply the outline if (m_outlineThickness != 0) { - const Glyph& glyph = m_font->getGlyph(curChar, m_characterSize, isBold, m_outlineThickness); + const Glyph& glyph = getFont().getGlyph(curChar, m_characterSize, isBold, m_outlineThickness); // Add the outline glyph to the vertices addGlyphQuad(m_outlineVertices, Vector2f(x, y), m_outlineColor, glyph, italicShear); } // Extract the current glyph's description - const Glyph& glyph = m_font->getGlyph(curChar, m_characterSize, isBold); + const Glyph& glyph = getFont().getGlyph(curChar, m_characterSize, isBold); // Add the glyph to the vertices addGlyphQuad(m_vertices, Vector2f(x, y), m_fillColor, glyph, italicShear); diff --git a/test/Graphics/Sprite.test.cpp b/test/Graphics/Sprite.test.cpp index 73afc4114..48053a1d8 100644 --- a/test/Graphics/Sprite.test.cpp +++ b/test/Graphics/Sprite.test.cpp @@ -20,36 +20,72 @@ TEST_CASE("[Graphics] sf::Sprite", runDisplayTests()) } const sf::Texture texture; + const auto texturePtr = std::make_shared(); SECTION("Construction") { SECTION("Texture constructor") { - const sf::Sprite sprite(texture); - CHECK(&sprite.getTexture() == &texture); - CHECK(sprite.getTextureRect() == sf::IntRect()); - CHECK(sprite.getColor() == sf::Color::White); - CHECK(sprite.getLocalBounds() == sf::FloatRect()); - CHECK(sprite.getGlobalBounds() == sf::FloatRect()); + SECTION("const Texture&") + { + const sf::Sprite sprite(texture); + CHECK(&sprite.getTexture() == &texture); + CHECK(sprite.getTextureRect() == sf::IntRect()); + CHECK(sprite.getColor() == sf::Color::White); + CHECK(sprite.getLocalBounds() == sf::FloatRect()); + CHECK(sprite.getGlobalBounds() == sf::FloatRect()); + } + + SECTION("std::shared_ptr") + { + const sf::Sprite sprite(texturePtr); + CHECK(&sprite.getTexture() == texturePtr.get()); + CHECK(sprite.getTextureRect() == sf::IntRect()); + CHECK(sprite.getColor() == sf::Color::White); + CHECK(sprite.getLocalBounds() == sf::FloatRect()); + CHECK(sprite.getGlobalBounds() == sf::FloatRect()); + } } SECTION("Texture and rectangle constructor") { - const sf::Sprite sprite(texture, {{0, 0}, {40, 60}}); - CHECK(&sprite.getTexture() == &texture); - CHECK(sprite.getTextureRect() == sf::IntRect({0, 0}, {40, 60})); - CHECK(sprite.getColor() == sf::Color::White); - CHECK(sprite.getLocalBounds() == sf::FloatRect({0, 0}, {40, 60})); - CHECK(sprite.getGlobalBounds() == sf::FloatRect({0, 0}, {40, 60})); + SECTION("const Texture&") + { + const sf::Sprite sprite(texture, {{0, 0}, {40, 60}}); + CHECK(&sprite.getTexture() == &texture); + CHECK(sprite.getTextureRect() == sf::IntRect({0, 0}, {40, 60})); + CHECK(sprite.getColor() == sf::Color::White); + CHECK(sprite.getLocalBounds() == sf::FloatRect({0, 0}, {40, 60})); + CHECK(sprite.getGlobalBounds() == sf::FloatRect({0, 0}, {40, 60})); + } + + SECTION("std::shared_ptr") + { + const sf::Sprite sprite(texturePtr, {{0, 0}, {40, 60}}); + CHECK(&sprite.getTexture() == texturePtr.get()); + CHECK(sprite.getTextureRect() == sf::IntRect({0, 0}, {40, 60})); + CHECK(sprite.getColor() == sf::Color::White); + CHECK(sprite.getLocalBounds() == sf::FloatRect({0, 0}, {40, 60})); + CHECK(sprite.getGlobalBounds() == sf::FloatRect({0, 0}, {40, 60})); + } } } SECTION("Set/get texture") { - sf::Sprite sprite(texture); - const sf::Texture otherTexture; - sprite.setTexture(otherTexture); - CHECK(&sprite.getTexture() == &otherTexture); + SECTION("const Texture&") + { + sf::Sprite sprite(texturePtr); + sprite.setTexture(texture); + CHECK(&sprite.getTexture() == &texture); + } + + SECTION("std::shared_ptr") + { + sf::Sprite sprite(texture); + sprite.setTexture(texturePtr); + CHECK(&sprite.getTexture() == texturePtr.get()); + } } SECTION("Set/get texture rect") diff --git a/test/Graphics/Text.test.cpp b/test/Graphics/Text.test.cpp index f65b328b7..52bab57bf 100644 --- a/test/Graphics/Text.test.cpp +++ b/test/Graphics/Text.test.cpp @@ -19,7 +19,8 @@ TEST_CASE("[Graphics] sf::Text", runDisplayTests()) STATIC_CHECK(std::is_nothrow_move_assignable_v); } - sf::Font font; + const auto fontPtr = std::make_shared(); + sf::Font font; REQUIRE(font.loadFromFile("Graphics/tuffy.ttf")); SECTION("Construction") @@ -78,19 +79,39 @@ TEST_CASE("[Graphics] sf::Text", runDisplayTests()) SECTION("Font, string, and character size constructor") { - const sf::Text text(font, "abcdefghijklmnopqrstuvwxyz", 24); - CHECK(text.getString() == "abcdefghijklmnopqrstuvwxyz"); - CHECK(&text.getFont() == &font); - CHECK(text.getCharacterSize() == 24); - CHECK(text.getLetterSpacing() == 1.f); - CHECK(text.getLineSpacing() == 1.f); - CHECK(text.getStyle() == sf::Text::Regular); - CHECK(text.getFillColor() == sf::Color::White); - CHECK(text.getOutlineColor() == sf::Color::Black); - CHECK(text.getOutlineThickness() == 0); - CHECK(text.findCharacterPos(0) == sf::Vector2f()); - CHECK(text.getLocalBounds() == sf::FloatRect({1, 7}, {290, 22})); - CHECK(text.getGlobalBounds() == sf::FloatRect({1, 7}, {290, 22})); + SECTION("const Font&") + { + const sf::Text text(font, "abcdefghijklmnopqrstuvwxyz", 24); + CHECK(text.getString() == "abcdefghijklmnopqrstuvwxyz"); + CHECK(&text.getFont() == &font); + CHECK(text.getCharacterSize() == 24); + CHECK(text.getLetterSpacing() == 1.f); + CHECK(text.getLineSpacing() == 1.f); + CHECK(text.getStyle() == sf::Text::Regular); + CHECK(text.getFillColor() == sf::Color::White); + CHECK(text.getOutlineColor() == sf::Color::Black); + CHECK(text.getOutlineThickness() == 0); + CHECK(text.findCharacterPos(0) == sf::Vector2f()); + CHECK(text.getLocalBounds() == sf::FloatRect({1, 7}, {290, 22})); + CHECK(text.getGlobalBounds() == sf::FloatRect({1, 7}, {290, 22})); + } + + SECTION("std::shared_ptr") + { + const sf::Text text(fontPtr, "abcdefghijklmnopqrstuvwxyz", 24); + CHECK(text.getString() == "abcdefghijklmnopqrstuvwxyz"); + CHECK(&text.getFont() == fontPtr.get()); + CHECK(text.getCharacterSize() == 24); + CHECK(text.getLetterSpacing() == 1.f); + CHECK(text.getLineSpacing() == 1.f); + CHECK(text.getStyle() == sf::Text::Regular); + CHECK(text.getFillColor() == sf::Color::White); + CHECK(text.getOutlineColor() == sf::Color::Black); + CHECK(text.getOutlineThickness() == 0); + CHECK(text.findCharacterPos(0) == sf::Vector2f()); + CHECK(text.getLocalBounds() == sf::FloatRect({0, 24}, {0, 0})); + CHECK(text.getGlobalBounds() == sf::FloatRect({0, 24}, {0, 0})); + } } } @@ -103,10 +124,19 @@ TEST_CASE("[Graphics] sf::Text", runDisplayTests()) SECTION("Set/get font") { - sf::Text text(font); - const sf::Font otherFont; - text.setFont(otherFont); - CHECK(&text.getFont() == &otherFont); + SECTION("const Font&") + { + sf::Text text(fontPtr); + text.setFont(font); + CHECK(&text.getFont() == &font); + } + + SECTION("std::shared_ptr") + { + sf::Text text(font); + text.setFont(fontPtr); + CHECK(&text.getFont() == fontPtr.get()); + } } SECTION("Set/get character size")