fix(retargeters): guard against invalid controller grip poses in Se3 retargeters#743
fix(retargeters): guard against invalid controller grip poses in Se3 retargeters#743hougantc-nvda wants to merge 2 commits into
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR adds a guard in both Estimated code review effort: 2 (Simple) | ~12 minutes Sequence Diagram(s)sequenceDiagram
participant Controller
participant Se3AbsRetargeter
participant Se3RelRetargeter
Controller->>Se3AbsRetargeter: ControllerInput (GRIP_IS_VALID=false)
Se3AbsRetargeter->>Se3AbsRetargeter: write ee_pose from _last_pose
Se3AbsRetargeter-->>Controller: return early
Controller->>Se3RelRetargeter: ControllerInput (GRIP_IS_VALID=false)
Se3RelRetargeter->>Se3RelRetargeter: set _first_frame=True, ee_delta=0
Se3RelRetargeter-->>Controller: return early
Controller->>Se3RelRetargeter: ControllerInput (GRIP_IS_VALID=true, next frame)
Se3RelRetargeter->>Se3RelRetargeter: re-baseline from valid input
Se3RelRetargeter-->>Controller: emit ee_delta (no jump)
Related issues: None referenced. Related PRs: None referenced. Suggested labels: bug, tests Suggested reviewers: None identified from provided data. 🐇 A quaternion of zero, a grip gone astray, 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/retargeters/se3_retargeter.py`:
- Around line 394-401: Reset the retargeter’s smoothing state when grip tracking
is invalid in se3_retargeter.py. In the invalid-grip early return inside the SE3
retargeter logic, do not only set _first_frame; also clear the cached smoothing
buffers (_smoothed_delta_pos and _smoothed_delta_rot, or their equivalents in
the class) so the next valid sample starts clean instead of blending with stale
motion. Use the existing invalid-input branch around the grip validity check to
locate the fix.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: cfc47aaf-1540-4bc1-b9fb-d7511ef6cb6f
📒 Files selected for processing (2)
src/core/retargeting_engine_tests/python/test_se3_retargeter_pose_validity.pysrc/retargeters/se3_retargeter.py
Re-arming _first_frame alone drops only the rebaseline sample; the smoothed-delta EMA buffers still hold pre-loss motion at full strength (they never decay while the invalid branch returns early), so the second valid frame after recovery blends stale motion into the output. Zero both buffers in the invalid-grip branch, matching the reset handler's convention for re-arming the first-frame baseline. Addresses CodeRabbit review feedback on #743. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Hougant Chen <hougantc@nvidia.com>
…retargeters A connected controller with pose tracking lost (e.g. resting on a table) produces a present ControllerInput group with grip_is_valid=False and a zero-norm grip orientation. Se3AbsRetargeter and Se3RelRetargeter fed that quaternion straight to Rotation.from_quat, raising 'ValueError: Found zero norm quaternions' and killing the retargeting pipeline (in pipelined mode the async retarget worker dies permanently, forcing consumers into session teardown/restart loops). Guard the controller branch with GRIP_IS_VALID, mirroring the hand branch's joint_valid handling: Abs holds the last pose (same as the is_none path); Rel emits a zero delta and re-arms the first-frame baseline so the first valid frame after recovery does not jump. Signed-off-by: Hougant Chen <hougantc@nvidia.com>
Re-arming _first_frame alone drops only the rebaseline sample; the smoothed-delta EMA buffers still hold pre-loss motion at full strength (they never decay while the invalid branch returns early), so the second valid frame after recovery blends stale motion into the output. Zero both buffers in the invalid-grip branch, matching the reset handler's convention for re-arming the first-frame baseline. Addresses CodeRabbit review feedback on #743. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Hougant Chen <hougantc@nvidia.com>
60d4e7b to
86a8c62
Compare
Description
A connected controller with pose tracking lost (e.g. resting on a table) produces a present
ControllerInputgroup withgrip_is_valid=Falseand a zero-norm grip orientation.Se3AbsRetargeterandSe3RelRetargeterfed that quaternion straight toRotation.from_quat, raisingValueError: Found zero norm quaternionsand killing the retargeting pipeline (in pipelined mode the async retarget worker dies permanently, forcing consumers into session teardown/restart loops).This change guards the controller branch with
GRIP_IS_VALID, mirroring the hand branch'sjoint_validhandling:is_nonepath).Type of change
Testing
Added
test/python/test_se3_retargeter_pose_validity.pycovering both retargeters: invalid grip pose no longer raises, Abs holds the last valid pose, Rel emits a zero delta and does not jump on the first valid frame after recovery.Checklist
SKIP=check-copyright-year pre-commit run --all-filesgit commit -s) per the DCO🤖 Generated with Claude Code
Summary by CodeRabbit