Replace renderOneFrame for per-workspace update calls (gazebosim#353)

* 🎈 5.1.0 (gazebosim#351)

Signed-off-by: Louise Poubel <louise@openrobotics.org>

* Replace renderOneFrame for per-workspace update calls

Add Scene::PostRenderGpuFlush

Breaks ABI.
Needs changes to other components as well, so they call
Scene::PostRenderGpuFlush

Affects gazebosim#323

Signed-off-by: Matias N. Goldberg <dark_sylinc@yahoo.com.ar>

* Add Scene::SetLegacyAutoGpuFlush

Affects gazebosim#323

Signed-off-by: Matias N. Goldberg <dark_sylinc@yahoo.com.ar>

* Add Scene::SetNumCameraPassesPerGpuFlush

Renamed PostRenderGpuFlush to PostRender
PostRenderGpuFlush was leaking too much internal behavior of how the
engine works into the user.

The new way of forcing the user to pair PreRender and PostRender calls
is much easier to understand and user-friendly

Signed-off-by: Matias N. Goldberg <dark_sylinc@yahoo.com.ar>

* Assert when PreRender/PostRender calls are used incorrectly

Signed-off-by: Matias N. Goldberg <dark_sylinc@yahoo.com.ar>

* Fix warnings generated by gazebo in legacy mode

Signed-off-by: Matias N. Goldberg <dark_sylinc@yahoo.com.ar>

* Add missing listener triggers

Fixes GpuRays/GpuRaysTest.RaysParticles failure
Particle FXs should now be working as intended

Signed-off-by: Matias N. Goldberg <dark_sylinc@yahoo.com.ar>

* Rename functions to be consistent w/ Ignition naming convention

SetNumCameraPassesPerGpuFlush -> SetCameraPassCountPerGpuFlush
GetNumCameraPassesPerGpuFlush -> GetCameraPassCountPerGpuFlush
GetLegacyAutoGpuFlush -> LegacyAutoGpuFlush

Signed-off-by: Matias N. Goldberg <dark_sylinc@yahoo.com.ar>

* Document that SetCameraPassCountPerGpuFlush is an upper bound

Signed-off-by: Matias N. Goldberg <dark_sylinc@yahoo.com.ar>

* Document Camera::Update shouldn't be called if there's many cameras

Signed-off-by: Matias N. Goldberg <dark_sylinc@yahoo.com.ar>

* Check in Render() call that we're inside PreRender / PostRender

Signed-off-by: Matias N. Goldberg <dark_sylinc@yahoo.com.ar>

* Fix INTEGRATION_camera crash

Signed-off-by: Matias N. Goldberg <dark_sylinc@yahoo.com.ar>

* Fix EndFrame not getting called when in non-legacy mode

INTEGRATION_depth_camera now succeeds

Signed-off-by: Matias N. Goldberg <dark_sylinc@yahoo.com.ar>

* Fix wrong documentation about Camera::Capture

Signed-off-by: Matias N. Goldberg <dark_sylinc@yahoo.com.ar>

* Update Migration notes w/ Scene::PostRender

Signed-off-by: Matias N. Goldberg <dark_sylinc@yahoo.com.ar>

* Add missing call in doc's recomendations

Signed-off-by: Matias N. Goldberg <dark_sylinc@yahoo.com.ar>

* Improve documentation and make asserts more helpful

Signed-off-by: Matias N. Goldberg <dark_sylinc@yahoo.com.ar>

* Default cameraPassCountPerGpuFlush until all modules are merged

Fix wrong documentation about currNumCameraPasses

Signed-off-by: Matias N. Goldberg <dark_sylinc@yahoo.com.ar>

* Remove unnecessary headers

Signed-off-by: Matias N. Goldberg <dark_sylinc@yahoo.com.ar>

* Style changes for consistency with the rest of the codebase

Better document code in FlushGpuCommandsOnly

Signed-off-by: Matias N. Goldberg <dark_sylinc@yahoo.com.ar>

* Multiple fixes

- Do not recommend the user to call Camera::PreRender in docs. This is
done implicitly via Scene::PreRender
- Rename GetCameraPassCountPerGpuFlush -> CameraPassCountPerGpuFlush
- CameraPassCountPerGpuFlush was incorrectly always returning 0
- Grammar mistakes in comments

Signed-off-by: Matias N. Goldberg <dark_sylinc@yahoo.com.ar>

* Make cameraPassCountPerGpuFlush default to 6

Signed-off-by: Matias N. Goldberg <dark_sylinc@yahoo.com.ar>

Co-authored-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: William Lew <WilliamMilesLew@gmail.com>

Migration.md

@@ -5,6 +5,32 @@ Deprecated code produces compile-time warnings. These warning serve as
 notification to users that their code should be upgraded. The next major
 release will remove the deprecated code.
 
+## Ignition Rendering 5.x to 6.x
+
+### Modifications
+
+1. **Scene.hh**
+    + Added `Scene::PostRender`. The function `Camera::Render` must be executed
+      between calls to `Scene::PreRender` and `Scene::PostRender`. Failure to do
+      so will result in asserts triggering informing users to correct their code.
+      Alternatively calling `Scene::SetCameraPassCountPerGpuFlush( 0 )` avoids
+      this strict requirement.
+      Users handling only one Camera can call `Camera::Update` or `Camera::Capture`
+      and thus do not need to worry.
+      However for more than one camera (of any type) the optimum way to handle them is to update them in the following pattern:
+      ```
+      scene->PreRender();
+      for( auto& camera in cameras )
+          camera->Render();
+      for( auto& camera in cameras )
+          camera->PostRender();
+      scene->PostRender();
+      ```
+      This pattern maximizes the chances of improving performance.
+      *Note*: Calling instead `Camera::Update` for each camera is a waste of CPU resources.
+    + It is invalid to modify the scene between `Scene::PreRender` and `Scene::PostRender` (e.g. add/remove objects, lights, etc)
+    + Added `Scene::SetCameraPassCountPerGpuFlush`. Setting this value to 0 forces legacy behavior which eases porting.
+    + Systems that rely on Graphics components like particle FXs and postprocessing are explicitly affected by Scene's Pre/PostRender. Once `Scene::PostRender` is called, the particle FXs' simulation is moved forward, as well as time values sent to postprocessing shaders. In previous ign-rendering versions each `Camera::Render` call would move the particle simulation forward, which could cause subtle bugs or inconsistencies when Cameras were rendering the same frame from different angles. Setting SetCameraPassCountPerGpuFlush to 0 will also cause these subtle bugs to reappear.
 
 ## Ignition Rendering 4.0 to 4.1
 

include/ignition/rendering/Camera.hh

@@ -143,8 +143,8 @@ namespace ignition
       /// \brief Renders a new frame.
       /// This is a convenience function for single-camera scenes. It wraps the
       /// pre-render, render, and post-render into a single
-      /// function. This should be used in applications with multiple cameras
-      /// or multiple consumers of a single camera's images.
+      /// function. This should NOT be used in applications with multiple
+      /// cameras or multiple consumers of a single camera's images.
       public: virtual void Update() = 0;
 
       /// \brief Created an empty image buffer for capturing images. The
@@ -157,8 +157,8 @@ namespace ignition
       /// \brief Renders a new frame and writes the results to the given image.
       /// This is a convenience function for single-camera scenes. It wraps the
       /// pre-render, render, post-render, and get-image calls into a single
-      /// function. This should be used in applications with multiple cameras
-      /// or multiple consumers of a single camera's images.
+      /// function. This should NOT be used in applications with multiple
+      /// cameras or multiple consumers of a single camera's images.
       /// \param[out] _image Output image buffer
       public: virtual void Capture(Image &_image) = 0;
 

include/ignition/rendering/Scene.hh

@@ -1019,6 +1019,120 @@ namespace ignition
       /// changes by traversing scene-graph, calling PreRender on all objects
       public: virtual void PreRender() = 0;
 
+      /// \brief Call this function after you're done updating ALL cameras
+      /// \remark Each PreRender must have a correspondent PostRender
+      /// \remark Particle FX simulation is moved forward after this call
+      ///
+      /// \see Scene::SetCameraPassCountPerGpuFlush
+      public: virtual void PostRender() = 0;
+
+      /// \brief
+      /// The ideal render loop is as follows:
+      ///
+      /// \code
+      ///   scene->PreRender();
+      ///   for( auto& camera in cameras )
+      ///     camera->Render();
+      ///   for( auto& camera in cameras )
+      ///     camera->PostRender();
+      ///   scene->PostRender();
+      /// \endcode
+      ///
+      /// Now... Camera Render calls MUST happen between Scene::PreRender and
+      /// Scene::PostRender.
+      ///
+      /// The scene must not be modified (e.g. add/remove objects, lights, etc)
+      /// while inside Scene PreRender/PostRender
+      ///
+      /// # Legacy mode: Set this value to 0.
+      ///
+      /// Old projects migrating to newer ign versions may break
+      /// these rules (e.g. not calling Render between Scene's
+      /// Pre/PostRender).
+      ///
+      /// Setting this value to 0 forces Gazebo to flush commands for
+      /// every camera; thus avoiding the need to call PostRender at all
+      ///
+      /// This is much slower but will ease porting, specially
+      /// if it's not easy to adapt your code to call PostRender for some
+      /// reason (in non-legacy mode each call *must* correspond to a
+      /// previous PreRender call)
+      ///
+      /// Legacy mode forces Particle FX simulations to move forward
+      /// after each camera render, which can cause inconsistencies
+      /// when Cameras are supposed to be rendering the same frame from
+      /// different angles
+      ///
+      /// # New mode i.e. values greater than 0:
+      ///
+      /// The CPU normally queues up of rendering commands from each Camera and
+      /// then waits for the GPU to finish up.
+      ///
+      ///   1. If we flush too often, the CPU will often have to wait for
+      ///      the GPU to finish.
+      ///   2. If we flush infrequently, RAM consumption will rise due to
+      ///      queueing up too much unsubmitted work.
+      ///
+      /// Larger values values queue up more work; lower values flush more
+      /// frequently.
+      ///
+      /// Note that work may be submitted earlier if required by a specific
+      /// operation (e.g. reading GPU -> CPU)
+      ///
+      /// A sensible value in the range of [2; 6] is probably the best
+      /// ratio between parallel performance / RAM cost.
+      ///
+      /// Actual value depends on scene complexity and number of shadow
+      /// casting lights
+      ///
+      /// If you're too tight on RAM consumption, try setting this value to 1.
+      ///
+      /// ## Example:
+      ///
+      /// Cubemap rendering w/ 3 probes and 5 shadowmaps can cause
+      /// a blow up of passes:
+      ///
+      /// (5 shadow maps per face + 1 regular render) x 6 faces x 3 probes =
+      /// 108 render_scene passes.
+      /// 108 is way too much, causing out of memory situations;
+      ///
+      /// so setting the value to 6 (1 cubemap face = 1 pass) will
+      /// force one flush per cubemap face, flushing a total of 3 times
+      /// (one per cubemap).
+      ///
+      /// ## Upper bound
+      ///
+      /// Once Scene::PostRender is called, a flush is always forced.
+      ///
+      /// If you set a value of e.g. 6, but you have a single camera, it
+      /// will be flushed after Scene::PostRender, thus having a value of 1 or
+      /// 6 won't matter as the result will be exactly the same (in every term:
+      /// performance, memory consumption)
+      ///
+      /// A value of 6 is like an upper bound.
+      /// We may queue _up to_ 6 render passes or less; but never more.
+      ///
+      /// \remarks Not all rendering engines care about this.
+      /// ogre2 plugin does.
+      ///
+      /// \param[in] _numPass 0 for old projects who can't or don't know
+      /// when to call PostRender and prefer to penalize rendering
+      /// performance
+      /// Value in range [1; 255]
+      public: virtual void SetCameraPassCountPerGpuFlush(uint8_t _numPass) = 0;
+
+      /// \brief Returns the value set in SetCameraPassCountPerGpuFlush
+      /// \return Value in range [0; 255].
+      /// ALWAYS returns 0 for plugins that ignore
+      /// SetCameraPassCountPerGpuFlush
+      public: virtual uint8_t CameraPassCountPerGpuFlush() const = 0;
+
+      /// \brief Checks if SetCameraPassCountPerGpuFlush is 0
+      /// \return True if Gazebo is using the old method (i.e. 0).
+      /// ALWAYS returns true for plugins that ignore
+      /// SetCameraPassCountPerGpuFlush
+      public: virtual bool LegacyAutoGpuFlush() const = 0;
+
       /// \brief Remove and destroy all objects from the scene graph. This does
       /// not completely destroy scene resources, so new objects can be created
       /// and added to the scene afterwards.

include/ignition/rendering/base/BaseCamera.hh

@@ -388,6 +388,10 @@ namespace ignition
       this->Scene()->PreRender();
       this->Render();
       this->PostRender();
+      if (!this->Scene()->LegacyAutoGpuFlush())
+      {
+        this->Scene()->PostRender();
+      }
     }
 
     //////////////////////////////////////////////////

include/ignition/rendering/base/BaseScene.hh

@@ -481,6 +481,19 @@ namespace ignition
 
       public: virtual void Destroy() override;
 
+      // Documentation inherited.
+      public: virtual void PostRender() override;
+
+      // Documentation inherited.
+      public: virtual void SetCameraPassCountPerGpuFlush(
+            uint8_t _numPass) override;
+
+      // Documentation inherited.
+      public: virtual uint8_t CameraPassCountPerGpuFlush() const override;
+
+      // Documentation inherited.
+      public: virtual bool LegacyAutoGpuFlush() const override;
+
       protected: virtual unsigned int CreateObjectId();
 
       protected: virtual std::string CreateObjectName(unsigned int _id,

ogre2/include/ignition/rendering/ogre2/Ogre2Scene.hh

@@ -94,11 +94,81 @@ namespace ignition
       // Documentation inherited
       public: virtual bool SkyEnabled() const override;
 
+      // Documentation inherited.
+      public: virtual void SetCameraPassCountPerGpuFlush(
+            uint8_t _numPass) override;
+
+      // Documentation inherited.
+      public: virtual uint8_t CameraPassCountPerGpuFlush() const override;
+
+      // Documentation inherited.
+      public: virtual bool LegacyAutoGpuFlush() const override;
+
       /// \brief Get a pointer to the ogre scene manager
       /// \return Pointer to the ogre scene manager
       public: virtual Ogre::SceneManager *OgreSceneManager() const;
 
+      // Documentation inherited
+      public: virtual void PostRender() override;
+
       /// \cond PRIVATE
+      /// \brief Certain functions like Ogre2Camera::VisualAt would
+      /// need to call PreRender and PostFrame, which is very unintuitive
+      /// and user-hostile.
+      ///
+      /// More over, it's likely that we don't want to advance the frame
+      /// in those cases (e.g. particle FXs should not advance), but we
+      /// still have to initialize and cleanup Ogre once we're done.
+      ///
+      /// This function performs some PreRender steps but only if we're
+      /// not already inside PreRender/PostRender, necessary for rendering
+      /// Ogre2Camera::VisualAt (via Ogre2SelectionBuffer)
+      public: void StartForcedRender();
+
+      /// \brief Opposite of StartForcedRender
+      ///
+      /// This function performs some PostRender steps but only if we're
+      /// not already inside PreRender/PostRender pairs
+      public: void EndForcedRender();
+
+      /// \internal
+      /// \brief When LegacyAutoGpuFlush(), this function mimics
+      /// legacy behavior.
+      /// When not, it verifies PreRender has been called
+      public: void StartRendering();
+
+      /// \internal
+      /// \brief Every Render() function calls this function with
+      /// the number of pass_scene passes it just performed, so
+      /// that we decide if we should flush or not (based on
+      /// SetCameraPassCountPerGpuFlush)
+      ///
+      /// \param[in] _numPasses Number of pass_scene passes just performed
+      /// (excluding shadow nodes', otherwise it becomes too unpredictable)
+      /// \param[in] _startNewFrame whether we ignore
+      /// SetCameraPassCountPerGpuFlush.
+      /// Only PostRender should set this to true.
+      public: void FlushGpuCommandsAndStartNewFrame(uint8_t _numPasses,
+                                                    bool _startNewFrame);
+
+      /// \internal
+      /// \brief Performs actual flushing to GPU
+      protected: void FlushGpuCommandsOnly();
+
+      /// \internal
+      /// \brief Ends the frame, i.e. PostRender wants to do this.
+      ///
+      /// Ogre::SceneManager::updateSceneGraph can't be called again until
+      /// this function is called
+      ///
+      /// After calling this function again,
+      /// Ogre::SceneManager::updateSceneGraph must be called before
+      /// rendering anything (i.e. done inside PreRender)
+      ///
+      /// This is why every PreRender should be paired with a PostRender
+      /// call when in LegacyAutoGpuFlush == false
+      protected: void EndFrame();
+
       /// \internal
       /// \brief Mark shadows dirty to rebuild compostior shadow node
       /// This is set when the number of shadow casting lighst changes

ogre2/src/Ogre2DepthCamera.cc

@@ -940,11 +940,19 @@ void Ogre2DepthCamera::CreateWorkspaceInstance()
 //////////////////////////////////////////////////
 void Ogre2DepthCamera::Render()
 {
+  this->scene->StartRendering();
+
   // update the compositors
-  this->dataPtr->ogreCompositorWorkspace->setEnabled(true);
-  auto engine = Ogre2RenderEngine::Instance();
-  engine->OgreRoot()->renderOneFrame();
-  this->dataPtr->ogreCompositorWorkspace->setEnabled(false);
+  this->dataPtr->ogreCompositorWorkspace->_validateFinalTarget();
+  this->dataPtr->ogreCompositorWorkspace->_beginUpdate(false);
+  this->dataPtr->ogreCompositorWorkspace->_update();
+  this->dataPtr->ogreCompositorWorkspace->_endUpdate(false);
+
+  Ogre::vector<Ogre::RenderTarget*>::type swappedTargets;
+  swappedTargets.reserve(2u);
+  this->dataPtr->ogreCompositorWorkspace->_swapFinalTarget(swappedTargets);
+
+  this->scene->FlushGpuCommandsAndStartNewFrame(1u, false);
 }
 
 //////////////////////////////////////////////////

ogre2/src/Ogre2GpuRays.cc

@@ -1089,29 +1089,49 @@ void Ogre2GpuRays::CreateGpuRaysTextures()
 /////////////////////////////////////////////////
 void Ogre2GpuRays::UpdateRenderTarget1stPass()
 {
+  Ogre::vector<Ogre::RenderTarget*>::type swappedTargets;
+  swappedTargets.reserve(2u);
+
   // update the compositors
   for (auto i : this->dataPtr->cubeFaceIdx)
+  {
     this->dataPtr->ogreCompositorWorkspace1st[i]->setEnabled(true);
-  auto engine = Ogre2RenderEngine::Instance();
-  engine->OgreRoot()->renderOneFrame();
-  for (auto i : this->dataPtr->cubeFaceIdx)
+
+    this->dataPtr->ogreCompositorWorkspace1st[i]->_validateFinalTarget();
+    this->dataPtr->ogreCompositorWorkspace1st[i]->_beginUpdate(false);
+    this->dataPtr->ogreCompositorWorkspace1st[i]->_update();
+    this->dataPtr->ogreCompositorWorkspace1st[i]->_endUpdate(false);
+
+    swappedTargets.clear();
+    this->dataPtr->ogreCompositorWorkspace1st[i]->_swapFinalTarget(
+          swappedTargets );
+
     this->dataPtr->ogreCompositorWorkspace1st[i]->setEnabled(false);
+  }
 }
 
 /////////////////////////////////////////////////
 void Ogre2GpuRays::UpdateRenderTarget2ndPass()
 {
-  this->dataPtr->ogreCompositorWorkspace2nd->setEnabled(true);
-  auto engine = Ogre2RenderEngine::Instance();
-  engine->OgreRoot()->renderOneFrame();
-  this->dataPtr->ogreCompositorWorkspace2nd->setEnabled(false);
+  this->dataPtr->ogreCompositorWorkspace2nd->_validateFinalTarget();
+  this->dataPtr->ogreCompositorWorkspace2nd->_beginUpdate(false);
+  this->dataPtr->ogreCompositorWorkspace2nd->_update();
+  this->dataPtr->ogreCompositorWorkspace2nd->_endUpdate(false);
+
+  Ogre::vector<Ogre::RenderTarget*>::type swappedTargets;
+  swappedTargets.reserve(2u);
+  this->dataPtr->ogreCompositorWorkspace2nd->_swapFinalTarget(swappedTargets);
 }
 
 //////////////////////////////////////////////////
 void Ogre2GpuRays::Render()
 {
+  this->scene->StartRendering();
+
   this->UpdateRenderTarget1stPass();
   this->UpdateRenderTarget2ndPass();
+
+  this->scene->FlushGpuCommandsAndStartNewFrame(6u, false);
 }
 
 //////////////////////////////////////////////////