Repository navigation
Add the OV renderer lease and teleop-owned OVXR adapter - #8787
Open
hougantc-nvda wants to merge 31 commits into
Open
hougantc-nvda wants to merge 31 commits into
hougantc-nvda wants to merge 31 commits into
Conversation
This was referenced Oct 23, 2026
hougantc-nvda
force-pushed
the
hougantc/kitless-borrowed-renderer
branch
from
October 26, 2026 17:12
9e86c66 to
f63b4c2
Compare
hougantc-nvda
changed the title
github-actions
Bot
added
the
documentation
hougantc-nvda
changed the title
This was referenced Oct 26, 2026
hougantc-nvda
force-pushed
the
hougantc/kitless-borrowed-renderer
branch
3 times, most recently
from
October 29, 2026 16:05
d6d62fe to
5acf5b9
Compare
Contributor
Author
|
@greptile Please review the latest changes in this PR. |
Contributor
 [Medium impact] The PR affects OVXR teleoperation and the OVRTX renderer paths it borrows. Update the renderer double in the failed-seal tests before merging. Findings
SummaryAdds an explicit renderer lease and a teleop-owned OVXR adapter.
DiagramsequenceDiagram
participant Lab
participant Lease
participant Adapter
participant Capture
Lab->>Lease: Reserve renderer and stage
Adapter->>Capture: Start render worker
Capture->>Lease: Claim render ownership
Lab->>Lease: Write scene and dirty materials
Lease-->>Adapter: Complete native writes
Adapter->>Capture: Supply sealed scene ordinal
Capture->>Lease: Retire borrowed access
Lab->>Lab: Destroy owned resources
Reviews (4) · Last reviewed commit: "Publish dirty visual materials in leased..." · Reviewed by Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
Author
hougantc-nvda
force-pushed
the
hougantc/kitless-borrowed-renderer
branch
from
October 30, 2026 01:00
1ebf0ab to
91d4de5
Compare
Contributor
Author
|
@greptile please review this PR again at the latest head ( |
hougantc-nvda
marked this pull request as ready for review
October 30, 2026 01:48
hougantc-nvda
requested review from
AntoineRichard,
huidongc,
kellyguo11,
marcodiiga,
pbarejko,
rilei-nvidia and
rwiltz
as code owners
October 30, 2026 01:48
![isaaclab-review-bot[bot]](https://avatars.githubusercontent.com/u/157846462?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Isaac Lab Review Bot
Needs attention: Design and architecture (2), Scope and over-engineering (2), Implementation (2), Style consistency (3), Test quality (4). Other reviewed categories look good.
- PR description: Update the body to include CPU publication and authored partition preservation, narrow the scene-write claim, and either validate a83a3e3 or label earlier results; trim stacked-PR dependency prose; no benchmark is needed without a performance claim.
- Design and architecture: Lease acquisition depends on the unset ISAAC_LAB_OVRTX_USE_OVSTAGE global, so the documented session fails by default; expose private-OVStage selection in configuration and export the lease directly from render_lease.
- Scope and over-engineering: Minimal implementation: keep render_lease.py for lease lifecycle, publication_buffers.py for native-write retention, ovxr_session.py for SimulationContext bootstrap, and ov_libraries_xr.py for the Capture adapter; delete _lane_lease and inline _import_ovxr. Scope is cohesive; no split recommended.
- API: API changes look good.
- Compatibility and deprecation: No breaking changes identified.
- Implementation: log_mesh still mutates renderer scene state during an active lease, violating exclusivity; apply reject_ordinary_operation before creating or updating runtime meshes.
- Style consistency: stage.py still claims perspective cameras see every partition although authored false settings now win; update the comment to describe the preserved-opinion behavior.
- Test quality: Added tests include self-derived, assertion-free, and branch-identical cases that cannot catch distinct regressions; remove those probes and keep explicit observable contract assertions.
Significant concerns. Posted 13 inline findings.
Automated review; human maintainers own approval decisions.
- Move OvLibrariesXrConfig to the runtime-free isaaclab_teleop.ovxr_cfg so launchers can build it without Capture's OVXR stack, and raise a clear error naming the install guide when the pinned Capture release lacks ovxr_session_manager. Skip the adapter tests in that case. - Ship the kitless install guide with this layer. - Replace the XrAnchorManager hint with the OvLibrariesXrSession.ExternalAnchorState protocol and reject other objects on the caller before the worker claims the lease. - Validate the environment's sim device and dt against the prepared context, and opt the bootstrap into deferred Stage materialization. - Wait on the SceneData stream instead of the whole device when reading anchor reference transforms. - Keep the stub-bound adapter out of sys.modules after the fixture. - Add tests for poisoned retirement in Capture's close order, anchor seeding, epoch changes, re-adoption and protocol rejection. Review: isaac-sim#8787-1, isaac-sim#8787-5, isaac-sim#8787-8, isaac-sim#8787-9, isaac-sim#8787-10
Gate only the adapter tests on Capture's OVXR session module, and check the configuration import in a fresh process where Capture and the native OV packages are absent, so the check cannot pass on a cached module.
Leased and ordinary OVRTX renderers both reject CPU material buffers, so a CPU environment with VisualMaterial assets fails at reset either way.
Stage deferral moved the leased one-environment check onto the render worker, after it claims the lease. Reject num_envs != 1 in validate_environment so start_render_worker fails on the caller, and resolve a bare 'cuda' environment device the way SimulationContext does instead of comparing strings literally.
The bootstrap always defers Stage materialization, so an unclaimed bind without camera sensors deferred again and failed with 'not bound to a prepared Stage'. Reject materialization before the claim whenever deferral is selected, and document the precondition and start_render_worker's validation errors.
XrAnchorManager implements ExternalAnchorState only once the dynamic-anchor layer lands; until then passing one is rejected.
The contribution guide asks optional dependencies to raise Python's own ModuleNotFoundError rather than a wrapped installation hint. The prefix check could also mislabel a broken nested Capture module as a missing release. The source guide keeps the installation instructions.
publication_required() now rejects a closed publisher under the same lock as write_complete_scene(), so it cannot report a write that the writer would then refuse. The returned publisher is rendered in the API docs, with the publication order, guard, ordinal sealing, identity and failure rules it expects of a borrower. The renderer imports the lease and publication-buffer modules relatively, and the controller's private ordinal helper follows its public methods.
The lease and the OVXR adapter do not need OVRTX on a CPU simulation device, and the leased renderer rejects every camera path, so the CPU branches only changed ordinary OVRTX rendering. Restore the upstream render-product device selection, render-var mapping, scene and camera write streams, their tests and the matching docs. Kitless CPU scene publication and CPU camera buffers return with the layers that first use them.
The lease does not need a separate publication-buffer module, so return _AsyncWriteBuffers to its upstream place and text at the bottom of ovrtx_renderer.py with its imports, and drop the module and its doc references.
Extracting the material publish/drain block for leased publication is required, but the move dropped the upstream comment and renamed its variables. Restore both; only the log text changes, because the helper now also serves leased scene updates.
A leased pair registers its material writer before the Stage is populated, so "after scene population" no longer describes the writer factory. Say that it retains the buffers and that deferred scenes compile addresses after population.
No task hides prims through the adapter, and later layers hide the robot head from the XR eyes with a render-export partition instead. Remove the persistent USD-visibility helper, its start parameter and its test.
The render lease in this layer never reads _geometry_publication, which a later layer adds to the renderer.
The configuration lives in ovxr_cfg beside XrCfg, IsaacTeleopCfg and the other *Cfg classes, so use the package's existing naming. The class has not been released, so no deprecation alias is needed.
Manager-based environments always have cfg.sim, so a getattr default only hid a missing configuration and silently skipped the device and time-step check.
They are hard dependencies, not optional backends, so the OVXR session and adapter need no local imports for them.
Environment validation re-implemented the /physics/cudaDevice resolution that SimulationContext and launch_simulation apply to a bare "cuda". Give the OVXR session one resolve_cuda_device helper that the teleop entrypoints can reuse, and read the setting from the settings manager in its test.
An active lease makes OVStage rendering the unset default for every OVRTX renderer in the simulation and rejects an explicit OVStage opt-out or shared stage, which the routing guide did not say.
Properties follow the constructor. The address-compilation helper stays next to the constructor it was extracted from, so the upstream binding lines keep their place.
Replace the single publication test with a fixture that doubles each component endpoint, and check that the material, transform, geometry and material-drain writes share the supplied ordinal, that a failure in any of them poisons the pair without moving the commit boundary, and that thread, hashability, ordinal and closed-publisher checks reject a publication before any component runs.
Test that preparation rejects an existing simulation context and extension mode before it changes renderer globals, that the configuration requires exactly two stereo camera paths, and that a failed session construction aborts the bootstrap and keeps the first error.
Say which kitless XR work isaaclab_teleop owns and what it borrows from the renderer lease, and list the isaaclab_ov and native OVXR, OVRTX and OVStage dependencies that the adapter needs but the package does not install.
The adapter and lease tests double the native endpoints, so nothing in this layer exercised the transfer protocol against real OVRTX and OVStage. Run the same-thread and cross-thread owner-transfer cases on the upstream Isaac-Cartpole task with its OvPhysX preset, each in a child process, behind ISAACLAB_RUN_NATIVE_OVRTX_TRANSFER=1, and document the command in the adapter guide.
hougantc-nvda
added a commit
to hougantc-nvda/IsaacLab
that referenced
this pull request
Nov 2, 2026
…into isaac-sim#8791 # Conflicts: # source/isaaclab_ov/isaaclab_ov/renderers/ovrtx_renderer.py
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Review stack · 1/5: #8787 → #8788 → #8789 → #8790 → #8791
Review only this layer. These PRs target
main; the comparison excludes predecessors.Connect kitless Isaac Lab teleoperation to Isaac Capture's OVXR session using Lab's existing OVRTX renderer and OVStage. Capture borrows the resources through an explicit lease; Lab keeps their ownership and lifetime.
isaaclab_ovowns the renderer, stage, scene publication and retained write buffers. The lease checks render-thread ownership and rejects nested operations; the application publication guard supplies cross-thread exclusion.isaaclab_teleopimplements the OVXR host adapter, bootstrap, scene/anchor publication and coordinated shutdown.Shutdown retires input, XR and the lease before destroying Lab resources. Failed retirement retains resources for their owning thread. Ordinary rendering remains available without a lease. The scene publisher returned by the lease is documented in the renderer API reference. The adapter configuration is
OvLibrariesXrCfg.Dynamic anchor smoothing uses elapsed XR-frame time, with synchronous reset seeding. Scale and bootstrap bounds reject invalid input before allocation. Public signatures and configuration fields document frames, units and ownership.
See adapter ownership and lifecycle.
Installation: use published
isaaccapture==1.7.93a1from NVIDIA’s Python index with the scoped OVXR installation. The guide creates the optionalovxrselection; project-wide Kit/provider defaults are unchanged.Why changes outside
isaaclab_teleopare neededisaaclab_ovrenderer, lease and Stage: these modules own the native handles and scene writes. The lease prevents ordinary camera rendering, reset or destruction from racing an external render owner. Enforcing this only in teleop would leave other callers able to invalidate borrowed resources.The matching renderer tests, API exports and release notes document and protect this backend contract. Ordinary use remains available without a lease.
Type of change
Testing
b85423d366: 7 touched test files, 161 passed, 2 skipped, 0 failed; each file ran in a separate process. Changelog and environment-browser checks passed at that revision.Full-stack native/GPU, IWER and Kit results are summarized in #8791. The IWER receipts identify
c113e8341c, before the replay cleanup correction; they do not qualify this layer in isolation. Replay made one connection before its intentional duration cap, so it is not a three-cycle reconnect pass.Limits: no new native, performance or physical-headset run for this follow-up; no new Sphinx build. Two-GPU coverage has no CI route. Existing Torch/import deprecation warnings remain.
Checklist