From f8844318f992fd41a6946b3a440674364c0b0f18 Mon Sep 17 00:00:00 2001
From: "Matias N. Goldberg" <dark_sylinc@yahoo.com.ar>
Date: Sat, 24 Apr 2021 16:44:23 -0300
Subject: [PATCH] Force serialization of render commands

This avoids us to break ign-rendering ABI while also simplifying the
amount of work to be done

Serializing work is easier to maintain and debug
Only CPU-bound scenario would potentially benefit from parallel command
generation (in terms of UI responsiveness)
Parallel command generation can be added back later

Also fixed coding style

Refer to
https://github.com/ignitionrobotics/ign-rendering/issues/304#issuecomment-824130450
for discussion

Affects ign-rendering#304
---
 src/gui/plugins/scene3d/Scene3D.cc | 34 +++++++++++++++++++++++-------
 src/gui/plugins/scene3d/Scene3D.hh |  6 +++---
 2 files changed, 29 insertions(+), 11 deletions(-)

diff --git a/src/gui/plugins/scene3d/Scene3D.cc b/src/gui/plugins/scene3d/Scene3D.cc
index 5f055973ec9..40df9e853fe 100644
--- a/src/gui/plugins/scene3d/Scene3D.cc
+++ b/src/gui/plugins/scene3d/Scene3D.cc
@@ -449,7 +449,7 @@ using namespace gazebo;
 QList<QThread *> RenderWindowItemPrivate::threads;
 
 /////////////////////////////////////////////////
-void RenderSync::requestQtThreadToBlock(std::unique_lock<std::mutex> &lock)
+void RenderSync::RequestQtThreadToBlock(std::unique_lock<std::mutex> &lock)
 {
   this->renderStallState = RenderStallState::WorkerThreadRequested;
   this->cv.wait(lock, [this]
@@ -457,7 +457,7 @@ void RenderSync::requestQtThreadToBlock(std::unique_lock<std::mutex> &lock)
 }
 
 /////////////////////////////////////////////////
-void RenderSync::releaseQtThreadFromBlock(std::unique_lock<std::mutex> &lock)
+void RenderSync::ReleaseQtThreadFromBlock(std::unique_lock<std::mutex> &lock)
 {
   this->renderStallState = RenderStallState::Unblocked;
   lock.unlock();
@@ -465,7 +465,7 @@ void RenderSync::releaseQtThreadFromBlock(std::unique_lock<std::mutex> &lock)
 }
 
 /////////////////////////////////////////////////
-void RenderSync::waitForWorkerThread()
+void RenderSync::WaitForWorkerThread()
 {
   {
     std::unique_lock<std::mutex> lock(this->mutex);
@@ -526,10 +526,20 @@ void IgnRenderer::Render(RenderSync *renderSync)
 
   IGN_PROFILE_THREAD_NAME("RenderThread");
   IGN_PROFILE("IgnRenderer::Render");
+
+  std::unique_lock<std::mutex> lock(renderSync->mutex);
+  renderSync->RequestQtThreadToBlock(lock);
+
   if (this->textureDirty)
   {
-    std::unique_lock<std::mutex> lock(renderSync->mutex);
-    renderSync->requestQtThreadToBlock(lock);
+    // TODO(anyone) If SwapFromThread gets implemented,
+    // then we only need to lock when texture is dirty
+    // (but we still need to lock the whole routine if
+    // debugging from RenderDoc or if user is not willing
+    // to sacrifice VRAM)
+    //
+    // std::unique_lock<std::mutex> lock(renderSync->mutex);
+    // renderSync->RequestQtThreadToBlock(lock);
     this->dataPtr->camera->SetImageWidth(this->textureSize.width());
     this->dataPtr->camera->SetImageHeight(this->textureSize.height());
     this->dataPtr->camera->SetAspectRatio(this->textureSize.width() /
@@ -540,7 +550,9 @@ void IgnRenderer::Render(RenderSync *renderSync)
       this->dataPtr->camera->PreRender();
     }
     this->textureDirty = false;
-    renderSync->releaseQtThreadFromBlock(lock);
+
+    // TODO(anyone) See SwapFromThread comments
+    // renderSync->ReleaseQtThreadFromBlock(lock);
   }
 
   // texture id could change so get the value in every render update
@@ -899,7 +911,13 @@ void IgnRenderer::Render(RenderSync *renderSync)
   // this notifes ECM to continue updating the scene
   g_renderCv.notify_one();
 
-  this->dataPtr->camera->SwapFromThread();
+  // TODO(anyone) implement a SwapFromThread for parallel command generation
+  // See https://github.com/ignitionrobotics/ign-rendering/issues/304
+  // if( bForcedSerialization )
+  //   this->dataPtr->camera->SwapFromThread();
+  // else
+  //  renderSync->ReleaseQtThreadFromBlock(lock);
+  renderSync->ReleaseQtThreadFromBlock(lock);
 }
 
 /////////////////////////////////////////////////
@@ -2312,7 +2330,7 @@ void TextureNode::NewTexture(uint _id, const QSize &_size)
 /////////////////////////////////////////////////
 void TextureNode::PrepareNode()
 {
-  renderSync.waitForWorkerThread();
+  renderSync.WaitForWorkerThread();
   this->mutex.lock();
   uint newId = this->id;
   QSize sz = this->size;
diff --git a/src/gui/plugins/scene3d/Scene3D.hh b/src/gui/plugins/scene3d/Scene3D.hh
index 9a6025c8c75..575504052f4 100644
--- a/src/gui/plugins/scene3d/Scene3D.hh
+++ b/src/gui/plugins/scene3d/Scene3D.hh
@@ -197,12 +197,12 @@ inline namespace IGNITION_GAZEBO_VERSION_NAMESPACE {
 
     /// \brief Must be called from worker thread when we want to block
     /// \param[in] lock Acquired lock. Must be based on this->mutex
-    public: void requestQtThreadToBlock(std::unique_lock<std::mutex> &lock);
+    public: void RequestQtThreadToBlock(std::unique_lock<std::mutex> &lock);
     /// \brief Must be called from worker thread when we are done
     /// \param[in] lock Acquired lock. Must be based on this->mutex
-    public: void releaseQtThreadFromBlock(std::unique_lock<std::mutex> &lock);
+    public: void ReleaseQtThreadFromBlock(std::unique_lock<std::mutex> &lock);
     /// \brief Must be called from Qt thread periodically
-    public: void waitForWorkerThread();
+    public: void WaitForWorkerThread();
   };
 
   /// \brief Ign-rendering renderer.