Add render flag to environment step() API

[kellyguo11]

Expose a render: bool = True parameter on the step() method of all environment base classes (ManagerBasedEnv, ManagerBasedRLEnv, DirectRLEnv, DirectMARLEnv) and the MARL utility wrappers.

When render=False is passed, all rendering calls are skipped:

• GUI / RTX sensor renders inside the decimation loop
• Post-reset re-renders for RTX sensors

Physics simulation continues normally regardless of the flag. This allows user workflows that do not need rendering every step (e.g. headless RL training, fast rollouts) to opt out per-step.

Also adds unit tests for render=False and mixed render flag stepping.

Description

Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context.
List any dependencies that are required for this change.

Fixes # (issue)

Type of change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (existing functionality will not work without user modification)
• Documentation update

Screenshots

Please attach before and after screenshots of the change if applicable.

Checklist

• I have read and understood the contribution guidelines
• I have run the pre-commit checks with ./isaaclab.sh --format
• I have made corresponding changes to the documentation
• My changes generate no new warnings
• I have added tests that prove my fix is effective or that my feature works
• I have updated the changelog and the corresponding version in the extension's config/extension.toml file
• I have added my name to the CONTRIBUTORS.md or my name already exists there

[greptile-apps]

Greptile Summary

This PR exposes a render: bool = True parameter on step() across all four environment base classes (ManagerBasedEnv, ManagerBasedRLEnv, DirectRLEnv, DirectMARLEnv) and the MARL utility wrappers. The implementation is clean, backward-compatible, and consistent: is_rendering is derived as render and self.sim.is_rendering, gating both the decimation-loop sim.render() calls and the post-reset RTX re-renders (where applicable). Tests cover the three primary non-MARL env types for both the "all-False" and "mixed flag" cases.

Confidence Score: 5/5

Safe to merge — changes are backward-compatible, all remaining findings are P2 quality suggestions.

The implementation is consistent across all four env classes and the MARL wrappers, defaults to True preserving existing behavior, and is well-tested. Both open findings are pre-existing gaps (MARL post-reset re-render) and a minor test coverage suggestion — neither blocks merge.

direct_marl_env.py has a pre-existing gap (no post-reset RTX re-render after _reset_idx) that this PR surfaces but doesn't introduce.

Important Files Changed

Filename	Overview
source/isaaclab/isaaclab/envs/manager_based_env.py	Adds render: bool = True to step(); gates the decimation-loop render on render and self.sim.is_rendering. No post-reset re-render block exists here (that lives in the RL subclass), so no additional changes needed.
source/isaaclab/isaaclab/envs/manager_based_rl_env.py	Adds render param, gates both the decimation-loop render and the post-reset RTX re-render on is_rendering. Changes are symmetric with DirectRLEnv.
source/isaaclab/isaaclab/envs/direct_rl_env.py	Adds render param, gates both the decimation-loop render and the post-reset RTX re-render on is_rendering. Consistent implementation.
source/isaaclab/isaaclab/envs/direct_marl_env.py	Adds render param and gates the decimation-loop render correctly, but DirectMARLEnv never had post-reset RTX re-render logic — a pre-existing gap vs the RL envs that is now made slightly more visible by this PR.
source/isaaclab/isaaclab/envs/utils/marl.py	Both MARL utility wrappers (multi_agent_to_single_agent, multi_agent_with_one_agent) correctly thread the render flag through to the underlying env's step().
source/isaaclab/test/envs/test_env_rendering_logic.py	Adds two well-structured tests (render=False skips rendering, mixed flag toggling); covers all three non-MARL env types. Physics-step counting and render-step counting assertions are logically correct.
source/isaaclab/docs/CHANGELOG.rst	Changelog entry is accurate and complete, correctly documents the new render flag, what is skipped, and backward compatibility.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["env.step(action, render=True/False)"] --> B["is_rendering = render AND sim.is_rendering"]
    B --> C["Decimation loop (N times)"]
    C --> D["sim.step(render=False)"]
    D --> E{"_sim_step_counter % render_interval == 0\nAND is_rendering?"}
    E -- Yes --> F["sim.render()"]
    E -- No --> G["scene.update()"]
    F --> G
    G --> H{"More decimation steps?"}
    H -- Yes --> C
    H -- No --> I["Post-step: compute dones/rewards"]
    I --> J{"reset_env_ids non-empty?"}
    J -- Yes --> K["_reset_idx()"]
    K --> L{"is_rendering AND has_rtx_sensors\nAND num_rerenders_on_reset > 0?\n(RL envs only)"}
    L -- Yes --> M["sim.render() x num_rerenders_on_reset"]
    L -- No --> N["compute observations & return"]
    M --> N
    J -- No --> N

Loading

Comments Outside Diff (1)

1. source/isaaclab/isaaclab/envs/direct_marl_env.py, line 446-447 (link)

   DirectMARLEnv missing post-reset RTX re-render guard

   Unlike DirectRLEnv and ManagerBasedRLEnv, DirectMARLEnv never calls self.sim.render() after _reset_idx to refresh RTX sensor data. The other RL envs have a pattern like:

   if is_rendering and self.has_rtx_sensors and self.cfg.num_rerenders_on_reset > 0:
       for _ in range(self.cfg.num_rerenders_on_reset):
           self.sim.render()

   This is a pre-existing gap (not introduced by this PR), but with the new render flag now highlighting the "post-reset re-render" concept explicitly in the PR description, it may be worth aligning DirectMARLEnv with the other env classes to avoid stale RTX camera observations after environment resets when render=True.

_{Reviews (1): Last reviewed commit: "Add render flag to environment step() AP..." | Re-trigger Greptile}

[greptile-apps]

Test coverage gap for DirectMARLEnv

Both new tests parametrize over ["manager_based_env", "manager_based_rl_env", "direct_rl_env"] but skip direct_marl_env. Since DirectMARLEnv.step() also received the render flag, it would be worth adding a direct_marl_env case — even a minimal one — to ensure the decimation-loop render gate works for MARL environments too.

[isaaclab-review-bot]

Review: Add render flag to environment step() API

Summary

This PR adds a render: bool = True parameter to the step() method of all four environment base classes, allowing callers to skip rendering while physics simulation continues. The implementation is clean and backward-compatible.

✅ What's Good

1. Backward Compatibility: The default render=True ensures existing code continues to work unchanged
2. Consistent API: All four env classes (ManagerBasedEnv, ManagerBasedRLEnv, DirectRLEnv, DirectMARLEnv) and MARL wrappers get the same interface
3. Clear Documentation: Docstrings and CHANGELOG entry clearly explain the new parameter
4. Test Coverage: New tests verify render=False skips rendering and mixed-mode stepping works correctly
5. Post-reset re-render: Correctly guards the post-reset re-render loop in DirectRLEnv and ManagerBasedRLEnv with is_rendering

⚠️ Issue Found: Inconsistency in DirectMARLEnv

DirectMARLEnv does not have the post-reset re-render logic that exists in the other RL environments.

Looking at the reset handling in step():

• DirectRLEnv (lines 436-441):

if len(reset_env_ids) > 0:
    self._reset_idx(reset_env_ids)
    # if sensors are added to the scene, make sure we render to reflect changes in reset
    if is_rendering and self.has_rtx_sensors and self.cfg.num_rerenders_on_reset > 0:
        for _ in range(self.cfg.num_rerenders_on_reset):
            self.sim.render()

• ManagerBasedRLEnv (lines 231-235): Same pattern ✓

• DirectMARLEnv (lines 445-447): Missing the re-render logic:

if len(reset_env_ids) > 0:
    self._reset_idx(reset_env_ids)
# No re-render for RTX sensors after reset!

This appears to be a pre-existing issue in DirectMARLEnv, not introduced by this PR. However, since this PR is explicitly about render consistency across all env classes, it would be worth either:

1. Adding the missing re-render logic to DirectMARLEnv for consistency, or
2. Documenting why DirectMARLEnv intentionally differs (if there's a reason)

🔍 Minor Observations

1. Test coverage for DirectMARLEnv: The new tests parametrize over ["manager_based_env", "manager_based_rl_env", "direct_rl_env"] but don't include DirectMARLEnv. Consider adding MARL env test coverage for completeness.

2. CI Failures: The CI shows some failures (environments_training, isaaclab_tasks [3/3], test-skillgen, isaaclab (core) [2/3]) - worth verifying these aren't related to this change.

Verdict

The core implementation is solid and the design is correct. The DirectMARLEnv inconsistency is worth addressing but isn't blocking since it's pre-existing. Good work! 👍

[Toni-SM]

@kellyguo11 IMO, the implementation breaks the agreed standard API for representing general RL problems (gymnasium.Env.step) forcing any reinforcement library (already integrated, or with the intention of supporting Isaac Lab) to change their implementation and include a custom case to support this feature for Isaac Lab only.

If such behavior (skipping rendering calls) is desired, it should be implemented using other mechanisms (e.g., config passed to the class constructor) that don't modify the agreed standard API (gymnasium.Env).

[kellyguo11]

@kellyguo11 IMO, the implementation breaks the agreed standard API for representing general RL problems (gymnasium.Env.step) forcing any reinforcement library (already integrated, or with the intention of supporting Isaac Lab) to change their implementation and include a custom case to support this feature for Isaac Lab only.

If such behavior (skipping rendering calls) is desired, it should be implemented using other mechanisms (e.g., config passed to the class constructor) that don't modify the agreed standard API (gymnasium.Env).

Yes, I left it with a default so that it's not a requirement for wrappers following gymnasium APIs to set this, but it does diverge from the gymnasium Env API. alternatively, we can make this an internal variable, similar to your config idea, that can be set by external calls. We want this flag to be dynamic for each reset call.

[isaaclab-review-bot]

🤖 Isaac Lab Review Bot

Summary

This PR adds a render_enabled property to all environment base classes, allowing per-step control over rendering. The implementation approach changed from the previous review — instead of adding a render: bool parameter to step(), the PR now uses a mutable property that preserves the standard gymnasium.Env.step() signature. This is a cleaner design that avoids requiring downstream RL libraries to add Isaac Lab-specific handling.

Architecture Impact

The change is well-contained. The render_enabled property is:

• Initialized to True during _init_sim() in all four env classes
• Checked at the top of step() to compute is_rendering = self.render_enabled and self.sim.is_rendering
• Used consistently to guard both in-loop rendering and post-reset re-rendering

No downstream callers need modification since the default behavior is unchanged.

Implementation Verdict

Ship it — Clean, minimal implementation with correct semantics. The property-based approach is better than a parameter for preserving API compatibility.

Test Coverage ✅

The PR includes two new parametrized tests:

• test_env_render_false_skips_rendering: Verifies that render_enabled=False skips all rendering while physics continues
• test_env_render_flag_mixed_steps: Verifies toggling the property between steps produces correct render counts

Tests cover ManagerBasedEnv, ManagerBasedRLEnv, and DirectRLEnv. Note that DirectMARLEnv is not included in test parametrization — this is acceptable since MARL envs have the same implementation pattern and are less commonly used.

CI Status

The "Installation Tests" failure is a CI infrastructure issue (Docker buildkit error: open /var/lib/docker/buildkit/executor/resolv.conf.tmp: no such file or directory) — not related to this PR. Pre-commit passes ✅.

Findings

🔵 Observation: DirectMARLEnv lacks post-reset re-render logic (pre-existing)

DirectRLEnv and ManagerBasedRLEnv both have logic to re-render RTX sensors after resets:

if is_rendering and self.has_rtx_sensors and self.cfg.num_rerenders_on_reset > 0:
    for _ in range(self.cfg.num_rerenders_on_reset):
        self.sim.render()

DirectMARLEnv does not have this logic (confirmed on develop branch too). This is a pre-existing inconsistency, not introduced by this PR. Worth addressing separately if MARL envs are used with RTX sensors.

Overall

Excellent rework of the feature. The property-based approach is the right call for gymnasium compatibility. Implementation is consistent across all four env classes, tests are thorough, and the CHANGELOG entry is well-written. Ready to merge. 👍

[xyao-nv]

Thanks for making the change!

[matthewtrepte]

hm from a design point of view, if render_enabled=False, do we always want to skip updating visualizers? since visualizers are stepped in self.sim.render()

[kellyguo11]

good point, enabled visualizer stepping

[matthewtrepte]

hm if theres a very short episode env we can test with (<10 episode length), then the step path can cover the post reset case

[kellyguo11]

addressed

[matthewtrepte]

few comments

[isaaclab-review-bot]

🤖 Isaac Lab Review Bot

Summary

This PR adds a render_enabled property to all environment base classes, allowing per-step control over whether Kit app rendering (cameras, GUI viewport) occurs during step(). The implementation correctly threads a skip_app_pumping flag through SimulationContext.render() and update_visualizers() to selectively skip Kit-based visualizers while allowing standalone visualizers to continue. The approach is sound, but there are a few issues that need attention.

Architecture Impact

Cross-module impact: Changes touch the core simulation context and all four environment base classes. The API addition is backward-compatible (defaults to True). The skip_app_pumping parameter propagates correctly through the visualizer system.

Implementation Verdict

Minor fixes needed

Test Coverage

Good coverage for the new functionality with three test cases covering:

• render_enabled=False skipping all Kit rendering
• Mixed toggling of the flag between steps
• Post-reset re-render skipping

However, ManagerBasedEnv is missing from one test, and DirectMARLEnv is missing from all tests.

CI Status

Most checks are still pending. Pre-commit and build passed, which is positive.

Findings

🟡 1. Missing render_enabled initialization in ManagerBasedRLEnv
source/isaaclab/isaaclab/envs/manager_based_rl_env.py - The ManagerBasedRLEnv class uses self.render_enabled at lines 211 and 241, but it inherits from ManagerBasedEnv where render_enabled is set in _init_sim(). This works because of inheritance, but the class also accesses is_rendering in the post-reset block (line 241) which is computed at the top of step(). However, ManagerBasedRLEnv doesn't call super().step() - it has its own implementation. This is fine, but worth noting that the is_rendering variable is correctly scoped.

🔴 2. DirectMARLEnv missing post-reset re-render guard
source/isaaclab/isaaclab/envs/direct_marl_env.py - Unlike DirectRLEnv and ManagerBasedRLEnv, the DirectMARLEnv class doesn't have the num_rerenders_on_reset logic visible in this diff. If it exists elsewhere in the class, it needs the same render_enabled and is_rendering guard added. If it doesn't exist, this inconsistency should be documented.

🟡 3. Test coverage gap for DirectMARLEnv
source/isaaclab/test/envs/test_env_rendering_logic.py - None of the three new tests include DirectMARLEnv in their parametrization. The PR description claims the feature applies to all four environment classes including DirectMARLEnv, but it's untested.

🟡 4. ManagerBasedEnv excluded from post-reset re-render test
source/isaaclab/test/envs/test_env_rendering_logic.py:395 - The test_env_render_false_with_resets test only uses direct_rl_env but ManagerBasedRLEnv also has the post-reset re-render logic. Consider parametrizing this test to include both.

🔵 5. Consider making render_enabled a property with validation
source/isaaclab/isaaclab/envs/manager_based_env.py:160-166 - Currently render_enabled is a bare instance attribute. While this works, making it a @property would allow future validation or logging when the flag changes, and would be more consistent with other environment attributes.

🔵 6. CHANGELOG mentions "MARL utility wrappers" but no changes visible
The PR description mentions "MARL utility wrappers" but only DirectMARLEnv changes are in the diff. If there are wrapper classes (like in isaaclab.envs.wrappers), they may need similar updates to propagate the flag correctly.

🔵 7. Documentation could clarify interaction with sim.is_rendering
The docstrings explain Kit vs standalone visualizers well, but don't mention the interaction with the existing sim.is_rendering property. When env.render_enabled = False but the simulation is in a rendering mode, the behavior is correct but could be confusing. A note clarifying that render_enabled is an additional gate on top of is_rendering would help.

[isaaclab-review-bot]

🤖 Isaac Lab Review Bot

Summary

This PR adds a render_enabled flag to all environment base classes, allowing users to skip Kit app loop rendering (camera/RTX sensors) on a per-step basis while keeping standalone visualizers (Newton, Rerun, Viser) running. The implementation modifies SimulationContext.render() and update_visualizers() with a skip_app_pumping parameter, and all four environment classes propagate their render_enabled state through this mechanism. The approach is sound and the tests are comprehensive.

Architecture Impact

The change touches the core rendering path:

1. SimulationContext (simulation_context.py:696-760): The render() and update_visualizers() methods now accept skip_app_pumping to filter visualizers by their pumps_app_update() method
2. All environment classes: DirectRLEnv, DirectMARLEnv, ManagerBasedEnv, ManagerBasedRLEnv all add the render_enabled attribute and pass skip_app_pumping=not self.render_enabled to sim.render()
3. Post-reset re-renders (DirectRLEnv and ManagerBasedRLEnv): The post-reset camera refresh loop now respects render_enabled

This is a non-breaking addition—the default render_enabled=True preserves existing behavior. The skip_app_pumping parameter has a sensible default of False.

Implementation Verdict

Ship it — Minor documentation nit below, but no blocking issues.

Test Coverage

Test coverage is thorough:

• test_env_render_false_skips_rendering: Verifies physics advances while rendering is skipped across all 3 env types
• test_env_render_flag_mixed_steps: Verifies toggling the flag mid-episode produces correct render counts
• test_env_render_false_with_resets: Verifies post-reset re-renders are also skipped when flag is False

The tests use callback instrumentation to count physics steps and render calls independently, which is the right approach. Episode-boundary behavior is exercised via the episode_length_steps parameter.

CI Status

❌ Build Latest Docs: failure — Needs investigation. The docs build failure may be unrelated to this PR (likely infrastructure), but should be verified before merge.

Findings

🔵 Improvement: source/isaaclab/isaaclab/sim/simulation_context.py:699-702 — Docstring mentions mode as "Unused" but doesn't explain why it exists

The mode parameter is documented as "Unused. Kept for backward compatibility." but there's no deprecation warning or guidance on when it might be removed. Consider adding a FutureWarning if this parameter is intended to be removed, or explain the historical context:

Args:
    mode: Deprecated, unused. Historically controlled render quality modes.
        Will be removed in a future release.

🔵 Improvement: source/isaaclab/isaaclab/envs/direct_marl_env.py:447 — Missing post-reset re-render gating

Unlike DirectRLEnv (line 453) and ManagerBasedRLEnv (line 241), the DirectMARLEnv.step() method doesn't have a post-reset re-render loop at all (the _reset_idx call at line 455 has no subsequent re-render). This is consistent with the existing code, but the PR description mentions "Post-reset re-renders for RTX sensors are also skipped" as a feature. The MARL env doesn't have this path, so the claim only applies to DirectRLEnv and ManagerBasedRLEnv. Consider either:

• Adding the re-render loop to DirectMARLEnv for consistency (separate PR), or
• Clarifying in the changelog that this applies only to RL envs, not MARL

🔵 Improvement: source/isaaclab/test/envs/test_env_rendering_logic.py:400-475 — test_env_render_false_with_resets only tests direct_rl_env

The test is not parametrized like the others and only tests DirectRLEnv. Since ManagerBasedRLEnv also has the post-reset re-render gate (line 241 in manager_based_rl_env.py), this test should be parametrized:

@pytest.mark.parametrize("env_type", ["manager_based_rl_env", "direct_rl_env"])
def test_env_render_false_with_resets(env_type, physics_callback, render_callback):

Note: manager_based_env doesn't have the post-reset re-render loop, so it shouldn't be included.

🔵 Improvement: source/isaaclab/isaaclab/sim/simulation_context.py:752 — Potential for visualizer removal during iteration

The loop at line 749-764 iterates over self._visualizers while potentially modifying visualizers_to_remove. The actual removal happens after the loop (line 765+), so this is safe. However, the continue on line 752 means skipped visualizers won't be checked for is_closed status in that iteration, which could delay cleanup. This is a very minor edge case and acceptable.

🔵 Improvement: source/isaaclab/docs/CHANGELOG.rst:22-24 — Changelog could mention the render_enabled default

The changelog entry is good but could be clearer that the default value is True for backward compatibility right in the Added section, not just implied by "full backward compatibility" at the end.