Fixes the rendering logic regression in environments (#614)

# Description

The previous rendering logic had a bug where a render call would also
step physics `render_interval` number of times.

This was a regression introduced in
59493b89

## Type of change

- Bug fix (non-breaking change which fixes an issue)

## Checklist

- [x] I have run the [`pre-commit` checks](https://pre-commit.com/) with
`./isaaclab.sh --format`
- [ ] I have made corresponding changes to the documentation
- [x] My changes generate no new warnings
- [x] I have added tests that prove my fix is effective or that my
feature works
- [x] I have updated the changelog and the corresponding version in the
extension's `config/extension.toml` file
- [x] I have added my name to the `CONTRIBUTORS.md` or my name already
exists there

source/extensions/omni.isaac.lab/config/extension.toml

 [package]
 
 # Note: Semantic Versioning is used: https://semver.org/
-version = "0.18.5"
+version = "0.18.6"
 
 # Description
 title = "Isaac Lab framework for Robot Learning"

source/extensions/omni.isaac.lab/docs/CHANGELOG.rst

 Changelog
 ---------
 
+0.18.6 (2024-07-01)
+~~~~~~~~~~~~~~~~~~~
+
+Fixed
+^^^^^
+
+* Fixed the environment stepping logic. Earlier, the environments' rendering logic was updating the kit app which
+  would in turn step the physics :attr:`omni.isaac.lab.sim.SimulationCfg.render_interval` times. Now, a render
+  call only does rendering and does not step the physics.
+
+
 0.18.5 (2024-06-26)
 ~~~~~~~~~~~~~~~~~~~
 

source/extensions/omni.isaac.lab/omni/isaac/lab/app/app_launcher.py

@@ -534,7 +534,8 @@ class AppLauncher:
         for key, value in hacked_modules.items():
             sys.modules[key] = value
 
-    def _rendering_enabled(self):
+    def _rendering_enabled(self) -> bool:
+        """Check if rendering is required by the app."""
         # Indicates whether rendering is required by the app.
         # Extensions required for rendering bring startup and simulation costs, so we do not enable them if not required.
         return not self._headless or self._livestream >= 1 or self._enable_cameras

source/extensions/omni.isaac.lab/omni/isaac/lab/envs/direct_rl_env.py

@@ -288,9 +288,13 @@ class DirectRLEnv(gym.Env):
         # add action noise
         if self.cfg.action_noise_model:
             action = self._action_noise_model.apply(action.clone())
-
         # process actions
         self._pre_physics_step(action)
+
+        # check if we need to do rendering within the physics loop
+        # note: checked here once to avoid multiple checks within the loop
+        is_rendering = self.sim.has_gui() or self.sim.has_rtx_sensors()
+
         # perform physics stepping
         for _ in range(self.cfg.decimation):
             self._sim_step_counter += 1
@@ -298,11 +302,13 @@ class DirectRLEnv(gym.Env):
             self._apply_action()
             # set actions into simulator
             self.scene.write_data_to_sim()
-            render = self._sim_step_counter % self.cfg.sim.render_interval == 0 and (
-                self.sim.has_gui() or self.sim.has_rtx_sensors()
-            )
             # simulate
-            self.sim.step(render=render)
+            self.sim.step(render=False)
+            # render between steps only if the GUI or an RTX sensor needs it
+            # note: we assume the render interval to be the shortest accepted rendering interval.
+            #    If a camera needs rendering at a faster frequency, this will lead to unexpected behavior.
+            if self._sim_step_counter % self.cfg.sim.render_interval == 0 and is_rendering:
+                self.sim.render()
             # update buffers at sim dt
             self.scene.update(dt=self.physics_dt)
 
@@ -423,6 +429,8 @@ class DirectRLEnv(gym.Env):
     def close(self):
         """Cleanup for the environment."""
         if not self._is_closed:
+            # close entities related to the environment
+            # note: this is order-sensitive to avoid any dangling references
             if self.cfg.events:
                 del self.event_manager
             del self.scene

source/extensions/omni.isaac.lab/omni/isaac/lab/envs/manager_based_env.py

@@ -262,6 +262,11 @@ class ManagerBasedEnv:
         """
         # process actions
         self.action_manager.process_action(action)
+
+        # check if we need to do rendering within the physics loop
+        # note: checked here once to avoid multiple checks within the loop
+        is_rendering = self.sim.has_gui() or self.sim.has_rtx_sensors()
+
         # perform physics stepping
         for _ in range(self.cfg.decimation):
             self._sim_step_counter += 1
@@ -269,11 +274,13 @@ class ManagerBasedEnv:
             self.action_manager.apply_action()
             # set actions into simulator
             self.scene.write_data_to_sim()
-            render = self._sim_step_counter % self.cfg.sim.render_interval == 0 and (
-                self.sim.has_gui() or self.sim.has_rtx_sensors()
-            )
             # simulate
-            self.sim.step(render=render)
+            self.sim.step(render=False)
+            # render between steps only if the GUI or an RTX sensor needs it
+            # note: we assume the render interval to be the shortest accepted rendering interval.
+            #    If a camera needs rendering at a faster frequency, this will lead to unexpected behavior.
+            if self._sim_step_counter % self.cfg.sim.render_interval == 0 and is_rendering:
+                self.sim.render()
             # update buffers at sim dt
             self.scene.update(dt=self.physics_dt)
 

source/extensions/omni.isaac.lab/omni/isaac/lab/envs/manager_based_rl_env.py

@@ -154,6 +154,11 @@ class ManagerBasedRLEnv(ManagerBasedEnv, gym.Env):
         """
         # process actions
         self.action_manager.process_action(action)
+
+        # check if we need to do rendering within the physics loop
+        # note: checked here once to avoid multiple checks within the loop
+        is_rendering = self.sim.has_gui() or self.sim.has_rtx_sensors()
+
         # perform physics stepping
         for _ in range(self.cfg.decimation):
             self._sim_step_counter += 1
@@ -161,11 +166,13 @@ class ManagerBasedRLEnv(ManagerBasedEnv, gym.Env):
             self.action_manager.apply_action()
             # set actions into simulator
             self.scene.write_data_to_sim()
-            render = self._sim_step_counter % self.cfg.sim.render_interval == 0 and (
-                self.sim.has_gui() or self.sim.has_rtx_sensors()
-            )
             # simulate
-            self.sim.step(render=render)
+            self.sim.step(render=False)
+            # render between steps only if the GUI or an RTX sensor needs it
+            # note: we assume the render interval to be the shortest accepted rendering interval.
+            #    If a camera needs rendering at a faster frequency, this will lead to unexpected behavior.
+            if self._sim_step_counter % self.cfg.sim.render_interval == 0 and is_rendering:
+                self.sim.render()
             # update buffers at sim dt
             self.scene.update(dt=self.physics_dt)
 

source/extensions/omni.isaac.lab/test/envs/test_env_rendering_logic.py

+# Copyright (c) 2022-2024, The Isaac Lab Project Developers.
+# All rights reserved.
+#
+# SPDX-License-Identifier: BSD-3-Clause
+
+"""Launch Isaac Sim Simulator first."""
+
+from omni.isaac.lab.app import AppLauncher, run_tests
+
+# launch omniverse app
+# need to set "enable_cameras" true to be able to do rendering tests
+app_launcher = AppLauncher(headless=True, enable_cameras=True)
+simulation_app = app_launcher.app
+
+"""Rest everything follows."""
+
+import torch
+import unittest
+
+import omni.usd
+
+from omni.isaac.lab.envs import (
+    DirectRLEnv,
+    DirectRLEnvCfg,
+    ManagerBasedEnv,
+    ManagerBasedEnvCfg,
+    ManagerBasedRLEnv,
+    ManagerBasedRLEnvCfg,
+)
+from omni.isaac.lab.scene import InteractiveSceneCfg
+from omni.isaac.lab.sim import SimulationCfg, SimulationContext
+from omni.isaac.lab.utils import configclass
+
+
+@configclass
+class EmptyManagerCfg:
+    """Empty specifications for the environment."""
+
+    pass
+
+
+def create_manager_based_env(render_interval: int):
+    """Create a manager based environment."""
+
+    @configclass
+    class EnvCfg(ManagerBasedEnvCfg):
+        """Configuration for the test environment."""
+
+        decimation: int = 4
+        sim: SimulationCfg = SimulationCfg(dt=0.005, render_interval=render_interval)
+        scene: InteractiveSceneCfg = InteractiveSceneCfg(num_envs=1, env_spacing=1.0)
+        actions: EmptyManagerCfg = EmptyManagerCfg()
+        observations: EmptyManagerCfg = EmptyManagerCfg()
+
+    return ManagerBasedEnv(cfg=EnvCfg())
+
+
+def create_manager_based_rl_env(render_interval: int):
+    """Create a manager based RL environment."""
+
+    @configclass
+    class EnvCfg(ManagerBasedRLEnvCfg):
+        """Configuration for the test environment."""
+
+        decimation: int = 4
+        sim: SimulationCfg = SimulationCfg(dt=0.005, render_interval=render_interval)
+        scene: InteractiveSceneCfg = InteractiveSceneCfg(num_envs=1, env_spacing=1.0)
+        actions: EmptyManagerCfg = EmptyManagerCfg()
+        observations: EmptyManagerCfg = EmptyManagerCfg()
+
+    return ManagerBasedRLEnv(cfg=EnvCfg())
+
+
+def create_direct_rl_env(render_interval: int):
+    """Create a direct RL environment."""
+
+    @configclass
+    class EnvCfg(DirectRLEnvCfg):
+        """Configuration for the test environment."""
+
+        decimation: int = 4
+        num_actions: int = 0
+        num_observations: int = 0
+        sim: SimulationCfg = SimulationCfg(dt=0.005, render_interval=render_interval)
+        scene: InteractiveSceneCfg = InteractiveSceneCfg(num_envs=1, env_spacing=1.0)
+
+    class Env(DirectRLEnv):
+        """Test environment."""
+
+        def _pre_physics_step(self, actions):
+            pass
+
+        def _apply_action(self):
+            pass
+
+        def _get_observations(self):
+            return {}
+
+        def _get_rewards(self):
+            return {}
+
+        def _get_dones(self):
+            return torch.zeros(1, dtype=torch.bool), torch.zeros(1, dtype=torch.bool)
+
+    return Env(cfg=EnvCfg())
+
+
+class TestEnvRenderingLogic(unittest.TestCase):
+    """Test the rendering logic of the different environment workflows."""
+
+    def _physics_callback(self, dt):
+        # called at every physics step
+        self.physics_time += dt
+        self.num_physics_steps += 1
+
+    def _render_callback(self, event):
+        # called at every render step
+        self.render_time += event.payload["dt"]
+        self.num_render_steps += 1
+
+    def test_env_rendering_logic(self):
+        for env_type in ["manager_based_env", "manager_based_rl_env", "direct_rl_env"]:
+            for render_interval in [1, 2, 4, 8, 10]:
+                with self.subTest(env_type=env_type, render_interval=render_interval):
+                    # time tracking variables
+                    self.physics_time = 0.0
+                    self.render_time = 0.0
+                    # step tracking variables
+                    self.num_physics_steps = 0
+                    self.num_render_steps = 0
+
+                    # create a new stage
+                    omni.usd.get_context().new_stage()
+
+                    # create environment
+                    if env_type == "manager_based_env":
+                        env = create_manager_based_env(render_interval)
+                    elif env_type == "manager_based_rl_env":
+                        env = create_manager_based_rl_env(render_interval)
+                    else:
+                        env = create_direct_rl_env(render_interval)
+
+                    # enable the flag to render the environment
+                    # note: this is only done for the unit testing to "fake" camera rendering.
+                    #   normally this is set to True when cameras are created.
+                    env.sim.set_setting("/isaaclab/render/rtx_sensors", True)
+
+                    # disable the app from shutting down when the environment is closed
+                    # FIXME: Why is this needed in this test but not in the other tests?
+                    #   Without it, the test will exit after the environment is closed
+                    env.sim._app_control_on_stop_handle = None  # type: ignore
+
+                    # check that we are in partial rendering mode for the environment
+                    # this is enabled due to app launcher setting "enable_cameras=True"
+                    self.assertEqual(env.sim.render_mode, SimulationContext.RenderMode.PARTIAL_RENDERING)
+
+                    # add physics and render callbacks
+                    env.sim.add_physics_callback("physics_step", self._physics_callback)
+                    env.sim.add_render_callback("render_step", self._render_callback)
+
+                    # create a zero action tensor for stepping the environment
+                    actions = torch.zeros((env.num_envs, 0), device=env.device)
+
+                    # run the environment and check the rendering logic
+                    for i in range(50):
+                        # apply zero actions
+                        env.step(action=actions)
+
+                        # check that we have completed the correct number of physics steps
+                        self.assertEqual(
+                            self.num_physics_steps, (i + 1) * env.cfg.decimation, msg="Physics steps mismatch"
+                        )
+                        # check that we have simulated physics for the correct amount of time
+                        self.assertAlmostEqual(
+                            self.physics_time, self.num_physics_steps * env.cfg.sim.dt, msg="Physics time mismatch"
+                        )
+
+                        # check that we have completed the correct number of rendering steps
+                        self.assertEqual(
+                            self.num_render_steps,
+                            (i + 1) * env.cfg.decimation // env.cfg.sim.render_interval,
+                            msg="Render steps mismatch",
+                        )
+                        # check that we have rendered for the correct amount of time
+                        self.assertAlmostEqual(
+                            self.render_time,
+                            self.num_render_steps * env.cfg.sim.dt * env.cfg.sim.render_interval,
+                            msg="Render time mismatch",
+                        )
+
+                    # close the environment
+                    env.close()
+
+
+if __name__ == "__main__":
+    run_tests()