Expose is_tracked for OpenXR tracking loss - #909
Conversation
📝 WalkthroughWalkthrough
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant OpenXR
participant LiveHeadTrackerImpl
participant HeadSource
participant HeadTransform
OpenXR->>LiveHeadTrackerImpl: provide pose validity and tracking flags
LiveHeadTrackerImpl->>HeadSource: provide pose, is_valid, and is_tracked
HeadSource->>HeadTransform: provide head input fields
HeadTransform->>HeadSource: preserve validity and tracking state
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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/core/retargeting_engine_tests/python/test_sources.py`:
- Around line 270-275: Update the HeadSource documentation in output_spec and
_compute_fn to define missing output solely as tracked.data being None, and
explicitly require consumers to inspect HeadInputIndex.IS_VALID before using the
pose. Ensure the documented contract supports non-null placeholder or stale pose
data when is_valid is False.
🪄 Autofix
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: 565c20fc-90dc-4bef-84af-233574ec41e0
📒 Files selected for processing (3)
src/core/live_trackers/cpp/live_head_tracker_impl.cppsrc/core/retargeting_engine_tests/python/test_sources.pysrc/core/schema/fbs/head.fbs
9bab5fd to
81f787e
Compare
Map VALID/TRACKED bits separately so is_valid stays readable-pose semantics while is_tracked clears on CloudXR disconnect / tracking loss. Signed-off-by: Sergey Grizan <sgrizan@nvidia.com>
81f787e to
f49cb46
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
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/core/schema_tests/cpp/test_head.cpp`:
- Line 183: Update the test setup before core::HeadPoseRecord::Pack to
initialize the source record’s data is_tracked field to true, so the existing
deserialized->data()->is_tracked() == true assertion in the test remains valid.
🪄 Autofix
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: 15300e9d-dbd9-4e6c-931b-e9ed69b5f442
📒 Files selected for processing (11)
examples/teleop_ros2/python/tests/test_messages.pysrc/core/live_trackers/cpp/live_head_tracker_impl.cppsrc/core/retargeting_engine_tests/python/test_sources.pysrc/core/retargeting_engine_tests/python/test_transforms.pysrc/core/schema/fbs/head.fbssrc/core/schema/python/head_bindings.hsrc/core/schema_tests/cpp/test_head.cppsrc/core/schema_tests/python/test_head.pysrc/python/isaacteleop/retargeting_engine/deviceio_source_nodes/head_source.pysrc/python/isaacteleop/retargeting_engine/tensor_types/standard_types.pysrc/python/isaacteleop/retargeting_engine/utilities/head_transform.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/core/live_trackers/cpp/live_head_tracker_impl.cpp
| // separate so is_valid stays OpenXR-faithful (CloudXR may leave VALID set after disconnect). | ||
| const bool position_valid = (location.locationFlags & XR_SPACE_LOCATION_POSITION_VALID_BIT) != 0; | ||
| const bool orientation_valid = (location.locationFlags & XR_SPACE_LOCATION_ORIENTATION_VALID_BIT) != 0; | ||
| const bool position_tracked = (location.locationFlags & XR_SPACE_LOCATION_POSITION_TRACKED_BIT) != 0; |
There was a problem hiding this comment.
see #866: I don't think we are setting XR_SPACE_LOCATION_POSITION_TRACKED_BIT correctly...
There was a problem hiding this comment.
Doesn't the bug mention that the head poses are unaffected and forward the client's flags faithfully?
Signed-off-by: Sergey Grizan <sgrizan@nvidia.com>
Signed-off-by: Sergey Grizan <sgrizan@nvidia.com>
Require VALID and TRACKED bits for head is_valid so CloudXR client disconnect no longer leaves a placeholder pose marked valid.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation