Fix bug in camera replication skip - #6951
Conversation
Greptile SummaryThe PR expands source-only world-attached frame and camera views across clone-plan destinations without requiring replicated USD prims.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart LR
Plan[Clone plan] --> Sources[Resolve source-only frames]
Sources --> Anchors[Resolve destination environment anchors]
Anchors --> Project[Project source and parent transforms]
Project --> View[Populate synthetic FrameView rows]
Sources --> Cameras[Reconstruct camera prim mapping]
Cameras --> Render[Prepare per-environment camera rendering]
Reviews (2): Last reviewed commit: "Fix bug in camera replication for ovphys..." | Re-trigger Greptile |
| device = "cpu" | ||
| OVPHYSX_SIM_CFG.device = device | ||
| with build_simulation_context( | ||
| device=device, sim_cfg=OVPHYSX_SIM_CFG, auto_add_lighting=False, add_ground_plane=False |
There was a problem hiding this comment.
Rotation projection remains untested
The new transform-composition path is sensitive to matrix order, but this regression covers only translated environment anchors. Add a non-identity source or destination rotation so an ordering regression cannot report incorrect world-frame poses while this test still passes.
Knowledge Base Used: IsaacLab Core Simulation Layer
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 clone-plan-driven expansion for source-only world-attached cameras is coherent and covered by an OVPhysX regression test. Before merge, add the required Isaac Lab changelog fragment and normalize projected transforms before quaternion extraction so scaled or sheared Xforms do not produce invalid orientations.
- Design and architecture: The projection path consistently derives per-environment synthetic frame rows from the clone plan and connects them to camera initialization. The overall approach is sound, but projected world transforms must preserve the frame-view pose contract by removing scale and shear before conversion to position and quaternion.
- API: No public symbols or signatures change. Camera initialization does gain user-visible clone-plan expansion and stricter failure behavior, but this change is currently recorded only in the OVPhysX changelog; repository policy requires a separate fragment for the touched
isaaclabpackage. - Implementation: Source resolution, synthetic-path assignment, environment ordering, and buffer sizing are coherent. However,
_expand_world_sites_from_clone_plan.project_worldpasses composed matrices directly to quaternion extraction, unlike the adjacent resolver that orthonormalizes transforms first. Add equivalent normalization and addsource/isaaclab/changelog.d/<slug>.rstwith aFixedentry.
Minor fixes needed. Posted 2 actionable findings inline.
Automated review; human maintainers own approval decisions.
| self._frame = ProxyArray(wp.zeros(self._view.count, device=self._device, dtype=wp.int64)) | ||
|
|
||
| # Convert all encapsulated prims to Camera. Newton keeps only source USD camera prims. | ||
| # Convert source or destination prims to cameras. Source-only views project each source onto its clones. |
There was a problem hiding this comment.
🟡 Warning · Implementation — Missing changelog fragment for isaaclab package
This PR changes user-visible behavior in the isaaclab package (camera prim resolution now expands via the clone plan and raises instead of falling back to the first camera prim), but the only fragment added is under source/isaaclab_ovphysx/changelog.d/. Repository rules require one fragment per touched package, so the compiled isaaclab changelog and version bump will omit this fix. Add source/isaaclab/changelog.d/<slug>.rst with a Fixed entry.
| destination_anchor_world = xform_cache.GetLocalToWorldTransform(destination_anchor) | ||
|
|
||
| def project_world(prim: Usd.Prim) -> list[float]: | ||
| source_world = xform_cache.GetLocalToWorldTransform(prim) |
There was a problem hiding this comment.
🔵 Suggestion · Implementation — Projection skips orthonormalization before quaternion extraction
_resolve_rigid_body_ancestor orthonormalizes every world matrix before calling _gf_matrix_to_xform7, because quaternion extraction from a matrix carrying scale or shear yields an incorrect rotation. project_world composes the source, inverse anchor, and destination anchor transforms without any Orthonormalize() call, so a scaled source or env-root Xform produces invalid site orientations. Orthonormalize the composed transform to match the existing pattern.
Keep clone row ownership in OvPhysxFrameView so cameras consume row-aligned source prims without duplicating clone-plan queries. Correct the regression setup to exercise a genuinely source-only stage.
Drop the temporary kitless USD replication default from isaac-sim#6945. OVPhysX FrameView now expands source-only camera prims from the clone plan, so sensors no longer need to author destination prims.
Description
Fixed a bug where ovphysx was not correctly expanding world-attached cameras
Checklist
pre-commitchecks with./isaaclab.sh --formatsource/<pkg>/changelog.d/for every touched package (do not editCHANGELOG.rstor bumpextension.toml— CI handles that)CONTRIBUTORS.mdor my name already exists there