refactor(deviceio): generic FullBodyTracker + session-level vendor selection#781
Merged
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
aristarkhovNV
force-pushed
the
aaristarkhov/vendored_trackers
branch
from
July 17, 2026 02:00
8176770 to
884b3d7
Compare
aristarkhovNV
marked this pull request as ready for review
July 17, 2026 15:14
jiwenc-nv
reviewed
Jul 19, 2026
jiwenc-nv
left a comment
jiwenc-nv
left a comment
Collaborator
There was a problem hiding this comment.
The architecture is right: vendor-as-session-concern mirrors the existing live/replay split, the dispatch-table change is behavior-preserving for the other tracker types, and keeping the recorded schema/channel byte-identical preserves replay of existing recordings. The rename half is load-bearing on its own. My blocking items are two small contract defects on the new public surface (inline comments); both are same-day fixes.
Should-fix not anchored below: helpers.py::get_required_oxr_extensions_from_pipeline takes no vendor argument while required extensions are now vendor-dependent, so the documented external-oxr_handles flow computes wrong extensions for any future non-default vendor. Please add the optional parameter now, while the surface is fresh.
Nits (take or leave): tracker_supports_vendors type-probes via a side-effectful collect_extensions call with a discarded scratch set — an explicit type predicate would be cleaner; vendor ids are validated globally rather than per tracker type (a cross-type pairing fails later with a vaguer error); the same not-in-list failure is worded differently in deviceio_session.cpp vs live_deviceio_factory.cpp; error messages identify trackers only by vendor id — include get_name(); the [FullBodyTrackerPico] log prefix in live_full_body_tracker_pico_impl.cpp now names the deprecated alias of a different class; a get_tracker_vendors accessor on the VendorConfig binding would match its McapRecordingConfig sibling; "selected without recompiling core" in tracker_vendor.hpp is aspirational while the registry is a compile-time table.
aristarkhovNV
force-pushed
the
aaristarkhov/vendored_trackers
branch
from
July 20, 2026 19:35
884b3d7 to
48b284f
Compare
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/core/teleop_session_manager/python/teleop_session.py (1)
975-1002: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRevalidate vendor selection when entering replay.
A caller can construct this session in
LIVE, mutateconfig.modetoREPLAY, then enter it. The constructor-only check is bypassed and replay silently ignores the source vendor. Call_reject_vendor_selection_in_replay()at the start of_enter_resources()and add the mode-mutation regression test.Proposed fix
def _enter_resources(self, stack: ExitStack) -> None: """Acquire and initialize resources for one context-manager run.""" + self._reject_vendor_selection_in_replay() + # Reset run-scoped plugin containers on each context entry.🤖 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/teleop_session_manager/python/teleop_session.py` around lines 975 - 1002, Call _reject_vendor_selection_in_replay() at the beginning of _enter_resources(), before resource or replay configuration setup, so mutated config.mode values are revalidated on context entry. Add a regression test covering construction in LIVE mode, changing config.mode to REPLAY, and entering the session with a selected source vendor.
🤖 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.
Outside diff comments:
In `@src/core/teleop_session_manager/python/teleop_session.py`:
- Around line 975-1002: Call _reject_vendor_selection_in_replay() at the
beginning of _enter_resources(), before resource or replay configuration setup,
so mutated config.mode values are revalidated on context entry. Add a regression
test covering construction in LIVE mode, changing config.mode to REPLAY, and
entering the session with a selected source vendor.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a45d730d-cc2a-47de-acf4-a20310e4c7ac
📒 Files selected for processing (57)
docs/source/device/body_tracking.rstdocs/source/device/trackers.rstdocs/source/getting_started/teleop_session.rstexamples/deviceio_live_view/python/deviceio_viser.pyexamples/mcap_record_replay/python/common.pyexamples/oxr/python/test_full_body_tracker.pyexamples/teleop_ros2/cpp/integration_tests/mcap_generator.cppexamples/teleop_ros2/python/constants.pysrc/core/CMakeLists.txtsrc/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/live_trackers_tests/cpp/CMakeLists.txtsrc/core/live_trackers_tests/cpp/test_vendor_validation.cppsrc/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/retargeting_engine/python/deviceio_source_nodes/interface.pysrc/core/retargeting_engine/python/tensor_types/__init__.pysrc/core/retargeting_engine/python/tensor_types/indices.pysrc/core/retargeting_engine/python/tensor_types/standard_types.pysrc/core/schema/cpp/CMakeLists.txtsrc/core/schema/cpp/inc/schema/full_body_compat.hppsrc/core/schema/fbs/full_body.fbssrc/core/schema/python/full_body_bindings.hsrc/core/schema/python/schema_init.pysrc/core/schema/python/schema_module.cppsrc/core/schema_tests/cpp/test_full_body.cppsrc/core/schema_tests/python/test_full_body.pysrc/core/teleop_session_manager/python/config.pysrc/core/teleop_session_manager/python/helpers.pysrc/core/teleop_session_manager/python/teleop_session.pysrc/core/teleop_session_manager_tests/python/test_pipeline_introspection.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/replay_trackers/cpp/replay_full_body_tracker_pico_impl.hpp
- src/core/deviceio_trackers/cpp/full_body_tracker_pico.cpp
- src/core/deviceio_trackers/cpp/inc/deviceio_trackers/full_body_tracker_pico.hpp
🚧 Files skipped from review as they are similar to previous changes (24)
- src/core/CMakeLists.txt
- src/core/deviceio_session/python/deviceio_session_init.py
- src/core/live_trackers_tests/cpp/CMakeLists.txt
- docs/source/device/body_tracking.rst
- src/core/deviceio_base/cpp/inc/deviceio_base/tracker_vendor.hpp
- src/core/deviceio_base/cpp/inc/deviceio_base/full_body_tracker_base.hpp
- src/core/replay_trackers/cpp/CMakeLists.txt
- src/core/replay_trackers/cpp/inc/replay_trackers/replay_deviceio_factory.hpp
- src/core/replay_trackers/cpp/replay_full_body_tracker_impl.hpp
- src/core/deviceio_trackers/cpp/full_body_tracker.cpp
- src/core/deviceio_trackers/cpp/CMakeLists.txt
- src/core/deviceio_trackers/python/tracker_bindings.cpp
- docs/source/device/trackers.rst
- src/core/deviceio_trackers/cpp/inc/deviceio_trackers/full_body_tracker.hpp
- src/core/python/deviceio_init.py
- src/core/replay_trackers/cpp/replay_deviceio_factory.cpp
- src/core/live_trackers_tests/cpp/test_vendor_validation.cpp
- src/core/deviceio_session/cpp/inc/deviceio_session/deviceio_session.hpp
- src/core/replay_trackers/cpp/replay_full_body_tracker_impl.cpp
- src/core/retargeting_engine/python/deviceio_source_nodes/deviceio_tensor_types.py
- src/core/deviceio_session/cpp/deviceio_session.cpp
- src/core/deviceio_session/python/session_bindings.cpp
- src/core/live_trackers/cpp/inc/live_trackers/live_deviceio_factory.hpp
- src/core/live_trackers/cpp/live_deviceio_factory.cpp
…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.
Follow-up to the vendor-agnostic FullBodyTracker refactor: the FlatBuffers
full-body schema still carried the vendor-specific "...Pico" names, so a
non-Pico vendor would record into a schema whose name bakes in a competing
vendor. Rename the schema types to vendor-neutral names and keep the old
names as deprecated aliases.
- full_body.fbs: BodyJointPico -> BodyJoint, BodyJointsPico -> BodyJoints,
FullBodyPosePico/Tracked/Record -> FullBodyPose/Tracked/Record (+ root_type).
Field IDs (id: 0/1) and enum values are unchanged.
- MCAP schema name "core.FullBodyPosePicoRecord" -> "core.FullBodyPoseRecord".
- New <schema/full_body_compat.hpp> with [[deprecated]] using-aliases for every
old C++ type + all 24 BodyJointPico_* enumerators; wired onto isaacteleop_schema.
- Python: bindings register the generic names; schema_init.py re-exports them and
keeps the "...Pico" names as deprecated aliases. Retargeting layer follows suit
(NUM_BODY_JOINTS_PICO -> NUM_BODY_JOINTS, BodyJointPicoIndex -> BodyJointIndex,
both aliased). Internal C++/Python consumers and examples migrate to the new names.
- Docs: FullBodyTrackerPico -> FullBodyTracker (+ corrected header path) and the
MCAP schema / FlatBuffers type references in trackers.rst / body_tracking.rst.
Wire-compat: replay matches by channel name ("full_body") and FlatBuffer field
IDs, never by schema name, so renaming the types + schema-name string keeps the
wire layout byte-identical — existing recordings still replay. The field-ID
static_asserts in test_full_body.cpp guard this.
Vendor selection for vendored trackers now rides on the DeviceIO source object instead of a separate TeleopSessionConfig.tracker_vendors map keyed by source name. Because the vendor travels with the pipeline, extension discovery and session construction resolve it from one place. - Add vendor to the IDeviceIOSource base (get_vendor(), Optional vendor arg) and surface `vendor=` on FullBodySource. - Add helpers.build_vendor_config_from_sources(), shared by TeleopSession.start() and get_required_oxr_extensions_from_pipeline() so both build an identical VendorConfig. - Remove TeleopSessionConfig.tracker_vendors (and its now-unused imports) plus the name-map resolution in start(). This fixes the external-oxr_handles flow: get_required_oxr_extensions_from_pipeline is now vendor-aware with no extra argument, so callers that create their own OpenXR session no longer compute default-vendor extensions for a non-default vendor. C++ VendorConfig (keyed by ITracker*) is unchanged. Docs and pipeline-introspection tests updated for the source-carried model.
Drop the TrackerDispatchEntry::is_default flag and treat the first row for a tracker type in k_tracker_dispatch as its default vendor. The scanning loops already break on the first type-matching row, so row_selected now admits every row when no vendor is selected and lets the type-checked thunk pick the first match. Multi-vendor types express the default by listing that vendor's row first.
tracker_supports_vendors used a dispatch row's collect_extensions as a type probe, running the extension collection into a throwaway set purely to read the boolean it returns. Add a dedicated, side-effect-free type predicate (is_tracker_type) and carry it on each dispatch row via a new make_dispatch_entry helper, and have try_add_extensions delegate its type check to it so the dynamic_cast lives in one place. tracker_supports_vendors now probes types without the discarded scratch set.
validate_vendor_selections checked that a tracker's type supports some vendor and that the id exists somewhere in the dispatch table, but never that the id belonged to that tracker's type. A valid id paired with the wrong vendored type passed validation and only failed later, during impl creation, with a vaguer message. Add tracker_accepts_vendor (type-scoped via the row's matches predicate) and reject a cross-type pairing up front with a precise error.
The vendor selection absent-from-list failure was worded differently in the
session constructor than in LiveDeviceIOFactory::get_required_extensions for the
same condition. Match the factory's phrasing ("vendor selection '<id>' references
a tracker that is not in the trackers list") and align the recording-config
message's trailing clause so both session-side messages read consistently.
Vendor validation and absent-from-list errors identified the tracker only by its vendor id, leaving it unclear which tracker was misconfigured when several are present. Include the tracker's get_name() in these messages via a null-tolerant helper (validation can run before the list is known to be null-free).
…nding VendorConfig's Python binding only exposed its constructor, so a config's selections could not be read back. Add a get_tracker_vendors accessor returning (tracker, TrackerVendor) tuples, mirroring McapRecordingConfig.get_tracker_names and its McapReplayConfig sibling.
…tion The comment claimed a string id lets new plugin vendors be selected without recompiling core, but the selectable vendors are the live factory's compile-time dispatch table -- adding one requires editing that table and rebuilding. Reword to say what the string id actually buys (decoupling vendor routing from the tracker marker type) and drop the aspirational claim.
…xists TrackerVendor::params is documented and bound to Python but no impl reads it, so a non-empty map was silently accepted and dropped. Reject it in validate_vendor_selections so the contract starts strict; accepting params once a vendor consumes them is an additive, backward-compatible change. Update the field doc to say params is reserved and currently rejected, and cover the rejection in the vendor-validation tests.
Vendor selection is resolved only on the live path (build_vendor_config_from_sources), so a vendor carried on a source was silently ignored in SessionMode.REPLAY. Reject it at session construction, once sources are discovered and the mode is known, matching the fail-fast convention on the live path (unknown vendor ids and non-vendored trackers both raise). Vendors now travel on the sources rather than a manager-level config field, so the guard lives in TeleopSession -- where the resolved sources are known -- rather than the config's __post_init__.
…able The FullBodyTrackerPico / FullBodyPicoRecordingTraits / ReplayFullBodyTrackerPicoImpl aliases were comment-only, and FullBodyTrackerPico kept equal billing in __all__. Mark the C++ aliases [[deprecated]] (mirroring the schema compat aliases) and resolve the Python alias lazily via a module __getattr__ that emits a DeprecationWarning, dropping it from __all__ (both deviceio_trackers and the deviceio re-export) so import * no longer pulls the old name. Also correct the 'existing C++ consumers keep compiling' comment: the alias keeps type-name users compiling, but the removed full_body_tracker_pico*.hpp headers and the absent IFullBodyTrackerPicoImpl alias mean include-path/impl-interface users must update.
…precations The renamed full-body schema and tensor aliases (BodyJointPico, FullBodyPosePicoT, BodyJointPicoIndex, FullBodyPosePicoTrackedType, DeviceIOFullBodyPosePicoTracked, ...) were plain assignments with equal billing in __all__. Resolve each lazily via a module __getattr__ that emits a DeprecationWarning and returns the renamed generic type, and drop them from __all__ (and from the eager re-export imports in the tensor_types / deviceio_source_nodes packages so imports stay warning-free). Strengthen the schema deprecation test to assert the warning fires and the alias still resolves.
Signed-off-by: Andrei Aristarkhov <aaristarkhov@nvidia.com>
- live-trackers: rename stale user-facing 'FullBodyTrackerPico' strings (log/limp-mode/exception) to the renamed public 'FullBodyTracker'. - schema-tests: add compile-time coverage for the opt-in full_body_compat.hpp aliases so a mistyped ...Pico mapping can no longer ship silently. - tensor-types: route NUM_BODY_JOINTS_PICO through the lazy deprecation __getattr__ (warns, dropped from __all__) like every sibling alias, and migrate its lone consumer (teleop_ros2 test_messages) to NUM_BODY_JOINTS. - helpers: discover pipeline leaf nodes once in get_required_oxr_extensions_from_pipeline instead of traversing twice. - teleop-session tests: correct the misleading LIVE-mode vendor test docstring to describe what it actually verifies (replay-only guard, deferred to enter).
aristarkhovNV
force-pushed
the
aaristarkhov/vendored_trackers
branch
from
July 21, 2026 16:45
073bda5 to
d1e1ca2
Compare
config is a mutable dataclass, so a session built in LIVE with a vendored source (which passes the construction-time guard) could have config.mode flipped to REPLAY before __enter__, silently ignoring the vendor during replay. Re-run _reject_vendor_selection_in_replay() at the start of _enter_resources(), before any resource/replay-config setup, and add a regression test covering the LIVE->REPLAY mutation path.
Contributor
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/deviceio_session/cpp/inc/deviceio_session/deviceio_session.hpp`:
- Around line 73-76: Update DeviceIOSession::run and its initialization flow to
fully validate the supplied VendorConfig before any recording output is opened.
Ensure invalid vendor mappings are rejected before mcap_writer_->open(),
including for direct run calls with recording enabled, while preserving the
existing behavior for valid configurations.
🪄 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: 7ade73a4-8e97-4c58-a8c7-7302cf45eb51
📒 Files selected for processing (58)
docs/source/device/body_tracking.rstdocs/source/device/trackers.rstdocs/source/getting_started/teleop_session.rstexamples/deviceio_live_view/python/deviceio_viser.pyexamples/mcap_record_replay/python/common.pyexamples/oxr/python/test_full_body_tracker.pyexamples/teleop_ros2/cpp/integration_tests/mcap_generator.cppexamples/teleop_ros2/python/constants.pyexamples/teleop_ros2/python/tests/test_messages.pysrc/core/CMakeLists.txtsrc/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/live_trackers_tests/cpp/CMakeLists.txtsrc/core/live_trackers_tests/cpp/test_vendor_validation.cppsrc/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/retargeting_engine/python/deviceio_source_nodes/interface.pysrc/core/retargeting_engine/python/tensor_types/__init__.pysrc/core/retargeting_engine/python/tensor_types/indices.pysrc/core/retargeting_engine/python/tensor_types/standard_types.pysrc/core/schema/cpp/CMakeLists.txtsrc/core/schema/cpp/inc/schema/full_body_compat.hppsrc/core/schema/fbs/full_body.fbssrc/core/schema/python/full_body_bindings.hsrc/core/schema/python/schema_init.pysrc/core/schema/python/schema_module.cppsrc/core/schema_tests/cpp/test_full_body.cppsrc/core/schema_tests/python/test_full_body.pysrc/core/teleop_session_manager/python/config.pysrc/core/teleop_session_manager/python/helpers.pysrc/core/teleop_session_manager/python/teleop_session.pysrc/core/teleop_session_manager_tests/python/test_pipeline_introspection.pysrc/core/teleop_session_manager_tests/python/test_teleop_session.py
💤 Files with no reviewable changes (4)
- src/core/deviceio_trackers/cpp/full_body_tracker_pico.cpp
- src/core/deviceio_trackers/cpp/inc/deviceio_trackers/full_body_tracker_pico.hpp
- src/core/replay_trackers/cpp/replay_full_body_tracker_pico_impl.hpp
- src/core/deviceio_base/cpp/inc/deviceio_base/full_body_tracker_pico_base.hpp
🚧 Files skipped from review as they are similar to previous changes (48)
- src/core/deviceio_session/python/deviceio_session_init.py
- src/core/replay_trackers/cpp/replay_full_body_tracker_impl.hpp
- src/core/CMakeLists.txt
- docs/source/device/body_tracking.rst
- src/core/deviceio_trackers/cpp/full_body_tracker.cpp
- src/core/deviceio_base/cpp/inc/deviceio_base/full_body_tracker_base.hpp
- src/core/live_trackers_tests/cpp/CMakeLists.txt
- docs/source/device/trackers.rst
- src/core/schema/python/schema_module.cpp
- src/core/replay_trackers/cpp/CMakeLists.txt
- src/core/teleop_session_manager/python/config.py
- src/core/retargeting_engine/python/deviceio_source_nodes/interface.py
- src/core/schema/cpp/CMakeLists.txt
- src/core/deviceio_base/cpp/inc/deviceio_base/tracker_vendor.hpp
- examples/mcap_record_replay/python/common.py
- src/core/schema/python/schema_init.py
- examples/deviceio_live_view/python/deviceio_viser.py
- src/core/deviceio_trackers/cpp/CMakeLists.txt
- src/core/deviceio_trackers/python/tracker_bindings.cpp
- src/core/mcap/cpp/inc/mcap/recording_traits.hpp
- examples/teleop_ros2/python/constants.py
- src/core/replay_trackers/cpp/inc/replay_trackers/replay_deviceio_factory.hpp
- src/core/replay_trackers/cpp/replay_full_body_tracker_impl.cpp
- src/core/live_trackers_tests/cpp/test_vendor_validation.cpp
- src/core/replay_trackers/cpp/replay_deviceio_factory.cpp
- src/core/live_trackers/cpp/live_full_body_tracker_pico_impl.hpp
- src/core/deviceio_trackers/python/deviceio_trackers_init.py
- src/core/retargeting_engine/python/tensor_types/init.py
- src/core/python/deviceio_init.py
- src/core/deviceio_trackers/cpp/inc/deviceio_trackers/full_body_tracker.hpp
- src/core/live_trackers/cpp/inc/live_trackers/live_deviceio_factory.hpp
- docs/source/getting_started/teleop_session.rst
- src/core/teleop_session_manager/python/teleop_session.py
- src/core/retargeting_engine/python/tensor_types/indices.py
- src/core/retargeting_engine/python/deviceio_source_nodes/init.py
- src/core/schema/cpp/inc/schema/full_body_compat.hpp
- src/core/deviceio_session/python/session_bindings.cpp
- src/core/retargeting_engine/python/deviceio_source_nodes/deviceio_tensor_types.py
- examples/oxr/python/test_full_body_tracker.py
- src/core/deviceio_session/cpp/deviceio_session.cpp
- src/core/teleop_session_manager/python/helpers.py
- src/core/schema/python/full_body_bindings.h
- src/core/schema_tests/python/test_full_body.py
- src/core/schema_tests/cpp/test_full_body.cpp
- examples/teleop_ros2/cpp/integration_tests/mcap_generator.cpp
- src/core/teleop_session_manager_tests/python/test_pipeline_introspection.py
- src/core/retargeting_engine/python/deviceio_source_nodes/full_body_source.py
- src/core/live_trackers/cpp/live_deviceio_factory.cpp
jiwenc-nv
approved these changes
Jul 21, 2026
DeviceIOSession is now the single owner of vendor-config validation. It runs the tracker-list presence check (it holds the list) and delegates the dispatch-driven vendor-validity check to the newly public free function core::validate_vendor_selections(). Validation runs up front -- before the MCAP writer opens and before the factory is constructed -- so an invalid vendor config never leaves a stray recording file on disk, and both entry points (construction and get_required_extensions) reject identical configs. LiveDeviceIOFactory now assumes a validated config: its constructor and get_required_extensions no longer self-validate (documented via @pre; an invalid config is undefined behavior). Both are only reachable through DeviceIOSession, which is the sole validation gate. Tests exercise validation through DeviceIOSession::get_required_extensions (the public entry point plugins call) plus a direct unit test of the list-independent validate_vendor_selections() primitive.
aristarkhovNV
enabled auto-merge (squash)
July 21, 2026 19:59
jiwenc-nv
added a commit
to jiwenc-nv/IsaacTeleop
that referenced
this pull request
Jul 22, 2026
Follow-up to the generic FullBodyTracker refactor (NVIDIA#781), which removed deviceio_trackers/full_body_tracker_pico.hpp and renamed the Pico types: FullBodyTrackerPico -> FullBodyTracker, FullBodyPosePicoT -> FullBodyPoseT, BodyJointPico_* -> BodyJoint_*. full_body_printer and record_full_body now use the new header and names; no behavior change (the session still selects the default vendor). Both targets build, rig tests 30/30, clang-format-14 and pre-commit clean. Signed-off-by: Jiwen Cai <jiwenc@nvidia.com>
jiwenc-nv
added a commit
that referenced
this pull request
Jul 22, 2026
Follow-up to the generic FullBodyTracker refactor (#781), which removed deviceio_trackers/full_body_tracker_pico.hpp and renamed the Pico types: FullBodyTrackerPico -> FullBodyTracker, FullBodyPosePicoT -> FullBodyPoseT, BodyJointPico_* -> BodyJoint_*. full_body_printer and record_full_body now use the new header and names; no behavior change (the session still selects the default vendor). Both targets build, rig tests 30/30, clang-format-14 and pre-commit clean. Signed-off-by: Jiwen Cai <jiwenc@nvidia.com>
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
2 participants
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
Make the full-body tracker agnostic to both vendor and live/replay. The tracker becomes a bare marker; the vendor is chosen at session construction, mirroring how live-vs-replay is already chosen.
FullBodyTrackerPico→FullBodyTrackerand the matching schema/impl/traits types drop theirPicosuffix. The native XR impl staysLiveFullBodyTrackerPicoImpl, now registered as thebody.pico-xrvendor (the default).TrackerVendor{id, params}+VendorConfig, threaded through the liveDeviceIOSessionand exposed per-source in the teleop session. The live factory owns a single vendor dispatch table (id → extensions + build); unknown ids, vendor selections on non-vendored trackers, and replay-mode selections are rejected up front.Piconames (marker, schema types, retargeting pose types) working with a deprecation warning.Type of change
Testing
Checklist
SKIP=check-copyright-year pre-commit run --all-filesgit commit -s) per the DCOSummary by CodeRabbit
New Features
FullBodyTrackerand body-joint APIs.Documentation
Bug Fixes
Compatibility