[feat] Improve Multisensor mode documentation#65
Conversation
Align the depth-or-overlap contract across APIs and tools, document experimental tracking limitations, and add focused configuration and tracking coverage.
Remove unnecessary standalone artifacts and keep wheel guidance version-independent.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Enterprise Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughMultisensor odometry now has expanded rig validation, Python/YAML configuration, tracking tests, API documentation, onboarding guidance, and example coverage. Documentation identifies cuNLS availability, pinhole-camera support, and experimental behavior. ChangesMultisensor mode support
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cuvslam-skills/cuvslam-onboard/SKILL.md`:
- Around line 216-221: Clarify the camera configuration table’s scope by
labeling entries as ready-made examples if the rows only list available scripts,
and state that em dashes mean no example exists rather than unsupported
hardware. Otherwise, update the ZED, OAK-D, and Orbbec Multisensor entries to
match the supported integrations described in README.md.
In `@cuvslam-skills/cuvslam-troubleshoot/SKILL.md`:
- Around line 83-86: Update the tracking-mode diagnostic guidance around
tracking_mode/OdometryMode to state that Multisensor uses IMU fusion
automatically when Rig.imus is non-empty, rather than inferring IMU usage from
the mode value alone. Explicitly distinguish Multisensor from Inertial mode and
direct diagnosis to verify the rig’s IMU list.
In `@examples/multisensor/README.md`:
- Line 19: Update the multisensor README guidance to require strictly increasing
(or strictly ascending) timestamps instead of non-decreasing timestamps,
matching the validation in the tracker timestamp-handling logic.
In `@libs/launcher/multi_camera_launcher_base.cpp`:
- Around line 122-123: Update the calibration error message in
MultiCameraBaseLauncher to depend on FLAGS_mode: use the
depth-camera-or-overlapping-pair wording only for Multisensor mode, while
preserving the existing stereo-pair requirement message for Multicamera and
Inertial modes.
In `@python/test/test_tracking.py`:
- Around line 129-135: Add the tracker return-type annotation to the
_create_multisensor_tracker helper, using the existing vslam.Tracker type, while
preserving its current exception handling and skip behavior.
- Around line 166-173: Adjust the timestamp generation in the test loop around
tracker.register_imu_measurement and tracker.track so every IMU timestamp is
strictly greater than the preceding tracking timestamp, including frame 1 sample
0. Preserve the existing measurement cadence while adding the necessary offset
between frames.
- Around line 151-157: Update the “stereo without depth” subtest in the tracking
test to pass two distinct image arrays to each tracker.track call instead of
reusing the same image buffer. Preserve the existing timestamps, tracker setup,
and multisensor configuration while ensuring the stereo inputs have separate
underlying NumPy storage.
In `@python/utils.py`:
- Around line 113-123: Validate that multisensor_settings["depth_camera_ids"] is
a list before iterating or converting its elements. In the depth_camera_ids
handling within the multisensor settings branch, raise a clear ValueError for
non-list values, while preserving the existing integer conversion for valid
lists.
In `@tools/cuvslam_app/cuvslam_app.py`:
- Around line 689-691: Update the --enable_depth_stereo_tracking argument
definition in the cuvslam_app argument parser so its default preserves the
intended enabled behavior for multisensor mode instead of argparse supplying
False. Ensure explicit user-provided values still override the default and
multisensor initialization receives True when the option is omitted.
In `@tools/cuvslam_app/edex_reader.py`:
- Around line 71-73: Preserve all configured multisensor depth streams: in
tools/cuvslam_app/edex_reader.py lines 71-73, update the RGB-D parsing,
remapping, and replay flow to retain every depth-camera ID instead of a single
OdometryRGBDSettings; in tools/cuvslam_app/cuvslam_app.py lines 203-207,
populate OdometryMultisensorSettings.depth_camera_ids from the complete parsed
list; and in tools/cuvslam_app/cuvslam_app.py lines 446-456, select a
multisensor-aware reader path rather than the binary rgbd_mode model while
retaining optional-depth behavior for overlapping-only rigs.
In `@TROUBLESHOOTING.md`:
- Around line 453-458: Revise the Multisensor comparison in the troubleshooting
documentation to remove the unconditional “matches or exceeds Multicamera”
ranking. Describe multi-stereo + RGB-D + IMU performance as an empirical
expectation that depends on conditions, or cite specific benchmark conditions
supporting the comparison, while preserving the existing rig-selection guidance.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 4e9936ac-91a4-4f80-84a6-3f5d732ffb56
📒 Files selected for processing (32)
README.mdTROUBLESHOOTING.mdcuvslam-skills/README.mdcuvslam-skills/cuvslam-onboard/SKILL.mdcuvslam-skills/cuvslam-onboard/references/dataset-guides.mdcuvslam-skills/cuvslam-onboard/references/live-cameras.mdcuvslam-skills/cuvslam-troubleshoot/SKILL.mddoc/index.mdexamples/euroc/README.mdexamples/multisensor/README.mdexamples/multisensor/track_multisensor_tartan.pyexamples/realsense/README.mdexamples/realsense/run_multisensor.pyexamples/realsense/run_vio.pylibs/cuvslam/cuvslam2.cpplibs/cuvslam/cuvslam2.hlibs/launcher/launcher_create.cpplibs/launcher/multi_camera_launcher_base.cpplibs/odometry/svo_config.hlibs/utils/test/cuvslam_yaml_config_test.cpppython/cuvslam2.cpppython/docs/api.rstpython/test/test_tracking.pypython/tracker.pypython/utils.pytools/cuvslam_api_launcher/README.mdtools/cuvslam_api_launcher/api_config.yamltools/cuvslam_api_launcher/cuvslam_api_launcher.cpptools/cuvslam_app/README.mdtools/cuvslam_app/conversions.pytools/cuvslam_app/cuvslam_app.pytools/cuvslam_app/edex_reader.py
Keep the API and documentation change focused while tool support receives separate validation.
Test Results
cuVSLAM Evaluation KPIs
Artifacts |
Match the tracker input contract by giving each camera its own image allocation.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
python/test/test_tracking.py (1)
176-190: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSkip configuration-error tests when Multisensor support is unavailable.
These tests bypass
_create_multisensor_tracker. On builds without cuNLS, construction raises the cuNLS-support error before the expected depth/duplicate-camera validation, so both tests fail instead of being skipped. Add a shared capability guard before asserting the configuration-specific errors.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/test/test_tracking.py` around lines 176 - 190, Update test_multisensor_requires_depth_or_overlap and test_multisensor_rejects_duplicate_depth_camera_ids to share a capability guard that skips both tests when Multisensor/cuNLS support is unavailable. Place the guard before constructing the tracker or asserting configuration-specific ValueErrors, while preserving the existing validation assertions when support is available.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@python/test/test_tracking.py`:
- Around line 176-190: Update test_multisensor_requires_depth_or_overlap and
test_multisensor_rejects_duplicate_depth_camera_ids to share a capability guard
that skips both tests when Multisensor/cuNLS support is unavailable. Place the
guard before constructing the tracker or asserting configuration-specific
ValueErrors, while preserving the existing validation assertions when support is
available.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 14cb6c45-5a58-4e31-a81a-525f3269fbe0
📒 Files selected for processing (1)
python/test/test_tracking.py
Clarify runtime behavior and make tests and YAML validation enforce the documented input contracts.
Document non-decreasing cross-sensor ordering while retaining strict camera-frame ordering.
Align the depth-or-overlap contract across APIs and tools, document experimental tracking limitations, and add focused configuration and tracking coverage.
Summary by CodeRabbit
New Features
odometry_mode: "multisensor"withmultisensor_settings(depth camera IDs, scale factor, and depth-stereo tracking flag).Bug Fixes
Documentation
Tests