From 8d566abd76430f2e341c2da9fcf00d89743c5951 Mon Sep 17 00:00:00 2001 From: "Matias N. Goldberg" Date: Sun, 18 Apr 2021 20:08:30 -0300 Subject: [PATCH] Fix race condition when rendering the UI (#304) This fix also depends on on a fix in ign-gazebo so that it now can call SwapFromThread, otherwise the race condition will still happen. At the current moment, the implementation is very wasteful creating many more RenderTargets than needed (because 2 workspaces are created). This issue will be fixed in the next commit Affects ignitionrobotics/ign-rendering#304 Signed-off-by: Matias N. Goldberg --- include/ignition/rendering/Camera.hh | 7 +- include/ignition/rendering/RenderTarget.hh | 4 + include/ignition/rendering/base/BaseCamera.hh | 11 +++ .../rendering/base/BaseRenderTarget.hh | 9 +++ .../ignition/rendering/ogre2/Ogre2Camera.hh | 3 + .../rendering/ogre2/Ogre2RenderTarget.hh | 10 +++ ogre2/src/Ogre2Camera.cc | 6 ++ ogre2/src/Ogre2RenderTarget.cc | 78 +++++++++++++++---- 8 files changed, 113 insertions(+), 15 deletions(-) diff --git a/include/ignition/rendering/Camera.hh b/include/ignition/rendering/Camera.hh index 65fb55f65..0632c7605 100644 --- a/include/ignition/rendering/Camera.hh +++ b/include/ignition/rendering/Camera.hh @@ -279,9 +279,14 @@ namespace ignition /// \brief Get the OpenGL texture id associated with the render texture /// used by this camera. A valid id is returned only if the underlying /// render engine is OpenGL based. - /// \return Texture Id of type GLuint. + /// \return Texture Id of type GLuint. Returned value is + /// valid until the next SwapFromThread() call public: virtual unsigned int RenderTextureGLId() const = 0; + /// \brief Informs this Camera we're done updating from worker thread + /// and for this iteration of the loop + public: virtual void SwapFromThread() = 0; + /// \brief Add a render pass to the camera /// \param[in] _pass New render pass to add public: virtual void AddRenderPass(const RenderPassPtr &_pass) = 0; diff --git a/include/ignition/rendering/RenderTarget.hh b/include/ignition/rendering/RenderTarget.hh index 8a37987a1..3053be93b 100644 --- a/include/ignition/rendering/RenderTarget.hh +++ b/include/ignition/rendering/RenderTarget.hh @@ -93,6 +93,10 @@ namespace ignition /// \return Render pass at the specified index public: virtual RenderPassPtr RenderPassByIndex(unsigned int _index) const = 0; + + /// \brief Informs this RenderTarget we're done updating from worker + /// thread and for this iteration of the loop + public: virtual void SwapFromThread() = 0; }; /* \class RenderTexture RenderTexture.hh \ diff --git a/include/ignition/rendering/base/BaseCamera.hh b/include/ignition/rendering/base/BaseCamera.hh index 9f28c43db..94c41e5b6 100644 --- a/include/ignition/rendering/base/BaseCamera.hh +++ b/include/ignition/rendering/base/BaseCamera.hh @@ -164,6 +164,9 @@ namespace ignition // Documentation inherited. public: virtual unsigned int RenderTextureGLId() const override; + // Documentation inherited. + public: virtual void SwapFromThread() override; + // Documentation inherited. public: virtual void AddRenderPass(const RenderPassPtr &_pass) override; @@ -708,6 +711,14 @@ namespace ignition return 0u; } + ////////////////////////////////////////////////// + template + void BaseCamera::SwapFromThread() + { + ignerr << "SwapFromThread is not supported by current render" + << " engine" << std::endl; + } + ////////////////////////////////////////////////// template void BaseCamera::AddRenderPass(const RenderPassPtr &_pass) diff --git a/include/ignition/rendering/base/BaseRenderTarget.hh b/include/ignition/rendering/base/BaseRenderTarget.hh index c0c795c79..237bb8d8a 100644 --- a/include/ignition/rendering/base/BaseRenderTarget.hh +++ b/include/ignition/rendering/base/BaseRenderTarget.hh @@ -79,6 +79,9 @@ namespace ignition protected: virtual void RebuildImpl() = 0; + // Documentation inherited. + public: virtual void SwapFromThread() override; + protected: PixelFormat format = PF_UNKNOWN; protected: bool targetDirty = true; @@ -176,6 +179,12 @@ namespace ignition } } + ////////////////////////////////////////////////// + template + void BaseRenderTarget::SwapFromThread() + { + } + ////////////////////////////////////////////////// template unsigned int BaseRenderTarget::Width() const diff --git a/ogre2/include/ignition/rendering/ogre2/Ogre2Camera.hh b/ogre2/include/ignition/rendering/ogre2/Ogre2Camera.hh index 0f891dcdc..b1daee9e3 100644 --- a/ogre2/include/ignition/rendering/ogre2/Ogre2Camera.hh +++ b/ogre2/include/ignition/rendering/ogre2/Ogre2Camera.hh @@ -105,6 +105,9 @@ namespace ignition // Documentation inherited. public: virtual unsigned int RenderTextureGLId() const override; + // Documentation inherited. + public: virtual void SwapFromThread() override; + // Documentation inherited. public: virtual void Destroy() override; diff --git a/ogre2/include/ignition/rendering/ogre2/Ogre2RenderTarget.hh b/ogre2/include/ignition/rendering/ogre2/Ogre2RenderTarget.hh index dba97934f..577a99988 100644 --- a/ogre2/include/ignition/rendering/ogre2/Ogre2RenderTarget.hh +++ b/ogre2/include/ignition/rendering/ogre2/Ogre2RenderTarget.hh @@ -191,6 +191,9 @@ namespace ignition /// into a render target or render texture. protected: Ogre::CompositorWorkspace *ogreCompositorWorkspace = nullptr; + /// \brief See Ogre2RenderTexture::ogreTexture + protected: Ogre::CompositorWorkspace *ogreCompositorWorkspaceInDisplay = nullptr; + /// \brief Ogre's compositor workspace definition name protected: std::string ogreCompositorWorkspaceDefName; @@ -245,6 +248,9 @@ namespace ignition // Documentation inherited public: virtual unsigned int GLId() const override; + // Documentation inherited. + public: virtual void SwapFromThread() override; + // Documentation inherited. public: virtual Ogre::RenderTarget *RenderTarget() const override; @@ -258,8 +264,12 @@ namespace ignition protected: virtual void BuildTarget(); /// \brief Pointer to the internal ogre render texture object + /// This value may get swapped with ogreTextureInDisplay + /// after calling SwapFromThread protected: Ogre::Texture *ogreTexture = nullptr; + protected: Ogre::Texture *ogreTextureInDisplay = nullptr; + /// \brief Make scene our friend so it can create a ogre2 render texture private: friend class Ogre2Scene; }; diff --git a/ogre2/src/Ogre2Camera.cc b/ogre2/src/Ogre2Camera.cc index 1131a5c3b..987346a0f 100644 --- a/ogre2/src/Ogre2Camera.cc +++ b/ogre2/src/Ogre2Camera.cc @@ -207,6 +207,12 @@ unsigned int Ogre2Camera::RenderTextureGLId() const return rt->GLId(); } +////////////////////////////////////////////////// +void Ogre2Camera::SwapFromThread() +{ + this->renderTexture->SwapFromThread(); +} + ////////////////////////////////////////////////// void Ogre2Camera::SetSelectionBuffer() { diff --git a/ogre2/src/Ogre2RenderTarget.cc b/ogre2/src/Ogre2RenderTarget.cc index 91c1247bb..30a889ee6 100644 --- a/ogre2/src/Ogre2RenderTarget.cc +++ b/ogre2/src/Ogre2RenderTarget.cc @@ -280,21 +280,30 @@ void Ogre2RenderTarget::BuildCompositor() ////////////////////////////////////////////////// void Ogre2RenderTarget::DestroyCompositor() { - if (!this->ogreCompositorWorkspace) + if (!this->ogreCompositorWorkspace && !this->ogreCompositorWorkspaceInDisplay) return; auto engine = Ogre2RenderEngine::Instance(); auto ogreRoot = engine->OgreRoot(); Ogre::CompositorManager2 *ogreCompMgr = ogreRoot->getCompositorManager2(); - this->ogreCompositorWorkspace->setListener(nullptr); - ogreCompMgr->removeWorkspace(this->ogreCompositorWorkspace); + if (this->ogreCompositorWorkspace) + { + this->ogreCompositorWorkspace->setListener(nullptr); + ogreCompMgr->removeWorkspace(this->ogreCompositorWorkspace); + } + if (ogreCompositorWorkspaceInDisplay) + { + this->ogreCompositorWorkspaceInDisplay->setListener(nullptr); + ogreCompMgr->removeWorkspace(this->ogreCompositorWorkspaceInDisplay); + } ogreCompMgr->removeWorkspaceDefinition(this->ogreCompositorWorkspaceDefName); ogreCompMgr->removeNodeDefinition(this->ogreCompositorWorkspaceDefName + "/" + this->dataPtr->kBaseNodeName); - ogreCompMgr->removeNodeDefinition(this->ogreCompositorWorkspaceDefName + - "/" + this->dataPtr->kFinalNodeName); + //ogreCompMgr->removeNodeDefinition(this->ogreCompositorWorkspaceDefName + + // "/" + this->dataPtr->kFinalNodeName); this->ogreCompositorWorkspace = nullptr; + this->ogreCompositorWorkspaceInDisplay = nullptr; delete this->dataPtr->rtListener; this->dataPtr->rtListener = nullptr; } @@ -447,11 +456,23 @@ void Ogre2RenderTarget::UpdateBackgroundColor() if (this->colorDirty) { // set background color in compositor clear pass def - auto nodeSeq = this->ogreCompositorWorkspace->getNodeSequence(); - auto pass = nodeSeq[0]->_getPasses()[0]->getDefinition(); - auto clearPass = dynamic_cast(pass); - const_cast(clearPass)->mColourValue = - this->ogreBackgroundColor; + if (this->ogreCompositorWorkspace) + { + auto nodeSeq = this->ogreCompositorWorkspace->getNodeSequence(); + auto pass = nodeSeq[0]->_getPasses()[0]->getDefinition(); + auto clearPass = dynamic_cast(pass); + const_cast(clearPass)->mColourValue = + this->ogreBackgroundColor; + } + + if (this->ogreCompositorWorkspaceInDisplay) + { + auto nodeSeq = this->ogreCompositorWorkspaceInDisplay->getNodeSequence(); + auto pass = nodeSeq[0]->_getPasses()[0]->getDefinition(); + auto clearPass = dynamic_cast(pass); + const_cast(clearPass)->mColourValue = + this->ogreBackgroundColor; + } this->colorDirty = false; } @@ -1074,18 +1095,27 @@ void Ogre2RenderTexture::RebuildTarget() ////////////////////////////////////////////////// void Ogre2RenderTexture::DestroyTarget() { - if (nullptr == this->ogreTexture) + if (nullptr == this->ogreTexture && nullptr == this->ogreTextureInDisplay) return; auto &manager = Ogre::TextureManager::getSingleton(); - manager.unload(this->ogreTexture->getName()); - manager.remove(this->ogreTexture->getName()); + if (this->ogreTexture) + { + manager.unload(this->ogreTexture->getName()); + manager.remove(this->ogreTexture->getName()); + } + if (this->ogreTextureInDisplay) + { + manager.unload(this->ogreTextureInDisplay->getName()); + manager.remove(this->ogreTextureInDisplay->getName()); + } // TODO(anyone) there is memory leak when a render texture is destroyed. // The RenderSystem::_cleanupDepthBuffers method used in ogre1 does not // seem to work in ogre2 this->ogreTexture = nullptr; + this->ogreTextureInDisplay = nullptr; } ////////////////////////////////////////////////// @@ -1115,9 +1145,14 @@ void Ogre2RenderTexture::BuildTarget() ogre2FSAAWarn = true; } } + fsaa = 0; + + std::string texNameSuffix; + if (this->ogreTextureInDisplay) + texNameSuffix += "InDisplay1"; // Ogre 2 PBS expects gamma correction to be enabled - this->ogreTexture = (manager.createManual(this->name, "General", + this->ogreTexture = (manager.createManual(this->name + texNameSuffix, "General", Ogre::TEX_TYPE_2D, this->width, this->height, 0, ogreFormat, Ogre::TU_RENDERTARGET, 0, true, fsaa)).get(); } @@ -1134,6 +1169,21 @@ unsigned int Ogre2RenderTexture::GLId() const return static_cast(texId); } +////////////////////////////////////////////////// +void Ogre2RenderTexture::SwapFromThread() +{ + std::swap( this->ogreCompositorWorkspace, this->ogreCompositorWorkspaceInDisplay ); + std::swap( this->ogreTexture, this->ogreTextureInDisplay ); + + // If this is the first time swapping, we're being aware we're in + // different threads, thus initialize the 2nd pair of resources + if (!this->ogreTexture && this->ogreTextureInDisplay) + { + BuildTarget(); + BuildCompositor(); + } +} + ////////////////////////////////////////////////// void Ogre2RenderTexture::PreRender() {