Pre-comp dev merge - #27
Open
osu-uwrt-bot2 wants to merge 81 commits into
Open
Conversation
Added binary classifier to split between 2 instances of a class. Updated dummy dets to support this behavior, and moved list to the config. Mapping now supports binary classifier. Normal behavior when binary classifier is not active. Mapping config yaml now has optional lock_orientation_to_config for object (ie slalom). Added reset mapping service so we don't have to restart mapping to clear previous dets/locations before starting a new run. Tested on multiple objects in sim, but not with autonomy
Table now published from vision as warning+helmet, toggleable via setBool service. Point objects at parent now a config option in mapping (for table). Dummy dets now has parenting for easier testing pos stuff
…perception into BinaryClassifier
Binary classifier
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds binary instance classification and bin geometry fitting to ChangesRiptide Mapping Binary Classifier and Bin Geometry
Tensor Detector YOLO Pipeline Rewrite
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Camera
participant MappingNode
participant BinaryClassifier
participant Location
Camera->>MappingNode: vision_callback(detections)
MappingNode->>BinaryClassifier: observe(DetectionSample)
BinaryClassifier-->>MappingNode: selected target
MappingNode->>Location: update_object_with_pose()
MappingNode->>MappingNode: publish_pose()
sequenceDiagram
participant YoloOrientationNode
participant YoloModel
participant DetectionProcessor
participant PointCloudBuilder
YoloOrientationNode->>YoloModel: infer(image)
YoloModel-->>YoloOrientationNode: YOLO results
YoloOrientationNode->>DetectionProcessor: process(results, frame)
DetectionProcessor->>PointCloudBuilder: extract(feature points)
DetectionProcessor-->>YoloOrientationNode: detections and markers
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (4)
tensor_detector/CMakeLists.txt (1)
40-43: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGLOB without CONFIGURE_DEPENDS won't pick up new/removed files on incremental builds.
CMake's own docs warn against this: "we do not recommend using GLOB to collect a list of source files from your source tree" since "If no CMakeLists.txt file changes when a source is added or removed then the generated build system cannot know when to ask CMake to regenerate." Adding a new module under
src/won't be installed until a full reconfigure (e.g.,colcon build --cmake-clean-cache), which can cause confusing missing-module errors during development.
CONFIGURE_DEPENDSfixes this but requires CMake ≥ 3.12, while this file declarescmake_minimum_required(VERSION 3.8).♻️ Suggested fix (requires bumping minimum CMake version)
-cmake_minimum_required(VERSION 3.8) +cmake_minimum_required(VERSION 3.12) ... -file(GLOB src_py_files RELATIVE ${PROJECT_SOURCE_DIR} src/*.py) +file(GLOB src_py_files CONFIGURE_DEPENDS RELATIVE ${PROJECT_SOURCE_DIR} src/*.py)🤖 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 `@tensor_detector/CMakeLists.txt` around lines 40 - 43, The install-time file glob in the CMakeLists for src_py_files won’t update on incremental builds, so new or removed Python modules under src/ can be missed until a manual reconfigure. Update the globbing approach used by file(GLOB src_py_files ...) so the build system regenerates when files change, and if you use CONFIGURE_DEPENDS then also bump the cmake_minimum_required version accordingly. Keep the change localized to the src_py_files collection and install(PROGRAMS) flow.riptide_mapping/riptide_mapping2/mapping.py (1)
567-567: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer
not infor membership tests.Ruff flags
not child in ...(E713) at both Line 567 and Line 646;child not in ...is clearer and idiomatic.Also applies to: 646-646
🤖 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 `@riptide_mapping/riptide_mapping2/mapping.py` at line 567, The membership checks in the mapping logic use the non-idiomatic form `not child in ...`, which Ruff flags as E713. Update the conditional in the relevant object-handling code paths, including the checks near the `self.objects` access in the mapping methods, to use `child not in ...` instead so the intent is clearer and lint-compliant.Source: Linters/SAST tools
riptide_mapping/riptide_mapping2/dummydetections.py (2)
116-117: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTwo parameters for the same flag.
Both
publish_invalid_orientationandpub_invalid_orientationare declared (and OR'd together at Lines 237-238). The config only setspub_invalid_orientation. Carrying two names invites future drift where someone sets one and expects the other; consider consolidating on the single name the YAML actually uses.🤖 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 `@riptide_mapping/riptide_mapping2/dummydetections.py` around lines 116 - 117, The detection flag is duplicated in DummyDetections parameter setup, with both publish_invalid_orientation and pub_invalid_orientation being declared and combined later in the class. Update the DummyDetections parameter handling to use a single canonical parameter name throughout the detection_data logic, and align it with the YAML-configured pub_invalid_orientation so the declaration, lookup, and any OR logic all reference the same symbol.
90-90: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAvoid the private
self._parametersattribute.
self.get_parameters(self._parameters.keys())reaches into rclpy's private_parametersdict, which is not part of the public Node API and can break across rclpy versions. Prefer iterating the names you declared (orlist_parameters) to gather them.🤖 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 `@riptide_mapping/riptide_mapping2/dummydetections.py` at line 90, The parameter refresh in the Dummydetections node is reaching into the private Node state via self._parameters, so update the logic around self.updateParams and get_parameters to use only the public API. Replace the use of self._parameters.keys() with the set of parameter names you declared or with list_parameters, then pass those names into get_parameters so the code no longer depends on rclpy internals.
🤖 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 `@riptide_mapping/config/config.yaml`:
- Around line 160-183: The blood_hole_large and blood_hole_small entries are
reusing the same poses as the fire_hole targets, causing both openings to map to
identical locations. Update the blood_hole_* definitions in the config so they
have distinct pose values under torpedo_frame, using the existing fire_hole_*
entries as a reference but assigning separate coordinates for the blood targets.
In `@riptide_mapping/config/dummy_detections.yaml`:
- Around line 23-30: The dummy detections config includes bin_target in the
objects list without a matching detection_data.bin_target block, so
dummydetections.py falls back to default params and publishes a fake map-origin
detection. Fix this in dummy_detections.yaml by either removing bin_target from
the objects array or adding a bin_target entry under detection_data with the
correct settings (likely publish: false if it is only an anchor). Also verify
the object name aligns with the target names used by config.yaml
(bin_target1/bin_target2) so the mapping logic stays consistent.
- Around line 142-145: The torpedo_fire_hole_small entry in
dummy_detections.yaml appears to reuse the same pose as torpedo_fire_hole_large,
likely from a copy-paste mistake. Update the pose for torpedo_fire_hole_small in
the torpedo object block so it is mirrored consistently with the other hole
placements (using the existing torpedo_fire_hole_large and blood-hole poses as
the reference), while keeping the parent and class_id unchanged.
In `@riptide_mapping/riptide_mapping2/dummydetections.py`:
- Around line 260-265: The invalid-orientation sentinel is being applied to
mapPose too early, before the frame conversion to framePose, so the marker is
lost before publishing. Update the detection flow in the block using
publishInvalid and the pose transform logic so the (2,2,2,2) orientation is set
on the final published pose object after the frame transform/rotation, not on
the intermediate pose.
In `@riptide_mapping/riptide_mapping2/mapping.py`:
- Line 178: The subscription topic in create_subscription currently calls
.format(self.get_namespace()) on a string with no placeholder, so the namespace
argument is ignored. Update the detected_objects subscription in mapping.py to
remove the dead .format(...) call and keep the topic string as-is, using the
create_subscription call in the mapping class to locate it.
- Around line 316-347: The start_binary_classifier_callback flow currently
allows any two existing objects, but seeding assumes both targets share the same
parent frame. Update the validation in start_binary_classifier_callback to
reject target1/target2 pairs whose parent frames differ, using the existing
objects lookup and parent-frame metadata before calling binary_classifier.start.
If mixed parents must be supported, transform the centroid into the second
object’s frame before resetting locations and seeding.
In `@tensor_detector/src/detection.py`:
- Around line 284-296: The slalom-specific branch in detection should reject
zero-depth pixels before computing the centroid, since the current
`SLALOM_CLASS` path in `Detection._...` accepts `depth_value == 0` and can
produce a bogus point near the camera origin. Update the guard alongside the
existing `np.isnan`/`math.isinf` checks to also return `None` when `depth_value`
is zero, mirroring the behavior used in the generic point-cloud path that drops
`z == 0`.
- Around line 158-183: The detector’s shared state is still mutable from other
callback groups while process() is running, so a race can occur between
per-frame inference and detector reconfiguration. Add a shared lock in the
detector class and acquire it in process() as well as in the reconfiguration
paths used by the service callbacks and delayed camera-switch timer, so updates
to _frame, _mask, and other mutable detector state are serialized with image
processing.
In `@tensor_detector/src/geometry.py`:
- Around line 113-121: The radius_outlier_mask path uses
cKDTree.query_ball_point(return_length=...), which is only available in newer
SciPy versions. Update the dependency for this code path to require SciPy 1.3.0
or higher, or add a version guard/fallback around radius_outlier_mask so older
environments avoid calling return_length. Keep the fix localized to
radius_outlier_mask and any package dependency metadata that governs this
geometry module.
In `@tensor_detector/src/yolo_orientation.py`:
- Around line 215-243: Serialize the camera state swap in setup_camera() with
image_callback() to prevent mixed old/new model and config usage during
reconfiguration. Protect the shared camera fields in
yolo_orientation.py—especially self.model, self.conf, self.iou, self.frame_id,
and self.class_id_map—using a shared lock, or run delayed_setup()/setup_camera()
in the same mutually exclusive callback group as image processing so
image_callback() cannot overlap the swap.
---
Nitpick comments:
In `@riptide_mapping/riptide_mapping2/dummydetections.py`:
- Around line 116-117: The detection flag is duplicated in DummyDetections
parameter setup, with both publish_invalid_orientation and
pub_invalid_orientation being declared and combined later in the class. Update
the DummyDetections parameter handling to use a single canonical parameter name
throughout the detection_data logic, and align it with the YAML-configured
pub_invalid_orientation so the declaration, lookup, and any OR logic all
reference the same symbol.
- Line 90: The parameter refresh in the Dummydetections node is reaching into
the private Node state via self._parameters, so update the logic around
self.updateParams and get_parameters to use only the public API. Replace the use
of self._parameters.keys() with the set of parameter names you declared or with
list_parameters, then pass those names into get_parameters so the code no longer
depends on rclpy internals.
In `@riptide_mapping/riptide_mapping2/mapping.py`:
- Line 567: The membership checks in the mapping logic use the non-idiomatic
form `not child in ...`, which Ruff flags as E713. Update the conditional in the
relevant object-handling code paths, including the checks near the
`self.objects` access in the mapping methods, to use `child not in ...` instead
so the intent is clearer and lint-compliant.
In `@tensor_detector/CMakeLists.txt`:
- Around line 40-43: The install-time file glob in the CMakeLists for
src_py_files won’t update on incremental builds, so new or removed Python
modules under src/ can be missed until a manual reconfigure. Update the globbing
approach used by file(GLOB src_py_files ...) so the build system regenerates
when files change, and if you use CONFIGURE_DEPENDS then also bump the
cmake_minimum_required version accordingly. Keep the change localized to the
src_py_files collection and install(PROGRAMS) flow.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b71a2713-d769-4c5a-95c1-b2275e2ebd0a
📒 Files selected for processing (31)
.gitignore.gitmoduleslog/latestlog/latest_deployriptide_mapping/config/binary_classifier.yamlriptide_mapping/config/config.yamlriptide_mapping/config/dummy_detections.yamlriptide_mapping/launch/mapping.launch.pyriptide_mapping/riptide_mapping2/bin_geometry.pyriptide_mapping/riptide_mapping2/binary_classifier.pyriptide_mapping/riptide_mapping2/dummydetections.pyriptide_mapping/riptide_mapping2/location.pyriptide_mapping/riptide_mapping2/mapping.pytensor_detector/CMakeLists.txttensor_detector/config/yolo_orientation.yamltensor_detector/launch/tensorrt.launch.pytensor_detector/src/colors.pytensor_detector/src/detection.pytensor_detector/src/geometry.pytensor_detector/src/outputs.pytensor_detector/src/pointcloud.pytensor_detector/src/yolo_model.pytensor_detector/src/yolo_orientation.pytensor_detector/weights/dfc_rs_26.enginetensor_detector/weights/dfc_rs_26.pttensor_detector/weights/ffc_rs_26.pttensor_detector/weights/rs25_3_4_26.pttensor_detector/weights/rs25_3_7_26.pttensor_detector/weights/rs26_ffc.enginetensor_detector/weights/rs26_ffc.pttensor_detector/weights/rs26_gen.pt
💤 Files with no reviewable changes (3)
- log/latest_deploy
- .gitmodules
- log/latest
This was referenced Jul 3, 2026
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@tensor_detector/src/detection.py`:
- Around line 571-574: Move the “using table pair quat” log statement inside the
table_quat is not None branch so it only runs when quat is actually replaced by
table_quat. Keep the plane-fit fallback path and its existing behavior
unchanged.
- Around line 605-611: Update `_table_pair_quat` to use the Z-up world normal
`[0.0, 0.0, 1.0]` wherever the table orientation currently treats `[0.0, 1.0,
0.0]` as up, including the cross product and
`geometry.quat_from_normal_and_inplane_dir` call. Preserve the existing
horizontal-direction and quaternion composition logic.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f9b05385-e7bb-41fa-94ac-2279bef22b2b
📒 Files selected for processing (6)
riptide_mapping/config/config.yamlriptide_mapping/config/dummy_detections.yamlriptide_mapping/riptide_mapping2/mapping.pytensor_detector/config/yolo_orientation.yamltensor_detector/src/detection.pytensor_detector/src/geometry.py
🚧 Files skipped from review as they are similar to previous changes (4)
- tensor_detector/src/geometry.py
- riptide_mapping/config/dummy_detections.yaml
- riptide_mapping/riptide_mapping2/mapping.py
- riptide_mapping/config/config.yaml
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
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
use_sim_timelaunch support, improved launch/config parameterization, and cleaned up repo ignore/deploy markers.