From caf843c85c4a8ce9a4e22e29425ac9ac2f44ae78 Mon Sep 17 00:00:00 2001 From: Chris Thrasher Date: Fri, 20 Sep 2024 17:15:31 -0500 Subject: [PATCH] Add move semantics to `sf::VertexBuffer` --- include/SFML/Graphics/VertexBuffer.hpp | 42 ++++++++++++------ src/SFML/Graphics/VertexBuffer.cpp | 60 ++++++++++++++++++++------ test/Graphics/VertexBuffer.test.cpp | 30 +++++++++++-- 3 files changed, 104 insertions(+), 28 deletions(-) diff --git a/include/SFML/Graphics/VertexBuffer.hpp b/include/SFML/Graphics/VertexBuffer.hpp index deb1c97b4..1e4cb8d02 100644 --- a/include/SFML/Graphics/VertexBuffer.hpp +++ b/include/SFML/Graphics/VertexBuffer.hpp @@ -107,6 +107,12 @@ public: //////////////////////////////////////////////////////////// VertexBuffer(PrimitiveType type, Usage usage); + //////////////////////////////////////////////////////////// + /// \brief Destructor + /// + //////////////////////////////////////////////////////////// + ~VertexBuffer() override; + //////////////////////////////////////////////////////////// /// \brief Copy constructor /// @@ -116,10 +122,32 @@ public: VertexBuffer(const VertexBuffer& copy); //////////////////////////////////////////////////////////// - /// \brief Destructor + /// \brief Overload of assignment operator + /// + /// \param right Instance to assign + /// + /// \return Reference to self /// //////////////////////////////////////////////////////////// - ~VertexBuffer() override; + VertexBuffer& operator=(const VertexBuffer& right); + + //////////////////////////////////////////////////////////// + /// \brief Move constructor + /// + /// \param right Instance to move + /// + //////////////////////////////////////////////////////////// + VertexBuffer(VertexBuffer&& right) noexcept; + + //////////////////////////////////////////////////////////// + /// \brief Move assignment + /// + /// \param right Instance to assign + /// + /// \return Reference to self + /// + //////////////////////////////////////////////////////////// + VertexBuffer& operator=(VertexBuffer&& right) noexcept; //////////////////////////////////////////////////////////// /// \brief Create the vertex buffer @@ -210,16 +238,6 @@ public: //////////////////////////////////////////////////////////// [[nodiscard]] bool update(const VertexBuffer& vertexBuffer); - //////////////////////////////////////////////////////////// - /// \brief Overload of assignment operator - /// - /// \param right Instance to assign - /// - /// \return Reference to self - /// - //////////////////////////////////////////////////////////// - VertexBuffer& operator=(const VertexBuffer& right); - //////////////////////////////////////////////////////////// /// \brief Swap the contents of this vertex buffer with those of another /// diff --git a/src/SFML/Graphics/VertexBuffer.cpp b/src/SFML/Graphics/VertexBuffer.cpp index a83b4bdf6..1ec92d2fc 100644 --- a/src/SFML/Graphics/VertexBuffer.cpp +++ b/src/SFML/Graphics/VertexBuffer.cpp @@ -81,6 +81,18 @@ VertexBuffer::VertexBuffer(PrimitiveType type, Usage usage) : m_primitiveType(ty } +//////////////////////////////////////////////////////////// +VertexBuffer::~VertexBuffer() +{ + if (m_buffer) + { + const TransientContextLock contextLock; + + glCheck(GLEXT_glDeleteBuffers(1, &m_buffer)); + } +} + + //////////////////////////////////////////////////////////// VertexBuffer::VertexBuffer(const VertexBuffer& copy) : GlResource(copy), @@ -102,14 +114,49 @@ m_usage(copy.m_usage) //////////////////////////////////////////////////////////// -VertexBuffer::~VertexBuffer() +VertexBuffer& VertexBuffer::operator=(const VertexBuffer& right) { + VertexBuffer temp(right); + + swap(temp); + + return *this; +} + + +//////////////////////////////////////////////////////////// +VertexBuffer::VertexBuffer(VertexBuffer&& right) noexcept : +m_buffer(std::exchange(right.m_buffer, 0)), +m_size(std::exchange(right.m_size, 0)), +m_primitiveType(std::exchange(right.m_primitiveType, PrimitiveType::Points)), +m_usage(std::exchange(right.m_usage, Usage::Stream)) +{ +} + + +//////////////////////////////////////////////////////////// +VertexBuffer& VertexBuffer::operator=(VertexBuffer&& right) noexcept +{ + // Make sure we aren't moving ourselves + if (&right == this) + { + return *this; + } + if (m_buffer) { const TransientContextLock contextLock; glCheck(GLEXT_glDeleteBuffers(1, &m_buffer)); } + + // Move the contents of right + m_buffer = std::exchange(right.m_buffer, 0); + m_size = std::exchange(right.m_size, 0); + m_primitiveType = std::exchange(right.m_primitiveType, PrimitiveType::Points); + m_usage = std::exchange(right.m_usage, Usage::Stream); + + return *this; } @@ -258,17 +305,6 @@ bool VertexBuffer::update([[maybe_unused]] const VertexBuffer& vertexBuffer) } -//////////////////////////////////////////////////////////// -VertexBuffer& VertexBuffer::operator=(const VertexBuffer& right) -{ - VertexBuffer temp(right); - - swap(temp); - - return *this; -} - - //////////////////////////////////////////////////////////// void VertexBuffer::swap(VertexBuffer& right) noexcept { diff --git a/test/Graphics/VertexBuffer.test.cpp b/test/Graphics/VertexBuffer.test.cpp index 40af922e1..e027979ca 100644 --- a/test/Graphics/VertexBuffer.test.cpp +++ b/test/Graphics/VertexBuffer.test.cpp @@ -16,10 +16,8 @@ TEST_CASE("[Graphics] sf::VertexBuffer", "[.display]") { STATIC_CHECK(std::is_copy_constructible_v); STATIC_CHECK(std::is_copy_assignable_v); - STATIC_CHECK(std::is_move_constructible_v); - STATIC_CHECK(!std::is_nothrow_move_constructible_v); - STATIC_CHECK(std::is_move_assignable_v); - STATIC_CHECK(!std::is_nothrow_move_assignable_v); + STATIC_CHECK(std::is_nothrow_move_constructible_v); + STATIC_CHECK(std::is_nothrow_move_assignable_v); STATIC_CHECK(std::is_nothrow_swappable_v); } @@ -90,6 +88,30 @@ TEST_CASE("[Graphics] sf::VertexBuffer", "[.display]") } } + SECTION("Move semantics") + { + SECTION("Construction") + { + sf::VertexBuffer vertexBuffer(sf::PrimitiveType::LineStrip, sf::VertexBuffer::Usage::Dynamic); + const sf::VertexBuffer vertexBufferCopy(std::move(vertexBuffer)); + CHECK(vertexBufferCopy.getVertexCount() == 0); + CHECK(vertexBufferCopy.getNativeHandle() == 0); + CHECK(vertexBufferCopy.getPrimitiveType() == sf::PrimitiveType::LineStrip); + CHECK(vertexBufferCopy.getUsage() == sf::VertexBuffer::Usage::Dynamic); + } + + SECTION("Assignment") + { + sf::VertexBuffer vertexBuffer(sf::PrimitiveType::LineStrip, sf::VertexBuffer::Usage::Dynamic); + sf::VertexBuffer vertexBufferCopy; + vertexBufferCopy = std::move(vertexBuffer); + CHECK(vertexBufferCopy.getVertexCount() == 0); + CHECK(vertexBufferCopy.getNativeHandle() == 0); + CHECK(vertexBufferCopy.getPrimitiveType() == sf::PrimitiveType::LineStrip); + CHECK(vertexBufferCopy.getUsage() == sf::VertexBuffer::Usage::Dynamic); + } + } + SECTION("create()") { sf::VertexBuffer vertexBuffer;