From 4315c3d290f14e9037f1ffe557ddf9ddd5fb0b82 Mon Sep 17 00:00:00 2001 From: Chris Thrasher Date: Tue, 16 Apr 2024 10:41:43 -0600 Subject: [PATCH] Revert change to `Drawable::draw` function signature This change was made in 359fe90 due to recommendations from tooling. On its face this change makes sense since it removes a copy that isn't always necessary. In practice it caused ergonomic issues due to now being forced to make a copy of the render states when needed. The performance gains of eliding this copy are unsubstantiated. We have not done any profiling to measure its impact. For lack of such measurements I'd rather err on the side of improved user experience. If future benchmarks prove this copy is rather expensive then we can reconsider removing it with that evidence in mind. --- examples/shader/Effect.hpp | 8 ++--- examples/shader/Shader.cpp | 40 +++++++++++-------------- include/SFML/Graphics/Drawable.hpp | 9 +++--- include/SFML/Graphics/Shape.hpp | 2 +- include/SFML/Graphics/Sprite.hpp | 2 +- include/SFML/Graphics/Text.hpp | 2 +- include/SFML/Graphics/Transformable.hpp | 7 ++--- include/SFML/Graphics/VertexArray.hpp | 2 +- include/SFML/Graphics/VertexBuffer.hpp | 2 +- src/SFML/Graphics/Shape.cpp | 16 +++++----- src/SFML/Graphics/Sprite.cpp | 12 ++++---- src/SFML/Graphics/Text.cpp | 14 ++++----- src/SFML/Graphics/VertexArray.cpp | 2 +- src/SFML/Graphics/VertexBuffer.cpp | 2 +- test/Graphics/Drawable.test.cpp | 2 +- 15 files changed, 54 insertions(+), 68 deletions(-) diff --git a/examples/shader/Effect.hpp b/examples/shader/Effect.hpp index ed2e8f965..2008f9cd4 100644 --- a/examples/shader/Effect.hpp +++ b/examples/shader/Effect.hpp @@ -38,7 +38,7 @@ public: onUpdate(time, x, y); } - void draw(sf::RenderTarget& target, const sf::RenderStates& states) const override + void draw(sf::RenderTarget& target, sf::RenderStates states) const override { if (m_isLoaded) { @@ -66,9 +66,9 @@ protected: private: // Virtual functions to be implemented in derived effects - virtual bool onLoad() = 0; - virtual void onUpdate(float time, float x, float y) = 0; - virtual void onDraw(sf::RenderTarget& target, const sf::RenderStates& states) const = 0; + virtual bool onLoad() = 0; + virtual void onUpdate(float time, float x, float y) = 0; + virtual void onDraw(sf::RenderTarget& target, sf::RenderStates states) const = 0; std::string m_name; bool m_isLoaded{}; diff --git a/examples/shader/Shader.cpp b/examples/shader/Shader.cpp index 66aba8d34..50da66f5b 100644 --- a/examples/shader/Shader.cpp +++ b/examples/shader/Shader.cpp @@ -51,11 +51,10 @@ public: m_shader.setUniform("pixel_threshold", (x + y) / 30); } - void onDraw(sf::RenderTarget& target, const sf::RenderStates& states) const override + void onDraw(sf::RenderTarget& target, sf::RenderStates states) const override { - sf::RenderStates statesCopy(states); - statesCopy.shader = &m_shader; - target.draw(*m_sprite, statesCopy); + states.shader = &m_shader; + target.draw(*m_sprite, states); } private: @@ -111,11 +110,10 @@ public: m_shader.setUniform("blur_radius", (x + y) * 0.008f); } - void onDraw(sf::RenderTarget& target, const sf::RenderStates& states) const override + void onDraw(sf::RenderTarget& target, sf::RenderStates states) const override { - sf::RenderStates statesCopy(states); - statesCopy.shader = &m_shader; - target.draw(m_text, statesCopy); + states.shader = &m_shader; + target.draw(m_text, states); } private: @@ -165,11 +163,10 @@ public: m_shader.setUniform("blink_alpha", 0.5f + std::cos(time * 3) * 0.25f); } - void onDraw(sf::RenderTarget& target, const sf::RenderStates& states) const override + void onDraw(sf::RenderTarget& target, sf::RenderStates states) const override { - sf::RenderStates statesCopy(states); - statesCopy.shader = &m_shader; - target.draw(m_points, statesCopy); + states.shader = &m_shader; + target.draw(m_points, states); } private: @@ -245,11 +242,10 @@ public: m_surface.display(); } - void onDraw(sf::RenderTarget& target, const sf::RenderStates& states) const override + void onDraw(sf::RenderTarget& target, sf::RenderStates states) const override { - sf::RenderStates statesCopy(states); - statesCopy.shader = &m_shader; - target.draw(sf::Sprite(m_surface.getTexture()), statesCopy); + states.shader = &m_shader; + target.draw(sf::Sprite(m_surface.getTexture()), states); } private: @@ -318,17 +314,15 @@ public: m_shader.setUniform("size", sf::Vector2f(size, size)); } - void onDraw(sf::RenderTarget& target, const sf::RenderStates& states) const override + void onDraw(sf::RenderTarget& target, sf::RenderStates states) const override { - sf::RenderStates statesCopy(states); - // Prepare the render state - statesCopy.shader = &m_shader; - statesCopy.texture = &m_logoTexture; - statesCopy.transform = m_transform; + states.shader = &m_shader; + states.texture = &m_logoTexture; + states.transform = m_transform; // Draw the point cloud - target.draw(m_pointCloud, statesCopy); + target.draw(m_pointCloud, states); } private: diff --git a/include/SFML/Graphics/Drawable.hpp b/include/SFML/Graphics/Drawable.hpp index 79cfb0f70..ce095e603 100644 --- a/include/SFML/Graphics/Drawable.hpp +++ b/include/SFML/Graphics/Drawable.hpp @@ -63,7 +63,7 @@ protected: /// \param states Current render states /// //////////////////////////////////////////////////////////// - virtual void draw(RenderTarget& target, const RenderStates& states) const = 0; + virtual void draw(RenderTarget& target, RenderStates states) const = 0; }; } // namespace sf @@ -94,15 +94,14 @@ protected: /// /// private: /// -/// void draw(sf::RenderTarget& target, const sf::RenderStates& states) const override +/// void draw(sf::RenderTarget& target, sf::RenderStates states) const override /// { /// // You can draw other high-level objects /// target.draw(m_sprite, states); /// /// // ... or use the low-level API -/// sf::RenderStates statesCopy(states); -/// statesCopy.texture = &m_texture; -/// target.draw(m_vertices, statesCopy); +/// states.texture = &m_texture; +/// target.draw(m_vertices, states); /// /// // ... or draw with OpenGL directly /// glBegin(GL_TRIANGLES); diff --git a/include/SFML/Graphics/Shape.hpp b/include/SFML/Graphics/Shape.hpp index f514ea0a6..2dcd7c9e3 100644 --- a/include/SFML/Graphics/Shape.hpp +++ b/include/SFML/Graphics/Shape.hpp @@ -281,7 +281,7 @@ private: /// \param states Current render states /// //////////////////////////////////////////////////////////// - void draw(RenderTarget& target, const RenderStates& states) const override; + void draw(RenderTarget& target, RenderStates states) const override; //////////////////////////////////////////////////////////// /// \brief Update the fill vertices' color diff --git a/include/SFML/Graphics/Sprite.hpp b/include/SFML/Graphics/Sprite.hpp index d09cd277d..c2b000a71 100644 --- a/include/SFML/Graphics/Sprite.hpp +++ b/include/SFML/Graphics/Sprite.hpp @@ -208,7 +208,7 @@ private: /// \param states Current render states /// //////////////////////////////////////////////////////////// - void draw(RenderTarget& target, const RenderStates& states) const override; + void draw(RenderTarget& target, RenderStates states) const override; //////////////////////////////////////////////////////////// /// \brief Update the vertices' positions diff --git a/include/SFML/Graphics/Text.hpp b/include/SFML/Graphics/Text.hpp index f4a10a295..b032de900 100644 --- a/include/SFML/Graphics/Text.hpp +++ b/include/SFML/Graphics/Text.hpp @@ -398,7 +398,7 @@ private: /// \param states Current render states /// //////////////////////////////////////////////////////////// - void draw(RenderTarget& target, const RenderStates& states) const override; + void draw(RenderTarget& target, RenderStates states) const override; //////////////////////////////////////////////////////////// /// \brief Make sure the text's geometry is updated diff --git a/include/SFML/Graphics/Transformable.hpp b/include/SFML/Graphics/Transformable.hpp index 7a10014a9..1aa355eea 100644 --- a/include/SFML/Graphics/Transformable.hpp +++ b/include/SFML/Graphics/Transformable.hpp @@ -288,11 +288,10 @@ private: /// \code /// class MyEntity : public sf::Transformable, public sf::Drawable /// { -/// void draw(sf::RenderTarget& target, const sf::RenderStates& states) const override +/// void draw(sf::RenderTarget& target, sf::RenderStates states) const override /// { -/// sf::RenderStates statesCopy(states); -/// statesCopy.transform *= getTransform(); -/// target.draw(..., statesCopy); +/// states.transform *= getTransform(); +/// target.draw(..., states); /// } /// }; /// diff --git a/include/SFML/Graphics/VertexArray.hpp b/include/SFML/Graphics/VertexArray.hpp index cd2e2a694..9a3a8b534 100644 --- a/include/SFML/Graphics/VertexArray.hpp +++ b/include/SFML/Graphics/VertexArray.hpp @@ -183,7 +183,7 @@ private: /// \param states Current render states /// //////////////////////////////////////////////////////////// - void draw(RenderTarget& target, const RenderStates& states) const override; + void draw(RenderTarget& target, RenderStates states) const override; //////////////////////////////////////////////////////////// // Member data diff --git a/include/SFML/Graphics/VertexBuffer.hpp b/include/SFML/Graphics/VertexBuffer.hpp index 6f343d902..5b01996aa 100644 --- a/include/SFML/Graphics/VertexBuffer.hpp +++ b/include/SFML/Graphics/VertexBuffer.hpp @@ -329,7 +329,7 @@ private: /// \param states Current render states /// //////////////////////////////////////////////////////////// - void draw(RenderTarget& target, const RenderStates& states) const override; + void draw(RenderTarget& target, RenderStates states) const override; //////////////////////////////////////////////////////////// // Member data diff --git a/src/SFML/Graphics/Shape.cpp b/src/SFML/Graphics/Shape.cpp index 578c3c5e9..ef2bace33 100644 --- a/src/SFML/Graphics/Shape.cpp +++ b/src/SFML/Graphics/Shape.cpp @@ -234,22 +234,20 @@ void Shape::update() //////////////////////////////////////////////////////////// -void Shape::draw(RenderTarget& target, const RenderStates& states) const +void Shape::draw(RenderTarget& target, RenderStates states) const { - RenderStates statesCopy(states); - - statesCopy.transform *= getTransform(); - statesCopy.coordinateType = CoordinateType::Pixels; + states.transform *= getTransform(); + states.coordinateType = CoordinateType::Pixels; // Render the inside - statesCopy.texture = m_texture; - target.draw(m_vertices, statesCopy); + states.texture = m_texture; + target.draw(m_vertices, states); // Render the outline if (m_outlineThickness != 0) { - statesCopy.texture = nullptr; - target.draw(m_outlineVertices, statesCopy); + states.texture = nullptr; + target.draw(m_outlineVertices, states); } } diff --git a/src/SFML/Graphics/Sprite.cpp b/src/SFML/Graphics/Sprite.cpp index 678fd9659..21db6ef17 100644 --- a/src/SFML/Graphics/Sprite.cpp +++ b/src/SFML/Graphics/Sprite.cpp @@ -126,14 +126,12 @@ FloatRect Sprite::getGlobalBounds() const //////////////////////////////////////////////////////////// -void Sprite::draw(RenderTarget& target, const RenderStates& states) const +void Sprite::draw(RenderTarget& target, RenderStates states) const { - RenderStates statesCopy(states); - - statesCopy.transform *= getTransform(); - statesCopy.texture = m_texture; - statesCopy.coordinateType = CoordinateType::Pixels; - target.draw(m_vertices.data(), m_vertices.size(), PrimitiveType::TriangleStrip, statesCopy); + states.transform *= getTransform(); + states.texture = m_texture; + states.coordinateType = CoordinateType::Pixels; + target.draw(m_vertices.data(), m_vertices.size(), PrimitiveType::TriangleStrip, states); } diff --git a/src/SFML/Graphics/Text.cpp b/src/SFML/Graphics/Text.cpp index 4ded96815..33ca33c88 100644 --- a/src/SFML/Graphics/Text.cpp +++ b/src/SFML/Graphics/Text.cpp @@ -339,21 +339,19 @@ FloatRect Text::getGlobalBounds() const //////////////////////////////////////////////////////////// -void Text::draw(RenderTarget& target, const RenderStates& states) const +void Text::draw(RenderTarget& target, RenderStates states) const { ensureGeometryUpdate(); - RenderStates statesCopy(states); - - statesCopy.transform *= getTransform(); - statesCopy.texture = &m_font->getTexture(m_characterSize); - statesCopy.coordinateType = CoordinateType::Pixels; + states.transform *= getTransform(); + states.texture = &m_font->getTexture(m_characterSize); + states.coordinateType = CoordinateType::Pixels; // Only draw the outline if there is something to draw if (m_outlineThickness != 0) - target.draw(m_outlineVertices, statesCopy); + target.draw(m_outlineVertices, states); - target.draw(m_vertices, statesCopy); + target.draw(m_vertices, states); } diff --git a/src/SFML/Graphics/VertexArray.cpp b/src/SFML/Graphics/VertexArray.cpp index 0f5c52c8b..a502f0005 100644 --- a/src/SFML/Graphics/VertexArray.cpp +++ b/src/SFML/Graphics/VertexArray.cpp @@ -135,7 +135,7 @@ FloatRect VertexArray::getBounds() const //////////////////////////////////////////////////////////// -void VertexArray::draw(RenderTarget& target, const RenderStates& states) const +void VertexArray::draw(RenderTarget& target, RenderStates states) const { if (!m_vertices.empty()) target.draw(m_vertices.data(), m_vertices.size(), m_primitiveType, states); diff --git a/src/SFML/Graphics/VertexBuffer.cpp b/src/SFML/Graphics/VertexBuffer.cpp index d69a02d15..05a126725 100644 --- a/src/SFML/Graphics/VertexBuffer.cpp +++ b/src/SFML/Graphics/VertexBuffer.cpp @@ -348,7 +348,7 @@ bool VertexBuffer::isAvailable() //////////////////////////////////////////////////////////// -void VertexBuffer::draw(RenderTarget& target, const RenderStates& states) const +void VertexBuffer::draw(RenderTarget& target, RenderStates states) const { if (m_buffer && m_size) target.draw(*this, 0, m_size, states); diff --git a/test/Graphics/Drawable.test.cpp b/test/Graphics/Drawable.test.cpp index e1296b746..8e1372a61 100644 --- a/test/Graphics/Drawable.test.cpp +++ b/test/Graphics/Drawable.test.cpp @@ -16,7 +16,7 @@ public: } private: - void draw(sf::RenderTarget&, const sf::RenderStates&) const override + void draw(sf::RenderTarget&, sf::RenderStates) const override { ++m_callCount; }