Skip to content

Commit

Permalink
Fix race condition when rendering the UI (gazebosim#304)
Browse files Browse the repository at this point in the history
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 gazebosim#304

Signed-off-by: Matias N. Goldberg <[email protected]>
  • Loading branch information
darksylinc committed Apr 18, 2021
1 parent 7b9ac81 commit 8d566ab
Show file tree
Hide file tree
Showing 8 changed files with 113 additions and 15 deletions.
7 changes: 6 additions & 1 deletion include/ignition/rendering/Camera.hh
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
4 changes: 4 additions & 0 deletions include/ignition/rendering/RenderTarget.hh
Original file line number Diff line number Diff line change
Expand Up @@ -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 \
Expand Down
11 changes: 11 additions & 0 deletions include/ignition/rendering/base/BaseCamera.hh
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -708,6 +711,14 @@ namespace ignition
return 0u;
}

//////////////////////////////////////////////////
template <class T>
void BaseCamera<T>::SwapFromThread()
{
ignerr << "SwapFromThread is not supported by current render"
<< " engine" << std::endl;
}

//////////////////////////////////////////////////
template <class T>
void BaseCamera<T>::AddRenderPass(const RenderPassPtr &_pass)
Expand Down
9 changes: 9 additions & 0 deletions include/ignition/rendering/base/BaseRenderTarget.hh
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -176,6 +179,12 @@ namespace ignition
}
}

//////////////////////////////////////////////////
template <class T>
void BaseRenderTarget<T>::SwapFromThread()
{
}

//////////////////////////////////////////////////
template <class T>
unsigned int BaseRenderTarget<T>::Width() const
Expand Down
3 changes: 3 additions & 0 deletions ogre2/include/ignition/rendering/ogre2/Ogre2Camera.hh
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down
10 changes: 10 additions & 0 deletions ogre2/include/ignition/rendering/ogre2/Ogre2RenderTarget.hh
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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;

Expand All @@ -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;
};
Expand Down
6 changes: 6 additions & 0 deletions ogre2/src/Ogre2Camera.cc
Original file line number Diff line number Diff line change
Expand Up @@ -207,6 +207,12 @@ unsigned int Ogre2Camera::RenderTextureGLId() const
return rt->GLId();
}

//////////////////////////////////////////////////
void Ogre2Camera::SwapFromThread()
{
this->renderTexture->SwapFromThread();
}

//////////////////////////////////////////////////
void Ogre2Camera::SetSelectionBuffer()
{
Expand Down
78 changes: 64 additions & 14 deletions ogre2/src/Ogre2RenderTarget.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down Expand Up @@ -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<const Ogre::CompositorPassClearDef *>(pass);
const_cast<Ogre::CompositorPassClearDef *>(clearPass)->mColourValue =
this->ogreBackgroundColor;
if (this->ogreCompositorWorkspace)
{
auto nodeSeq = this->ogreCompositorWorkspace->getNodeSequence();
auto pass = nodeSeq[0]->_getPasses()[0]->getDefinition();
auto clearPass = dynamic_cast<const Ogre::CompositorPassClearDef *>(pass);
const_cast<Ogre::CompositorPassClearDef *>(clearPass)->mColourValue =
this->ogreBackgroundColor;
}

if (this->ogreCompositorWorkspaceInDisplay)
{
auto nodeSeq = this->ogreCompositorWorkspaceInDisplay->getNodeSequence();
auto pass = nodeSeq[0]->_getPasses()[0]->getDefinition();
auto clearPass = dynamic_cast<const Ogre::CompositorPassClearDef *>(pass);
const_cast<Ogre::CompositorPassClearDef *>(clearPass)->mColourValue =
this->ogreBackgroundColor;
}

this->colorDirty = false;
}
Expand Down Expand Up @@ -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;
}

//////////////////////////////////////////////////
Expand Down Expand Up @@ -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();
}
Expand All @@ -1134,6 +1169,21 @@ unsigned int Ogre2RenderTexture::GLId() const
return static_cast<unsigned int>(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()
{
Expand Down

0 comments on commit 8d566ab

Please sign in to comment.