Skip to content

Fix render-state refresh for headless video capture - #7864

Open
nblauch wants to merge 2 commits into
isaac-sim:developfrom
nblauch:nblauch/fix-headless-capture-transforms
Open

nblauch wants to merge 2 commits into
isaac-sim:developfrom
nblauch:nblauch/fix-headless-capture-transforms

Conversation

@nblauch

@nblauch nblauch commented Sep 17, 2026

Copy link
Copy Markdown
Contributor

Description

Headless Newton simulations recorded through the Kit visualizer can show stale body poses after #7642, even while physics advances. The native recorder calls sim.forward() before on-demand capture, but Newton publishes poses to the rendering stage through pre_render(), reached by sim.render().

Replace that capture-time forward() with render() under the existing not sim.is_rendering guard. Update the existing regression test to distinguish kinematic updates from renderer-state publication, and document the required synchronization for custom visualizer recorders.

Possible solution for the native-recorder failure in #7863. This does not automatically fix downstream recorders that call visualizers directly. Those callers must synchronize before on-demand capture or use a synchronized capture entry point. The issue intentionally remains open for that broader compatibility discussion.

Type of change

  • Bug fix
  • Documentation update

Release backport

  • Backport this pull request to the active release branch after it merges into develop

Backport together with #7642 if that change is brought into the release branch.

Validation

  • The updated regression test fails with the original sim.forward() implementation and passes with this change.
  • All 147 tests across test_video_recorder.py, test_simulation_context_visualizers.py, and test_newton_adapter.py pass.
  • All 7 tests in source/isaaclab_newton/test/physics/test_newton_fabric_body_sync.py pass on RTX 6000 Ada. The recorder regression checks real Fabric transforms after pose writes and CUDA graph replay, no physics advancement, and exactly one actual Kit app update per capture. Restoring only the original sim.forward() call makes it fail on stale transforms.
  • The lightweight orchestration test also rejects an intentionally injected second Kit app update. Mixed-visualizer and multiple-recorder edge cases remain outside the GPU-tested coverage; see the review replies below.
  • Full repository pre-commit checks pass via uv run --no-project --active isaaclab -f in the prepared environment.
  • GPU A/B validation on RTX 6000 Ada: same headless Newton/MuJoCo-Warp articulated scene, 48 environments, seed, two training updates, and eight-frame native capture. At develop 7a7575df8746080a5c10f14d3727a550aa9da45a with Newton 1.6.0, unpatched capture shows stale initial poses; this change restores the correct reset poses. The earlier Isaac Lab pin also produced correct poses. Details are in [Bug]: Headless Newton/Kit video records stale poses after #7642 #7863.

The GPU evidence covers Newton + headless Kit capture. Other visualizer/backend combinations were not exhaustively GPU-tested. The guard preserves on-demand behavior between recording windows; capture-time throughput has not been benchmarked.

Checklist

  • I have read and understood the contribution guidelines.
  • I have run the pre-commit checks using the Isaac Lab format command.
  • I have updated the recording documentation.
  • The change adds no warnings.
  • I have updated the existing regression test and verified failure before the fix.
  • I have added a changelog fragment under source/isaaclab/changelog.d/.
  • My name already exists in CONTRIBUTORS.md.

@nblauch
nblauch requested a review from a team September 17, 2026 03:35
@github-actions github-actions Bot added bug Something isn't working documentation Improvements or additions to documentation isaac-lab Related to Isaac Lab team labels Sep 17, 2026
@greptile-apps

greptile-apps Bot commented Sep 17, 2026

Copy link
Copy Markdown
Contributor

RetriggerConfidence Score: 5/5

The PR appears safe to merge, with the implementation, regression test, and documentation aligned around the required render-state refresh.

Summary

This PR fixes stale poses in headless, on-demand visualizer recordings by performing a render-state refresh before capture instead of only forwarding kinematics.

  • Calls sim.render() for capture-only visualizers when continuous rendering is disabled.
  • Updates the regression test to verify render-state publication.
  • Documents synchronization requirements for custom visualizer recorders.
  • Adds a changelog fragment for the fix.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[VideoRecorder requests frame] --> B{Continuous rendering active?}
    B -- No --> C[sim.render]
    C --> D[Publish backend transforms]
    D --> E[Refresh visualizers]
    B -- Yes --> F[Use continuously refreshed state]
    E --> G[Capture RGB frame]
    F --> G
Loading

Reviews (1) · Last reviewed commit: "Refresh render state before on-demand vi..."

@isaaclab-review-bot isaaclab-review-bot Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isaac Lab Review Bot

The capture path now uses sim.render() under the existing not sim.is_rendering guard, with corresponding test, documentation, and changelog updates. The new documentation subsection has an undersized reStructuredText title underline that can trigger a Sphinx warning and should be corrected before merge.

  • Design and architecture: The synchronization change remains localized to on-demand visualizer capture and uses the simulation’s existing public rendering lifecycle without changing recorder ownership or source routing.
  • API: VideoRecorder construction, source strings, and frame return types remain unchanged. The externally visible capture synchronization behavior is documented, but the new subsection’s heading markup must be fixed so warning-as-error documentation builds remain clean.
  • Implementation: The updated capture branch calls sim.render() before both standard and tiled visualizer capture when continuous rendering is disabled, and the regression test exercises that branch. In record_video.rst, the underline beneath “Custom visualizer recorders” is one character shorter than the title, which Sphinx reports as a title-underline warning; extend it to at least the title length.

Minor fixes needed. Posted 1 actionable finding inline.

Automated review; human maintainers own approval decisions.

interval options.

Custom visualizer recorders
~~~~~~~~~~~~~~~~~~~~~~~~~~

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Suggestion · Implementation — Section underline shorter than title

The new subsection title "Custom visualizer recorders" is 27 characters, but the ~ underline on this line is 26. Sphinx emits a "Title underline too short" warning for this, which fails docs builds configured with warnings-as-errors. Extend the underline by at least one character to match the title length.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure this is the right way since this would trigger a full render and then another one for the viz rendering? Is there no way to have a sync transform instead?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AntoineRichard I added regression coverage in d54a1c3 and checked the render paths more closely.

For the single headless Newton + Kit visualizer case, this does not double-render: sim.render() publishes transforms through pre_render(), but headless KitVisualizer.step() returns before pumping Kit. The subsequent render_rgb_array() performs the app update. The GPU test verifies exactly one actual Kit app update per capture, including after CUDA graph replay. In normal non-headless/continuous rendering, the existing not sim.is_rendering guard skips the new call.

However, your broader concern is valid: sim.render() updates every visualizer and dispatches render callbacks. There are configurations where that can add rendering:

  • Mixed headless visualizers: recording Kit can also refresh a Rerun streaming camera, or a Viser streaming camera with connected clients. With a non-Isaac-RTX streaming renderer, these can run while sim.is_rendering is false.
  • Multiple recorders sharing a headless Newton tiled view: each sim.render() advances the visualizer step counter, invalidating its cached composite and potentially rendering the same camera again within the same physics step.
  • Registered render callbacks can themselves perform additional rendering.

Those broader cases are findings from tracing the code, not GPU-tested reproductions. The added tests establish freshness and render cadence for the covered paths, not an absence of extra rendering in every configuration.

My recommendation is that the Isaac Lab team propose a more complete capture-preparation/synchronization solution, covering transform publication, necessary visualizer state updates, and shared capture deduplication, and likely revert #7642 in the meantime. That would restore the previous behavior for both native and downstream custom recorders while the complete solution is developed, at the cost of restoring the previous continuous-rendering overhead. This PR fixes the native-recorder regression but does not automatically repair custom recorders that call visualizers directly.


def test_visualizer_source_refreshes_physics_before_on_demand_capture():
"""On-demand capture reads a frame after physics transforms are synchronized."""
def test_visualizer_source_refreshes_render_state_before_on_demand_capture():

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hm does this modified test fail without the change to video_recorder? that it now detects the previous regression with stale rendering in headless video recording?

if not, it'd be good to add a test that does so that we don't reintroduce the previous issue

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@matthewtrepte Yes: the original updated test fails when the recorder is switched back to sim.forward(). I have now expanded the coverage in d54a1c3, including a real GPU/Fabric regression rather than relying only on the mocked recorder test.

The video_recorder case of test_root_pose_write_is_visible_on_next_render_without_step reads the real Fabric world matrices consumed by Kit/RTX after pose writes and CUDA graph replay. It checks that capture publishes the current pose without advancing physics and performs exactly one actual Kit app update per capture.

Verified by temporarily restoring only the recorder's original sim.forward() call: the GPU test fails on stale Fabric transforms. Restoring the fix makes it pass. The lightweight orchestration test also rejects an intentionally injected second Kit app update.

Validation:

  • 147 tests passed across the recorder, simulation/visualizer orchestration, and Newton adapter suites.
  • All 7 tests in the GPU/Fabric file passed on RTX 6000 Ada.
  • Full repository checks passed with uv run --no-project --active isaaclab -f in the prepared environment.

The added coverage also checks repeated and tiled capture, the continuous-rendering guard, idle recording windows, and headless Newton GL/RTX capture cadence. I am also replying to Antoine with the remaining mixed-visualizer/multiple-recorder concerns. My recommendation is for the Isaac Lab team to propose a more complete solution and likely revert #7642 in the meantime; this native-recorder patch does not resolve the downstream custom-recorder compatibility issue in #7863.

@nblauch
nblauch requested a review from hujc7 as a code owner September 18, 2026 20:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working documentation Improvements or additions to documentation isaac-lab Related to Isaac Lab team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants