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

* 🎈 5.1.0 (#351) Signed-off-by: Louise Poubel <[email protected]> * 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 #323 Signed-off-by: Matias N. Goldberg <[email protected]> * Add Scene::SetLegacyAutoGpuFlush Affects #323 Signed-off-by: Matias N. Goldberg <[email protected]> * 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 <[email protected]> * Assert when PreRender/PostRender calls are used incorrectly Signed-off-by: Matias N. Goldberg <[email protected]> * Fix warnings generated by gazebo in legacy mode Signed-off-by: Matias N. Goldberg <[email protected]> * Add missing listener triggers Fixes GpuRays/GpuRaysTest.RaysParticles failure Particle FXs should now be working as intended Signed-off-by: Matias N. Goldberg <[email protected]> * Rename functions to be consistent w/ Ignition naming convention SetNumCameraPassesPerGpuFlush -> SetCameraPassCountPerGpuFlush GetNumCameraPassesPerGpuFlush -> GetCameraPassCountPerGpuFlush GetLegacyAutoGpuFlush -> LegacyAutoGpuFlush Signed-off-by: Matias N. Goldberg <[email protected]> * Document that SetCameraPassCountPerGpuFlush is an upper bound Signed-off-by: Matias N. Goldberg <[email protected]> * Document Camera::Update shouldn't be called if there's many cameras Signed-off-by: Matias N. Goldberg <[email protected]> * Check in Render() call that we're inside PreRender / PostRender Signed-off-by: Matias N. Goldberg <[email protected]> * Fix INTEGRATION_camera crash Signed-off-by: Matias N. Goldberg <[email protected]> * Fix EndFrame not getting called when in non-legacy mode INTEGRATION_depth_camera now succeeds Signed-off-by: Matias N. Goldberg <[email protected]> * Fix wrong documentation about Camera::Capture Signed-off-by: Matias N. Goldberg <[email protected]> * Update Migration notes w/ Scene::PostRender Signed-off-by: Matias N. Goldberg <[email protected]> * Add missing call in doc's recomendations Signed-off-by: Matias N. Goldberg <[email protected]> * Improve documentation and make asserts more helpful Signed-off-by: Matias N. Goldberg <[email protected]> * Default cameraPassCountPerGpuFlush until all modules are merged Fix wrong documentation about currNumCameraPasses Signed-off-by: Matias N. Goldberg <[email protected]> * Remove unnecessary headers Signed-off-by: Matias N. Goldberg <[email protected]> * Style changes for consistency with the rest of the codebase Better document code in FlushGpuCommandsOnly Signed-off-by: Matias N. Goldberg <[email protected]> * 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 <[email protected]> * Make cameraPassCountPerGpuFlush default to 6 Signed-off-by: Matias N. Goldberg <[email protected]> Co-authored-by: Louise Poubel <[email protected]>
gazebosim · Jul 19, 2021 · b7d7d63 · b7d7d63
1 parent f3f75a5
commit b7d7d63
Show file tree

Hide file tree

Showing 14 changed files with 603 additions and 50 deletions.
diff --git a/Changelog.md b/Changelog.md
@@ -8,6 +8,89 @@
 
 ### Ignition Rendering 5.X.X (20XX-XX-XX)
 
+### Ignition Rendering 5.1.0 (2021-06-22)
+
+1. add ifdef for apple in integration test
+    * [Pull request #349](https://github.com/ignitionrobotics/ign-rendering/pull/349)
+
+1. Update light map tutorial
+    * [Pull request #346](https://github.com/ignitionrobotics/ign-rendering/pull/346)
+
+1. relax gaussian test tolerance
+    * [Pull request #344](https://github.com/ignitionrobotics/ign-rendering/pull/344)
+
+1. Fix custom shaders uniforms ign version number
+    * [Pull request #343](https://github.com/ignitionrobotics/ign-rendering/pull/343)
+
+1. recreate node only when needed
+    * [Pull request #342](https://github.com/ignitionrobotics/ign-rendering/pull/342)
+
+1. Backport memory fixes found by ASAN
+    * [Pull request #340](https://github.com/ignitionrobotics/ign-rendering/pull/340)
+
+1. Fix FSAA in UI and lower VRAM consumption
+    * [Pull request #313](https://github.com/ignitionrobotics/ign-rendering/pull/313)
+
+1. Fix depth alpha
+    * [Pull request #316](https://github.com/ignitionrobotics/ign-rendering/pull/316)
+
+1. Fix floating point precision bug handling alpha channel (#332)
+    * [Pull request #333](https://github.com/ignitionrobotics/ign-rendering/pull/333)
+
+1. Fix heap overflow when reading
+    * [Pull request #337](https://github.com/ignitionrobotics/ign-rendering/pull/337)
+
+1. Fix new [] / delete mismatch
+    * [Pull request #338](https://github.com/ignitionrobotics/ign-rendering/pull/338)
+
+1. Test re-enabling depth camera integration test on mac
+    * [Pull request #335](https://github.com/ignitionrobotics/ign-rendering/pull/335)
+
+1. Include MoveTo Helper class to ign-rendering
+    * [Pull request #311](https://github.com/ignitionrobotics/ign-rendering/pull/311)
+
+1. Remove `tools/code_check` and update codecov
+    * [Pull request #321](https://github.com/ignitionrobotics/ign-rendering/pull/321)
+
+1. [OGRE 1.x] Uniform buffer shader support
+    * [Pull request #294](https://github.com/ignitionrobotics/ign-rendering/pull/294)
+
+1. Helper function to get a scene
+    * [Pull request #320](https://github.com/ignitionrobotics/ign-rendering/pull/320)
+
+1. fix capsule mouse picking
+    * [Pull request #319](https://github.com/ignitionrobotics/ign-rendering/pull/319)
+
+1. Fix depth alpha
+    * [Pull request #316](https://github.com/ignitionrobotics/ign-rendering/pull/316)
+
+1. Add shadows to Ogre2DepthCamera without crashing
+    * [Pull request #303](https://github.com/ignitionrobotics/ign-rendering/pull/303)
+
+1. Reduce lidar data discretization
+    * [Pull request #296](https://github.com/ignitionrobotics/ign-rendering/pull/296)
+
+1. update light visual size
+    * [Pull request #306](https://github.com/ignitionrobotics/ign-rendering/pull/306)
+
+1. Improve build times by reducing included headers
+    * [Pull request #299](https://github.com/ignitionrobotics/ign-rendering/pull/299)
+
+1. Add light map tutorial
+    * [Pull request #302](https://github.com/ignitionrobotics/ign-rendering/pull/302)
+
+1. Prevent console warnings when multiple texture coordinates are present
+    * [Pull request #301](https://github.com/ignitionrobotics/ign-rendering/pull/301)
+
+1. Fix gazebo scene viewer build
+    * [Pull request #289](https://github.com/ignitionrobotics/ign-rendering/pull/289)
+
+1. Silence noisy sky error
+    * [Pull request #282](https://github.com/ignitionrobotics/ign-rendering/pull/282)
+
+1. Added command line argument to pick version of Ogre
+    * [Pull request #277](https://github.com/ignitionrobotics/ign-rendering/pull/277)
+
 ### Ignition Rendering 5.0.0 (2021-03-30)
 
 1. Add ogre2 skybox support

diff --git a/Migration.md b/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
 

diff --git a/include/ignition/rendering/Camera.hh b/include/ignition/rendering/Camera.hh
@@ -152,8 +152,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
@@ -166,8 +166,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;
 

diff --git a/include/ignition/rendering/Scene.hh b/include/ignition/rendering/Scene.hh
@@ -1077,6 +1077,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.

diff --git a/include/ignition/rendering/base/BaseCamera.hh b/include/ignition/rendering/base/BaseCamera.hh
@@ -409,6 +409,10 @@ namespace ignition
       this->Scene()->PreRender();
       this->Render();
       this->PostRender();
+      if (!this->Scene()->LegacyAutoGpuFlush())
+      {
+        this->Scene()->PostRender();
+      }
     }
 
     //////////////////////////////////////////////////

diff --git a/include/ignition/rendering/base/BaseScene.hh b/include/ignition/rendering/base/BaseScene.hh
@@ -522,6 +522,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,

diff --git a/ogre2/include/ignition/rendering/ogre2/Ogre2Scene.hh b/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