Conversation
|
There was a problem hiding this comment.
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:
VideoRecorderconstruction, 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. Inrecord_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 | ||
| ~~~~~~~~~~~~~~~~~~~~~~~~~~ |
There was a problem hiding this comment.
🔵 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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
@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_renderingis 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(): |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
@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 -fin 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.
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 throughpre_render(), reached bysim.render().Replace that capture-time
forward()withrender()under the existingnot sim.is_renderingguard. 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
Release backport
developBackport together with #7642 if that change is brought into the release branch.
Validation
sim.forward()implementation and passes with this change.test_video_recorder.py,test_simulation_context_visualizers.py, andtest_newton_adapter.pypass.source/isaaclab_newton/test/physics/test_newton_fabric_body_sync.pypass 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 originalsim.forward()call makes it fail on stale transforms.uv run --no-project --active isaaclab -fin the prepared environment.7a7575df8746080a5c10f14d3727a550aa9da45awith 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
source/isaaclab/changelog.d/.CONTRIBUTORS.md.