Skip to content

Replaces deprecated TiledCamera with Camera in visuotactile sensor#5269

Closed
AntoineRichard wants to merge 1 commit intodevelopfrom
antoiner/fix-visuotactile-test
Closed

Replaces deprecated TiledCamera with Camera in visuotactile sensor#5269
AntoineRichard wants to merge 1 commit intodevelopfrom
antoiner/fix-visuotactile-test

Conversation

@AntoineRichard
Copy link
Copy Markdown
Collaborator

Description

TiledCamera was merged into Camera in #5162, leaving TiledCamera as a deprecated wrapper. The visuotactile sensor still used the old classes, which caused CI failures due to the extra deprecation layer interacting poorly with the refactored Camera renderer/Fabric initialization.

Also fix Camera.del crash when init raises before _renderer is assigned by moving data attribute initialization earlier in init.

Type of change

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

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 updated the changelog and the corresponding version in the extension's config/extension.toml file
  • I have added my name to the CONTRIBUTORS.md or my name already exists there

TiledCamera was merged into Camera in #5162, leaving TiledCamera as a
deprecated wrapper. The visuotactile sensor still used the old classes,
which caused CI failures due to the extra deprecation layer interacting
poorly with the refactored Camera renderer/Fabric initialization.

Also fix Camera.__del__ crash when __init__ raises before _renderer is
assigned by moving data attribute initialization earlier in __init__.
@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 Apr 14, 2026
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Apr 14, 2026

Greptile Summary

This PR replaces the deprecated TiledCamera / TiledCameraCfg with Camera / CameraCfg in the visuotactile sensor, and fixes an AttributeError in Camera.__del__ that occurred when __init__ raised before _renderer was assigned. Both changelogs and version files are correctly bumped.

Confidence Score: 5/5

  • Safe to merge — changes are a straightforward deprecation replacement with a targeted bug fix; all remaining findings are non-blocking style suggestions.
  • Both P2 findings (incomplete __del__ guard and pre-existing direct __del__ call pattern) are style/hardening concerns that do not affect current runtime correctness. The core TiledCamera → Camera replacement and the _renderer = None initialization fix are correct.
  • No files require special attention.

Important Files Changed

Filename Overview
source/isaaclab/isaaclab/sensors/camera/camera.py Initializes _renderer = None early in __init__ to prevent AttributeError in __del__; the guard is not yet fully defensive (see comment on getattr).
source/isaaclab_contrib/isaaclab_contrib/sensors/tacsl_sensor/visuotactile_sensor.py Replaces TiledCamera with Camera; existing __del__ pattern of calling _camera_sensor.__del__() directly now risks double-cleanup since Camera.__del__ performs renderer teardown.
source/isaaclab_contrib/isaaclab_contrib/sensors/tacsl_sensor/visuotactile_sensor_cfg.py Replaces TiledCameraCfg with CameraCfg; straightforward, correct change.
source/isaaclab_contrib/test/sensors/test_visuotactile_sensor.py All test fixtures updated to use CameraCfg; test coverage is thorough across minimum config, camera-only, and camera+force-field scenarios.
source/isaaclab/docs/CHANGELOG.rst Adds Fixed entry under new 4.6.1 heading; version matches extension.toml.
source/isaaclab_contrib/docs/CHANGELOG.rst New 0.3.1 heading with correct Changed entry; matches extension.toml.
source/isaaclab_contrib/config/extension.toml Version bumped to 0.3.1 matching the new changelog entry.

Sequence Diagram

sequenceDiagram
    participant VTS as VisuoTactileSensor
    participant CAM as Camera
    participant RND as BaseRenderer

    VTS->>VTS: "__init__(cfg)<br/>initialize attrs first"
    VTS->>CAM: Camera(camera_cfg)
    CAM->>CAM: _check_supported_data_types(cfg)
    CAM->>CAM: super().__init__(cfg)
    CAM->>CAM: "_renderer = None  ← fix"
    CAM->>CAM: spawn asset (may raise)

    VTS->>CAM: _initialize_impl()
    CAM->>RND: create renderer
    CAM-->>VTS: initialized

    VTS->>CAM: update(dt) / data.output
    CAM->>RND: read_output(render_data, camera_data)
    RND-->>CAM: filled CameraData
    CAM-->>VTS: data

    Note over VTS,RND: Teardown
    VTS->>CAM: __del__() ← called directly (pre-existing)
    CAM->>CAM: super().__del__()
    CAM->>RND: cleanup(render_data)
    Note over CAM,RND: GC also calls Camera.__del__ again<br/>→ cleanup called twice
Loading

Comments Outside Diff (2)

  1. source/isaaclab/isaaclab/sensors/camera/camera.py, line 179-185 (link)

    P2 __del__ still fragile if __init__ raises before super().__init__()

    self._renderer = None is set after super().__init__(cfg), so if _check_supported_data_types(cfg) or super().__init__(cfg) itself raises, __del__ will still hit AttributeError on self._renderer. Using getattr makes the guard unconditionally safe regardless of how far __init__ got.

  2. source/isaaclab_contrib/isaaclab_contrib/sensors/tacsl_sensor/visuotactile_sensor.py, line 128-133 (link)

    P2 Calling __del__ directly risks double-cleanup of the renderer

    self._camera_sensor.__del__() is called here explicitly, but Python's GC will also invoke Camera.__del__ again when the _camera_sensor reference count drops to zero after VisuoTactileSensor is torn down. With the new Camera.__del__ now explicitly calling self._renderer.cleanup(self._render_data), and _renderer not set to None after that call, this produces a double invocation of cleanup(). If cleanup() is not idempotent the second call can silently corrupt renderer state or raise.

    The standard fix is to not call __del__ directly; let Python handle it, or expose a dedicated close() / cleanup() method on Camera and call that instead.

Reviews (1): Last reviewed commit: "Replace deprecated TiledCamera with Came..." | Re-trigger Greptile

Copy link
Copy Markdown

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

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 replaces the deprecated TiledCamera/TiledCameraCfg with Camera/CameraCfg in the visuotactile sensor (isaaclab_contrib), removing the unnecessary deprecation shim that was causing CI failures. It also fixes a real AttributeError crash in Camera.__del__ by moving data-attribute initialization earlier in __init__ — before any code paths that could raise.

Both changes are well-motivated, correctly scoped, and low-risk.

Design Assessment

Design is sound. The TiledCameraCamera migration is the intended upgrade path established in #5162. Since TiledCameraCfg is just a CameraCfg subclass with class_type pointing to TiledCamera (which itself is just a thin Camera subclass emitting a deprecation warning), the behavioral equivalence is guaranteed. The __init__ reordering in camera.py is a defensive hardening that makes __del__ safe regardless of where __init__ fails — a good practice for classes with custom destructors.

Findings

🔵 Suggestion: Changelog section for isaaclab_contribsource/isaaclab_contrib/docs/CHANGELOG.rst

The contrib changelog files this under Changed, which is reasonable since it's migrating to a newer API. However, since the PR description states this is a bug fix (CI failures caused by the deprecation layer), consider adding a Fixed entry as well — or re-categorizing the existing entry under Fixed — to make it clear this resolves a CI regression, not just a cosmetic API migration.

🔵 Suggestion: Pre-existing __del__ anti-patternsource/isaaclab_contrib/isaaclab_contrib/sensors/tacsl_sensor/visuotactile_sensor.py:130

Not introduced by this PR, but worth noting: VisuoTactileSensor.__del__ directly calls self._camera_sensor.__del__(). Explicit __del__() calls are an anti-pattern in Python — they don't prevent the garbage collector from calling __del__ again later (Python doesn't track manual calls), which means Camera.__del__ could run twice: once explicitly here, once during GC. This is now safe thanks to the if self._renderer is not None guard in Camera.__del__, but a cleaner pattern would be to set self._camera_sensor = None after cleanup or use del self._camera_sensor. This is out of scope for this PR but worth a follow-up.

Test Coverage

  • Bug fix: The existing visuotactile sensor tests (test_visuotactile_sensor.py) are updated to use CameraCfg instead of TiledCameraCfg, which validates the new code path. Since the bug manifested as CI failures in these exact tests, the tests themselves serve as the regression test — if the deprecation layer interaction recurs, these tests would fail again.
  • Camera.__del__ fix: No dedicated unit test for the __del__ crash. This is acceptable — testing destructor behavior in Python is inherently fragile and the fix is self-evidently correct (moving initialization before potential raise points). The defensive if self._renderer is not None guard makes the fix robust without needing a test.
  • Coverage verdict: Adequate for a bug fix of this nature.

CI Status

Core checks passing (pre-commit ✅, broken links ✅, labeler ✅). Docker/docs builds are still pending — these are infrastructure jobs unlikely to be affected by Python source changes.

Verdict

Ship it 🚀

Clean, minimal, well-documented bug fix. The migration is mechanical and correct, the __del__ hardening is a genuine improvement, changelogs and version bumps are included, and tests are updated. No blocking issues.

@AntoineRichard AntoineRichard deleted the antoiner/fix-visuotactile-test branch April 20, 2026 07:17
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.

1 participant