Optimize shared camera image observations - #7440
Conversation
Greptile SummaryThe PR consolidates camera observations onto the shared image MDP path and removes redundant image, manager, and sensor-update work.
Confidence Score: 4/5The PR should not merge until the Kuka Allegro camera migration preserves the established RGB and depth observation semantics or explicitly migrates the policy contract. The shared image path applies per-frame mean subtraction to RGB and leaves finite depth in raw distance units, whereas the removed Kuka term used fixed RGB centering and bounded tanh depth values consumed directly by its CNN. Files Needing Attention: source/isaaclab_tasks/isaaclab_tasks/core/lift/config/kuka_allegro/camera_cfg.py; source/isaaclab_tasks/isaaclab_tasks/core/lift/mdp/observations.py Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart LR
R[Renderer] --> C[Camera ProxyArray]
C --> I[Shared image MDP term]
I --> N[Normalize and convert layout]
N --> O[Observation manager]
O --> P[Policy CNN]
Reviews (1): Last reviewed commit: "Optimize shared camera image observation..." | Re-trigger Greptile |
| func=mdp.image, | ||
| params={ | ||
| "sensor_cfg": SceneEntityCfg("base_camera"), | ||
| "data_type": None, | ||
| "permute": True, | ||
| "clone": False, |
There was a problem hiding this comment.
Camera normalization contract changes
When a Kuka Allegro task selects an RGB or depth camera preset, the shared mdp.image term replaces the previous fixed RGB centering and bounded depth transformation with per-frame RGB mean subtraction or raw finite depth values. The policy CNN consumes these values directly, so existing policies and checkpoints receive a different observation distribution and produce incorrect behavior.
Knowledge Base Used:
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Isaac Lab Review Bot
The optimization direction is sound, but the Kuka Allegro migration changes established camera normalization semantics, removes the exported vision_camera API without deprecation, and breaks positional callers of normalize_image_uint8 by inserting a parameter before out. These compatibility issues should be resolved before merge.
- Design and architecture: Consolidating task-specific camera observations onto the shared image MDP term reduces duplicated work, but the shared term does not preserve the stationary RGB/depth normalization and NaN handling required by the migrated Kuka Allegro path. The migration therefore changes model inputs rather than serving as a behavior-preserving consolidation.
- API: Deleting the publicly exported
isaaclab_tasks.core.lift.mdp.vision_cameraviolates the repository’s deprecation requirement and lacks migration guidance. Additionally, insertingoutput_channel_dimbefore the existingoutparameter innormalize_image_uint8rebinds valid positional calls; the new parameter should be appended or made keyword-only. - Implementation: The fused Warp layout conversion and single-term observation shortcut are supported by the shown implementation and tests. However, replacing
vision_camerawithmdp.imagechanges RGB normalization fromx/255 - 0.5to per-image mean subtraction, changes depth normalization fromtanh(depth/2) - 0.5to infinity replacement only, and no longer performs the same NaN sanitization. The prior transforms should be preserved or introduced through an explicit compatibility path.
Significant concerns. Posted 3 actionable findings inline.
Automated review; human maintainers own approval decisions.
| Fixed | ||
| ^^^^^ | ||
|
|
||
| * Reused the shared image observation term across camera tasks, including dexterous Lift, and |
There was a problem hiding this comment.
🟡 Warning · Api — Removed public vision_camera lacks deprecation path
vision_camera is exported from isaaclab_tasks.core.lift.mdp (__all__ and __init__.pyi) and is deleted outright, so downstream configs importing it now fail with AttributeError. Repository rules require a prior deprecation and migration path, and changelog fragments must mark breaking changes with migration guidance; this fragment mentions only reuse of the shared term. Keep a deprecated alias or document the removal and replacement.
| noise=Unoise(n_min=-0.0, n_max=0.0), | ||
| clip=(-1.0, 1.0), | ||
| params={"sensor_cfg": SceneEntityCfg("base_camera")}, | ||
| func=mdp.image, |
There was a problem hiding this comment.
🟡 Warning · Api — Kuka camera normalization semantics silently changed
The replaced vision_camera applied nan_to_num_ plus stationary scaling (x/255 - 0.5, and tanh(d/2) - 0.5 for the depth/raycaster presets). mdp.image with normalize=True instead subtracts a per-frame spatial mean for RGB and only maps inf -> 0 for depth, leaving NaNs and unbounded metres. Existing Isaac-Lift-KukaAllegro-Camera configs and checkpoints therefore see a different observation distribution. Preserve the prior transforms or make the break explicit.
| def normalize_image_uint8( | ||
| src: torch.Tensor, | ||
| channel_dim: int = -1, | ||
| output_channel_dim: int | None = None, |
There was a problem hiding this comment.
🔵 Suggestion · Api — New parameter inserted before existing out argument
normalize_image_uint8 previously had the signature (src, channel_dim=-1, out=None). Placing output_channel_dim third rebinds an existing positional call such as normalize_image_uint8(src, 1, out_buf), so the tensor is treated as a channel index and fails during resolution. Appending the new parameter after out, or making it keyword-only, preserves the existing positional contract.
Summary
Performance
Measured with 4096
Isaac-Lift-KukaAllegro-Cameraenvironments using Newton physics, the Newton renderer, and 128x128 RGB:Validation
The repository-wide changelog hook remains blocked by a pre-existing modified immutable
isaaclab_ovfragment and unrelated missing package fragments.