From 59689a3530121f630c2217cb698c1255d8ed0ad0 Mon Sep 17 00:00:00 2001 From: Benjamin Kloster Date: Fri, 17 May 2013 10:15:57 +0200 Subject: [PATCH 1/3] Add convenience "hasChanges" flag to SharedData This relieves systems of the burden to remember the last time they updated their internal state. They can now just query the hasChanges() method. When they are done, they should call untouch() to reset the flag. --- src/engine/shared_data.h | 62 ++++++++++++++++++++------- src/engine/tests/shared_data.cpp | 72 ++++++++++++++++++++++++++++++++ 2 files changed, 119 insertions(+), 15 deletions(-) diff --git a/src/engine/shared_data.h b/src/engine/shared_data.h index cbd81720757..2c8aba96dd6 100644 --- a/src/engine/shared_data.h +++ b/src/engine/shared_data.h @@ -139,8 +139,6 @@ namespace thrive { * */ -using DataVersion = unsigned int; - /** * @brief Enumeration naming the three state buffers */ @@ -533,6 +531,18 @@ class SharedData : public detail::SharedDataBase { */ SharedData& operator= (const SharedData&) = delete; + /** + * @brief Whether the stable buffer is outdated + * + * @return + \c true if there have been changes to the current stable buffer since + the last call to untouch(), \c false otherwise + */ + bool + hasChanges() const { + return m_lastUntouch < this->getLastBufferChange(StateBuffer::Stable); + } + /** * @brief Returns the latest data buffer */ @@ -550,21 +560,22 @@ class SharedData : public detail::SharedDataBase { } /** - * @brief Returns the last frame the stable buffer was changed + * @brief Marks the working copy as changed */ - DataVersion - stableVersion() const { + void + touch() { State& state = State::instance(); - short bufferIndex = state.getBufferIndex(StateBuffer::Stable); - return m_bufferVersions[bufferIndex]; + // +1 because we are rendering the *next* frame + m_lastTouch = state.getBufferFrame(StateBuffer::WorkingCopy) + 1; + this->setLastBufferChange(StateBuffer::WorkingCopy, m_lastTouch); } /** - * @brief Marks the working copy as changed + * @brief Resets the hasChanges() flag */ void - touch() { - m_touchedVersion += 1; + untouch() { + m_lastUntouch = this->getLastBufferChange(StateBuffer::Stable); } /** @@ -577,13 +588,12 @@ class SharedData : public detail::SharedDataBase { updateBuffer( short bufferIndex ) override { - if (m_bufferVersions[bufferIndex] < m_touchedVersion) { + if (m_lastBufferChanges[bufferIndex] < m_lastTouch) { m_buffers[bufferIndex] = this->latest(); - m_bufferVersions[bufferIndex] = m_touchedVersion; + m_lastBufferChanges[bufferIndex] = m_lastTouch; } } - /** * @brief Returns the working copy data buffer */ @@ -612,11 +622,33 @@ class SharedData : public detail::SharedDataBase { return m_buffers[bufferIndex]; } - DataVersion m_touchedVersion = 0; + FrameIndex + getLastBufferChange( + StateBuffer buffer + ) const { + State& state = State::instance(); + short bufferIndex = state.getBufferIndex(buffer); + return m_lastBufferChanges[bufferIndex]; + } + + void + setLastBufferChange( + StateBuffer buffer, + FrameIndex lastChange + ) { + State& state = State::instance(); + short bufferIndex = state.getBufferIndex(buffer); + m_lastBufferChanges[bufferIndex] = lastChange; + } std::array m_buffers; - std::array m_bufferVersions = {{0, 0, 0}}; + std::array m_lastBufferChanges = {{0, 0, 0}}; + + FrameIndex m_lastTouch = 0; + + FrameIndex m_lastUntouch = 0; + }; diff --git a/src/engine/tests/shared_data.cpp b/src/engine/tests/shared_data.cpp index 4edb4e3d90b..e0793773c20 100644 --- a/src/engine/tests/shared_data.cpp +++ b/src/engine/tests/shared_data.cpp @@ -123,6 +123,78 @@ TEST(SharedData, UpdateWorkingCopy) { } +TEST(SharedData, Untouch) { + State& state = State::instance(); + state.reset(); + RenderData data(1); + // Touch + state.lockWorkingCopy(); + data.touch(); + state.releaseWorkingCopy(); + // Lock stable, should have changes + state.lockStable(); + EXPECT_TRUE(data.hasChanges()); + state.releaseStable(); + // Relock stable, should still have changes + state.lockStable(); + EXPECT_TRUE(data.hasChanges()); + // Untouch, no changes anymore + data.untouch(); + EXPECT_FALSE(data.hasChanges()); + state.releaseStable(); + // Relocking, still no changes + state.lockStable(); + EXPECT_FALSE(data.hasChanges()); + state.releaseStable(); + state.reset(); +} + + +TEST(SharedData, TouchDuringUntouch) { + State& state = State::instance(); + state.reset(); + RenderData data(1); + // Touch + state.lockWorkingCopy(); + data.touch(); + state.releaseWorkingCopy(); + // Untouch (no release) + state.lockStable(); + data.untouch(); + EXPECT_FALSE(data.hasChanges()); + // Touch again + state.lockWorkingCopy(); + data.touch(); + state.releaseWorkingCopy(); + // Should still have no changes + EXPECT_FALSE(data.hasChanges()); + state.releaseStable(); + state.reset(); +} + + +TEST(SharedData, UntouchDuringTouch) { + State& state = State::instance(); + state.reset(); + RenderData data(1); + // Touch (no release) + state.lockWorkingCopy(); + data.touch(); + // Untouch + state.lockStable(); + EXPECT_FALSE(data.hasChanges()); + data.untouch(); + state.releaseStable(); + // Release working copy + state.releaseWorkingCopy(); + // Now it should have changes + state.lockStable(); + EXPECT_TRUE(data.hasChanges()); + state.releaseStable(); + state.reset(); +} + + TEST(SharedData, DataTransfer) { State& state = State::instance(); state.reset(); From d12de6bccd950028e7c456b4830014b9f9d62b32 Mon Sep 17 00:00:00 2001 From: Benjamin Kloster Date: Fri, 17 May 2013 10:22:20 +0200 Subject: [PATCH 2/3] Update current systems to make use of SharedData::hasChanges() --- src/ogre/light_system.cpp | 4 ++++ src/ogre/scene_node_system.cpp | 24 ++++++++++++++---------- src/ogre/sky_system.cpp | 3 ++- 3 files changed, 20 insertions(+), 11 deletions(-) diff --git a/src/ogre/light_system.cpp b/src/ogre/light_system.cpp index 07d7b74f591..e69e1096f8b 100644 --- a/src/ogre/light_system.cpp +++ b/src/ogre/light_system.cpp @@ -147,6 +147,9 @@ OgreLightSystem::update(int) { } for (const auto& value : m_impl->m_entities) { OgreLightComponent* lightComponent = std::get<0>(value.second); + if (not lightComponent->m_properties.hasChanges()) { + continue; + } Ogre::Light* light = lightComponent->m_light; const auto& properties = lightComponent->m_properties.stable(); light->setType(properties.type); @@ -164,6 +167,7 @@ OgreLightSystem::update(int) { properties.spotlightFalloff ); light->setSpotlightNearClipDistance(properties.spotlightNearClipDistance); + lightComponent->m_properties.untouch(); } for (EntityId entityId : m_impl->m_entities.removedEntities()) { Ogre::Light* light = m_impl->m_lights[entityId]; diff --git a/src/ogre/scene_node_system.cpp b/src/ogre/scene_node_system.cpp index 9c557304797..a5caad22b88 100644 --- a/src/ogre/scene_node_system.cpp +++ b/src/ogre/scene_node_system.cpp @@ -185,17 +185,21 @@ OgreUpdateSceneNodeSystem::shutdown() { void OgreUpdateSceneNodeSystem::update(int) { for (const auto& entry : m_impl->m_entities) { - OgreSceneNodeComponent* sceneNodeComponent = std::get<0>(entry.second); TransformComponent* transformComponent = std::get<1>(entry.second); - sceneNodeComponent->m_sceneNode->setOrientation( - transformComponent->m_properties.stable().orientation - ); - sceneNodeComponent->m_sceneNode->setPosition( - transformComponent->m_properties.stable().position - ); - sceneNodeComponent->m_sceneNode->setScale( - transformComponent->m_properties.stable().scale - ); + if (transformComponent->m_properties.hasChanges()) { + Ogre::SceneNode* sceneNode = std::get<0>(entry.second)->m_sceneNode; + const auto& transformProperties = transformComponent->m_properties.stable(); + sceneNode->setOrientation( + transformProperties.orientation + ); + sceneNode->setPosition( + transformProperties.position + ); + sceneNode->setScale( + transformProperties.scale + ); + transformComponent->m_properties.untouch(); + } } } diff --git a/src/ogre/sky_system.cpp b/src/ogre/sky_system.cpp index d7a4110c045..d7713b43a95 100644 --- a/src/ogre/sky_system.cpp +++ b/src/ogre/sky_system.cpp @@ -118,7 +118,7 @@ void SkySystem::update(int) { for (auto& value : m_impl->m_skyEntities) { SkyPlaneComponent* plane = std::get<0>(value.second); - if (plane) { + if (plane and plane->m_properties.hasChanges()) { const SkyPlaneComponent::Properties& properties = plane->m_properties.stable(); m_impl->m_sceneManager->setSkyPlane( properties.enabled, @@ -132,6 +132,7 @@ SkySystem::update(int) { properties.ysegments, properties.groupName ); + plane->m_properties.untouch(); } } } From e91d554912029f5ae05997cc2aba58441e33ead8 Mon Sep 17 00:00:00 2001 From: Benjamin Kloster Date: Fri, 17 May 2013 10:33:33 +0200 Subject: [PATCH 3/3] Clarify documentation of SharedData::touch() et. al. --- src/engine/shared_data.h | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/src/engine/shared_data.h b/src/engine/shared_data.h index 2c8aba96dd6..983cc01d93a 100644 --- a/src/engine/shared_data.h +++ b/src/engine/shared_data.h @@ -108,6 +108,14 @@ namespace thrive { * obj.m_sharedProperties.touch(); * \endcode * + * @subsection touch() and untouch() + * + * When the writing thread has modified the data, it should call + * SharedData::touch() to mark the buffer as changed. The reading thread can + * call SharedData::hasChanges() to query whether the current stable buffer + * has any changes. Once it has processed these changes, it should call + * SharedData::untouch() to mark them as such. + * * @section shared_data_lua SharedData in Lua * * How you can access shared data from Lua depends on whether the script @@ -535,8 +543,8 @@ class SharedData : public detail::SharedDataBase { * @brief Whether the stable buffer is outdated * * @return - \c true if there have been changes to the current stable buffer since - the last call to untouch(), \c false otherwise + * \c true if there have been changes to the current stable buffer since + * the last call to untouch(), \c false otherwise */ bool hasChanges() const { @@ -561,17 +569,21 @@ class SharedData : public detail::SharedDataBase { /** * @brief Marks the working copy as changed + * + * Only the writing thread should call this */ void touch() { State& state = State::instance(); - // +1 because we are rendering the *next* frame + // +1 because we are currently rendering the *next* frame m_lastTouch = state.getBufferFrame(StateBuffer::WorkingCopy) + 1; this->setLastBufferChange(StateBuffer::WorkingCopy, m_lastTouch); } /** * @brief Resets the hasChanges() flag + * + * Only the reading thread should call this. */ void untouch() {