Skip to content

Remove obsolete de-instancing workaround in test and fix rendering test failures due to reuse of SimpleShadingSD - #6729

Merged
huidongc merged 8 commits into
isaac-sim:developfrom
huidongc:remove-deinstancing-workaround-from-rendering-tests
Jul 28, 2026
Merged

Remove obsolete de-instancing workaround in test and fix rendering test failures due to reuse of SimpleShadingSD#6729
huidongc merged 8 commits into
isaac-sim:developfrom
huidongc:remove-deinstancing-workaround-from-rendering-tests

Conversation

@huidongc

@huidongc huidongc commented Jul 26, 2026

Copy link
Copy Markdown
Collaborator

Description

  • Fixed :class:~isaaclab_physx.renderers.IsaacRtxRenderer failing with Annotator SimpleShadingSD is not attached to any render products when multiple environments sequentially request simple_shading_* camera outputs in the same Kit process. Each tiled render product is named from the requested data types, and the owned HydraTexture is destroyed on cleanup to avoid leaking render products across env create/destroy cycles.

  • Remove the NVBUG#6418121 workaround from Franka cloth and soft-body rendering tests now that the issue has been fixed by upstream Kit components.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • Test change

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

@huidongc
huidongc requested a review from a team July 26, 2026 10:20
@github-actions github-actions Bot added the isaac-lab Related to Isaac Lab team label Jul 26, 2026
@huidongc
huidongc marked this pull request as draft July 26, 2026 10:20
@greptile-apps

greptile-apps Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR updates RTX render-product ownership and rendering-test expectations.

  • Names tiled render products from requested camera output types.
  • Retains and explicitly destroys each renderer-owned tiled render product during cleanup.
  • Clears renderer-owned annotator, output, ISP, and scratch-buffer references during cleanup.
  • Removes the obsolete rendering-instancing workaround and updates affected Franka cloth golden images.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename Overview
source/isaaclab_physx/isaaclab_physx/renderers/isaac_rtx_renderer.py Changes render-product data ownership, assigns output-derived names, and explicitly releases renderer resources during camera cleanup.
source/isaaclab_tasks/test/rendering_test_utils.py Removes the obsolete USD-instancing workaround and re-enables the associated rendering-test paths.
source/isaaclab_tasks/test/golden_images/franka_cloth/newton-isaacsim_rtx_renderer-rgb.png Updates the expected Franka cloth RGB rendering output after removing the workaround.
source/isaaclab_tasks/test/golden_images/franka_cloth/newton-isaacsim_rtx_renderer-rgba.png Updates the expected Franka cloth RGBA rendering output after removing the workaround.

Sequence Diagram

sequenceDiagram
    participant Camera
    participant Renderer as IsaacRtxRenderer
    participant RP as Tiled Render Product
    participant Annotator
    Camera->>Renderer: create_render_data(spec)
    Renderer->>RP: "create(name=data_types)"
    Renderer->>Annotator: attach([rp.path])
    Camera->>Renderer: render/read outputs
    Camera->>Renderer: cleanup(render_data)
    Renderer->>Annotator: detach([rp.path])
    Renderer->>RP: destroy()
    Renderer->>Renderer: clear retained state
Loading

Reviews (2): Last reviewed commit: "Merge branch 'develop' into remove-deins..." | Re-trigger Greptile

@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

Summary

This PR removes the NVBUG#6418121 USD-instancing workaround from the shared rendering test helpers: the _maybe_disable_instancing_for_current_stage helper and its two call sites in rendering_test_franka_cloth / rendering_test_franka_soft are deleted, along with the isaacsim_rtx_renderer + motion_vectors skip in the soft-body test. A .skip changelog fragment is added under source/isaaclab_tasks/changelog.d/, which is the correct tier for a test-only change. The deletion is self-consistent — no remaining references to the removed helper appear in the modified file, and no runtime/public API is touched. The main open question is baseline coverage: the removal both re-enables a previously skipped parameterization and changes the stage state (instance proxies now retained) under which the existing golden images and golden stages were captured.

Architecture impact

None. The change is confined to test utilities in source/isaaclab_tasks/test/ and removes a temporary workaround; no public API, config schema, or runtime behavior is affected.

Test coverage

No new assertions are added; coverage changes by re-enabling previously skipped isaacsim_rtx_renderer + motion_vectors cases in rendering_test_franka_soft. Since validate_camera_outputs bootstraps and then fails when a golden image is missing, and maybe_save_stage compares prim structure exactly, the PR should state which parameterized runs were executed to confirm existing and newly enabled baselines still pass with instancing enabled.

Implementation verdict

Minor fixes needed. Posted 1 actionable finding inline.

The PR exceeded the automated context budget, so some file content was truncated.

Automated comment-only review; human maintainers own approval decisions.

@huidongc huidongc changed the title Remove obsolete rendering instancing workaround Remove obsolete rendering instancing workaround and fix rendering test failures due to reuse of SimpleShadingSD Jul 26, 2026
@huidongc
huidongc marked this pull request as ready for review July 27, 2026 21:13
@huidongc
huidongc requested a review from rilei-nvidia July 27, 2026 21:13
@huidongc huidongc changed the title Remove obsolete rendering instancing workaround and fix rendering test failures due to reuse of SimpleShadingSD Remove obsolete de-instancing workaround in test and fix rendering test failures due to reuse of SimpleShadingSD Jul 27, 2026
Comment thread source/isaaclab_physx/isaaclab_physx/renderers/isaac_rtx_renderer.py Outdated
Comment thread source/isaaclab_physx/isaaclab_physx/renderers/isaac_rtx_renderer.py Outdated
Comment thread source/isaaclab_physx/isaaclab_physx/renderers/isaac_rtx_renderer.py Outdated
Comment thread source/isaaclab_physx/isaaclab_physx/renderers/isaac_rtx_renderer.py Outdated

@rilei-nvidia rilei-nvidia left a comment

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.

Had a minor comment, Piotr covered most of them. Preemptive LGTM

huidongc added 4 commits July 28, 2026 09:57
Remove the NVBUG#6418121 workaround from Franka cloth and soft-body
rendering tests now that the fix has landed in the kit version pinned by Isaac Lab.
Give tiled render products output-specific names and release all owned resources during cleanup. This fixes "omni.replicator.core.scripts.annotators.AnnotatorRegistryError:
Annotator SimpleShadingSD is not attached to any render products".

Update Franka cloth RTX golden images.
Annotate the owned Replicator render product with viewport_manager.HydraTexture
and always destroy it during cleanup.
Data-type-based names collide once multiple tiled cameras share the
same outputs. Use rp_<uuid4.hex> so names stay unique and SDF-safe
across concurrent cameras and sequential Kit env create/destroy
cycles. Add unit coverage for the naming contract.
@huidongc
huidongc force-pushed the remove-deinstancing-workaround-from-rendering-tests branch from 4d6f984 to 815bf6b Compare July 28, 2026 01:57
Treat legacy Replicator and UUID rp_<hex> HydraTextures names as
equivalent so cartpole golden stage checks stay stable across runs.
@huidongc
huidongc merged commit 5858918 into isaac-sim:develop Jul 28, 2026
43 checks passed
@huidongc
huidongc deleted the remove-deinstancing-workaround-from-rendering-tests branch July 28, 2026 05:51
matthewtrepte pushed a commit to matthewtrepte/IsaacLab that referenced this pull request Aug 4, 2026
…st failures due to reuse of `SimpleShadingSD` (isaac-sim#6729)

# Description

* Fixed :class:`~isaaclab_physx.renderers.IsaacRtxRenderer` failing with
``Annotator SimpleShadingSD is not attached to any render products``
when multiple environments sequentially request ``simple_shading_*``
camera outputs in the same Kit process. Each tiled render product is
named from the requested data types, and the owned HydraTexture is
destroyed on cleanup to avoid leaking render products across env
create/destroy cycles.

* Remove the NVBUG#6418121 workaround from Franka cloth and soft-body
rendering tests now that the issue has been fixed by upstream Kit
components.

## Type of change

- Bug fix (non-breaking change which fixes an issue)
- Test change

## Checklist

- [x] I have read and understood the [contribution
guidelines](https://isaac-sim.github.io/IsaacLab/main/source/refs/contributing.html)
- [x] I have run the [`pre-commit` checks](https://pre-commit.com/) with
`./isaaclab.sh --format`
- [x] I have made corresponding changes to the documentation
- [x] My changes generate no new warnings
- [x] I have added tests that prove my fix is effective or that my
feature works
- [x] 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)
- [x] I have added my name to the `CONTRIBUTORS.md` or my name already
exists there
kellyguo11 pushed a commit that referenced this pull request Sep 4, 2026
# Description

Revives #6308 on current `develop` and supersedes #3728 with a single
backend-neutral contract for kinematic rigid-object rendering.

## Architecture

`source/isaaclab/test/renderers/rigid_object_rendering_contract.py` is
the composition root. It owns the cloned scene, kinematic pose sequence,
depth measurements, and assertions. Package-local adapters own only
availability checks, simulation/renderer selection, and backend cleanup:

- Isaac RTX + PhysX on CPU and CUDA, with and without a coexisting
articulation;
- Newton Warp + PhysX on CUDA;
- OVRTX + OVPhysX on CUDA through both legacy and OVStage scene
ownership (OVStage runs when installed).

The dependency direction is adapter -> shared test contract -> public
Isaac Lab APIs. An AST architecture gate rejects backend imports in the
shared contract and rejects scene, asset, sensor, or class ownership in
adapters.

The contract creates two cloned instanceable DexCubes with root-level
nonuniform scale, verifies their depth silhouettes, moves both kinematic
bodies through the public rigid-object tensor API, verifies the physics
poses, and requires opposite rendered centroid displacement.

## Current-develop audit

Most production changes in the old PR have since landed through newer
ownership boundaries: Isaac RTX render-product lifetime in #6729, Newton
shadow-state copying in #6773, OVRTX scale-aware transform writes in
#7010, and Newton Fabric scale preservation in #7481. This revival
removes those stale patches rather than carrying duplicate
implementations.

The revived contract exposed one remaining OVRTX bug: composed scale was
captured only for clone-plan source paths, while OVRTX creates
non-source destinations after exporting the host USD stage. Those
destinations therefore defaulted to unit scale. As a deliberately
temporary bridge, this PR projects only captured non-unit scales through
the existing `isaaclab.cloner.query.path_env_ids` and `path_to_clone`
boundary using the already-validated `ClonePlan`; real destination
scales take precedence. It adds no plan fields, query APIs, renderer
configuration, or per-body fallback, and the bridge can be deleted as
one unit when SDP supplies composed scale aligned with canonical
rigid-body paths.

Historical context: [Isaac Sim forum
report](https://forums.developer.nvidia.com/t/rigidbody-prim-is-not-updated-in-rendering-pipeline-if-set-to-kinematic/346608).

## Type of change

- Bug fix
- Shared regression coverage

## Testing

- Isaac RTX contract: 4 passed (CUDA/CPU x articulation absent/present).
- Newton Warp contract: 1 passed.
- OVRTX contract: 2 passed (legacy and OVStage).
- OVRTX renderer unit surface: 167 passed.
- Core architecture and Newton visualization suites: 25 passed.
- OVRTX clone-plan suite: 17 passed.
- Cloner query and rendering-contract architecture suites: 87 passed.
- Controlled OVRTX regression: failed before the production fix with
clone silhouettes of 264 vs. 36 pixels; passed after the fix.
- Incoming #7462 NumPy-backed `ClonePlan` query-boundary smoke check:
passed unchanged, including non-dense environment ids.
- `uv run isaaclab -f`: all hooks passed against the exact upstream
`develop` base, including changelog validation.

## Checklist

- [x] I have read and understood the contribution guidelines
- [x] I have run the pre-commit checks
- [x] Documentation changes are not applicable
- [x] I have added unit and integration regression coverage
- [x] I have added changelog fragments for every touched package
- [x] My name is already present in `CONTRIBUTORS.md`
kellyguo11 added a commit that referenced this pull request Sep 5, 2026
#7587)

# Description

Backports #6308 to `release/3.0.0`.

The canonical merged commit `ab34e8c5e3ee7a2c5f260d1511714e5be3bed3eb`
was cherry-picked with `-x` provenance. Eleven of its twelve source
paths replay exactly.
`source/isaaclab_ov/isaaclab_ov/renderers/ovrtx_renderer.py` required a
localized release-compatible conflict resolution, so this PR is
intentionally a draft for release-maintainer review.

| Field | Commit |
|---|---|
| Original merged change | `ab34e8c5e3ee7a2c5f260d1511714e5be3bed3eb` |
| Release base used | `1c754876008f0806fdcfda8e3a6b2f593b34d6fc` |
| Proposed backport | `740e3d7b2efe15c5a25f671583d29c034602e36f` |

## Conflict resolution

The release renderer already contains the prerequisite work from #6729,
#6773, #7010, and #7481, but differs from the source parent around
clone-plan handling and method documentation.

The resolution preserves the release branch's tensor-backed `ClonePlan`
validation and existing renderer structure, then adds only #6308's
semantic change:

- imports the existing `isaaclab.cloner.query` API;
- passes the validated release clone plan into `_capture_object_scales`;
- projects captured non-unit source scales to active clone destinations
with `path_env_ids` and `path_to_clone`;
- retains real destination scales via `setdefault`.

No paths outside the original PR are changed.

## Type of change

- Bug fix
- Shared regression coverage

## Validation

- Repository backport candidate validation passed across all 12 original
source paths.
- Per-file stable patch IDs match on 11 paths; only the
conflict-resolved renderer path differs.
- Shared rendering-contract architecture tests — 2 passed.
- Focused clone-query tests for `path_env_ids` and `path_to_clone` — 4
passed.
- Python compilation passed for all changed Python files.
- All changed-file pre-commit hooks passed, including changelog and Git
LFS checks.
- `git diff --check upstream/release/3.0.0...HEAD` passed.
- The focused OVRTX runtime test was retried with the documented `ov`
extra, but the release lock has no macOS/arm64 environment. Backend
rendering tests and the canonical `uv run isaaclab -f` remain pending
Linux CI.

## Checklist

- [x] I have read and understood the contribution guidelines
- [x] I have run the available pre-commit checks
- [x] Documentation changes are not applicable
- [x] The original unit and integration regression coverage is preserved
- [x] Changelog fragments are preserved for every touched package
- [x] The original contributor is already listed in `CONTRIBUTORS.md`

Co-authored-by: ooctipus <zhengyuz@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.

4 participants