[BUG FIX] Fix sensor camera 'lookat' being ignored when 'entity_idx' is set.#2614
Conversation
|
Please share the PR on HuggingFace here. |
|
@Lidang-Jiang Still waiting for your PR on HuggingFace: |
|
@Lidang-Jiang The snapshot in your PR is blank. This is not ok. |
In fixing ,pr will be updated soon. |
|
Hi @duburcqa, here's the HuggingFace snapshot PR: Update: Verified that both CPU and GPU ( Will update |
|
@Lidang-Jiang Still a blank image. Not ok. Add a tiled ground plane. We never see the sphere. Maybe add several sphere of colours at key locations, one of them being visible for each camera. Moreover, there is a single snapshot instead of one per camera. This is not ok. |
|
Updated — addressed all feedback:
Code pushed in a625019. |
|
@Lidang-Jiang great, thank you. Still, yellow and blue sphere are barely visible and the ground plane is not tiled (remove surface=gs.surfaces.Rough(color=(0.4, 0.4, 0.4)),). |
|
Fixed in 93c975e:
HuggingFace snapshots updated. |
Last change, please move camera further away instead. |
|
merged on HF. |
|
HF PR has been merged, and I updated |
|
Also reran |
|
Ok let me finish this PR at this point. |
When attaching a camera to an entity via entity_idx, the lookat and up parameters were silently ignored because move_to_attach() used trans_to_T(pos) which only sets translation, discarding orientation. Replace trans_to_T(pos) with pos_lookat_up_to_T(pos, lookat, up) to preserve the camera orientation defined by lookat/up parameters. Fixes Genesis-Embodied-AI#2591 Signed-off-by: Lidang-Jiang <lidangjiang@gmail.com>
- Merge 3 tests into single test_camera_lookat_entity - Remove comments, docstrings, and unused pos_lookat_up_to_T import - Add png_snapshot check for attached camera rendering Signed-off-by: Lidang-Jiang <lidangjiang@gmail.com>
- Remove _blurred_kernel_size (not proven necessary) - Remove surface parameter from sphere entity - Use common_options dict + for loop for camera creation - Use for loop for pair-wise image assertions Signed-off-by: Lidang-Jiang <lidangjiang@gmail.com>
- Add gray ground plane and 4 colored spheres (red/green/blue/yellow) at distinct cardinal positions for visual clarity - Add snapshot assertion for every camera (4 total) instead of just one - Increase sphere radius and adjust camera distances so spheres are clearly visible in 64x64 snapshots Addresses reviewer feedback on snapshot quality. Signed-off-by: Lidang-Jiang <lidangjiang@gmail.com>
- Remove surface override to use default tiled ground plane - Move blue/yellow spheres closer to attach entity for visibility - Brighten blue (0.3,0.3,1.0) and yellow (1.0,1.0,0.0) colors - Reposition attached cameras for clearer sphere views Signed-off-by: Lidang-Jiang <lidangjiang@gmail.com>
Move attached camera offset from (0,0,0.5) to (0,0,1.5) and restore blue/yellow sphere positions to 1.5m from center as reviewer requested. Signed-off-by: Lidang-Jiang <lidangjiang@gmail.com>
Signed-off-by: Lidang-Jiang <lidangjiang@gmail.com>
duburcqa
force-pushed
the
fix/camera-lookat-entity
branch
3 times, most recently
from
6fc7436 to
e47cf34
Compare
|
@claude review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
duburcqa
force-pushed
the
fix/camera-lookat-entity
branch
from
e47cf34 to
966b622
Compare
duburcqa
force-pushed
the
fix/camera-lookat-entity
branch
from
390cf90 to
12d0a8e
Compare
|
@claude review |
|
@claude review |
There was a problem hiding this comment.
LGTM — all three previously flagged issues have been addressed.
Extended reasoning...
Overview
The PR fixes a bug where BaseCameraSensor.move_to_attach() and RaytracerCameraSensor.build() used trans_to_T(pos) (translation-only) instead of pos_lookat_up_to_T(pos, lookat, up) when computing the offset transform for entity-attached cameras. Additional changes in rasterizer.py, viewer.py, and ext/pyrender/viewer.py thread an env_separate_rigid override through the render path so that per-env camera poses trigger env-separate rendering automatically.
Security risks
None. All changes are internal rendering-path logic with no auth, crypto, or permission surfaces involved.
Level of scrutiny
Moderate. The fix is a small, well-motivated one-liner (plus test scaffolding), the logic is easy to audit, and the regression tests are comprehensive. The env_separate_rigid threading change is mechanical and correctly saves/restores the flag around each render call.
Resolution of prior review
My previous review flagged three issues, all of which have been resolved in the current revision: (1) the Raytracer build() path now uses pos_lookat_up_to_T; (2) the shadow-disable loop in test_camera_lookat_entity now uses the correct loop variable; (3) test_rasterizer_attached_batched now uses trans_quat_to_T with the sphere quaternion. The HuggingFace snapshot PR was merged and HUGGINGFACE_SNAPSHOT_REVISION updated accordingly.
duburcqa
changed the title
duburcqa
changed the title
|
🔴 Benchmark Regression Detected ➡️ Report |
2 participants
Summary
BaseCameraSensor.move_to_attach()silently ignoring thelookatandupparameters when a camera is attached to an entity viaentity_idx.trans_to_T(pos)(translation-only) withpos_lookat_up_to_T(pos, lookat, up)to preserve the full camera orientation.offset_T, and non-regression for detached cameras.Fixes #2591
Root cause
In
BaseCameraSensor.move_to_attach(), when no explicitoffset_Tis provided, the offset transform was computed usingtrans_to_T(pos), which only encodes the position as a translation matrix and discards the rotation derived fromlookat/up. This caused all entity-attached cameras to default to identity rotation regardless of theirlookattarget.Fix: One-line change from
trans_to_T(pos)topos_lookat_up_to_T(pos, lookat, up), which correctly constructs a 4x4 transform encoding both position and orientation.Snapshot update required
This fix changes the rendering output for
test_rasterizer_attached_batchedbecause the attached camera now correctly applies thelookatorientation instead of using identity rotation. The snapshot for this test needs to be regenerated by a maintainer with the reference hardware/GPU setup.Before fix (lookat ignored)
Two cameras attached to the same entity with different
lookattargets produce identical images (mean pixel: 102.3313for both). Thelookatparameter is silently ignored.After fix (lookat correctly applied)
Two cameras with different
lookattargets now produce different images (mean pixel: 20.3333vs50.6091), confirming thelookatparameter is correctly applied.Unit test results (6 passed, 7 skipped)
test_attached_camera_lookat_affects_orientation-- different lookat targets produce different imagestest_attached_camera_lookat_matches_offset_T-- lookat-derived transform matches equivalent explicit offset_Ttest_detached_camera_lookat_still_works-- non-regression: detached cameras still work correctlyTest plan
lookatproduce identical images (before fix)test_attached_camera_lookat_affects_orientation-- PASSEDtest_attached_camera_lookat_matches_offset_T-- PASSEDtest_detached_camera_lookat_still_works-- PASSED (non-regression)test_rasterizer_attached_batchedsnapshot needs regeneration by maintainer (expected due to corrected rendering)