Skip to content

Replace map transforms with pre-owned and reused buffer - #7233

Merged
pbarejko merged 4 commits into
isaac-sim:developfrom
pbarejko:pbarejko/remove-free-through-map
Aug 22, 2026
Merged

Replace map transforms with pre-owned and reused buffer#7233
pbarejko merged 4 commits into
isaac-sim:developfrom
pbarejko:pbarejko/remove-free-through-map

Conversation

@pbarejko

Copy link
Copy Markdown
Collaborator

Description

Perf optimization - replace map transforms with pre-owned and reused buffer

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 added a changelog fragment under source/<pkg>/changelog.d/ for every touched package (do not edit CHANGELOG.rst or bump extension.toml — CI handles that)
  • I have added my name to the CONTRIBUTORS.md or my name already exists there

@pbarejko
pbarejko requested a review from a team August 21, 2026 01:47
@github-actions github-actions Bot added the isaac-lab Related to Isaac Lab team label Aug 21, 2026
@greptile-apps

greptile-apps Bot commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

Replaces per-frame mapped OVRTX transform writes with caller-owned Warp buffers and asynchronous GPU-stream-aware writes.

  • Adds a persistent object-transform buffer allocated during legacy binding setup and released during shutdown.
  • Writes object and camera transforms through OVRTX bindings using DataAccess.ASYNC and the active Warp CUDA stream.
  • Adds contract tests covering buffer selection, stream propagation, and cleanup, plus a changelog fragment.

Confidence Score: 5/5

The PR appears safe to merge, with no concrete blocking or non-blocking defects identified in the changed transform-buffer paths.

The persistent object buffer is initialized and released with the legacy renderer lifecycle, while both transform write paths pass the producing Warp stream and rely on the documented blocking OVRTX write contract to preserve buffer validity.

Important Files Changed

Filename Overview
source/isaaclab_ov/isaaclab_ov/renderers/ovrtx_renderer.py Introduces persistent object-transform storage and replaces mapped object and camera updates with stream-aware caller-owned writes; no actionable defect was established.
source/isaaclab_ov/test/test_ovrtx_deformable_bindings.py Adds focused tests confirming the object and camera write paths pass the expected buffers, asynchronous access mode, and CUDA streams.
source/isaaclab_ov/test/test_ovrtx_renderer_contract.py Extends shutdown coverage to verify the persistent object-transform buffer is released.
source/isaaclab_ov/changelog.d/ovrtx-caller-owned-transform-write.rst Documents the transform-write optimization accurately.

Sequence Diagram

sequenceDiagram
    participant Newton as Newton state
    participant Warp as Warp CUDA stream
    participant Buffer as Caller-owned transform buffer
    participant OVRTX as OVRTX binding
    Newton->>Warp: Provide body transforms
    Warp->>Buffer: Launch transform kernel
    Buffer->>OVRTX: write(ASYNC, cuda_stream)
    OVRTX->>OVRTX: Wait on producer stream and consume buffer
Loading

Reviews (1): Last reviewed commit: "Replace map transforms with pre-owned an..." | Re-trigger Greptile

@pbarejko
pbarejko requested a review from pv-nvidia August 21, 2026 01:49

@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

Reviewed the legacy OVRTX transform update change from mapped attributes to caller-owned GPU buffers passed through binding.write with DataAccess.ASYNC and the active Warp CUDA stream. The object-buffer lifecycle, camera write path, regression tests, and package changelog are consistent with the intended optimization.

  • Design and architecture: The persistent object transform buffer remains internal to the legacy renderer path: it is initialized with legacy fields, allocated alongside the object binding, used for transform updates, and released during legacy shutdown. This follows the existing asynchronous OVRTX write pattern used by geometry buffers.
  • API: No public API is added, removed, or renamed. The changed fields and methods are private, while the object transform buffer retains the bound object count and wp.mat44d representation used by the prior mapped-write path. The touched package includes a changelog fragment.
  • Implementation: The obsolete mapping import and both transform mapping call sites are removed together. Object writes reuse a persistent buffer, camera writes pass the generated buffer directly, and both provide the producing Warp stream to OVRTX. Focused tests verify buffer identity, asynchronous access mode, CUDA stream forwarding, and object-buffer cleanup. The remaining camera allocation is a performance tradeoff rather than a demonstrated correctness problem.

No blocking issues. No inline issue met the actionable-evidence threshold; the assessment above records the review feedback.

Automated review; human maintainers own approval decisions.

Comment thread source/isaaclab_ov/isaaclab_ov/renderers/ovrtx_renderer.py
@pbarejko
pbarejko force-pushed the pbarejko/remove-free-through-map branch from 6890fc6 to c2658d2 Compare August 21, 2026 22:29
@pbarejko
pbarejko merged commit 6423ba2 into isaac-sim:develop Aug 22, 2026
47 of 48 checks passed
kellyguo11 added a commit that referenced this pull request Aug 26, 2026
## Description

Backports five merged `develop` PRs not already represented on
`release/3.0.0` or covered by #7300:

| Source PR | Source commit | Backport commit | Scope |
| --- | --- | --- | --- |
| #7235 | `4fa20fc8a5f` | `8110c816d0a` | Report distinct exit codes for
failed, crashed, timed-out, and startup-hung test runs. |
| #6597 | `2ae0878a266` | `68b21a60158` | Correct the stale
wheel-builder dependency-pin reference. |
| #7320 | `7ed3ad45546` | `1a775d46fcf` | Add success-driven ADR
guidance and evaluation coverage to the domain-randomization skill. |
| #7319 | `2c07bbb43a9` | `4249489a089` | Vectorize clone-plan
environment-ID queries. |
| #7316 | `a00278f0fde` | `8af682bfcb9` | Extend Core Lift and Reorient
episodes and pose-command resampling intervals. |

Each merged source commit was cherry-picked with `-x`. All five applied
without conflicts and have stable patch IDs identical to their source
commits.

#7233 is intentionally excluded because its existing backport is #7300.

## Type of change

- Bug fix
- Performance improvement
- Task configuration update
- Developer tooling improvement
- Documentation update

## Validation

- `uv run --frozen --extra test python -m pytest -q
source/isaaclab/test/cloner/test_clone_plan_algebra.py` — 85 passed.
- Validated the six representative `resolve_exit_code` outcomes,
including mixed-failure precedence.
- `uv run --no-project python tools/skills/cli.py check` — 21 skills
validated.
- `uv run --frozen --extra test python -m pytest --noconftest -q
source/isaaclab_tasks/test/core/test_lift_env_cfg.py` — 3 passed.
- Programmatic Lift/Reorient config assertions confirmed the 4–6 second
resampling interval and 12-second episode length.
- Ruff, Ruff-format, and RST hooks passed for the #7316 paths.
- The original four-PR branch passed
`ISAACLAB_CHANGELOG_BASE_REF=release/3.0.0 uv run --frozen isaaclab -f`;
#7316's source commit also passed its full formatting and changelog
validation.
- `git diff --check
kelly/kellyg/backport-remaining-develop-prs-3.0.0...HEAD` — passed for
the added #7316 commit.
- Stable patch IDs match all five merged source commits; #7316 also has
exact mode/blob parity on both affected paths.

## Screenshots

Not applicable.

## Checklist

- [x] I have read and understood the contribution guidelines.
- [x] I have run the repository formatting or focused pre-commit checks
appropriate to each source PR.
- [x] Documentation and skill guidance from the source PRs are included
unchanged.
- [x] My changes generate no new warnings.
- [x] Existing and backported tests cover the changed behavior.
- [x] Touched release-managed source packages carry their source
changelog fragments.
- [x] Source authorship and `-x` provenance are preserved.

---------

Co-authored-by: myurasov-nv <168484206+myurasov-nv@users.noreply.github.com>
Co-authored-by: AntoineRichard <antoiner@nvidia.com>
Co-authored-by: ooctipus <zhengyuz@nvidia.com>
Co-authored-by: camevor <camevor@nvidia.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

isaac-lab Related to Isaac Lab team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants