refactor(deviceio): generic FullBodyTracker + session-level vendor selection#781
Draft
aristarkhovNV wants to merge 10 commits into
Draft
refactor(deviceio): generic FullBodyTracker + session-level vendor selection#781aristarkhovNV wants to merge 10 commits into
aristarkhovNV wants to merge 10 commits into
Conversation
Contributor
|
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:
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/core/mcap/cpp/inc/mcap/recording_traits.hpp (1)
41-46: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPreserve the deprecated compatibility aliases.
AddFullBodyPicoRecordingTraitsandReplayFullBodyTrackerPicoImplas deprecated aliases, or keep a forwarding header, so existing consumers of the old public names still compile.🤖 Prompt for 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. In `@src/core/mcap/cpp/inc/mcap/recording_traits.hpp` around lines 41 - 46, Preserve backward compatibility by adding deprecated aliases for FullBodyPicoRecordingTraits in recording_traits.hpp and ReplayFullBodyTrackerPicoImpl in replay_full_body_tracker_impl.hpp, or retain forwarding headers exposing those legacy public names. Ensure existing consumers can continue compiling without changing the current FullBodyRecordingTraits and replay implementation.src/core/live_trackers/cpp/live_deviceio_factory.cpp (1)
193-215: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winValidate the complete vendor mapping during extension discovery.
This path only examines mappings matching a
FullBodyTracker; entries for absent/non-vendored trackers and duplicate mappings are silently ignored, while the factory constructor later rejects them. Validate membership, tracker support, vendor IDs, and duplicates here using the same shared routine so extension discovery and session creation accept identical configurations.🤖 Prompt for 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. In `@src/core/live_trackers/cpp/live_deviceio_factory.cpp` around lines 193 - 215, Update LiveDeviceIOFactory::get_required_extensions to validate the complete tracker_vendors mapping before resolving extensions, using the same shared validation routine as the factory constructor. Ensure it rejects mappings for absent or unsupported trackers, invalid vendor IDs, and duplicate entries, so extension discovery and session creation accept identical configurations.src/core/deviceio_session/python/session_bindings.cpp (1)
130-154: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAccept
Noneforvendor_configor stop passing it from the live-session path.teleop_session.pypassesvendor_config=Nonewhen no vendors are configured, but these bindings require a concretecore::VendorConfig. That makes the default live-session path raise aTypeError; normalizeNonetocore::VendorConfig{}before callingget_required_extensions/run, or construct an empty config in the caller.🤖 Prompt for 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. In `@src/core/deviceio_session/python/session_bindings.cpp` around lines 130 - 154, Normalize a Python None vendor_config to an empty core::VendorConfig before the live-session calls to get_required_extensions and run, or stop passing None from teleop_session.py. Ensure both bindings accept the default no-vendor path without a TypeError while preserving explicitly supplied VendorConfig values.
🤖 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/teleop_session_manager/python/teleop_session.py`:
- Around line 972-992: Initialize vendor_config as an empty
deviceio.VendorConfig instead of None in the teleop session setup. Preserve the
existing tracker_vendors loop and populate that config with vendor_entries when
selections are provided, then pass the VendorConfig to both
DeviceIOSession.get_required_extensions() and DeviceIOSession.run().
---
Outside diff comments:
In `@src/core/deviceio_session/python/session_bindings.cpp`:
- Around line 130-154: Normalize a Python None vendor_config to an empty
core::VendorConfig before the live-session calls to get_required_extensions and
run, or stop passing None from teleop_session.py. Ensure both bindings accept
the default no-vendor path without a TypeError while preserving explicitly
supplied VendorConfig values.
In `@src/core/live_trackers/cpp/live_deviceio_factory.cpp`:
- Around line 193-215: Update LiveDeviceIOFactory::get_required_extensions to
validate the complete tracker_vendors mapping before resolving extensions, using
the same shared validation routine as the factory constructor. Ensure it rejects
mappings for absent or unsupported trackers, invalid vendor IDs, and duplicate
entries, so extension discovery and session creation accept identical
configurations.
In `@src/core/mcap/cpp/inc/mcap/recording_traits.hpp`:
- Around line 41-46: Preserve backward compatibility by adding deprecated
aliases for FullBodyPicoRecordingTraits in recording_traits.hpp and
ReplayFullBodyTrackerPicoImpl in replay_full_body_tracker_impl.hpp, or retain
forwarding headers exposing those legacy public names. Ensure existing consumers
can continue compiling without changing the current FullBodyRecordingTraits and
replay implementation.
🪄 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: 3048dd41-9569-4c50-a116-e80c3206cb89
📒 Files selected for processing (37)
docs/source/device/body_tracking.rstdocs/source/device/trackers.rstdocs/source/getting_started/teleop_session.rstexamples/oxr/python/test_full_body_tracker.pyexamples/teleop_ros2/cpp/integration_tests/mcap_generator.cppsrc/core/deviceio_base/cpp/inc/deviceio_base/full_body_tracker_base.hppsrc/core/deviceio_base/cpp/inc/deviceio_base/full_body_tracker_pico_base.hppsrc/core/deviceio_base/cpp/inc/deviceio_base/tracker_vendor.hppsrc/core/deviceio_session/cpp/deviceio_session.cppsrc/core/deviceio_session/cpp/inc/deviceio_session/deviceio_session.hppsrc/core/deviceio_session/python/deviceio_session_init.pysrc/core/deviceio_session/python/session_bindings.cppsrc/core/deviceio_trackers/cpp/CMakeLists.txtsrc/core/deviceio_trackers/cpp/full_body_tracker.cppsrc/core/deviceio_trackers/cpp/full_body_tracker_pico.cppsrc/core/deviceio_trackers/cpp/inc/deviceio_trackers/full_body_tracker.hppsrc/core/deviceio_trackers/cpp/inc/deviceio_trackers/full_body_tracker_pico.hppsrc/core/deviceio_trackers/python/deviceio_trackers_init.pysrc/core/deviceio_trackers/python/tracker_bindings.cppsrc/core/live_trackers/cpp/inc/live_trackers/live_deviceio_factory.hppsrc/core/live_trackers/cpp/live_deviceio_factory.cppsrc/core/live_trackers/cpp/live_full_body_tracker_pico_impl.cppsrc/core/live_trackers/cpp/live_full_body_tracker_pico_impl.hppsrc/core/mcap/cpp/inc/mcap/recording_traits.hppsrc/core/python/deviceio_init.pysrc/core/replay_trackers/cpp/CMakeLists.txtsrc/core/replay_trackers/cpp/inc/replay_trackers/replay_deviceio_factory.hppsrc/core/replay_trackers/cpp/replay_deviceio_factory.cppsrc/core/replay_trackers/cpp/replay_full_body_tracker_impl.cppsrc/core/replay_trackers/cpp/replay_full_body_tracker_impl.hppsrc/core/replay_trackers/cpp/replay_full_body_tracker_pico_impl.hppsrc/core/retargeting_engine/python/deviceio_source_nodes/__init__.pysrc/core/retargeting_engine/python/deviceio_source_nodes/deviceio_tensor_types.pysrc/core/retargeting_engine/python/deviceio_source_nodes/full_body_source.pysrc/core/teleop_session_manager/python/config.pysrc/core/teleop_session_manager/python/teleop_session.pysrc/core/teleop_session_manager_tests/python/test_teleop_session.py
💤 Files with no reviewable changes (4)
- src/core/deviceio_base/cpp/inc/deviceio_base/full_body_tracker_pico_base.hpp
- src/core/deviceio_trackers/cpp/inc/deviceio_trackers/full_body_tracker_pico.hpp
- src/core/deviceio_trackers/cpp/full_body_tracker_pico.cpp
- src/core/replay_trackers/cpp/replay_full_body_tracker_pico_impl.hpp
…lection
Rename the Pico full-body tracker stack to a vendor-agnostic FullBodyTracker
marker and introduce a session-level vendor mechanism, so the tracker is
agnostic to both vendor and live/replay — vendor is chosen at live-session
construction, mirroring how live-vs-replay is already chosen.
- FullBodyTrackerPico -> FullBodyTracker (bare marker; DEFAULT_VENDOR_ID
"body.pico-xr"). IFullBodyTrackerPicoImpl -> IFullBodyTrackerImpl;
ReplayFullBodyTrackerPicoImpl -> ReplayFullBodyTrackerImpl (vendor-neutral);
FullBodyPicoRecordingTraits -> FullBodyRecordingTraits. The native XR impl
keeps its name (LiveFullBodyTrackerPicoImpl) as the "body.pico-xr" vendor impl.
- New TrackerVendor{id, params} + VendorConfig, threaded through the live
DeviceIOSession::run and get_required_extensions. The live factory owns a
hardcoded string-id vendor registry (id -> {required_extensions, build});
the full-body entry resolves the tracker's vendor id (default when unlisted)
and routes params to the impl. Unknown ids and vendor selections on
non-vendored trackers are rejected at session construction.
- Bind TrackerVendor/VendorConfig and the vendor_config arg; keep
FullBodyTrackerPico and the retargeting FullBodyPosePico* names as deprecated
aliases so existing scripts run unchanged.
Replay stays vendor-neutral: ReplayFullBodyTrackerImpl reads the recorded
full-body channel regardless of which live vendor produced it. The vendor
registry is open for additional pre-built plugin vendors without touching the
tracker marker.
Let a teleop session select the vendor for its vendored trackers, complementing the DeviceIO vendor mechanism (VendorConfig / TrackerVendor). DeviceIO source nodes own their trackers internally, so the end user cannot key a VendorConfig by instance; instead the selection is provided by config and keyed by source name. - TeleopSessionConfig.tracker_vendors: dict[str, deviceio.TrackerVendor], mapping a DeviceIO source name to the vendor used to source its tracker. Sources left out use their tracker's default vendor. - In live mode, TeleopSession resolves each source name to its source-owned tracker instance, builds a VendorConfig, and passes it to both DeviceIOSession.get_required_extensions() and DeviceIOSession.run(). Unknown source names fail fast. Source nodes stay vendor-agnostic.
Reflect the deviceio refactor and per-source vendor selection on the docs: - trackers.rst: rename the FullBodyTrackerPico entry to the vendor-agnostic FullBodyTracker (new header/import; note the deprecated FullBodyTrackerPico alias) and add a Vendor Selection section covering TrackerVendor/VendorConfig, the default body.pico-xr vendor, and vendor-neutral replay. - body_tracking.rst: update FullBodyTrackerPico references to FullBodyTracker. - teleop_session.rst: document TeleopSessionConfig.tracker_vendors (per-source, live-only) with an example, cross-linked to the DeviceIO vendor mechanism.
… None In live mode, when no tracker_vendors are configured, vendor_config was left as None and passed positionally to DeviceIOSession.get_required_extensions() and run(). The pybind11 bindings take a VendorConfig by value (default empty), and None is not convertible to it, so the call raised TypeError: "get_required_extensions(): incompatible function arguments ... Invoked with: [], None". This failed teleop_session_manager_test_external_oxr_handles, which exercises the real get_required_extensions binding (only run() is mocked). Initialize vendor_config to an empty deviceio.VendorConfig() -- the native default -- so the no-selection path matches the single-arg behavior.
…nsions get_required_extensions() previously ignored the tracker_vendors mapping, silently accepting selections that DeviceIOSession construction rejects (absent trackers, non-vendored tracker types, unknown vendor ids, duplicates). Extension discovery and session creation could therefore disagree on the same config. Extract the factory constructor's per-entry checks into a shared validate_vendor_selections() (unsupported type, unknown id, duplicate) and call it from both the constructor and get_required_extensions(). get_required_extensions additionally rejects selections for trackers absent from its trackers list, mirroring the DeviceIOSession constructor's order (absent first, then type/id/ duplicate) so both paths accept identical vendor configurations.
The vendor-agnostic rename dropped FullBodyPicoRecordingTraits and ReplayFullBodyTrackerPicoImpl. Add deprecated using-aliases to the renamed FullBodyRecordingTraits and ReplayFullBodyTrackerImpl so out-of-tree consumers of the legacy public names keep compiling unchanged.
Fold the vendored FullBodyTracker special-case into the shared k_tracker_dispatch table so every tracker is a (vendor_id, is_default) row: FullBody is a single body.pico-xr default row, every other tracker is its sole default vendor. Extension discovery and impl creation share one scan that filters rows by the selected (or default) vendor id, then runs the row's type-checked thunk. - Remove the parallel k_full_body_vendors registry and its resolve/build thunks; routing now lives entirely in the dispatch table. - Consolidate the factory's per-tracker recording name and vendor selection into one tracker_data_ map (TrackerData). - Drop the now-unused FullBodyTracker::DEFAULT_VENDOR_ID. - Preserve this base's eager vendor validation (validate_vendor_selections + the get_required_extensions "tracker not in list" check), re-sourcing the unknown-id check from the dispatch table instead of the removed registry. Live-session create thunks stay 2-arg and the non-full-body rows are unchanged, keeping the dispatch aligned with the pre-vendor layout. The vendor_id / is_default row machinery is retained so a second FullBody vendor row drops in additively. Adapted from first/astrorix-main 96efd452 with all Noitom/PN changes omitted: this branch is the noitom-free base onto which that work will stack later. Signed-off-by: Andrei Aristarkhov <aaristarkhov@nvidia.com>
Replace the hardcoded dynamic_cast<const FullBodyTracker*> in validate_vendor_selections with a table-driven tracker_supports_vendors() helper: a tracker supports vendor selection iff some dispatch row with a non-empty vendor id matches its type (collect_extensions doubles as the row's type probe). Adding a vendored row for a new tracker type now needs no change here. Also swap the bespoke unordered_set presence check in get_required_extensions for a direct std::any_of scan. Behavior, error messages, and check ordering are unchanged.
Add a Catch2 suite that exercises every outcome of the live factory's vendor validation through the public LiveDeviceIOFactory::get_required_extensions: accepted (default and explicit vendor), plus rejection of a selection on a non-vendored tracker, an unknown vendor id, a duplicate selection, and a tracker absent from the list.
Address review findings on the vendor-selection change:
- Reject an empty TrackerVendor id up front: dispatch_has_vendor("")
previously matched the empty vendor_id sentinel on non-vendored dispatch
rows, so an empty selection passed validation and failed later with a
misleading "no live vendor ''" message. Now reported as an unknown
vendor id; covered by a new test_vendor_validation section.
- Add the C++ `using FullBodyTrackerPico = FullBodyTracker` alias, matching
the Python alias and the ReplayFullBodyTrackerPicoImpl /
FullBodyPicoRecordingTraits aliases so existing C++ consumers keep
compiling.
- Fix the TeleopSessionConfig.tracker_vendors docstring to use the
registered "body.pico-xr" vendor (was "body.noitom-pn", which this branch
does not register, so the documented example failed at session start).
- Drop the redundant tracker_vendors guard/duplicate VendorConfig in
TeleopSession; an empty dict already yields an empty VendorConfig.
aristarkhovNV
force-pushed
the
aaristarkhov/vendored_trackers
branch
from
July 17, 2026 02:00
8176770 to
884b3d7
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Rename the Pico full-body tracker stack to a vendor-agnostic FullBodyTracker marker and introduce a session-level vendor mechanism, so the tracker is agnostic to both vendor and live/replay — vendor is chosen at live-session construction, mirroring how live-vs-replay is already chosen.
Replay stays vendor-neutral: ReplayFullBodyTrackerImpl reads the recorded full-body channel regardless of which live vendor produced it. The vendor registry is open for additional pre-built plugin vendors without touching the tracker marker.
Type of change
Testing
Checklist
SKIP=check-copyright-year pre-commit run --all-filesgit commit -s) per the DCOSummary by CodeRabbit
FullBodyTrackerand updated full-body retargeting interfaces.