Replaces deprecated TiledCamera with Camera in visuotactile sensor#5269
Replaces deprecated TiledCamera with Camera in visuotactile sensor#5269AntoineRichard wants to merge 1 commit intodevelopfrom
Conversation
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__.
Greptile SummaryThis PR replaces the deprecated Confidence Score: 5/5
Important Files Changed
Sequence DiagramsequenceDiagram
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
|
There was a problem hiding this comment.
🤖 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 TiledCamera → Camera 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_contrib — source/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-pattern — source/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 useCameraCfginstead ofTiledCameraCfg, 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 potentialraisepoints). The defensiveif self._renderer is not Noneguard 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.
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
Checklist
pre-commitchecks with./isaaclab.sh --formatconfig/extension.tomlfileCONTRIBUTORS.mdor my name already exists there