Conversation
|
Important Review skippedThis PR was authored by the user configured for CodeRabbit reviews. CodeRabbit does not review PRs authored by this user. It's recommended to use a dedicated user account to post CodeRabbit review feedback. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughMCAP recording was moved from a standalone McapRecorder API to DeviceIOSession-managed recording via McapRecordingConfig; trackers and live tracker factory were updated to optionally create per-tracker MCAP channel objects and emit MCAP messages during update(); SchemaTrackerBase centralizes tensor/sample reading; Python McapRecorder bindings removed. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant Session as DeviceIOSession
participant Factory as LiveDeviceIOFactory
participant Tracker as LiveTrackerImpl
participant Writer as mcap::McapWriter
App->>Session: run(trackers, handles, McapRecordingConfig)
alt recording enabled
Session->>Writer: open McapWriter (filename, topic/options)
Session->>Factory: construct with writer & tracker_names
Factory->>Tracker: construct Live*TrackerImpl(..., mcap_channels)
else recording disabled
Session->>Factory: construct without writer (null)
end
loop Per-frame
App->>Session: session.update()
Session->>Tracker: Tracker.update()
alt recording enabled
Tracker->>Writer: mcap_channels->write(channel_index, timestamp, data)
Writer-->>Tracker: write status
end
Session-->>App: update() result
end
App->>Session: destroy session
alt recording enabled
Session->>Writer: close McapWriter
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
nvddr
force-pushed
the
devdeepr/refactorTrackerCreation
branch
from
b206e5f to
23add19
Compare
nvddr
force-pushed
the
devdeepr/refactorMcap
branch
from
c77465f to
ae7e889
Compare
nvddr
force-pushed
the
devdeepr/refactorTrackerCreation
branch
25 times, most recently
from
111aa6a to
fedd5b6
Compare
nvddr
force-pushed
the
devdeepr/refactorMcap
branch
from
39e200b to
0789621
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/core/live_trackers/cpp/schema_tracker_base.cpp (2)
249-260:⚠️ Potential issue | 🟡 MinorDon’t advance
m_last_sample_indexbefore the sample is known-good.Line 252 marks the sample as consumed before the size guard on Lines 255-260. If
sampleBatchStride < totalSampleSize, this branch drops the sample and the next read starts atsampleIndex + 1, whileread_all_samples()still reports success.🛠️ Proposed fix
- // Update last sample index - if (!m_last_sample_index.has_value() || metadata.sampleIndex > m_last_sample_index.value()) - { - m_last_sample_index = metadata.sampleIndex; - } - // Guard against invalid runtime state: batch stride must cover at least one full sample. if (dataBuffer.size() < m_sample_size) { std::cerr << "[SchemaTracker] read_next_sample: dataBuffer (" << dataBuffer.size() << " B) is smaller than m_sample_size (" << m_sample_size << " B); skipping copy." << std::endl; return false; } + + if (!m_last_sample_index.has_value() || metadata.sampleIndex > m_last_sample_index.value()) + { + m_last_sample_index = metadata.sampleIndex; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/live_trackers/cpp/schema_tracker_base.cpp` around lines 249 - 260, The code advances m_last_sample_index prematurely in read_next_sample: move the update of m_last_sample_index so it only happens after the size guard and successful handling of the sample (i.e., after the dataBuffer.size() >= m_sample_size check and after the copy/accept logic completes) to avoid skipping a sample when sampleBatchStride < totalSampleSize; update the logic in the method that currently checks dataBuffer.size() and references m_sample_size and m_last_sample_index so the index is only set when the sample is known-good, ensuring read_all_samples() semantics remain correct.
61-76: 🧹 Nitpick | 🔵 TrivialReuse the cached collection index on the steady-state path.
poll_for_updates()already clearsm_target_collection_indexwhen the tensor-list generation changes, butensure_collection()still rescans every collection on every update. On theDeviceIOSessionhot path that adds avoidable OpenXR property queries per tracker per frame.♻️ Proposed fast path
bool SchemaTrackerBase::ensure_collection() { poll_for_updates(); + + if (m_target_collection_index.has_value()) + { + return true; + } auto new_index = find_target_collection(); if (!new_index) { return false;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/live_trackers/cpp/schema_tracker_base.cpp` around lines 61 - 76, ensure_collection currently calls find_target_collection every time after poll_for_updates causing repeated rescans; change it to reuse the cached m_target_collection_index when present by only calling find_target_collection if m_target_collection_index is empty (poll_for_updates is responsible for clearing it on changes). Keep the existing update path: when find_target_collection returns a different index, assign it to m_target_collection_index, reset m_last_sample_index, and log the discovery; otherwise return true quickly without extra OpenXR/property queries.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@examples/oxr/python/modular_example_with_mcap.py`:
- Around line 73-77: The example checks the return value of session.update() but
DeviceIOSession::update (in src/core/deviceio_session/cpp/deviceio_session.cpp)
always returns true, so remove the dead branch in
examples/oxr/python/modular_example_with_mcap.py: replace the "if not
session.update(): print('Update failed'); break" pattern with a plain call to
session.update() (or catch exceptions if you prefer failure signaling), or
alternatively implement proper error detection in DeviceIOSession::update and
have it return false on failure and ensure that return value is exposed through
the Python binding; use the symbols session.update() and DeviceIOSession::update
to locate the spots to change.
In `@examples/oxr/python/test_oak_camera.py`:
- Around line 116-118: The multiline print call for the test output (the
print(f" ✓ {name}: {metadata_samples.get(name, 0)} metadata samples")) is not
formatted according to ruff; collapse it into a single-line print or run the
project's formatter (ruff/black) and commit the resulting change so the print
becomes: print(f" ✓ {name}: {metadata_samples.get(name, 0)} metadata samples");
ensure you modify the print invocation where name and metadata_samples are
referenced.
- Around line 86-90: The code branches on session.update() even though
DeviceIOSession::update() currently returns true unconditionally; remove the
conditional branch and warning: call session.update() without checking its
return value (so the loop always proceeds as intended) and delete the "Warning:
Session update failed" path; if you want to handle tracker failures later, add
that when DeviceIOSession::update() exposes failure semantics (refer to
session.update() and DeviceIOSession::update()).
In `@src/core/deviceio_session/cpp/inc/deviceio_session/deviceio_session.hpp`:
- Line 38: tracker_names may contain null ITracker pointers which will break
building name_map_; update DeviceioSession::DeviceioSession to validate entries
in tracker_names (check each pair.first for nullptr), and either
skip/null-filter those entries or throw a clear std::invalid_argument with
context; ensure name_map_ population uses only non-null ITracker* and add an
assertion or log if any nulls were found to make the precondition explicit.
In
`@src/core/deviceio_trackers/cpp/inc/deviceio_trackers/frame_metadata_tracker_oak.hpp`:
- Around line 66-69: get_channel_names() exposes MCAP/recording concerns on
FrameMetadataTrackerOak by returning m_channel_names; remove this public
accessor and keep the tracker recording-agnostic. Delete or make
get_channel_names() private, stop using it from outside, and instead
derive/cache channel names inside LiveFrameMetadataTrackerOakImpl (specifically
in LiveFrameMetadataTrackerOakImpl::create_mcap_channels()) or a shared internal
helper; update callers to obtain names from that impl/helper rather than from
FrameMetadataTrackerOak and remove m_channel_names dependency from the public
interface.
In `@src/core/live_trackers/cpp/inc/live_trackers/schema_tracker.hpp`:
- Around line 32-40: The SchemaTracker constructor stores a raw Channels*
(mcap_channels_) as a non-owning pointer but lacks documentation about required
lifetime guarantees; add a doc comment on the SchemaTracker constructor (or
above the class) stating that mcap_channels_ is non-owning, must outlive the
SchemaTracker instance (e.g., created and owned by the factory that also creates
SchemaTracker/McapTrackerChannels), and suggest alternatives (e.g.,
gsl::not_null<Channels*> or std::weak_ptr/std::shared_ptr) if ownership
semantics change in the future; reference the SchemaTracker constructor,
mcap_channels_, and mcap_channel_index_ in the comment for clarity.
In `@src/core/live_trackers/cpp/live_deviceio_factory.cpp`:
- Around line 30-34: The get_name method returns a std::string_view that
references data in name_map_, which can dangle if the map or factory is modified
or outlives the caller; change LiveDeviceIOFactory::get_name to return a const
std::string& (or optionally std::optional<std::reference_wrapper<const
std::string>>), update callers such as make_mcap_channels to accept a const
std::string& (or handle optional), and implement a safe fallback by returning a
static empty std::string when the tracker is not found; this ensures the
returned reference remains valid without relying on caller lifetime constraints
and keeps name_map_ as the single owner of the storage.
In `@src/core/live_trackers/cpp/live_frame_metadata_tracker_oak_impl.cpp`:
- Around line 61-67: The update() currently always returns true and ignores
failures from each stream.reader->update; change
LiveFrameMetadataTrackerOakImpl::update so it checks the return/result of
stream.reader->update(stream.tracked.data) for each entry in m_streams and
immediately propagate a failure (return false) if any reader reports an error
(treat "no new samples" as a non-failure only if the reader API distinguishes
that; otherwise propagate its error result). Locate the loop over m_streams in
LiveFrameMetadataTrackerOakImpl::update and replace the blind calls with a check
of the reader->update outcome, returning false on error and only returning true
after all streams updated successfully.
In `@src/core/live_trackers/cpp/live_hand_tracker_impl.cpp`:
- Around line 45-57: The constructor currently ignores failure from xrGetSystem,
leaving system_id uninitialized and skipping the hand-tracking capability check;
detect non-success from xrGetSystem (XrResult result returned by xrGetSystem)
and throw a std::runtime_error (or return/propagate the error) immediately if it
fails, before using system_id or calling pfn_create_hand_tracker_; keep the
existing XR_SUCCEEDED(result) branch for capability checks using
XrSystemHandTrackingPropertiesEXT and XrSystemProperties but add an else that
throws, so pfn_create_hand_tracker_ and subsequent logic only run when
xrGetSystem succeeded.
In `@src/core/live_trackers/cpp/live_head_tracker_impl.cpp`:
- Around line 18-23: Extract the MCAP schema name and channel list used in
LiveHeadTrackerImpl::create_mcap_channels (currently the literals
"core.HeadPoseRecord" and {"head"}) into a single constexpr trait/struct (e.g.,
HeadTrackerMcapInfo with members like kSchemaName and kChannels) and then use
that trait when constructing HeadMcapChannels; do the same pattern for the
hand/controller/full-body tracker impls so the generated type, schema name and
channel/topic metadata are centralized and checked at compile time.
In `@src/core/mcap_tests/cpp/CMakeLists.txt`:
- Around line 10-13: The CMake target currently links only mcap::mcap_core but
the tests instantiate mcap::McapReader (multiple times) and mcap::McapWriter
which are not provided by that target; update the target_link_libraries call
(the target using target_link_libraries and the linked symbol mcap::mcap_core)
to link the full MCAP target that provides Reader/Writer implementations (e.g.,
replace mcap::mcap_core with mcap::mcap or the appropriate full mcap target) so
the undefined reference linker errors are resolved.
In `@src/core/mcap_tests/cpp/test_mcap_recorder.cpp`:
- Around line 241-252: The test currently only proves multiple instances of the
same schema share one writer because both HeadChannels instances (head_ch and
ctrl_ch) use the same specialization; update the test to exercise mixed-schema
sharing by creating a second channel with a different McapTrackerChannels
specialization (e.g., replace ctrl_ch with a different templated channel type)
so the writer-sharing path is validated across heterogeneous trackers, or
alternatively rename the TEST_CASE("McapTrackerChannels: multiple tracker
instances share one writer", ...) to indicate it only covers same-schema
instances (HeadChannels) if you choose not to change the channels.
- Around line 47-52: The TempFileCleanup destructor currently calls
fs::exists(path) and fs::remove(path) which can throw; change the destructor
signature to be noexcept and perform best-effort cleanup using the
std::error_code overloads (e.g., fs::exists(path, ec) and fs::remove(path, ec))
so any errors are ignored or logged but not thrown; ensure you reference the
TempFileCleanup::~TempFileCleanup, the member path, and replace throwing calls
with the non-throwing overloads using a local std::error_code variable.
In `@src/core/mcap/cpp/inc/mcap/tracker_channels.hpp`:
- Around line 59-88: The write(...) method silently returns when channel_index
>= channel_ids_.size(), which can hide configuration bugs; update the check in
write (method name write, variables channel_index and channel_ids_) to either
assert (e.g., assert(channel_index < channel_ids_.size())) or emit a debug/error
log including the invalid index and current channel_ids_.size() before returning
so misindexed writes are visible during development and debugging.
---
Outside diff comments:
In `@src/core/live_trackers/cpp/schema_tracker_base.cpp`:
- Around line 249-260: The code advances m_last_sample_index prematurely in
read_next_sample: move the update of m_last_sample_index so it only happens
after the size guard and successful handling of the sample (i.e., after the
dataBuffer.size() >= m_sample_size check and after the copy/accept logic
completes) to avoid skipping a sample when sampleBatchStride < totalSampleSize;
update the logic in the method that currently checks dataBuffer.size() and
references m_sample_size and m_last_sample_index so the index is only set when
the sample is known-good, ensuring read_all_samples() semantics remain correct.
- Around line 61-76: ensure_collection currently calls find_target_collection
every time after poll_for_updates causing repeated rescans; change it to reuse
the cached m_target_collection_index when present by only calling
find_target_collection if m_target_collection_index is empty (poll_for_updates
is responsible for clearing it on changes). Keep the existing update path: when
find_target_collection returns a different index, assign it to
m_target_collection_index, reset m_last_sample_index, and log the discovery;
otherwise return true quickly without extra OpenXR/property queries.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: cf9dd936-c905-48e9-bf4c-1a6bb799ef76
📒 Files selected for processing (45)
examples/oxr/python/modular_example_with_mcap.pyexamples/oxr/python/test_oak_camera.pysrc/core/deviceio_base/cpp/inc/deviceio_base/tracker.hppsrc/core/deviceio_session/cpp/CMakeLists.txtsrc/core/deviceio_session/cpp/deviceio_session.cppsrc/core/deviceio_session/cpp/inc/deviceio_session/deviceio_session.hppsrc/core/deviceio_trackers/cpp/controller_tracker.cppsrc/core/deviceio_trackers/cpp/frame_metadata_tracker_oak.cppsrc/core/deviceio_trackers/cpp/full_body_tracker_pico.cppsrc/core/deviceio_trackers/cpp/generic_3axis_pedal_tracker.cppsrc/core/deviceio_trackers/cpp/hand_tracker.cppsrc/core/deviceio_trackers/cpp/head_tracker.cppsrc/core/deviceio_trackers/cpp/inc/deviceio_trackers/controller_tracker.hppsrc/core/deviceio_trackers/cpp/inc/deviceio_trackers/frame_metadata_tracker_oak.hppsrc/core/deviceio_trackers/cpp/inc/deviceio_trackers/full_body_tracker_pico.hppsrc/core/deviceio_trackers/cpp/inc/deviceio_trackers/generic_3axis_pedal_tracker.hppsrc/core/deviceio_trackers/cpp/inc/deviceio_trackers/hand_tracker.hppsrc/core/deviceio_trackers/cpp/inc/deviceio_trackers/head_tracker.hppsrc/core/live_trackers/cpp/CMakeLists.txtsrc/core/live_trackers/cpp/inc/live_trackers/live_deviceio_factory.hppsrc/core/live_trackers/cpp/inc/live_trackers/schema_tracker.hppsrc/core/live_trackers/cpp/inc/live_trackers/schema_tracker_base.hppsrc/core/live_trackers/cpp/live_controller_tracker_impl.cppsrc/core/live_trackers/cpp/live_controller_tracker_impl.hppsrc/core/live_trackers/cpp/live_deviceio_factory.cppsrc/core/live_trackers/cpp/live_frame_metadata_tracker_oak_impl.cppsrc/core/live_trackers/cpp/live_frame_metadata_tracker_oak_impl.hppsrc/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/cpp/live_generic_3axis_pedal_tracker_impl.cppsrc/core/live_trackers/cpp/live_generic_3axis_pedal_tracker_impl.hppsrc/core/live_trackers/cpp/live_hand_tracker_impl.cppsrc/core/live_trackers/cpp/live_hand_tracker_impl.hppsrc/core/live_trackers/cpp/live_head_tracker_impl.cppsrc/core/live_trackers/cpp/live_head_tracker_impl.hppsrc/core/live_trackers/cpp/schema_tracker_base.cppsrc/core/mcap/cpp/CMakeLists.txtsrc/core/mcap/cpp/inc/mcap/recorder.hppsrc/core/mcap/cpp/inc/mcap/tracker_channels.hppsrc/core/mcap/cpp/mcap_recorder.cppsrc/core/mcap/python/CMakeLists.txtsrc/core/mcap/python/mcap_bindings.cppsrc/core/mcap/python/mcap_init.pysrc/core/mcap_tests/cpp/CMakeLists.txtsrc/core/mcap_tests/cpp/test_mcap_recorder.cpp
💤 Files with no reviewable changes (9)
- src/core/deviceio_trackers/cpp/full_body_tracker_pico.cpp
- src/core/deviceio_trackers/cpp/hand_tracker.cpp
- src/core/deviceio_trackers/cpp/controller_tracker.cpp
- src/core/deviceio_trackers/cpp/frame_metadata_tracker_oak.cpp
- src/core/deviceio_trackers/cpp/head_tracker.cpp
- src/core/deviceio_trackers/cpp/generic_3axis_pedal_tracker.cpp
- src/core/deviceio_trackers/cpp/inc/deviceio_trackers/generic_3axis_pedal_tracker.hpp
- src/core/mcap/cpp/inc/mcap/recorder.hpp
- src/core/mcap/cpp/mcap_recorder.cpp
src/core/deviceio_trackers/cpp/inc/deviceio_trackers/frame_metadata_tracker_oak.hpp
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/core/live_trackers/cpp/live_hand_tracker_impl.cpp (1)
111-125:⚠️ Potential issue | 🟠 MajorDon't hide one-sided hand-locate failures behind
||.
update_hand()already treats an inactive hand as success at Lines 157-160. Withleft_ok || right_ok, the only behavior change is that an actualxrLocateHandJointsEXTfailure on one side is reported as healthy, soDeviceIOSession::update()never increments the tracker failure count for that broken hand.Suggested fix
- return left_ok || right_ok; + return left_ok && right_ok;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/live_trackers/cpp/live_hand_tracker_impl.cpp` around lines 111 - 125, LiveHandTrackerImpl::update currently returns left_ok || right_ok which masks a single-hand locate failure (because update_hand treats an inactive hand as success); change the logic so a failure on either side propagates—replace the final result computation to require both sides succeed (use left_ok and right_ok) so DeviceIOSession::update sees one-sided xrLocateHandJointsEXT failures; update the return expression in LiveHandTrackerImpl::update accordingly.src/core/live_trackers/cpp/schema_tracker_base.cpp (1)
79-99:⚠️ Potential issue | 🟠 MajorEnforce
max_flatbuffer_sizebefore reading samples.The new
SchemaTrackerConfig::max_flatbuffer_sizeis never checked here.live_frame_metadata_tracker_oak_impl.cpp, Lines 25-28, populates that cap, but this implementation still trusts runtime-reportedm_sample_size/m_sample_batch_stridefor allocation and copy sizing. Worse, thedataBuffer.size() < m_sample_sizebranch at Lines 255-260 returnsfalse, andread_all_samples()at Lines 79-99 treats that as a normal end-of-data case, so malformed runtime state degrades into silent data loss.Suggested fix
if (std::strncmp(collProps.data.identifier, m_config.collection_id.c_str(), XR_MAX_TENSOR_IDENTIFIER_SIZE) == 0) { // Found matching collection m_sample_batch_stride = collProps.sampleBatchStride; m_sample_size = collProps.data.totalSampleSize; + if (m_sample_batch_stride < m_sample_size) + { + throw std::runtime_error("Tensor collection reported sampleBatchStride < totalSampleSize"); + } + if (m_sample_size > m_config.max_flatbuffer_size) + { + throw std::runtime_error("Tensor sample exceeds max_flatbuffer_size for " + m_config.localized_name); + } return i; } @@ - if (dataBuffer.size() < m_sample_size) - { - std::cerr << "[SchemaTracker] read_next_sample: dataBuffer (" << dataBuffer.size() - << " B) is smaller than m_sample_size (" << m_sample_size << " B); skipping copy." << std::endl; - return false; - } + if (dataBuffer.size() < m_sample_size) + { + m_target_collection_index = std::nullopt; + throw std::runtime_error("[SchemaTracker] sampleBatchStride is smaller than totalSampleSize"); + }Also applies to: 188-193, 255-260
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/live_trackers/cpp/schema_tracker_base.cpp` around lines 79 - 99, The read_all_samples method (SchemaTrackerBase::read_all_samples) never enforces SchemaTrackerConfig::max_flatbuffer_size and thus can trust malicious/buggy runtime values (m_sample_size / m_sample_batch_stride), causing silent data loss when dataBuffer.size() < m_sample_size is treated as EOF; update read_all_samples (and any callers like read_next_sample) to check the configured cap before allocating or copying: compare m_sample_size (and derived batch size using m_sample_batch_stride) against config.max_flatbuffer_size and reject (return false with m_target_collection_index preserved or set an explicit error state) when the runtime size exceeds the cap, and when dataBuffer.size() < m_sample_size treat it as a hard error (not normal EOF) so callers can surface the failure instead of draining silently. Ensure checks reference SchemaTrackerConfig::max_flatbuffer_size, m_sample_size, m_sample_batch_stride, dataBuffer.size(), SchemaTrackerBase::read_all_samples and read_next_sample.
♻️ Duplicate comments (5)
examples/oxr/python/test_oak_camera.py (2)
115-118:⚠️ Potential issue | 🟡 MinorCommit the formatter output for this
print().CI is already failing
ruff formaton this wrapped call.Minimal fix
- for name in stream_names: - print( - f" ✓ {name}: {metadata_samples.get(name, 0)} metadata samples" - ) + for name in stream_names: + print(f" ✓ {name}: {metadata_samples.get(name, 0)} metadata samples")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/oxr/python/test_oak_camera.py` around lines 115 - 118, The wrapped print call should be reformatted into a single-line statement to satisfy ruff format: replace the multi-line print in the loop over stream_names (the print referencing stream_names, name, and metadata_samples.get(name, 0)) with a single-line print of the f-string and commit that change; after updating, run the formatter (ruff format) to ensure CI passes.
86-90:⚠️ Potential issue | 🟡 MinorRemove the dead
session.update()warning path.
DeviceIOSession::update()insrc/core/deviceio_session/cpp/deviceio_session.cpp, Lines 112-135, currently returnstrueunconditionally, so this warning branch never executes.Minimal fix
- if not session.update(): - print(" Warning: Session update failed") - continue + session.update()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/oxr/python/test_oak_camera.py` around lines 86 - 90, Remove the dead warning branch that checks the boolean result of session.update() in the loop: since DeviceIOSession::update() (in DeviceIOSession::update) currently always returns true, delete the if/continue that prints "Warning: Session update failed" and simply call session.update() (or handle real error paths once update() gains meaningful failure semantics); update the loop in test_oak_camera.py to call plugin.check_health() and session.update() without the unreachable warning branch.examples/oxr/python/modular_example_with_mcap.py (1)
73-77:⚠️ Potential issue | 🟡 MinorDrop the unreachable
update()failure branch.
DeviceIOSession::update()insrc/core/deviceio_session/cpp/deviceio_session.cpp, Lines 112-135, currently returnstrueunconditionally, so this example advertises a failure path Python never receives.Minimal fix
- if not session.update(): - print("Update failed") - break + session.update()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/oxr/python/modular_example_with_mcap.py` around lines 73 - 77, Remove the unreachable failure branch around session.update(): since DeviceIOSession::update() (C++ implementation) always returns true, delete the conditional "if not session.update(): print('Update failed'); break" in examples/oxr/python/modular_example_with_mcap.py and replace it with a direct call to session.update() inside the loop (or simply call session.update() each iteration), keeping the loop timing logic unchanged.src/core/live_trackers/cpp/live_frame_metadata_tracker_oak_impl.cpp (1)
61-67:⚠️ Potential issue | 🟠 MajorPropagate per-stream reader failures out of
update().
DeviceIOSession::update()insrc/core/deviceio_session/cpp/deviceio_session.cpp, Lines 112-135, uses this bool for tracker-health accounting. Returningtrueunconditionally here masks an unreachable OAK stream and leaves staletracked.datalooking healthy.Suggested fix
bool LiveFrameMetadataTrackerOakImpl::update(XrTime /*time*/) { for (auto& stream : m_streams) { - stream.reader->update(stream.tracked.data); + if (!stream.reader->update(stream.tracked.data)) + { + return false; + } } return true; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/live_trackers/cpp/live_frame_metadata_tracker_oak_impl.cpp` around lines 61 - 67, The current LiveFrameMetadataTrackerOakImpl::update always returns true and thus hides per-stream reader failures; change it to propagate per-stream failures by checking the boolean (or error) result from each stream.reader->update(stream.tracked.data) and return false if any reader update fails, leaving a clear failure state to the caller (DeviceIOSession::update) for tracker-health accounting; ensure you reference m_streams and stream.tracked.data so each stream’s reader result is evaluated and propagated rather than ignored.src/core/live_trackers/cpp/live_hand_tracker_impl.cpp (1)
45-57:⚠️ Potential issue | 🟠 MajorFail fast when
xrGetSystemfails.This still skips the hand-tracking capability probe and falls through to extension loading / tracker creation after the runtime already reported a system-query failure. That makes the later error noisier and hides the root cause.
Suggested fix
XrResult result = core_funcs.xrGetSystem(handles.instance, &system_info, &system_id); if (XR_SUCCEEDED(result)) { XrSystemHandTrackingPropertiesEXT hand_tracking_props{ XR_TYPE_SYSTEM_HAND_TRACKING_PROPERTIES_EXT }; XrSystemProperties system_props{ XR_TYPE_SYSTEM_PROPERTIES }; system_props.next = &hand_tracking_props; result = core_funcs.xrGetSystemProperties(handles.instance, system_id, &system_props); if (XR_SUCCEEDED(result) && !hand_tracking_props.supportsHandTracking) { throw std::runtime_error("Hand tracking not supported by this system"); } } + else + { + throw std::runtime_error("OpenXR: failed to get system: " + std::to_string(result)); + }In the OpenXR spec for XR_EXT_hand_tracking, is hand-tracking support expected to be checked via xrGetSystem followed by xrGetSystemProperties(XrSystemHandTrackingPropertiesEXT), and what is the recommended caller behavior when xrGetSystem itself fails?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/live_trackers/cpp/live_hand_tracker_impl.cpp` around lines 45 - 57, The code currently ignores failures from xrGetSystem and continues probing for hand-tracking; ensure you fail fast if xrGetSystem(handles.instance, &system_info, &system_id) does not XR_SUCCEEDED: check the returned result immediately and propagate/throw an informative error (including result) instead of continuing to call xrGetSystemProperties or creating trackers; retain the subsequent check that uses XrSystemHandTrackingPropertiesEXT via xrGetSystemProperties but only after a successful xrGetSystem.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/core/deviceio_session/cpp/inc/deviceio_session/deviceio_session.hpp`:
- Around line 28-38: McapRecordingConfig::tracker_names must be validated
against the exact set of session trackers: in the code path that consumes
McapRecordingConfig (the run() call in LiveDeviceIOSession/LiveDeviceIOFactory)
iterate the session's trackers vector and build a set of raw pointers using
trackers[i].get(), then verify each entry in McapRecordingConfig::tracker_names
matches one of those exact pointers and that no pointer appears more than once;
if an entry points to a pointer not in that set or a duplicate exists, reject
the config (return an error/throw) instead of silently treating it as an empty
name. Ensure the error path reports which pointer/name caused the failure so
miswired configs fail fast.
In `@src/core/live_trackers/cpp/inc/live_trackers/live_deviceio_factory.hpp`:
- Around line 49-54: Change the risky non-owning return of get_name to return an
owning std::string: update the declaration of get_name(const ITracker* tracker)
to return std::string (instead of std::string_view), modify its implementation
to construct/return a std::string from name_map_ (e.g. by copying the mapped
value or using std::string(name_map_.at(tracker))), and update any callers
(notably McapTrackerChannels construction sites that currently accept a
std::string_view) to accept the std::string (they already copy it once, so no
behavioral change). Also keep the name_map_ member as std::unordered_map<const
ITracker*, std::string> and ensure no callers rely on pointer-stable views into
the map.
In `@src/core/live_trackers/cpp/inc/live_trackers/schema_tracker.hpp`:
- Around line 67-86: The loop reuses the same shared_ptr out_latest and calls
fb->UnPackTo(out_latest.get()) before calling mcap_channels_->write(...), which
is safe only because McapTrackerChannels::write currently serializes
synchronously and does not retain the pointer; update the code by adding a clear
comment immediately above the loop (or above the write call) stating that
samples_ are unpacked into out_latest and that McapTrackerChannels::write
performs synchronous serialization and does not hold onto the provided pointer
(so mutation is safe), and mention the specific symbols involved: out_latest,
fb->UnPackTo, mcap_channels_->write and mcap_channel_index_; if future changes
make MCAP writes async or retain pointers, convert to per-sample copies instead.
In `@src/core/live_trackers/cpp/live_hand_tracker_impl.cpp`:
- Around line 177-185: The code currently constructs Point/Quaternion/Pose and
reads joint_loc.radius unconditionally then sets HandJointPose even when
validity bits are unset; instead, check the validity bits
(XR_SPACE_LOCATION_POSITION_VALID_BIT and
XR_SPACE_LOCATION_ORIENTATION_VALID_BIT) first and only read joint_loc.pose and
joint_loc.radius when is_valid is true, otherwise construct a
default/zero-initialized Pose and use radius = 0.0f before creating
HandJointPose (update the logic around joint_loc, is_valid,
Point/Quaternion/Pose, and HandJointPose).
In `@src/core/mcap_tests/cpp/test_mcap_recorder.cpp`:
- Around line 183-207: The test shows writes with an out-of-range channel_index
in HeadChannels::write are silently ignored, while other write failures are
logged; update the implementation in tracker_channels.hpp (HeadChannels::write)
to either log a clear message when channel_index is invalid (matching the
existing stderr logging for write failures) or add a comment/docstring near
HeadChannels::write explaining that out-of-range indices are intentionally a
no-op; make sure to reference the same logging mechanism used for other failures
so behavior is consistent with the rest of the class.
In `@src/core/mcap/python/mcap_bindings.cpp`:
- Around line 8-13: The module _mcap defined via PYBIND11_MODULE(_mcap, m) only
sets m.doc() and exposes no symbols; either remove this binding entirely (delete
mcap_bindings.cpp and remove any build targets referencing PYBIND11_MODULE
_mcap) if you don't intend to provide bindings, or make its intent explicit by
adding a minimal exported symbol (e.g., expose a version string or placeholder
function like m.attr("__version__") = "0.0" or a no-op function) and a comment
so the stub is intentional; locate PYBIND11_MODULE(_mcap, m) and update or
remove accordingly.
---
Outside diff comments:
In `@src/core/live_trackers/cpp/live_hand_tracker_impl.cpp`:
- Around line 111-125: LiveHandTrackerImpl::update currently returns left_ok ||
right_ok which masks a single-hand locate failure (because update_hand treats an
inactive hand as success); change the logic so a failure on either side
propagates—replace the final result computation to require both sides succeed
(use left_ok and right_ok) so DeviceIOSession::update sees one-sided
xrLocateHandJointsEXT failures; update the return expression in
LiveHandTrackerImpl::update accordingly.
In `@src/core/live_trackers/cpp/schema_tracker_base.cpp`:
- Around line 79-99: The read_all_samples method
(SchemaTrackerBase::read_all_samples) never enforces
SchemaTrackerConfig::max_flatbuffer_size and thus can trust malicious/buggy
runtime values (m_sample_size / m_sample_batch_stride), causing silent data loss
when dataBuffer.size() < m_sample_size is treated as EOF; update
read_all_samples (and any callers like read_next_sample) to check the configured
cap before allocating or copying: compare m_sample_size (and derived batch size
using m_sample_batch_stride) against config.max_flatbuffer_size and reject
(return false with m_target_collection_index preserved or set an explicit error
state) when the runtime size exceeds the cap, and when dataBuffer.size() <
m_sample_size treat it as a hard error (not normal EOF) so callers can surface
the failure instead of draining silently. Ensure checks reference
SchemaTrackerConfig::max_flatbuffer_size, m_sample_size, m_sample_batch_stride,
dataBuffer.size(), SchemaTrackerBase::read_all_samples and read_next_sample.
---
Duplicate comments:
In `@examples/oxr/python/modular_example_with_mcap.py`:
- Around line 73-77: Remove the unreachable failure branch around
session.update(): since DeviceIOSession::update() (C++ implementation) always
returns true, delete the conditional "if not session.update(): print('Update
failed'); break" in examples/oxr/python/modular_example_with_mcap.py and replace
it with a direct call to session.update() inside the loop (or simply call
session.update() each iteration), keeping the loop timing logic unchanged.
In `@examples/oxr/python/test_oak_camera.py`:
- Around line 115-118: The wrapped print call should be reformatted into a
single-line statement to satisfy ruff format: replace the multi-line print in
the loop over stream_names (the print referencing stream_names, name, and
metadata_samples.get(name, 0)) with a single-line print of the f-string and
commit that change; after updating, run the formatter (ruff format) to ensure CI
passes.
- Around line 86-90: Remove the dead warning branch that checks the boolean
result of session.update() in the loop: since DeviceIOSession::update() (in
DeviceIOSession::update) currently always returns true, delete the if/continue
that prints "Warning: Session update failed" and simply call session.update()
(or handle real error paths once update() gains meaningful failure semantics);
update the loop in test_oak_camera.py to call plugin.check_health() and
session.update() without the unreachable warning branch.
In `@src/core/live_trackers/cpp/live_frame_metadata_tracker_oak_impl.cpp`:
- Around line 61-67: The current LiveFrameMetadataTrackerOakImpl::update always
returns true and thus hides per-stream reader failures; change it to propagate
per-stream failures by checking the boolean (or error) result from each
stream.reader->update(stream.tracked.data) and return false if any reader update
fails, leaving a clear failure state to the caller (DeviceIOSession::update) for
tracker-health accounting; ensure you reference m_streams and
stream.tracked.data so each stream’s reader result is evaluated and propagated
rather than ignored.
In `@src/core/live_trackers/cpp/live_hand_tracker_impl.cpp`:
- Around line 45-57: The code currently ignores failures from xrGetSystem and
continues probing for hand-tracking; ensure you fail fast if
xrGetSystem(handles.instance, &system_info, &system_id) does not XR_SUCCEEDED:
check the returned result immediately and propagate/throw an informative error
(including result) instead of continuing to call xrGetSystemProperties or
creating trackers; retain the subsequent check that uses
XrSystemHandTrackingPropertiesEXT via xrGetSystemProperties but only after a
successful xrGetSystem.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5bfc31e2-083a-4f8a-bf37-d59526e4dedf
📒 Files selected for processing (45)
examples/oxr/python/modular_example_with_mcap.pyexamples/oxr/python/test_oak_camera.pysrc/core/deviceio_base/cpp/inc/deviceio_base/tracker.hppsrc/core/deviceio_session/cpp/CMakeLists.txtsrc/core/deviceio_session/cpp/deviceio_session.cppsrc/core/deviceio_session/cpp/inc/deviceio_session/deviceio_session.hppsrc/core/deviceio_trackers/cpp/controller_tracker.cppsrc/core/deviceio_trackers/cpp/frame_metadata_tracker_oak.cppsrc/core/deviceio_trackers/cpp/full_body_tracker_pico.cppsrc/core/deviceio_trackers/cpp/generic_3axis_pedal_tracker.cppsrc/core/deviceio_trackers/cpp/hand_tracker.cppsrc/core/deviceio_trackers/cpp/head_tracker.cppsrc/core/deviceio_trackers/cpp/inc/deviceio_trackers/controller_tracker.hppsrc/core/deviceio_trackers/cpp/inc/deviceio_trackers/frame_metadata_tracker_oak.hppsrc/core/deviceio_trackers/cpp/inc/deviceio_trackers/full_body_tracker_pico.hppsrc/core/deviceio_trackers/cpp/inc/deviceio_trackers/generic_3axis_pedal_tracker.hppsrc/core/deviceio_trackers/cpp/inc/deviceio_trackers/hand_tracker.hppsrc/core/deviceio_trackers/cpp/inc/deviceio_trackers/head_tracker.hppsrc/core/live_trackers/cpp/CMakeLists.txtsrc/core/live_trackers/cpp/inc/live_trackers/live_deviceio_factory.hppsrc/core/live_trackers/cpp/inc/live_trackers/schema_tracker.hppsrc/core/live_trackers/cpp/inc/live_trackers/schema_tracker_base.hppsrc/core/live_trackers/cpp/live_controller_tracker_impl.cppsrc/core/live_trackers/cpp/live_controller_tracker_impl.hppsrc/core/live_trackers/cpp/live_deviceio_factory.cppsrc/core/live_trackers/cpp/live_frame_metadata_tracker_oak_impl.cppsrc/core/live_trackers/cpp/live_frame_metadata_tracker_oak_impl.hppsrc/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/cpp/live_generic_3axis_pedal_tracker_impl.cppsrc/core/live_trackers/cpp/live_generic_3axis_pedal_tracker_impl.hppsrc/core/live_trackers/cpp/live_hand_tracker_impl.cppsrc/core/live_trackers/cpp/live_hand_tracker_impl.hppsrc/core/live_trackers/cpp/live_head_tracker_impl.cppsrc/core/live_trackers/cpp/live_head_tracker_impl.hppsrc/core/live_trackers/cpp/schema_tracker_base.cppsrc/core/mcap/cpp/CMakeLists.txtsrc/core/mcap/cpp/inc/mcap/recorder.hppsrc/core/mcap/cpp/inc/mcap/tracker_channels.hppsrc/core/mcap/cpp/mcap_recorder.cppsrc/core/mcap/python/CMakeLists.txtsrc/core/mcap/python/mcap_bindings.cppsrc/core/mcap/python/mcap_init.pysrc/core/mcap_tests/cpp/CMakeLists.txtsrc/core/mcap_tests/cpp/test_mcap_recorder.cpp
💤 Files with no reviewable changes (9)
- src/core/deviceio_trackers/cpp/frame_metadata_tracker_oak.cpp
- src/core/deviceio_trackers/cpp/hand_tracker.cpp
- src/core/deviceio_trackers/cpp/controller_tracker.cpp
- src/core/deviceio_trackers/cpp/head_tracker.cpp
- src/core/deviceio_trackers/cpp/generic_3axis_pedal_tracker.cpp
- src/core/deviceio_trackers/cpp/full_body_tracker_pico.cpp
- src/core/mcap/cpp/inc/mcap/recorder.hpp
- src/core/deviceio_trackers/cpp/inc/deviceio_trackers/generic_3axis_pedal_tracker.hpp
- src/core/mcap/cpp/mcap_recorder.cpp
nvddr
force-pushed
the
devdeepr/refactorMcap
branch
4 times, most recently
from
95e2ebf to
bd78a75
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (2)
src/core/deviceio_trackers/cpp/inc/deviceio_trackers/frame_metadata_tracker_oak.hpp (1)
89-92: 🧹 Nitpick | 🔵 TrivialKeep MCAP channel naming out of the public tracker surface.
get_channel_names()leaks recording-specific metadata back intoFrameMetadataTrackerOak, even though this refactor otherwise moved MCAP wiring behind the live impl/factory boundary. Prefer deriving or caching the per-stream channel names insideLiveFrameMetadataTrackerOakImpl(or an internal helper) and keep the tracker API focused on stream configuration/query access.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/deviceio_trackers/cpp/inc/deviceio_trackers/frame_metadata_tracker_oak.hpp` around lines 89 - 92, get_channel_names() exposes MCAP/recording-specific channel naming from FrameMetadataTrackerOak; remove this public accessor and move channel name derivation/caching into the live implementation or helper. Specifically, delete or make get_channel_names() private on FrameMetadataTrackerOak and instead compute/store per-stream channel names inside LiveFrameMetadataTrackerOakImpl (or an internal helper class), exposing only stream-configuration/query methods on FrameMetadataTrackerOak; update any callers to use a new internal accessor on LiveFrameMetadataTrackerOakImpl or the helper rather than FrameMetadataTrackerOak::get_channel_names().src/core/live_trackers/cpp/live_hand_tracker_impl.cpp (1)
179-188:⚠️ Potential issue | 🟠 MajorDo not copy undefined pose and radius when validity bits are unset.
Per OpenXR spec,
poseandradiusfields inXrHandJointLocationEXTare undefined and must not be read when their corresponding validity bits (XR_SPACE_LOCATION_POSITION_VALID_BIT,XR_SPACE_LOCATION_ORIENTATION_VALID_BIT) are not set. The current code still copies these undefined values intoHandJointPoseeven whenis_validis false, leaking invalid data into live state and MCAP recordings.When validity bits are unset, use a default/zero-initialized
Poseandradius = 0.0finstead of reading the undefined fields.🛡️ Proposed fix
for (uint32_t i = 0; i < XR_HAND_JOINT_COUNT_EXT; ++i) { const auto& joint_loc = joint_locations[i]; - Point position(joint_loc.pose.position.x, joint_loc.pose.position.y, joint_loc.pose.position.z); - Quaternion orientation(joint_loc.pose.orientation.x, joint_loc.pose.orientation.y, joint_loc.pose.orientation.z, - joint_loc.pose.orientation.w); - Pose pose(position, orientation); - bool is_valid = (joint_loc.locationFlags & XR_SPACE_LOCATION_POSITION_VALID_BIT) && (joint_loc.locationFlags & XR_SPACE_LOCATION_ORIENTATION_VALID_BIT); - HandJointPose joint_pose(pose, is_valid, joint_loc.radius); + Pose pose; + float radius = 0.0f; + if (is_valid) + { + Point position(joint_loc.pose.position.x, joint_loc.pose.position.y, joint_loc.pose.position.z); + Quaternion orientation(joint_loc.pose.orientation.x, joint_loc.pose.orientation.y, + joint_loc.pose.orientation.z, joint_loc.pose.orientation.w); + pose = Pose(position, orientation); + radius = joint_loc.radius; + } + + HandJointPose joint_pose(pose, is_valid, radius); tracked.data->joints->mutable_poses()->Mutate(i, joint_pose); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/live_trackers/cpp/live_hand_tracker_impl.cpp` around lines 179 - 188, The code currently reads joint_loc.pose and joint_loc.radius even when validity bits are unset; change the logic in live_hand_tracker_impl.cpp around the construction of Pose and HandJointPose so that you first compute is_valid from joint_loc.locationFlags (using XR_SPACE_LOCATION_POSITION_VALID_BIT and XR_SPACE_LOCATION_ORIENTATION_VALID_BIT) and only read joint_loc.pose and joint_loc.radius when is_valid is true; otherwise construct a default/zero Pose and use radius = 0.0f, then create HandJointPose(pose, is_valid, radius) and call tracked.data->joints->mutable_poses()->Mutate(i, joint_pose).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@examples/oxr/python/test_oak_camera.py`:
- Around line 103-105: The loop that builds parts in test_oak_camera.py should
be replaced with a list comprehension to address Ruff PERF401: replace the
explicit loop that appends f"{name}={metadata_samples.get(name, 0)}" for each
name in stream_names with a single expression that constructs parts in one step
(i.e., assign parts using a comprehension over stream_names that uses
metadata_samples.get(name, 0)); update any references to parts accordingly.
In `@src/core/deviceio_session/cpp/deviceio_session.cpp`:
- Around line 66-73: The destructor DeviceIOSession::~DeviceIOSession currently
calls mcap_writer_->close() without checking its result; update it to capture
the return/status of mcap_writer_->close() and, if it indicates an error, emit a
warning log (e.g. using std::cerr or the project logger) that includes the
error/status text and context like "DeviceIOSession: MCAP file close failed" so
debugging truncated/corrupted MCAPs is possible; leave the normal success path
printing "DeviceIOSession: MCAP file closed" as-is.
In `@src/core/live_trackers/cpp/live_deviceio_factory.cpp`:
- Around line 35-39: LiveDeviceIOFactory::get_name currently dereferences
name_map_ iterator without checking presence, causing UB for unregistered
ITracker pointers; modify get_name to check name_map_.find(tracker) before
dereferencing, and either assert (e.g., assert(it != name_map_.end())) or return
a safe default std::string_view (like "" or "<unknown>") when not found; ensure
callers (including code using should_record()) keep existing behavior but this
function is now defensive and does not UB when passed an unknown tracker.
In `@src/core/live_trackers/cpp/live_generic_3axis_pedal_tracker_impl.cpp`:
- Around line 45-48: The update method currently always returns true which hides
failures; change LiveGeneric3AxisPedalTrackerImpl::update to return the boolean
result of m_schema_reader.update(m_tracked.data) instead of unconditionally
returning true—capture the return value from m_schema_reader.update(...) and
return it so DeviceIOSession::update() sees the actual success/failure of the
schema reader.
In `@src/core/live_trackers/cpp/live_hand_tracker_impl.cpp`:
- Around line 55-59: The code currently only throws when
hand_tracking_props.supportsHandTracking is false on XR_SUCCEEDED(result), but
does not handle the failure of xrGetSystemProperties itself; update the block
around xrGetSystemProperties (call using handles.instance, system_id,
system_props) to check the result and throw a std::runtime_error when
xrGetSystemProperties fails (include the result code or a clear message), before
proceeding to load extension functions and create trackers so constructor logic
does not continue on an invalid system properties result.
In `@src/core/live_trackers/cpp/schema_tracker_base.cpp`:
- Around line 63-68: SchemaTracker::update() can throw from OpenXR calls (e.g.,
m_get_latest_gen_fn, ensure_collection, read_next_sample, read_all_samples), so
wrap the call to read_all_samples() inside a try-catch that catches
std::exception (and a catch-all) and returns false on any exception; similarly
protect any direct calls to ensure_collection() and read_next_sample() inside
SchemaTracker (and the same call sites you flagged) so they convert thrown
std::runtime_error into a false return rather than letting exceptions propagate;
locate the methods by name (SchemaTracker::update, ensure_collection,
read_next_sample, read_all_samples) and add minimal try { ... } catch (const
std::exception&) { return false; } catch (...) { return false; } around the
failing calls.
---
Duplicate comments:
In
`@src/core/deviceio_trackers/cpp/inc/deviceio_trackers/frame_metadata_tracker_oak.hpp`:
- Around line 89-92: get_channel_names() exposes MCAP/recording-specific channel
naming from FrameMetadataTrackerOak; remove this public accessor and move
channel name derivation/caching into the live implementation or helper.
Specifically, delete or make get_channel_names() private on
FrameMetadataTrackerOak and instead compute/store per-stream channel names
inside LiveFrameMetadataTrackerOakImpl (or an internal helper class), exposing
only stream-configuration/query methods on FrameMetadataTrackerOak; update any
callers to use a new internal accessor on LiveFrameMetadataTrackerOakImpl or the
helper rather than FrameMetadataTrackerOak::get_channel_names().
In `@src/core/live_trackers/cpp/live_hand_tracker_impl.cpp`:
- Around line 179-188: The code currently reads joint_loc.pose and
joint_loc.radius even when validity bits are unset; change the logic in
live_hand_tracker_impl.cpp around the construction of Pose and HandJointPose so
that you first compute is_valid from joint_loc.locationFlags (using
XR_SPACE_LOCATION_POSITION_VALID_BIT and
XR_SPACE_LOCATION_ORIENTATION_VALID_BIT) and only read joint_loc.pose and
joint_loc.radius when is_valid is true; otherwise construct a default/zero Pose
and use radius = 0.0f, then create HandJointPose(pose, is_valid, radius) and
call tracked.data->joints->mutable_poses()->Mutate(i, joint_pose).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8629ca73-b734-497e-846d-68420f674a29
📒 Files selected for processing (45)
examples/oxr/python/modular_example_with_mcap.pyexamples/oxr/python/test_oak_camera.pysrc/core/deviceio_base/cpp/inc/deviceio_base/tracker.hppsrc/core/deviceio_session/cpp/CMakeLists.txtsrc/core/deviceio_session/cpp/deviceio_session.cppsrc/core/deviceio_session/cpp/inc/deviceio_session/deviceio_session.hppsrc/core/deviceio_trackers/cpp/controller_tracker.cppsrc/core/deviceio_trackers/cpp/frame_metadata_tracker_oak.cppsrc/core/deviceio_trackers/cpp/full_body_tracker_pico.cppsrc/core/deviceio_trackers/cpp/generic_3axis_pedal_tracker.cppsrc/core/deviceio_trackers/cpp/hand_tracker.cppsrc/core/deviceio_trackers/cpp/head_tracker.cppsrc/core/deviceio_trackers/cpp/inc/deviceio_trackers/controller_tracker.hppsrc/core/deviceio_trackers/cpp/inc/deviceio_trackers/frame_metadata_tracker_oak.hppsrc/core/deviceio_trackers/cpp/inc/deviceio_trackers/full_body_tracker_pico.hppsrc/core/deviceio_trackers/cpp/inc/deviceio_trackers/generic_3axis_pedal_tracker.hppsrc/core/deviceio_trackers/cpp/inc/deviceio_trackers/hand_tracker.hppsrc/core/deviceio_trackers/cpp/inc/deviceio_trackers/head_tracker.hppsrc/core/live_trackers/cpp/CMakeLists.txtsrc/core/live_trackers/cpp/inc/live_trackers/live_deviceio_factory.hppsrc/core/live_trackers/cpp/inc/live_trackers/schema_tracker.hppsrc/core/live_trackers/cpp/inc/live_trackers/schema_tracker_base.hppsrc/core/live_trackers/cpp/live_controller_tracker_impl.cppsrc/core/live_trackers/cpp/live_controller_tracker_impl.hppsrc/core/live_trackers/cpp/live_deviceio_factory.cppsrc/core/live_trackers/cpp/live_frame_metadata_tracker_oak_impl.cppsrc/core/live_trackers/cpp/live_frame_metadata_tracker_oak_impl.hppsrc/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/cpp/live_generic_3axis_pedal_tracker_impl.cppsrc/core/live_trackers/cpp/live_generic_3axis_pedal_tracker_impl.hppsrc/core/live_trackers/cpp/live_hand_tracker_impl.cppsrc/core/live_trackers/cpp/live_hand_tracker_impl.hppsrc/core/live_trackers/cpp/live_head_tracker_impl.cppsrc/core/live_trackers/cpp/live_head_tracker_impl.hppsrc/core/live_trackers/cpp/schema_tracker_base.cppsrc/core/mcap/cpp/CMakeLists.txtsrc/core/mcap/cpp/inc/mcap/recorder.hppsrc/core/mcap/cpp/inc/mcap/tracker_channels.hppsrc/core/mcap/cpp/mcap_recorder.cppsrc/core/mcap/python/CMakeLists.txtsrc/core/mcap/python/mcap_bindings.cppsrc/core/mcap/python/mcap_init.pysrc/core/mcap_tests/cpp/CMakeLists.txtsrc/core/mcap_tests/cpp/test_mcap_recorder.cpp
💤 Files with no reviewable changes (9)
- src/core/deviceio_trackers/cpp/hand_tracker.cpp
- src/core/deviceio_trackers/cpp/full_body_tracker_pico.cpp
- src/core/deviceio_trackers/cpp/head_tracker.cpp
- src/core/deviceio_trackers/cpp/generic_3axis_pedal_tracker.cpp
- src/core/deviceio_trackers/cpp/inc/deviceio_trackers/generic_3axis_pedal_tracker.hpp
- src/core/mcap/cpp/inc/mcap/recorder.hpp
- src/core/deviceio_trackers/cpp/frame_metadata_tracker_oak.cpp
- src/core/deviceio_trackers/cpp/controller_tracker.cpp
- src/core/mcap/cpp/mcap_recorder.cpp
src/core/live_trackers/cpp/live_generic_3axis_pedal_tracker_impl.cpp
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/core/python/CMakeLists.txt (1)
15-29:⚠️ Potential issue | 🟠 MajorClean the removed
isaacteleop.mcappackage before rebuilding the wheel.Dropping
mcap_pyfromDEPENDSstops rebuilding the extension, but it does not delete any previously emitted_mcapartifacts under${CMAKE_BINARY_DIR}/python_package/$<CONFIG>/isaacteleop/mcap. On an incremental build tree,uv buildcan still bundle that stale binary/stub and effectively resurrect the removed API.🧹 Minimal fix
add_custom_target(python_package ALL + COMMAND ${CMAKE_COMMAND} -E remove_directory + "${CMAKE_BINARY_DIR}/python_package/$<CONFIG>/isaacteleop/mcap" COMMAND ${CMAKE_COMMAND} -E make_directory "${CMAKE_BINARY_DIR}/python_package/$<CONFIG>/isaacteleop" COMMAND ${CMAKE_COMMAND} -E make_directory "${CMAKE_BINARY_DIR}/python_package/$<CONFIG>/isaacteleop/deviceio" COMMAND ${CMAKE_COMMAND} -E make_directory "${CMAKE_BINARY_DIR}/python_package/$<CONFIG>/isaacteleop/mcap"Also applies to: 43-43
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/python/CMakeLists.txt` around lines 15 - 29, The python_package custom target currently stops depending on mcap_py but does not remove previously emitted artifacts, so stale _mcap binaries can be packaged; update the python_package target (or its install/packaging step) to explicitly remove the "${CMAKE_BINARY_DIR}/python_package/$<CONFIG>/isaacteleop/mcap" directory (or any _mcap artifacts) before creating the package — e.g. add a pre-step using CMAKE_COMMAND -E remove_directory for that path so incremental builds won’t resurrect the removed mcap API when mcap_py is dropped from DEPENDS.
♻️ Duplicate comments (7)
src/core/deviceio_trackers/cpp/inc/deviceio_trackers/frame_metadata_tracker_oak.hpp (1)
18-20: 🛠️ Refactor suggestion | 🟠 MajorStill leaking MCAP metadata through
FrameMetadataTrackerOak.The new class docs and
get_channel_names()accessor keep recording-specific channel naming on the public tracker API, even though this refactor is otherwise removing MCAP concerns from tracker interfaces. Please derive/cache channel names insideLiveFrameMetadataTrackerOakImpl(or a shared internal helper) and keep this type focused on stream selection/state.Also applies to: 89-92
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/deviceio_trackers/cpp/inc/deviceio_trackers/frame_metadata_tracker_oak.hpp` around lines 18 - 20, FrameMetadataTrackerOak currently exposes MCAP-specific channel naming via its docs and the get_channel_names() accessor; remove MCAP concerns from FrameMetadataTrackerOak (and its public API) so it only handles stream selection/state, and move the channel name derivation/caching logic into LiveFrameMetadataTrackerOakImpl (or a new internal helper) where you should generate and cache the recording-specific channel names; update get_channel_names() on FrameMetadataTrackerOak to either be removed or to return only stream identifiers, and implement a private/internal method on LiveFrameMetadataTrackerOakImpl that returns the MCAP channel names used for recording.src/core/live_trackers/cpp/live_hand_tracker_impl.cpp (2)
55-59:⚠️ Potential issue | 🟠 MajorFail fast when
xrGetSystemPropertiesfails.Line 55 still falls through on
XR_FAILED(result), so the constructor can continue without a verified hand-tracking capability check and only fail later in a less actionable place. Mirror thexrGetSystemhandling and throw immediately.🐛 Minimal fix
result = core_funcs.xrGetSystemProperties(handles.instance, system_id, &system_props); - if (XR_SUCCEEDED(result) && !hand_tracking_props.supportsHandTracking) + if (XR_FAILED(result)) + { + throw std::runtime_error("Failed to get OpenXR system properties: " + std::to_string(result)); + } + if (!hand_tracking_props.supportsHandTracking) { throw std::runtime_error("Hand tracking not supported by this system"); }Verification: confirm the constructor still has no
XR_FAILED(result)branch afterxrGetSystemProperties.#!/bin/bash set -euo pipefail file=$(fd 'live_hand_tracker_impl.cpp$' src | head -n1) rg -n -C2 'xrGetSystemProperties|supportsHandTracking' "$file"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/live_trackers/cpp/live_hand_tracker_impl.cpp` around lines 55 - 59, The call to xrGetSystemProperties (using core_funcs.xrGetSystemProperties with handles.instance, system_id, and system_props) currently only throws if XR_SUCCEEDED(result) but hand tracking not supported, leaving XR_FAILED(result) to fall through; update the constructor to mirror the xrGetSystem handling by checking XR_FAILED(result) immediately after the call and throwing a std::runtime_error (with a clear message) if it fails, before inspecting hand_tracking_props.supportsHandTracking, so the constructor does not continue without a verified system properties result.
179-187:⚠️ Potential issue | 🟠 MajorDon't read joint fields before checking the OpenXR validity bits.
Lines 179-187 read
joint_loc.pose.position,joint_loc.pose.orientation, andjoint_loc.radiusbefore validating the correspondingXR_SPACE_LOCATION_*_VALID_BITs. OpenXR leaves those fields undefined when the bits are clear, so this can leak garbage intotracked.dataand MCAP output.🐛 Minimal fix
- Point position(joint_loc.pose.position.x, joint_loc.pose.position.y, joint_loc.pose.position.z); - Quaternion orientation(joint_loc.pose.orientation.x, joint_loc.pose.orientation.y, joint_loc.pose.orientation.z, - joint_loc.pose.orientation.w); - Pose pose(position, orientation); - - bool is_valid = (joint_loc.locationFlags & XR_SPACE_LOCATION_POSITION_VALID_BIT) && - (joint_loc.locationFlags & XR_SPACE_LOCATION_ORIENTATION_VALID_BIT); - - HandJointPose joint_pose(pose, is_valid, joint_loc.radius); + bool position_valid = (joint_loc.locationFlags & XR_SPACE_LOCATION_POSITION_VALID_BIT) != 0; + bool orientation_valid = (joint_loc.locationFlags & XR_SPACE_LOCATION_ORIENTATION_VALID_BIT) != 0; + bool is_valid = position_valid && orientation_valid; + + Point position = position_valid + ? Point(joint_loc.pose.position.x, joint_loc.pose.position.y, joint_loc.pose.position.z) + : Point(0.0f, 0.0f, 0.0f); + Quaternion orientation = orientation_valid + ? Quaternion(joint_loc.pose.orientation.x, joint_loc.pose.orientation.y, + joint_loc.pose.orientation.z, joint_loc.pose.orientation.w) + : Quaternion(0.0f, 0.0f, 0.0f, 1.0f); + Pose pose(position, orientation); + + float radius = position_valid ? joint_loc.radius : 0.0f; + HandJointPose joint_pose(pose, is_valid, radius);For OpenXR's XrHandJointLocationEXT, when XR_SPACE_LOCATION_POSITION_VALID_BIT and/or XR_SPACE_LOCATION_ORIENTATION_VALID_BIT are not set, are pose.position, pose.orientation, and radius defined to be read by applications?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/live_trackers/cpp/live_hand_tracker_impl.cpp` around lines 179 - 187, The code reads joint_loc.pose.position, joint_loc.pose.orientation, and joint_loc.radius before checking XR_SPACE_LOCATION_POSITION_VALID_BIT / XR_SPACE_LOCATION_ORIENTATION_VALID_BIT, which is undefined per OpenXR; update the construction of Pose and HandJointPose so you first test the bits on joint_loc.locationFlags and only read position when XR_SPACE_LOCATION_POSITION_VALID_BIT is set and only read orientation when XR_SPACE_LOCATION_ORIENTATION_VALID_BIT is set (and only read radius when the corresponding validity bit guarantees it's defined), otherwise use safe defaults (zero position / identity quaternion / zero radius) and preserve the is_valid boolean set from the flags; change the block around joint_loc, Pose, position, orientation, radius, and HandJointPose to conditionally populate fields based on those validity bits.src/core/deviceio_session/cpp/deviceio_session.cpp (1)
66-73: 🧹 Nitpick | 🔵 TrivialConsider checking
close()return status for debugging purposes.The destructor calls
mcap_writer_->close()without checking its returned status. While errors during close are rare and recovery is limited in a destructor, logging a warning could help debug truncated or corrupted MCAP files.♻️ Suggested enhancement
DeviceIOSession::~DeviceIOSession() { if (mcap_writer_) { - mcap_writer_->close(); - std::cout << "DeviceIOSession: MCAP file closed" << std::endl; + auto status = mcap_writer_->close(); + if (!status.ok()) + { + std::cerr << "DeviceIOSession: warning closing MCAP file: " << status.message << std::endl; + } + else + { + std::cout << "DeviceIOSession: MCAP file closed" << std::endl; + } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/deviceio_session/cpp/deviceio_session.cpp` around lines 66 - 73, The destructor DeviceIOSession::~DeviceIOSession currently calls mcap_writer_->close() but ignores its return status; modify the destructor to capture the close() return value from mcap_writer_->close(), check for an error/false result, and log a warning (using std::cerr or the existing logging mechanism) including any error detail or status to aid debugging of truncated/corrupt MCAP files while still ensuring resources are cleaned up.src/core/live_trackers/cpp/live_generic_3axis_pedal_tracker_impl.cpp (1)
45-49:⚠️ Potential issue | 🟠 MajorPropagate the schema-reader status from
update().
DeviceIOSession::update()uses this return value for failure accounting (seesrc/core/deviceio_session/cpp/deviceio_session.cppLines 117-132). Always returningtruehides pedal tracker failures from the session's health monitoring.🐛 Proposed fix
bool LiveGeneric3AxisPedalTrackerImpl::update(XrTime /*time*/) { - m_schema_reader.update(m_tracked.data); - return true; + return m_schema_reader.update(m_tracked.data); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/live_trackers/cpp/live_generic_3axis_pedal_tracker_impl.cpp` around lines 45 - 49, The update() method currently always returns true, hiding failures; change LiveGeneric3AxisPedalTrackerImpl::update to propagate the actual status from m_schema_reader.update(m_tracked.data) (i.e., return its boolean/result value), and if m_schema_reader.update doesn't already return a bool convert or map its result to a boolean so DeviceIOSession::update can correctly detect and account for failures.src/core/live_trackers/cpp/live_frame_metadata_tracker_oak_impl.cpp (1)
61-68:⚠️ Potential issue | 🟠 MajorPropagate tracker-read failures out of
update().Similar to
LiveHandTrackerImpl::update()which returnsleft_ok || right_ok, this method should aggregate the per-stream update results. Always returningtruehides frame-metadata tracker outages fromDeviceIOSession's failure accounting.🐛 Proposed fix
bool LiveFrameMetadataTrackerOakImpl::update(XrTime /*time*/) { + bool any_ok = false; for (auto& stream : m_streams) { - stream.reader->update(stream.tracked.data); + if (stream.reader->update(stream.tracked.data)) + { + any_ok = true; + } } - return true; + return any_ok || m_streams.empty(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/live_trackers/cpp/live_frame_metadata_tracker_oak_impl.cpp` around lines 61 - 68, The update method currently ignores per-stream failures by always returning true; change LiveFrameMetadataTrackerOakImpl::update to capture each stream.reader->update(...) return value and aggregate them (e.g., overall_ok = overall_ok || stream_ok) across m_streams, then return the aggregated boolean so DeviceIOSession sees outages; mirror the pattern used in LiveHandTrackerImpl::update (use OR across streams rather than unconditionally returning true).src/core/live_trackers/cpp/schema_tracker_base.cpp (1)
122-142:⚠️ Potential issue | 🔴 CriticalStill let OpenXR failures escape the bool-based drain path.
Lines 124-130 call helpers that throw on routine OpenXR failures, so
read_all_samples()can still unwind callers instead of returningfalse. That breaks the new contract here and can still take down the whole tracker update path.🚑 Proposed containment
bool SchemaTrackerBase::read_all_samples(std::vector<SampleResult>& samples) { - if (!ensure_collection()) - { - return false; - } - - SampleResult result; - while (read_next_sample(result)) + try { - samples.push_back(std::move(result)); + if (!ensure_collection()) + { + return false; + } + + SampleResult result; + while (read_next_sample(result)) + { + samples.push_back(std::move(result)); + } + } + catch (const std::exception& e) + { + std::cerr << "[SchemaTracker] read_all_samples failed: " << e.what() << std::endl; + m_target_collection_index.reset(); + return false; + } + catch (...) + { + std::cerr << "[SchemaTracker] read_all_samples failed with unknown exception." << std::endl; + m_target_collection_index.reset(); + return false; } // read_next_sample clears m_target_collection_index on API failure.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/live_trackers/cpp/schema_tracker_base.cpp` around lines 122 - 142, SchemaTrackerBase::read_all_samples calls ensure_collection() and read_next_sample() which may throw, letting OpenXR errors escape instead of returning false; wrap the body in a try/catch that catches std::exception (and a catch(...) fallback), translate any thrown error into a false return, and ensure m_target_collection_index is cleared/left in the same state as an API failure (match the behavior of read_next_sample on failure) before returning false so callers see a boolean failure rather than an exception.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/core/deviceio_base/cpp/inc/deviceio_base/tracker.hpp`:
- Around line 66-70: Update the comment on the tracker factory contract to
reflect the new writer-based flow: remove the old "query a recorder for a
channel writer" wording and state that the factory (e.g., LiveDeviceIOFactory)
is now provided the optional MCAP writer and channel wiring up front so it can
attach the writer/channel to the tracker implementation during construction;
reference the tracker factory method and LiveDeviceIOFactory in the comment so
it's clear which symbols use the new contract.
In `@src/core/live_trackers/cpp/inc/live_trackers/schema_tracker.hpp`:
- Around line 73-85: The loop currently calls
flatbuffers::GetRoot<DataTableT>(...) and then UnPackTo(out_latest.get()) on
untrusted OpenXR samples; add a proper buffer verification step using
flatbuffers::Verifier::VerifyBuffer<DataTableT>() before calling UnPackTo to
avoid unpacking malformed data. Concretely, in the loop that iterates over
samples_ (where GetRoot<DataTableT> and UnPackTo are used), construct a
flatbuffers::Verifier with the sample.buffer.data() and sample.buffer.size() and
only call UnPackTo(out_latest.get()) when VerifyBuffer returns true; if
verification fails, skip that sample and optionally log or continue. Ensure you
reference DataTableT, GetRoot, UnPackTo, samples_ and use
flatbuffers::Verifier::VerifyBuffer<DataTableT>() for the guard.
In `@src/core/live_trackers/cpp/schema_tracker_base.cpp`:
- Around line 106-108: The code currently assigns
coll_props.data.totalSampleSize to m_sample_size without validation; guard this
assignment by enforcing SchemaTrackerConfig::max_flatbuffer_size: check
coll_props.data.totalSampleSize against the configured max_flatbuffer_size in
the SchemaTrackerConfig used by SchemaTrackerBase and if it exceeds the limit
reject/abort the collection registration (log an error and return/fail) or
cap/normalize only after explicit, safe decision; reference m_sample_size,
m_sample_batch_stride, coll_props.sampleBatchStride,
coll_props.data.totalSampleSize, SchemaTrackerConfig::max_flatbuffer_size, and
read_next_sample() when adding the check and error path so oversized samples
cannot be accepted.
In `@src/core/mcap_tests/cpp/test_mcap_recorder.cpp`:
- Around line 94-123: The readback loop in the TEST_CASE "McapTrackerChannels:
typed write produces readable MCAP with correct topic and schema" currently only
checks view.message.logTime and dataSize; instead deserialize the FlatBuffer
payload from view.message (use the FlatBuffers root type for
core::HeadPoseRecord) and assert for the populated write (the one written via
HeadChannels::write with head_data) that the deserialized record's timestamp
equals 42 and record->data()->is_valid() is true; likewise for the null-data
case (second write) deserialize and assert timestamp == 10 and record->data() ==
nullptr; locate the checks in the reader.readMessages() loop and replace the
weak dataSize checks with these concrete FlatBuffer deserialization and
assertions referencing HeadChannels, core::HeadPoseRecord, and view.message.
---
Outside diff comments:
In `@src/core/python/CMakeLists.txt`:
- Around line 15-29: The python_package custom target currently stops depending
on mcap_py but does not remove previously emitted artifacts, so stale _mcap
binaries can be packaged; update the python_package target (or its
install/packaging step) to explicitly remove the
"${CMAKE_BINARY_DIR}/python_package/$<CONFIG>/isaacteleop/mcap" directory (or
any _mcap artifacts) before creating the package — e.g. add a pre-step using
CMAKE_COMMAND -E remove_directory for that path so incremental builds won’t
resurrect the removed mcap API when mcap_py is dropped from DEPENDS.
---
Duplicate comments:
In `@src/core/deviceio_session/cpp/deviceio_session.cpp`:
- Around line 66-73: The destructor DeviceIOSession::~DeviceIOSession currently
calls mcap_writer_->close() but ignores its return status; modify the destructor
to capture the close() return value from mcap_writer_->close(), check for an
error/false result, and log a warning (using std::cerr or the existing logging
mechanism) including any error detail or status to aid debugging of
truncated/corrupt MCAP files while still ensuring resources are cleaned up.
In
`@src/core/deviceio_trackers/cpp/inc/deviceio_trackers/frame_metadata_tracker_oak.hpp`:
- Around line 18-20: FrameMetadataTrackerOak currently exposes MCAP-specific
channel naming via its docs and the get_channel_names() accessor; remove MCAP
concerns from FrameMetadataTrackerOak (and its public API) so it only handles
stream selection/state, and move the channel name derivation/caching logic into
LiveFrameMetadataTrackerOakImpl (or a new internal helper) where you should
generate and cache the recording-specific channel names; update
get_channel_names() on FrameMetadataTrackerOak to either be removed or to return
only stream identifiers, and implement a private/internal method on
LiveFrameMetadataTrackerOakImpl that returns the MCAP channel names used for
recording.
In `@src/core/live_trackers/cpp/live_frame_metadata_tracker_oak_impl.cpp`:
- Around line 61-68: The update method currently ignores per-stream failures by
always returning true; change LiveFrameMetadataTrackerOakImpl::update to capture
each stream.reader->update(...) return value and aggregate them (e.g.,
overall_ok = overall_ok || stream_ok) across m_streams, then return the
aggregated boolean so DeviceIOSession sees outages; mirror the pattern used in
LiveHandTrackerImpl::update (use OR across streams rather than unconditionally
returning true).
In `@src/core/live_trackers/cpp/live_generic_3axis_pedal_tracker_impl.cpp`:
- Around line 45-49: The update() method currently always returns true, hiding
failures; change LiveGeneric3AxisPedalTrackerImpl::update to propagate the
actual status from m_schema_reader.update(m_tracked.data) (i.e., return its
boolean/result value), and if m_schema_reader.update doesn't already return a
bool convert or map its result to a boolean so DeviceIOSession::update can
correctly detect and account for failures.
In `@src/core/live_trackers/cpp/live_hand_tracker_impl.cpp`:
- Around line 55-59: The call to xrGetSystemProperties (using
core_funcs.xrGetSystemProperties with handles.instance, system_id, and
system_props) currently only throws if XR_SUCCEEDED(result) but hand tracking
not supported, leaving XR_FAILED(result) to fall through; update the constructor
to mirror the xrGetSystem handling by checking XR_FAILED(result) immediately
after the call and throwing a std::runtime_error (with a clear message) if it
fails, before inspecting hand_tracking_props.supportsHandTracking, so the
constructor does not continue without a verified system properties result.
- Around line 179-187: The code reads joint_loc.pose.position,
joint_loc.pose.orientation, and joint_loc.radius before checking
XR_SPACE_LOCATION_POSITION_VALID_BIT / XR_SPACE_LOCATION_ORIENTATION_VALID_BIT,
which is undefined per OpenXR; update the construction of Pose and HandJointPose
so you first test the bits on joint_loc.locationFlags and only read position
when XR_SPACE_LOCATION_POSITION_VALID_BIT is set and only read orientation when
XR_SPACE_LOCATION_ORIENTATION_VALID_BIT is set (and only read radius when the
corresponding validity bit guarantees it's defined), otherwise use safe defaults
(zero position / identity quaternion / zero radius) and preserve the is_valid
boolean set from the flags; change the block around joint_loc, Pose, position,
orientation, radius, and HandJointPose to conditionally populate fields based on
those validity bits.
In `@src/core/live_trackers/cpp/schema_tracker_base.cpp`:
- Around line 122-142: SchemaTrackerBase::read_all_samples calls
ensure_collection() and read_next_sample() which may throw, letting OpenXR
errors escape instead of returning false; wrap the body in a try/catch that
catches std::exception (and a catch(...) fallback), translate any thrown error
into a false return, and ensure m_target_collection_index is cleared/left in the
same state as an API failure (match the behavior of read_next_sample on failure)
before returning false so callers see a boolean failure rather than an
exception.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9ae43066-d8f0-43eb-a8a7-8fb70d5b2147
📒 Files selected for processing (47)
examples/oxr/python/modular_example_with_mcap.pyexamples/oxr/python/test_oak_camera.pysrc/core/deviceio_base/cpp/inc/deviceio_base/tracker.hppsrc/core/deviceio_session/cpp/CMakeLists.txtsrc/core/deviceio_session/cpp/deviceio_session.cppsrc/core/deviceio_session/cpp/inc/deviceio_session/deviceio_session.hppsrc/core/deviceio_trackers/cpp/controller_tracker.cppsrc/core/deviceio_trackers/cpp/frame_metadata_tracker_oak.cppsrc/core/deviceio_trackers/cpp/full_body_tracker_pico.cppsrc/core/deviceio_trackers/cpp/generic_3axis_pedal_tracker.cppsrc/core/deviceio_trackers/cpp/hand_tracker.cppsrc/core/deviceio_trackers/cpp/head_tracker.cppsrc/core/deviceio_trackers/cpp/inc/deviceio_trackers/controller_tracker.hppsrc/core/deviceio_trackers/cpp/inc/deviceio_trackers/frame_metadata_tracker_oak.hppsrc/core/deviceio_trackers/cpp/inc/deviceio_trackers/full_body_tracker_pico.hppsrc/core/deviceio_trackers/cpp/inc/deviceio_trackers/generic_3axis_pedal_tracker.hppsrc/core/deviceio_trackers/cpp/inc/deviceio_trackers/hand_tracker.hppsrc/core/deviceio_trackers/cpp/inc/deviceio_trackers/head_tracker.hppsrc/core/live_trackers/cpp/CMakeLists.txtsrc/core/live_trackers/cpp/inc/live_trackers/live_deviceio_factory.hppsrc/core/live_trackers/cpp/inc/live_trackers/schema_tracker.hppsrc/core/live_trackers/cpp/inc/live_trackers/schema_tracker_base.hppsrc/core/live_trackers/cpp/live_controller_tracker_impl.cppsrc/core/live_trackers/cpp/live_controller_tracker_impl.hppsrc/core/live_trackers/cpp/live_deviceio_factory.cppsrc/core/live_trackers/cpp/live_frame_metadata_tracker_oak_impl.cppsrc/core/live_trackers/cpp/live_frame_metadata_tracker_oak_impl.hppsrc/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/cpp/live_generic_3axis_pedal_tracker_impl.cppsrc/core/live_trackers/cpp/live_generic_3axis_pedal_tracker_impl.hppsrc/core/live_trackers/cpp/live_hand_tracker_impl.cppsrc/core/live_trackers/cpp/live_hand_tracker_impl.hppsrc/core/live_trackers/cpp/live_head_tracker_impl.cppsrc/core/live_trackers/cpp/live_head_tracker_impl.hppsrc/core/live_trackers/cpp/schema_tracker_base.cppsrc/core/mcap/CMakeLists.txtsrc/core/mcap/cpp/CMakeLists.txtsrc/core/mcap/cpp/inc/mcap/recorder.hppsrc/core/mcap/cpp/inc/mcap/tracker_channels.hppsrc/core/mcap/cpp/mcap_recorder.cppsrc/core/mcap/python/CMakeLists.txtsrc/core/mcap/python/mcap_bindings.cppsrc/core/mcap/python/mcap_init.pysrc/core/mcap_tests/cpp/CMakeLists.txtsrc/core/mcap_tests/cpp/test_mcap_recorder.cppsrc/core/python/CMakeLists.txt
💤 Files with no reviewable changes (12)
- src/core/deviceio_trackers/cpp/head_tracker.cpp
- src/core/deviceio_trackers/cpp/generic_3axis_pedal_tracker.cpp
- src/core/deviceio_trackers/cpp/inc/deviceio_trackers/generic_3axis_pedal_tracker.hpp
- src/core/deviceio_trackers/cpp/hand_tracker.cpp
- src/core/mcap/python/mcap_bindings.cpp
- src/core/deviceio_trackers/cpp/frame_metadata_tracker_oak.cpp
- src/core/mcap/CMakeLists.txt
- src/core/deviceio_trackers/cpp/full_body_tracker_pico.cpp
- src/core/deviceio_trackers/cpp/controller_tracker.cpp
- src/core/mcap/cpp/inc/mcap/recorder.hpp
- src/core/mcap/python/CMakeLists.txt
- src/core/mcap/cpp/mcap_recorder.cpp
nvddr
force-pushed
the
devdeepr/refactorMcap
branch
2 times, most recently
from
b8b0af1 to
93e83ad
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/core/python/CMakeLists.txt (1)
4-4: 🧹 Nitpick | 🔵 TrivialConsider updating the comment to reflect MCAP changes.
The comment mentions "MCAP" as one of the combined modules, but since the MCAP native module (
mcap_py) has been removed, this comment may be slightly misleading. Consider updating it to reflect the current state if the mcap package no longer contains native bindings.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/python/CMakeLists.txt` at line 4, Update the top-of-file comment in CMakeLists.txt that currently lists "MCAP" among the combined modules for Python wheel packaging; change the wording to reflect that the mcap package no longer includes native bindings (mcap_py removed) — e.g., replace "MCAP" or "MCAP (native mcap_py)" with "mcap (pure-Python, no native bindings)" or remove MCAP from the list so the comment accurately describes the wheel contents and avoid implying a native module exists.
♻️ Duplicate comments (8)
src/core/deviceio_trackers/cpp/inc/deviceio_trackers/frame_metadata_tracker_oak.hpp (1)
18-20: 🛠️ Refactor suggestion | 🟠 MajorKeep MCAP channel naming out of
FrameMetadataTrackerOak’s public surface.The new accessor and the updated class description put recording metadata back on the tracker type even though this refactor is moving MCAP concerns into
DeviceIOSession/ the live implementations. Please derive or cache the per-stream channel names inside the live implementation (or an internal helper) instead of exposing them here.Also applies to: 89-92
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/deviceio_trackers/cpp/inc/deviceio_trackers/frame_metadata_tracker_oak.hpp` around lines 18 - 20, The class FrameMetadataTrackerOak currently exposes MCAP channel naming in its public surface (and its docstring); remove the public accessor that returns per-stream channel names and revert the class description to avoid mentioning MCAP/channels, and instead have the live DeviceIOSession implementations or an internal helper derive/cache per-stream channel names; update any calls that used FrameMetadataTrackerOak::getChannelNames (or similarly named accessor) to obtain names from the live implementation or helper (e.g. DeviceIOSession::deriveChannelNameForStream or an internal ChannelNameCache) so MCAP concerns are kept out of FrameMetadataTrackerOak.examples/oxr/python/test_oak_camera.py (1)
103-105: 🧹 Nitpick | 🔵 TrivialCollapse this transformed-list loop into a comprehension.
Ruff is still flagging this block with PERF401.
♻️ Minimal cleanup
- parts = [] - for name in stream_names: - parts.append(f"{name}={metadata_samples.get(name, 0)}") + parts = [ + f"{name}={metadata_samples.get(name, 0)}" + for name in stream_names + ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/oxr/python/test_oak_camera.py` around lines 103 - 105, Replace the explicit loop that builds parts by appending per-name strings with a single list comprehension: create parts from stream_names by producing a string for each name that combines the name and the value retrieved via metadata_samples.get(name, 0). Update the code that currently iterates "for name in stream_names: parts.append(...)" to a single assignment to parts using that comprehension so Ruff's PERF401 is resolved.src/core/mcap_tests/cpp/test_mcap_recorder.cpp (1)
94-123:⚠️ Potential issue | 🟠 MajorDeserialize the
HeadPoseRecordpayloads in these readback checks.These assertions only prove that bytes were written. A regression that writes the wrong record timestamp or drops
datawould still pass. Parseview.messageback intocore::HeadPoseRecordand assert the populated case preservestimestamp == 42plus validdata(), and the null-data case preservestimestamp == 10withdata() == nullptr.Also applies to: 126-149
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/mcap_tests/cpp/test_mcap_recorder.cpp` around lines 94 - 123, Update the readback assertions to deserialize the MCAP payload into core::HeadPoseRecord and assert field values instead of just byte checks: for messages read from reader.readMessages() parse view.message.data (using the same serialization/deserialization used by HeadChannels) into a core::HeadPoseRecord and CHECK that record.timestamp == 42 and record.data() is non-null for the populated case, and similarly add/modify the null-data case to deserialize and CHECK record.timestamp == 10 and record.data() == nullptr; reference HeadChannels, HeadPoseT, core::HeadPoseRecord and view.message when locating where to replace the byte-size/time checks with proper deserialization and field assertions.src/core/deviceio_base/cpp/inc/deviceio_base/tracker.hpp (1)
67-70:⚠️ Potential issue | 🟡 MinorUpdate the factory contract comment.
The text still describes the removed recorder/channel-writer path. After this refactor the factory receives the optional MCAP wiring up front, so this now documents the wrong construction flow.
📝 Suggested wording
- * concrete type, passing `this` so the factory can query a recorder for a - * channel writer and attach it to the impl at construction time. + * concrete type, passing `this` so the factory can look up any configured + * MCAP channel wiring and attach it to the impl at construction time.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/deviceio_base/cpp/inc/deviceio_base/tracker.hpp` around lines 67 - 70, Update the factory contract comment to remove references to the old recorder/channel-writer flow and state that the factory parameter now provides optional MCAP wiring up front during construction; specifically reword the `@param` factory description for the Tracker constructor/factory method (and mention concrete factories like LiveDeviceIOFactory) to indicate the factory supplies any optional MCAP writer or wiring at creation time rather than querying a recorder on `this`. Ensure the comment references the factory parameter and MCAP wiring clearly and drops the obsolete "channel writer"/"recorder" language.src/core/live_trackers/cpp/live_hand_tracker_impl.cpp (2)
181-189:⚠️ Potential issue | 🟠 MajorReading undefined pose/radius fields when validity bits are unset.
Per OpenXR specification,
poseandradiusfields inXrHandJointLocationEXTare undefined when their corresponding validity bits (XR_SPACE_LOCATION_POSITION_VALID_BIT,XR_SPACE_LOCATION_ORIENTATION_VALID_BIT) are not set. The code reads these values unconditionally (Lines 181-184, 189) before checkingis_valid, which leaks undefined data into theHandJointPosestructure and potentially MCAP recordings.Consider using default/zero values when validity bits are unset.
🐛 Proposed fix
for (uint32_t i = 0; i < XR_HAND_JOINT_COUNT_EXT; ++i) { const auto& joint_loc = joint_locations[i]; - Point position(joint_loc.pose.position.x, joint_loc.pose.position.y, joint_loc.pose.position.z); - Quaternion orientation(joint_loc.pose.orientation.x, joint_loc.pose.orientation.y, joint_loc.pose.orientation.z, - joint_loc.pose.orientation.w); - Pose pose(position, orientation); - bool is_valid = (joint_loc.locationFlags & XR_SPACE_LOCATION_POSITION_VALID_BIT) && (joint_loc.locationFlags & XR_SPACE_LOCATION_ORIENTATION_VALID_BIT); - HandJointPose joint_pose(pose, is_valid, joint_loc.radius); + Pose pose; + float radius = 0.0f; + if (is_valid) + { + Point position(joint_loc.pose.position.x, joint_loc.pose.position.y, joint_loc.pose.position.z); + Quaternion orientation(joint_loc.pose.orientation.x, joint_loc.pose.orientation.y, joint_loc.pose.orientation.z, + joint_loc.pose.orientation.w); + pose = Pose(position, orientation); + radius = joint_loc.radius; + } + + HandJointPose joint_pose(pose, is_valid, radius); tracked.data->joints->mutable_poses()->Mutate(i, joint_pose); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/live_trackers/cpp/live_hand_tracker_impl.cpp` around lines 181 - 189, The code reads joint_loc.pose and joint_loc.radius unconditionally and constructs Point, Quaternion, Pose and HandJointPose before checking validity bits; change LiveHandTrackerImpl to first evaluate validity using joint_loc.locationFlags (XR_SPACE_LOCATION_POSITION_VALID_BIT and XR_SPACE_LOCATION_ORIENTATION_VALID_BIT) and only populate Point/Quaternion/Pose with real values when those bits are set, otherwise use zero/default position, identity orientation and zero radius, then construct HandJointPose(joint_pose) with the computed pose, is_valid flag, and safe radius to avoid leaking undefined data from joint_loc into HandJointPose.
56-60:⚠️ Potential issue | 🟡 MinorSilent failure when
xrGetSystemPropertiesfails.When
xrGetSystemPropertiesfails (Line 56), the code silently proceeds to load extension functions and create hand trackers. The capability check is only performed on success. If properties retrieval fails, the system may not support hand tracking, but the constructor proceeds anyway—which could fail later with a confusing error or silently produce invalid data.🐛 Proposed fix
result = core_funcs.xrGetSystemProperties(handles.instance, system_id, &system_props); - if (XR_SUCCEEDED(result) && !hand_tracking_props.supportsHandTracking) + if (XR_FAILED(result)) + { + throw std::runtime_error("Failed to get OpenXR system properties: " + std::to_string(result)); + } + if (!hand_tracking_props.supportsHandTracking) { throw std::runtime_error("Hand tracking not supported by this system"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/live_trackers/cpp/live_hand_tracker_impl.cpp` around lines 56 - 60, The xrGetSystemProperties call's failure is ignored, so when result is not XR_SUCCEEDED the constructor continues; update the constructor in live_hand_tracker_impl.cpp to check the return value of xrGetSystemProperties (result) and treat non-success as a hard failure: throw a std::runtime_error (or return an error) that includes the failing result code and context (e.g., "xrGetSystemProperties failed: <result>") instead of proceeding to load extensions and create hand trackers; keep the existing supportsHandTracking check (hand_tracking_props.supportsHandTracking) for the success path.src/core/live_trackers/cpp/live_generic_3axis_pedal_tracker_impl.cpp (1)
45-49:⚠️ Potential issue | 🟠 MajorReturn the schema-reader status from
update().
DeviceIOSession::update()uses this boolean for failure accounting and health monitoring. Always returningtruehides lost/unreachable pedal data and leaves stale samples looking healthy. Per the relevant code snippet fromdeviceio_session.cpp(Lines 112-135), failed updates increment failure counters and trigger warnings.🐛 Proposed fix
bool LiveGeneric3AxisPedalTrackerImpl::update(XrTime /*time*/) { - m_schema_reader.update(m_tracked.data); - return true; + return m_schema_reader.update(m_tracked.data); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/live_trackers/cpp/live_generic_3axis_pedal_tracker_impl.cpp` around lines 45 - 49, The update() method currently always returns true which hides failures; change LiveGeneric3AxisPedalTrackerImpl::update to return the actual schema-reader result by calling m_schema_reader.update(m_tracked.data) (or storing its boolean result and returning that) so DeviceIOSession::update can detect and count failures; ensure you propagate that boolean return value rather than a hardcoded true.src/core/live_trackers/cpp/live_frame_metadata_tracker_oak_impl.cpp (1)
61-68:⚠️ Potential issue | 🟠 MajorPropagate tracker-read failures out of
update().Similar to the pedal tracker, this always returns
trueregardless of whether any stream reader succeeded.DeviceIOSession::update()uses the return value for tracker health accounting. Consider propagating failures:🐛 Proposed fix - option 1: fail if any stream fails
bool LiveFrameMetadataTrackerOakImpl::update(XrTime /*time*/) { + bool all_ok = true; for (auto& stream : m_streams) { - stream.reader->update(stream.tracked.data); + if (!stream.reader->update(stream.tracked.data)) + { + all_ok = false; + } } - return true; + return all_ok; }🐛 Proposed fix - option 2: fail only if all streams fail
bool LiveFrameMetadataTrackerOakImpl::update(XrTime /*time*/) { + bool any_ok = false; for (auto& stream : m_streams) { - stream.reader->update(stream.tracked.data); + if (stream.reader->update(stream.tracked.data)) + { + any_ok = true; + } } - return true; + return any_ok || m_streams.empty(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/live_trackers/cpp/live_frame_metadata_tracker_oak_impl.cpp` around lines 61 - 68, The update method currently ignores per-stream reader results; modify LiveFrameMetadataTrackerOakImpl::update to collect the boolean result of each stream.reader->update(stream.tracked.data) call across m_streams and return an aggregated success value instead of always true; specifically, iterate m_streams, call stream.reader->update(...), track successes, and return false when all updates fail (or alternatively return false if any update fails if you prefer the "fail-fast" behavior) so DeviceIOSession::update can accurately account tracker health.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/core/live_trackers/cpp/live_full_body_tracker_pico_impl.cpp`:
- Around line 143-153: The code is reading joint_loc.pose fields regardless of
validity bits which can be undefined; update the loop in
live_full_body_tracker_pico_impl.cpp so you first compute is_valid from
joint_loc.locationFlags, and only construct Point, Quaternion, and Pose from
joint_loc.pose when is_valid is true; when is_valid is false, create a safe
default Pose (e.g., zero position and identity orientation) or otherwise mark
the BodyJointPose as invalid, then call BodyJointPose(pose, is_valid) and use
tracked_.data->joints->mutable_joints()->Mutate(i, joint_pose) as before to
avoid storing undefined data.
In `@src/core/mcap_tests/cpp/test_mcap_recorder.cpp`:
- Around line 4-12: Add one end-to-end test in test_mcap_recorder.cpp that
records via the DeviceIOSession/LiveDeviceIOFactory path instead of calling
McapTrackerChannels or McapRecordingConfig directly: instantiate a
LiveDeviceIOFactory (or the factory used by production), create a
DeviceIOSession configured to use the same tracker name/channel used by existing
Mcap tests, perform a real write through DeviceIOSession (emit the same RecordT
payload), close/flush the session, then open the resulting MCAP buffer/file with
mcap::Reader and assert the expected topic/tracker name and payload are present;
this will validate tracker-name lookup and injection wiring rather than only
exercising McapTrackerChannels or McapRecordingConfig.
In `@src/core/mcap/python/mcap_init.py`:
- Around line 6-9: The Python binding in session_bindings.cpp must be updated to
accept and forward MCAP recording parameters to the C++ API: add parameters for
mcap_filename and mcap_channels (or a recording_config dict/struct) to the
DeviceIOSession.run binding, construct a McapRecordingConfig from those kwargs
(honoring defaults/nulls) and pass it into DeviceIOSession::run instead of the
hardcoded default; update the binding signature and argument parsing to avoid
TypeError when tests like test_oak_camera.py and modular_example_with_mcap.py
pass mcap_filename/mcap_channels, and ensure behavior matches the C++
McapRecordingConfig expected fields.
---
Outside diff comments:
In `@src/core/python/CMakeLists.txt`:
- Line 4: Update the top-of-file comment in CMakeLists.txt that currently lists
"MCAP" among the combined modules for Python wheel packaging; change the wording
to reflect that the mcap package no longer includes native bindings (mcap_py
removed) — e.g., replace "MCAP" or "MCAP (native mcap_py)" with "mcap
(pure-Python, no native bindings)" or remove MCAP from the list so the comment
accurately describes the wheel contents and avoid implying a native module
exists.
---
Duplicate comments:
In `@examples/oxr/python/test_oak_camera.py`:
- Around line 103-105: Replace the explicit loop that builds parts by appending
per-name strings with a single list comprehension: create parts from
stream_names by producing a string for each name that combines the name and the
value retrieved via metadata_samples.get(name, 0). Update the code that
currently iterates "for name in stream_names: parts.append(...)" to a single
assignment to parts using that comprehension so Ruff's PERF401 is resolved.
In `@src/core/deviceio_base/cpp/inc/deviceio_base/tracker.hpp`:
- Around line 67-70: Update the factory contract comment to remove references to
the old recorder/channel-writer flow and state that the factory parameter now
provides optional MCAP wiring up front during construction; specifically reword
the `@param` factory description for the Tracker constructor/factory method (and
mention concrete factories like LiveDeviceIOFactory) to indicate the factory
supplies any optional MCAP writer or wiring at creation time rather than
querying a recorder on `this`. Ensure the comment references the factory
parameter and MCAP wiring clearly and drops the obsolete "channel
writer"/"recorder" language.
In
`@src/core/deviceio_trackers/cpp/inc/deviceio_trackers/frame_metadata_tracker_oak.hpp`:
- Around line 18-20: The class FrameMetadataTrackerOak currently exposes MCAP
channel naming in its public surface (and its docstring); remove the public
accessor that returns per-stream channel names and revert the class description
to avoid mentioning MCAP/channels, and instead have the live DeviceIOSession
implementations or an internal helper derive/cache per-stream channel names;
update any calls that used FrameMetadataTrackerOak::getChannelNames (or
similarly named accessor) to obtain names from the live implementation or helper
(e.g. DeviceIOSession::deriveChannelNameForStream or an internal
ChannelNameCache) so MCAP concerns are kept out of FrameMetadataTrackerOak.
In `@src/core/live_trackers/cpp/live_frame_metadata_tracker_oak_impl.cpp`:
- Around line 61-68: The update method currently ignores per-stream reader
results; modify LiveFrameMetadataTrackerOakImpl::update to collect the boolean
result of each stream.reader->update(stream.tracked.data) call across m_streams
and return an aggregated success value instead of always true; specifically,
iterate m_streams, call stream.reader->update(...), track successes, and return
false when all updates fail (or alternatively return false if any update fails
if you prefer the "fail-fast" behavior) so DeviceIOSession::update can
accurately account tracker health.
In `@src/core/live_trackers/cpp/live_generic_3axis_pedal_tracker_impl.cpp`:
- Around line 45-49: The update() method currently always returns true which
hides failures; change LiveGeneric3AxisPedalTrackerImpl::update to return the
actual schema-reader result by calling m_schema_reader.update(m_tracked.data)
(or storing its boolean result and returning that) so DeviceIOSession::update
can detect and count failures; ensure you propagate that boolean return value
rather than a hardcoded true.
In `@src/core/live_trackers/cpp/live_hand_tracker_impl.cpp`:
- Around line 181-189: The code reads joint_loc.pose and joint_loc.radius
unconditionally and constructs Point, Quaternion, Pose and HandJointPose before
checking validity bits; change LiveHandTrackerImpl to first evaluate validity
using joint_loc.locationFlags (XR_SPACE_LOCATION_POSITION_VALID_BIT and
XR_SPACE_LOCATION_ORIENTATION_VALID_BIT) and only populate Point/Quaternion/Pose
with real values when those bits are set, otherwise use zero/default position,
identity orientation and zero radius, then construct HandJointPose(joint_pose)
with the computed pose, is_valid flag, and safe radius to avoid leaking
undefined data from joint_loc into HandJointPose.
- Around line 56-60: The xrGetSystemProperties call's failure is ignored, so
when result is not XR_SUCCEEDED the constructor continues; update the
constructor in live_hand_tracker_impl.cpp to check the return value of
xrGetSystemProperties (result) and treat non-success as a hard failure: throw a
std::runtime_error (or return an error) that includes the failing result code
and context (e.g., "xrGetSystemProperties failed: <result>") instead of
proceeding to load extensions and create hand trackers; keep the existing
supportsHandTracking check (hand_tracking_props.supportsHandTracking) for the
success path.
In `@src/core/mcap_tests/cpp/test_mcap_recorder.cpp`:
- Around line 94-123: Update the readback assertions to deserialize the MCAP
payload into core::HeadPoseRecord and assert field values instead of just byte
checks: for messages read from reader.readMessages() parse view.message.data
(using the same serialization/deserialization used by HeadChannels) into a
core::HeadPoseRecord and CHECK that record.timestamp == 42 and record.data() is
non-null for the populated case, and similarly add/modify the null-data case to
deserialize and CHECK record.timestamp == 10 and record.data() == nullptr;
reference HeadChannels, HeadPoseT, core::HeadPoseRecord and view.message when
locating where to replace the byte-size/time checks with proper deserialization
and field assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 58d74d84-396b-436c-ad3d-64899782602d
📒 Files selected for processing (47)
examples/oxr/python/modular_example_with_mcap.pyexamples/oxr/python/test_oak_camera.pysrc/core/deviceio_base/cpp/inc/deviceio_base/tracker.hppsrc/core/deviceio_session/cpp/CMakeLists.txtsrc/core/deviceio_session/cpp/deviceio_session.cppsrc/core/deviceio_session/cpp/inc/deviceio_session/deviceio_session.hppsrc/core/deviceio_trackers/cpp/controller_tracker.cppsrc/core/deviceio_trackers/cpp/frame_metadata_tracker_oak.cppsrc/core/deviceio_trackers/cpp/full_body_tracker_pico.cppsrc/core/deviceio_trackers/cpp/generic_3axis_pedal_tracker.cppsrc/core/deviceio_trackers/cpp/hand_tracker.cppsrc/core/deviceio_trackers/cpp/head_tracker.cppsrc/core/deviceio_trackers/cpp/inc/deviceio_trackers/controller_tracker.hppsrc/core/deviceio_trackers/cpp/inc/deviceio_trackers/frame_metadata_tracker_oak.hppsrc/core/deviceio_trackers/cpp/inc/deviceio_trackers/full_body_tracker_pico.hppsrc/core/deviceio_trackers/cpp/inc/deviceio_trackers/generic_3axis_pedal_tracker.hppsrc/core/deviceio_trackers/cpp/inc/deviceio_trackers/hand_tracker.hppsrc/core/deviceio_trackers/cpp/inc/deviceio_trackers/head_tracker.hppsrc/core/live_trackers/cpp/CMakeLists.txtsrc/core/live_trackers/cpp/inc/live_trackers/live_deviceio_factory.hppsrc/core/live_trackers/cpp/inc/live_trackers/schema_tracker.hppsrc/core/live_trackers/cpp/inc/live_trackers/schema_tracker_base.hppsrc/core/live_trackers/cpp/live_controller_tracker_impl.cppsrc/core/live_trackers/cpp/live_controller_tracker_impl.hppsrc/core/live_trackers/cpp/live_deviceio_factory.cppsrc/core/live_trackers/cpp/live_frame_metadata_tracker_oak_impl.cppsrc/core/live_trackers/cpp/live_frame_metadata_tracker_oak_impl.hppsrc/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/cpp/live_generic_3axis_pedal_tracker_impl.cppsrc/core/live_trackers/cpp/live_generic_3axis_pedal_tracker_impl.hppsrc/core/live_trackers/cpp/live_hand_tracker_impl.cppsrc/core/live_trackers/cpp/live_hand_tracker_impl.hppsrc/core/live_trackers/cpp/live_head_tracker_impl.cppsrc/core/live_trackers/cpp/live_head_tracker_impl.hppsrc/core/live_trackers/cpp/schema_tracker_base.cppsrc/core/mcap/CMakeLists.txtsrc/core/mcap/cpp/CMakeLists.txtsrc/core/mcap/cpp/inc/mcap/recorder.hppsrc/core/mcap/cpp/inc/mcap/tracker_channels.hppsrc/core/mcap/cpp/mcap_recorder.cppsrc/core/mcap/python/CMakeLists.txtsrc/core/mcap/python/mcap_bindings.cppsrc/core/mcap/python/mcap_init.pysrc/core/mcap_tests/cpp/CMakeLists.txtsrc/core/mcap_tests/cpp/test_mcap_recorder.cppsrc/core/python/CMakeLists.txt
💤 Files with no reviewable changes (12)
- src/core/deviceio_trackers/cpp/frame_metadata_tracker_oak.cpp
- src/core/deviceio_trackers/cpp/hand_tracker.cpp
- src/core/mcap/python/CMakeLists.txt
- src/core/deviceio_trackers/cpp/controller_tracker.cpp
- src/core/deviceio_trackers/cpp/generic_3axis_pedal_tracker.cpp
- src/core/deviceio_trackers/cpp/head_tracker.cpp
- src/core/mcap/CMakeLists.txt
- src/core/deviceio_trackers/cpp/inc/deviceio_trackers/generic_3axis_pedal_tracker.hpp
- src/core/mcap/cpp/inc/mcap/recorder.hpp
- src/core/deviceio_trackers/cpp/full_body_tracker_pico.cpp
- src/core/mcap/python/mcap_bindings.cpp
- src/core/mcap/cpp/mcap_recorder.cpp
nvddr
force-pushed
the
devdeepr/refactorMcap
branch
from
93e83ad to
530be87
Compare
nvddr
force-pushed
the
devdeepr/refactorMcap
branch
2 times, most recently
from
a506e7b to
f17bd8f
Compare
nvddr
force-pushed
the
devdeepr/refactorMcap
branch
from
f17bd8f to
b3df84f
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/core/live_trackers/cpp/live_hand_tracker_impl.cpp (1)
120-134:⚠️ Potential issue | 🟠 MajorRestore
&&for hand-update failure propagation.
update_hand()already returnstruewhen a hand is merely inactive, soleft_ok || right_okonly changes the real failure path. A single-hand update failure is now hidden fromDeviceIOSession::update(), which means the session clears its failure counter even though one side stopped updating.🔧 Suggested fix
- return left_ok || right_ok; + return left_ok && right_ok;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/live_trackers/cpp/live_hand_tracker_impl.cpp` around lines 120 - 134, The current LiveHandTrackerImpl::update sets left_ok and right_ok from update_hand but returns left_ok || right_ok, which hides single-hand update failures; change the return to use && so that update() fails if either update_hand fails (i.e., return left_ok && right_ok) to ensure DeviceIOSession::update() sees and propagates a single-side failure; update the return expression in LiveHandTrackerImpl::update accordingly and keep the existing mcap_channels_ write behavior unchanged.src/core/live_trackers/cpp/live_full_body_tracker_pico_impl.cpp (1)
70-81: 🧹 Nitpick | 🔵 TrivialAdd defensive null-check for consistency with similar code paths.
This code calls
loadExtensionFunction()which throws on failure, making the subsequent dereferences safe. However, bothLiveHandTrackerImplandActionContextFunctions::load()perform explicit null-checks after the same pattern, establishing a defensive-coding convention in this codebase. The body tracker should follow the same approach for consistency.Suggested fix
loadExtensionFunction(handles.instance, handles.xrGetInstanceProcAddr, "xrLocateBodyJointsBD", reinterpret_cast<PFN_xrVoidFunction*>(&pfn_locate_body_joints_)); + + if (!pfn_create_body_tracker_ || !pfn_destroy_body_tracker_ || !pfn_locate_body_joints_) + { + throw std::runtime_error("Failed to get body tracking function pointers"); + } XrBodyTrackerCreateInfoBD create_info{ XR_TYPE_BODY_TRACKER_CREATE_INFO_BD };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/live_trackers/cpp/live_full_body_tracker_pico_impl.cpp` around lines 70 - 81, After calling loadExtensionFunction for "xrCreateBodyTrackerBD", "xrDestroyBodyTrackerBD", and "xrLocateBodyJointsBD", add defensive null-checks for pfn_create_body_tracker_, pfn_destroy_body_tracker_, and pfn_locate_body_joints_ (matching the pattern used in LiveHandTrackerImpl and ActionContextFunctions::load) and bail out if any are null instead of proceeding; specifically, ensure you do not call pfn_create_body_tracker_ when it's null, log or return an appropriate error (or throw) and avoid creating XrBodyTrackerCreateInfoBD/body_tracker_ usage when the function pointers are missing.
♻️ Duplicate comments (3)
src/core/deviceio_session/cpp/deviceio_session.cpp (1)
31-64:⚠️ Potential issue | 🟠 MajorReject duplicate tracker mappings before opening the writer.
tracker_namesis documented as a mapping, but this block only checks membership. Repeating the sameITracker*twice leaves the effective base name ambiguous, and the current ordering has already opened/logged the MCAP file before the config is known-good.🔧 Suggested fix
+ std::set<const ITracker*> seen_trackers; for (const auto& [tracker_ptr, name] : recording_config.tracker_names) { + if (!seen_trackers.insert(tracker_ptr).second) + { + throw std::invalid_argument("DeviceIOSession: duplicate tracker entry in McapRecordingConfig for '" + + name + "'"); + } bool found = false; for (const auto& t : trackers) { if (t.get() == tracker_ptr)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/deviceio_session/cpp/deviceio_session.cpp` around lines 31 - 64, Validate recording_config.tracker_names for duplicates and consistency before creating or opening mcap_writer_: iterate over recording_config.tracker_names and (a) ensure each tracker_ptr exists in trackers (as currently done) and (b) reject duplicate tracker_ptr entries and duplicate name entries (throw std::invalid_argument if found) so the mapping is unambiguous; only after these checks succeed, create mcap_writer_ and call mcap_writer_->open(recording_config.filename, options) and then proceed to log the recording filename.src/core/live_trackers/cpp/schema_tracker_base.cpp (1)
106-107:⚠️ Potential issue | 🟠 MajorReject oversized or inconsistent samples when binding the collection.
SchemaTrackerConfig::max_flatbuffer_sizeis populated by tracker configs, but these assignments still trust the runtime-advertised sizes verbatim.read_next_sample()then allocatesdataBuffer(m_sample_batch_stride)andout.buffer.resize(m_sample_size)from those values, so a schema/runtime mismatch can cause oversized allocations, and the laterdataBuffer < m_sample_sizeguard only degrades that into a silent "present" result. Validate the sizes here and refuse the collection early.Suggested fix
m_sample_batch_stride = coll_props.sampleBatchStride; m_sample_size = coll_props.data.totalSampleSize; + if (m_sample_batch_stride < m_sample_size) + { + std::cerr << "[SchemaTracker] Collection '" << m_config.collection_id + << "' reports sampleBatchStride=" << m_sample_batch_stride + << " smaller than totalSampleSize=" << m_sample_size << std::endl; + m_target_collection_index.reset(); + m_last_sample_index.reset(); + return false; + } + if (m_config.max_flatbuffer_size != 0 && m_sample_size > m_config.max_flatbuffer_size) + { + std::cerr << "[SchemaTracker] Collection '" << m_config.collection_id + << "' sample size " << m_sample_size + << " exceeds max_flatbuffer_size=" << m_config.max_flatbuffer_size << std::endl; + m_target_collection_index.reset(); + m_last_sample_index.reset(); + return false; + } if (i != m_target_collection_index) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/live_trackers/cpp/schema_tracker_base.cpp` around lines 106 - 107, Validate the runtime-advertised sizes when binding a collection: in the binding code that sets m_sample_batch_stride and m_sample_size, check both values against SchemaTrackerConfig::max_flatbuffer_size and against each other (e.g., ensure m_sample_batch_stride >= m_sample_size and neither exceeds max_flatbuffer_size); if any check fails, refuse the collection binding early (return/error out) instead of accepting values that later allow oversized allocations in read_next_sample() where dataBuffer and out.buffer are sized. Ensure the error path logs/propagates a clear message so callers know the rejection reason.src/core/live_trackers/cpp/live_frame_metadata_tracker_oak_impl.cpp (1)
62-69:⚠️ Potential issue | 🟠 MajorDon't let one healthy Oak stream hide another failed one.
SchemaTracker::update()already returnsfalsewhen a stream collection is absent/unreachable. Using|=here means a single healthy stream keeps the whole tracker healthy, soDeviceIOSession::update()never increments failure counts for partial Oak outages and stale per-stream state can look fine.Suggested fix
bool LiveFrameMetadataTrackerOakImpl::update(XrTime /*time*/) { - bool any_present = false; + bool all_present = true; for (auto& stream : m_streams) { - any_present |= stream.reader->update(stream.tracked.data); + all_present &= stream.reader->update(stream.tracked.data); } - return any_present; + return all_present; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/live_trackers/cpp/live_frame_metadata_tracker_oak_impl.cpp` around lines 62 - 69, The current LiveFrameMetadataTrackerOakImpl::update uses |= across m_streams which lets one healthy stream mask others; change the logic to require all streams succeed: iterate m_streams and combine results from stream.reader->update(stream.tracked.data) using logical AND (e.g., compute an all_present flag initialized true and &= each update result) so the function returns false if any per-stream update fails, ensuring DeviceIOSession::update() can increment failure counts for partial outages.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/core/live_trackers/cpp/live_deviceio_factory.cpp`:
- Around line 23-27: The constructor LiveDeviceIOFactory currently builds
name_map_ from tracker_names with the map range constructor which silently
ignores duplicate ITracker* keys; change the constructor to populate name_map_
in the body by iterating tracker_names, track seen ITracker* pointers in a
std::unordered_set<const ITracker*> (or similar), and if a duplicate pointer is
encountered either throw std::invalid_argument (reject) or call the project's
logging facility to emit a clear error/warning and abort construction; insert
each unique pair into name_map_ (use name_map_.emplace(tracker, name)) so
duplicates are detected and handled rather than silently dropped.
In `@src/core/mcap_tests/cpp/test_mcap_tracker_channels.cpp`:
- Around line 101-126: The test currently only sets head_data->is_valid and
doesn't exercise serialization of the nested pose; set head_data->pose to a
populated shared_ptr (use std::make_shared for the FlatBuffer-generated table
type used by core::HeadPoseT/pose) and initialize a couple of pose fields (e.g.,
position / orientation or named fields used by live_head_tracker_impl.cpp)
before writing via HeadChannels, then after reading (where record->data() is
checked) assert those pose fields round-trip correctly (use the same field
accessors as in core::HeadPoseRecord/pose); ensure you allocate the nested
FlatBuffer table with std::make_shared (not make_unique) to match the shared_ptr
usage.
---
Outside diff comments:
In `@src/core/live_trackers/cpp/live_full_body_tracker_pico_impl.cpp`:
- Around line 70-81: After calling loadExtensionFunction for
"xrCreateBodyTrackerBD", "xrDestroyBodyTrackerBD", and "xrLocateBodyJointsBD",
add defensive null-checks for pfn_create_body_tracker_,
pfn_destroy_body_tracker_, and pfn_locate_body_joints_ (matching the pattern
used in LiveHandTrackerImpl and ActionContextFunctions::load) and bail out if
any are null instead of proceeding; specifically, ensure you do not call
pfn_create_body_tracker_ when it's null, log or return an appropriate error (or
throw) and avoid creating XrBodyTrackerCreateInfoBD/body_tracker_ usage when the
function pointers are missing.
In `@src/core/live_trackers/cpp/live_hand_tracker_impl.cpp`:
- Around line 120-134: The current LiveHandTrackerImpl::update sets left_ok and
right_ok from update_hand but returns left_ok || right_ok, which hides
single-hand update failures; change the return to use && so that update() fails
if either update_hand fails (i.e., return left_ok && right_ok) to ensure
DeviceIOSession::update() sees and propagates a single-side failure; update the
return expression in LiveHandTrackerImpl::update accordingly and keep the
existing mcap_channels_ write behavior unchanged.
---
Duplicate comments:
In `@src/core/deviceio_session/cpp/deviceio_session.cpp`:
- Around line 31-64: Validate recording_config.tracker_names for duplicates and
consistency before creating or opening mcap_writer_: iterate over
recording_config.tracker_names and (a) ensure each tracker_ptr exists in
trackers (as currently done) and (b) reject duplicate tracker_ptr entries and
duplicate name entries (throw std::invalid_argument if found) so the mapping is
unambiguous; only after these checks succeed, create mcap_writer_ and call
mcap_writer_->open(recording_config.filename, options) and then proceed to log
the recording filename.
In `@src/core/live_trackers/cpp/live_frame_metadata_tracker_oak_impl.cpp`:
- Around line 62-69: The current LiveFrameMetadataTrackerOakImpl::update uses |=
across m_streams which lets one healthy stream mask others; change the logic to
require all streams succeed: iterate m_streams and combine results from
stream.reader->update(stream.tracked.data) using logical AND (e.g., compute an
all_present flag initialized true and &= each update result) so the function
returns false if any per-stream update fails, ensuring DeviceIOSession::update()
can increment failure counts for partial outages.
In `@src/core/live_trackers/cpp/schema_tracker_base.cpp`:
- Around line 106-107: Validate the runtime-advertised sizes when binding a
collection: in the binding code that sets m_sample_batch_stride and
m_sample_size, check both values against
SchemaTrackerConfig::max_flatbuffer_size and against each other (e.g., ensure
m_sample_batch_stride >= m_sample_size and neither exceeds max_flatbuffer_size);
if any check fails, refuse the collection binding early (return/error out)
instead of accepting values that later allow oversized allocations in
read_next_sample() where dataBuffer and out.buffer are sized. Ensure the error
path logs/propagates a clear message so callers know the rejection reason.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2b74800c-68ba-4ed1-8155-ddb8bb1e0e8a
📒 Files selected for processing (52)
examples/oxr/python/modular_example_with_mcap.pyexamples/oxr/python/test_oak_camera.pysrc/core/deviceio_base/cpp/inc/deviceio_base/tracker.hppsrc/core/deviceio_session/cpp/CMakeLists.txtsrc/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/controller_tracker.cppsrc/core/deviceio_trackers/cpp/frame_metadata_tracker_oak.cppsrc/core/deviceio_trackers/cpp/full_body_tracker_pico.cppsrc/core/deviceio_trackers/cpp/generic_3axis_pedal_tracker.cppsrc/core/deviceio_trackers/cpp/hand_tracker.cppsrc/core/deviceio_trackers/cpp/head_tracker.cppsrc/core/deviceio_trackers/cpp/inc/deviceio_trackers/controller_tracker.hppsrc/core/deviceio_trackers/cpp/inc/deviceio_trackers/frame_metadata_tracker_oak.hppsrc/core/deviceio_trackers/cpp/inc/deviceio_trackers/full_body_tracker_pico.hppsrc/core/deviceio_trackers/cpp/inc/deviceio_trackers/generic_3axis_pedal_tracker.hppsrc/core/deviceio_trackers/cpp/inc/deviceio_trackers/hand_tracker.hppsrc/core/deviceio_trackers/cpp/inc/deviceio_trackers/head_tracker.hppsrc/core/live_trackers/cpp/CMakeLists.txtsrc/core/live_trackers/cpp/inc/live_trackers/live_deviceio_factory.hppsrc/core/live_trackers/cpp/inc/live_trackers/schema_tracker.hppsrc/core/live_trackers/cpp/inc/live_trackers/schema_tracker_base.hppsrc/core/live_trackers/cpp/live_controller_tracker_impl.cppsrc/core/live_trackers/cpp/live_controller_tracker_impl.hppsrc/core/live_trackers/cpp/live_deviceio_factory.cppsrc/core/live_trackers/cpp/live_frame_metadata_tracker_oak_impl.cppsrc/core/live_trackers/cpp/live_frame_metadata_tracker_oak_impl.hppsrc/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/cpp/live_generic_3axis_pedal_tracker_impl.cppsrc/core/live_trackers/cpp/live_generic_3axis_pedal_tracker_impl.hppsrc/core/live_trackers/cpp/live_hand_tracker_impl.cppsrc/core/live_trackers/cpp/live_hand_tracker_impl.hppsrc/core/live_trackers/cpp/live_head_tracker_impl.cppsrc/core/live_trackers/cpp/live_head_tracker_impl.hppsrc/core/live_trackers/cpp/schema_tracker_base.cppsrc/core/mcap/CMakeLists.txtsrc/core/mcap/cpp/CMakeLists.txtsrc/core/mcap/cpp/inc/mcap/recorder.hppsrc/core/mcap/cpp/inc/mcap/recording_traits.hppsrc/core/mcap/cpp/inc/mcap/tracker_channels.hppsrc/core/mcap/cpp/mcap_recorder.cppsrc/core/mcap/python/CMakeLists.txtsrc/core/mcap/python/mcap_bindings.cppsrc/core/mcap/python/mcap_init.pysrc/core/mcap_tests/cpp/CMakeLists.txtsrc/core/mcap_tests/cpp/test_mcap_recorder.cppsrc/core/mcap_tests/cpp/test_mcap_tracker_channels.cppsrc/core/python/CMakeLists.txtsrc/core/python/deviceio_init.py
💤 Files with no reviewable changes (12)
- src/core/mcap/CMakeLists.txt
- src/core/deviceio_trackers/cpp/full_body_tracker_pico.cpp
- src/core/deviceio_trackers/cpp/controller_tracker.cpp
- src/core/deviceio_trackers/cpp/generic_3axis_pedal_tracker.cpp
- src/core/deviceio_trackers/cpp/head_tracker.cpp
- src/core/deviceio_trackers/cpp/hand_tracker.cpp
- src/core/deviceio_trackers/cpp/inc/deviceio_trackers/generic_3axis_pedal_tracker.hpp
- src/core/mcap/cpp/inc/mcap/recorder.hpp
- src/core/mcap/python/mcap_bindings.cpp
- src/core/mcap/cpp/mcap_recorder.cpp
- src/core/mcap/python/CMakeLists.txt
- src/core/mcap_tests/cpp/test_mcap_recorder.cpp
aristarkhovNV
approved these changes
Mar 20, 2026
src/core/deviceio_session/cpp/inc/deviceio_session/deviceio_session.hpp
Outdated
Show resolved
Hide resolved
nvddr
force-pushed
the
devdeepr/refactorMcap
branch
2 times, most recently
from
d6398b8 to
b346fd1
Compare
nvddr
force-pushed
the
devdeepr/refactorMcap
branch
from
b346fd1 to
18d0fec
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
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.
Summary by CodeRabbit
New Features
Refactor
Tests
Other