Pre-comp dev merge#27
Open
osu-uwrt-bot2 wants to merge 68 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
Changed image subscription topic to compressed. Published image topic name change, also compressed. Performance improvement with SVD. Not using outlier removal for now, commented out (performance hit). Marker QOL improvements.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
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
📝 WalkthroughWalkthroughThis PR adds a binary instance classifier and bin geometry fitting subsystem to riptide_mapping, integrates them into the mapping node with new config/launch support, and rewrites the tensor_detector YOLO pipeline into modular geometry, output, point-cloud, detection-processing, and model-wrapper components with a refactored node. ChangesRiptide Mapping Binary Classifier and Bin Geometry
Estimated code review effort: 5 (Critical) | ~120 minutes 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)
alt classifier running
MappingNode->>MappingNode: handle_binary_classifier_detections()
MappingNode->>BinaryClassifier: observe(DetectionSample)
BinaryClassifier-->>MappingNode: target object
MappingNode->>Location: update_object_with_pose()
else normal mapping
MappingNode->>Location: try_update_pose()
end
MappingNode->>MappingNode: publish_pose()
sequenceDiagram
participant YoloOrientationNode
participant YoloModel
participant DetectionProcessor
participant PointCloudBuilder
YoloOrientationNode->>YoloModel: infer(image)
YoloModel-->>YoloOrientationNode: results
YoloOrientationNode->>DetectionProcessor: process(results, frame)
DetectionProcessor->>PointCloudBuilder: extract(feature_points)
DetectionProcessor-->>YoloOrientationNode: (Detection3DArray, markers)
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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
Comment on lines
+260
to
+265
| if publishInvalid: | ||
| #invalid quaternion indicating that mapping should not merge orientation | ||
| mapPose.orientation.w = 2.0 | ||
| mapPose.orientation.x = 2.0 | ||
| mapPose.orientation.y = 2.0 | ||
| mapPose.orientation.z = 2.0 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# How does mapping detect the "invalid orientation" sentinel? Confirm it checks components (e.g. == 2.0 / > 1.0).
rg -nP -C3 '(orientation\.(w|x|y|z)|invalid.*orient|>\s*1\.0|==\s*2\.0)' riptide_mapping/riptide_mapping2/Repository: osu-uwrt/riptide_perception
Length of output: 12524
Apply the invalid-orientation sentinel after the frame transform. mapPose is converted to framePose and rotated again before the detection is populated, so the (2,2,2,2) marker never reaches mapping. Set the sentinel on the final pose you publish instead.
🤖 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 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.
Comment on lines
+316
to
+347
| # both instance targets are caller-supplied; they must be real mapping objects we can publish/seed | ||
| if target1 not in self.objects.keys() or target2 not in self.objects.keys(): | ||
| response.success = False | ||
| response.message = f"instance targets must exist in mapping objects (got '{target1}', '{target2}')" | ||
| return response | ||
|
|
||
| success, message = self.binary_classifier.start( | ||
| class_name, | ||
| target1, | ||
| target2, | ||
| ) | ||
|
|
||
| if not success: | ||
| response.success = False | ||
| response.message = message | ||
| return response | ||
|
|
||
| self.objects[target1]["location"].reset() | ||
| self.objects[target2]["location"].reset() | ||
|
|
||
| self.instance1_seeded = False | ||
| self.instance2_seeded = False | ||
|
|
||
| self.target_object = target1 | ||
| self.lock_map = False | ||
| self.offset.cool_buffer() | ||
| self.outstanding_detections.clear() | ||
|
|
||
| response.success = True | ||
| response.message = message | ||
| self.get_logger().info(message) | ||
| return response |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the parent frame configured for likely binary-classifier targets (fire/blood/magnet holes/targets)
fd -e yaml . riptide_mapping/config | xargs rg -nP -C1 '(fire_hole|blood_hole|magnet)\w*'
# Then inspect their .parent entries
fd -e yaml . riptide_mapping/config | xargs rg -nP '\.parent\s*:'Repository: osu-uwrt/riptide_perception
Length of output: 4654
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant functions in mapping.py first
ast-grep outline riptide_mapping/riptide_mapping2/mapping.py --view expanded
# Read the binary-classifier and seeding-related sections with line numbers
sed -n '380,620p' riptide_mapping/riptide_mapping2/mapping.py | cat -n
# Inspect the config entries for the classifier targets
sed -n '120,280p' riptide_mapping/config/config.yaml | cat -nRepository: osu-uwrt/riptide_perception
Length of output: 19794
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
path = Path("riptide_mapping/riptide_mapping2/mapping.py")
text = path.read_text()
for name in ["seed_object_estimate", "publish_pose", "try_update_binary_classifier_pose", "start_binary_classifier_callback", "binary_classifier"]:
m = re.search(rf"def {name}\b.*?(?=^\s*def |\Z)", text, re.S | re.M)
if m:
print(f"\n### {name}\n")
print(m.group(0))
PYRepository: osu-uwrt/riptide_perception
Length of output: 10660
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show only the relevant functions and nearby logic with line numbers
for name in seed_object_estimate publish_pose try_update_binary_classifier_pose start_binary_classifier_callback maybe_seed_binary_instances; do
echo "===== $name ====="
rg -n -A80 -B20 "def $name|class .*" riptide_mapping/riptide_mapping2/mapping.py | sed -n '1,160p'
doneRepository: osu-uwrt/riptide_perception
Length of output: 32634
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline riptide_mapping/riptide_mapping2/binary_classifier.py --view expanded
sed -n '1,260p' riptide_mapping/riptide_mapping2/binary_classifier.py | cat -nRepository: osu-uwrt/riptide_perception
Length of output: 15917
Require both binary-classifier targets to share a parent frame
start_binary_classifier_callback accepts any existing objects, but seeding reuses a centroid computed in the first instance’s frame. If object1_name and object2_name have different parents, the second target gets seeded in the wrong frame. Reject mixed-parent pairs here, or transform the centroid before seeding.
🤖 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` around lines 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.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
repo="osu-uwrt/riptide_perception"
title="Reject mixed-parent binary-classifier target pairs or transform seed centroids"
body=$(cat <<'EOF'
## Summary
`riptide_mapping/riptide_mapping2/mapping.py` currently allows `MappingNode.start_binary_classifier_callback()` to start the binary classifier with any two existing mapping objects as `object1_name` and `object2_name`.
However, the later seeding flow reuses a centroid computed in the first instance target's parent frame. When the two targets have different configured parent frames, the second target can be seeded in the wrong frame.
## Why this matters
This can produce incorrect seeded poses/TF for the second binary-classifier target, which may cause downstream autonomy to navigate to or reason about an invalid location.
## Affected area
- `riptide_mapping/riptide_mapping2/mapping.py`
- `MappingNode.start_binary_classifier_callback`
- binary-classifier seeding flow (`maybe_seed_binary_instances`, `seed_object_estimate`, `try_update_binary_classifier_pose`)
## Required change
Before calling `self.binary_classifier.start(...)`, validate that `object1_name` and `object2_name` resolve to the same configured `init_data.<object>.parent` frame.
If mixed-parent targets must be supported, then the implementation should transform the centroid into the second target's parent frame before seeding/resetting the second object.
## Acceptance criteria
- Starting the binary classifier with two targets that have different parent frames is either:
- rejected with a clear error message, or
- handled correctly by transforming the centroid before seeding.
- Starting with two targets that share the same parent frame continues to work as expected.
- The behavior is documented in code comments and/or service response messages.
## Backlinks
- Follow-up from PR `#27`: https://github.com/osu-uwrt/riptide_perception/pull/27
- Review comment: https://github.com/osu-uwrt/riptide_perception/pull/27#discussion_r3516883620
Requested by: `@zehdari`
EOF
)
gh issue create \
--repo "$repo" \
--title "$title" \
--body "$body" \
--assignee "zehdari"Length of output: 2345
@zehdari, done — I created follow-up issue #29:
It covers the mixed-parent binary-classifier target validation concern and links back to PR #27 and the review comment.
Comment on lines
+158
to
+183
| def process(self, results, frame): | ||
| """Run one image through the pipeline. | ||
|
|
||
| Returns (detections, markers_to_publish). The node handles marker | ||
| throttling, the point cloud, the annotated image, and publishing. | ||
| """ | ||
| self._frame = frame | ||
| self._reset_frame_state() | ||
| self.markers.reset() | ||
|
|
||
| detections = Detection3DArray() | ||
| detections.header.frame_id = frame.frame_id | ||
| detections.header.stamp = self._stamp(frame) | ||
|
|
||
| if self._mask is None or self._mask.shape[:2] != frame.image.shape[:2]: | ||
| self._mask = np.zeros(frame.image.shape[:2], dtype=np.uint8) | ||
|
|
||
| # Build the full-frame segmentation mask | ||
| self._mask.fill(0) | ||
|
|
||
| for result in results: | ||
| if result is not None and result.masks is not None: | ||
| for contour in result.masks.xy: | ||
| contour = np.array(contour, dtype=np.int32) | ||
| cv2.fillPoly(self._mask, [contour], 255) | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect callback group / subscription wiring in the node
fd yolo_orientation.py --exec rg -nC3 'callback_group|MutuallyExclusive|Reentrant|create_subscription|image_callback|MultiThreadedExecutor'Repository: osu-uwrt/riptide_perception
Length of output: 2964
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="tensor_detector/src/yolo_orientation.py"
echo "=== callback definitions ==="
rg -n 'def .*callback|def .*_callback|create_service|create_timer|create_subscription|create_publisher|callback_group|MultiThreadedExecutor|MutuallyExclusiveCallbackGroup|ReentrantCallbackGroup' "$file"
echo
echo "=== processor state references ==="
rg -n 'self\.(image_cb_group|depth_cb_group|processor|_frame|_mask|pair_boxes|torpedo|slalom)' "$file"
echo
echo "=== process call sites ==="
rg -n '\.process\(' "$file"Repository: osu-uwrt/riptide_perception
Length of output: 2885
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="tensor_detector/src/yolo_orientation.py"
echo "=== relevant callback bodies ==="
sed -n '160,250p' "$file"
echo
echo "=== image/depth callback body excerpt ==="
sed -n '292,390p' "$file"Repository: osu-uwrt/riptide_perception
Length of output: 8654
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="tensor_detector/src/detection.py"
echo "=== class and methods map ==="
ast-grep outline "$file" --view expanded
echo
echo "=== stateful methods and process body ==="
rg -n 'def (process|set_|reset_|_resolve|_emit|create_detection3d_message|process_torpedo_task)' "$file"Repository: osu-uwrt/riptide_perception
Length of output: 4066
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="tensor_detector/src/detection.py"
sed -n '130,190p' "$file"
echo
sed -n '235,310p' "$file"
echo
sed -n '385,610p' "$file"Repository: osu-uwrt/riptide_perception
Length of output: 16355
Guard detector state across callback groups. image_callback is serialized, but the service callbacks and delayed camera-switch timer can still mutate the shared detector from another callback group while process() runs. Add a lock around detector reconfiguration and per-frame processing.
🤖 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/src/detection.py` around lines 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.
Comment on lines
+284
to
+296
| if class_name == SLALOM_CLASS: | ||
| depth_value = frame.depth[int(bbox_center_y), int(bbox_center_x)] | ||
| if (np.isnan(depth_value) or math.isinf(bbox_center_x) | ||
| or math.isinf(bbox_center_y) or math.isinf(depth_value)): | ||
| return None | ||
| centroid = geometry.pixel_to_3d(bbox_center_x, bbox_center_y, | ||
| float(depth_value), | ||
| frame.fx, frame.fy, frame.cx, frame.cy) | ||
| quat, _ = geometry.normal_to_quaternion(-self._default_normal, | ||
| self._default_normal) | ||
| detection = self._new_detection(frame) | ||
| detection.results.append(self._make_hypothesis(class_name, centroid, quat, conf)) | ||
| return detection |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Guard against zero depth in the slalom path.
The check accepts depth_value == 0, which for no-return depth pixels yields a centroid at (≈0,0,0) near the camera origin. The generic point-cloud path already drops z == 0; mirror that here.
🐛 Proposed guard
depth_value = frame.depth[int(bbox_center_y), int(bbox_center_x)]
- if (np.isnan(depth_value) or math.isinf(bbox_center_x)
+ if (np.isnan(depth_value) or depth_value == 0 or math.isinf(bbox_center_x)
or math.isinf(bbox_center_y) or math.isinf(depth_value)):
return None📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if class_name == SLALOM_CLASS: | |
| depth_value = frame.depth[int(bbox_center_y), int(bbox_center_x)] | |
| if (np.isnan(depth_value) or math.isinf(bbox_center_x) | |
| or math.isinf(bbox_center_y) or math.isinf(depth_value)): | |
| return None | |
| centroid = geometry.pixel_to_3d(bbox_center_x, bbox_center_y, | |
| float(depth_value), | |
| frame.fx, frame.fy, frame.cx, frame.cy) | |
| quat, _ = geometry.normal_to_quaternion(-self._default_normal, | |
| self._default_normal) | |
| detection = self._new_detection(frame) | |
| detection.results.append(self._make_hypothesis(class_name, centroid, quat, conf)) | |
| return detection | |
| if class_name == SLALOM_CLASS: | |
| depth_value = frame.depth[int(bbox_center_y), int(bbox_center_x)] | |
| if (np.isnan(depth_value) or depth_value == 0 or math.isinf(bbox_center_x) | |
| or math.isinf(bbox_center_y) or math.isinf(depth_value)): | |
| return None | |
| centroid = geometry.pixel_to_3d(bbox_center_x, bbox_center_y, | |
| float(depth_value), | |
| frame.fx, frame.fy, frame.cx, frame.cy) | |
| quat, _ = geometry.normal_to_quaternion(-self._default_normal, | |
| self._default_normal) | |
| detection = self._new_detection(frame) | |
| detection.results.append(self._make_hypothesis(class_name, centroid, quat, conf)) | |
| return detection |
🤖 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/src/detection.py` around lines 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`.
Comment on lines
+113
to
+121
| def radius_outlier_mask(points_3d, radius=1.0, min_neighbors=10): | ||
| """Boolean mask: True for points with more than min_neighbors others within radius.""" | ||
| n = len(points_3d) | ||
| if n == 0: | ||
| return np.zeros(0, dtype=bool) | ||
| tree = cKDTree(points_3d) | ||
| # count includes the point itself, matching the original > min_neighbors semantics | ||
| counts = tree.query_ball_point(points_3d, r=radius, return_length=True) | ||
| return counts > min_neighbors |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
scipy cKDTree query_ball_point return_length parameter added version
💡 Result:
The return_length parameter for the scipy.spatial.cKDTree.query_ball_point method was added in SciPy version 1.3.0 [1][2][3]. This parameter allows the method to return the count of points within the specified radius rather than a list of their indices [4][5].
Citations:
- 1: https://scipy.github.io/devdocs/reference/generated/scipy.spatial.cKDTree.query_ball_point.html
- 2: https://docs.scipy.org/doc/scipy-1.8.0/reference/generated/scipy.spatial.cKDTree.query_ball_point.html
- 3: https://docs.scipy.org/doc/scipy-1.10.0/reference/generated/scipy.spatial.cKDTree.query_ball_point.html
- 4: https://docs.scipy.org/doc/scipy/reference/generated/scipy.spatial.cKDTree.query_ball_point.html
- 5: https://docs.scipy.org/doc/scipy-1.9.3/reference/generated/scipy.spatial.cKDTree.query_ball_point.html
🏁 Script executed:
rg -n --hidden --glob '!**/.git/**' -S 'scipy|SciPy' .Repository: osu-uwrt/riptide_perception
Length of output: 556
🏁 Script executed:
git ls-files | rg '(^|/)(requirements|pyproject|setup|Pipfile|poetry|conda|environment|lock|deps).*|(^|/)tensor_detector/|(^|/)src/'Repository: osu-uwrt/riptide_perception
Length of output: 1903
🏁 Script executed:
sed -n '1,80p' tensor_detector/package.xmlRepository: osu-uwrt/riptide_perception
Length of output: 1400
Require SciPy >= 1.3.0 for this path
cKDTree.query_ball_point(return_length=...) needs SciPy 1.3.0+, so add a minimum version in the package dependency or guard this call for older deployments.
🤖 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/src/geometry.py` around lines 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.
Comment on lines
+215
to
+243
| def setup_camera(self): | ||
| self.get_logger().info(f"Active camera: {self.active_camera}") | ||
| self.camera_prefix = self.active_camera | ||
| self.frame_id = f'{self.robot_ns}/{self.camera_prefix}_left_camera_optical_frame' | ||
|
|
||
| yolo_model = self.get_parameter(f'{self.active_camera}_model').get_parameter_value().string_value | ||
| class_id_map_str = self.get_parameter(f'{self.active_camera}_class_id_map').get_parameter_value().string_value | ||
| self.conf = self.get_parameter(f'{self.active_camera}_threshold').get_parameter_value().double_value | ||
| self.iou = self.get_parameter(f'{self.active_camera}_iou').get_parameter_value().double_value | ||
|
|
||
| self.get_logger().info(f"Yolo Model: {yolo_model}") | ||
| self.get_logger().info(f"Class id map str: {class_id_map_str}") | ||
| self.get_logger().info(f"Confidence Threshold: {self.conf}") | ||
| self.get_logger().info(f"IOU: {self.iou}") | ||
|
|
||
| self.load_class_id_map(class_id_map_str) | ||
|
|
||
| # Task profile: torpedo (fire/blood) logic runs only on the chosen camera (ffc really, but generic here cause why not) | ||
| # On the other camera those classes fall through to the standard path (so bins works like normal) | ||
| self.detector.set_torpedo_enabled(self.active_camera == self.torpedo_task_camera) | ||
|
|
||
| weights_dir = os.path.join(get_package_share_directory("tensor_detector"), 'weights') | ||
| model_path = os.path.join(weights_dir, yolo_model) | ||
| self.get_logger().info(f"Loading model path: {model_path}") | ||
| self.model = YoloModel(model_path=model_path, export=self.export, logger=self.get_logger()) | ||
|
|
||
| self.reset_collection_variables() | ||
| self.destroy_subscriptions() | ||
| self.create_subscriptions() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm callback-group assignment of the reconfigure timer vs image callback.
fd -t f 'yolo_orientation.py' tensor_detector/src --exec sed -n '78,122p;207,243p;270,290p;334,377p'Repository: osu-uwrt/riptide_perception
Length of output: 7233
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="tensor_detector/src/yolo_orientation.py"
# Map the class structure and inspect the camera-switch, timer, and callback sections.
ast-grep outline "$file" --view expanded || true
printf '\n--- create_switch_service / delayed setup / subscriptions ---\n'
sed -n '120,220p' "$file"
printf '\n--- image callback and related shared state ---\n'
sed -n '220,320p' "$file"
printf '\n--- any locks / camera switch flags / timer creation ---\n'
rg -n "camera_switch_in_progress|delayed_timer|create_timer|MutuallyExclusiveCallbackGroup|ReentrantCallbackGroup|Lock|threading\.Lock|self\.model|self\.conf|self\.iou|self\.frame_id|self\.class_id_map|reset_collection_variables|destroy_subscriptions|create_subscriptions" "$file"Repository: osu-uwrt/riptide_perception
Length of output: 13074
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="tensor_detector/src/yolo_orientation.py"
echo "== create_timer / delayed_setup occurrences =="
rg -n "delayed_setup|create_timer|camera_switch_in_progress|delayed_timer" "$file"
echo
echo "== constructor and switch-service area =="
sed -n '1,140p' "$file"
echo
echo "== setup_camera / callbacks area =="
sed -n '140,320p' "$file"Repository: osu-uwrt/riptide_perception
Length of output: 16497
Serialize camera reconfiguration with image processing. delayed_setup() runs setup_camera() on the timer/default callback group, while image_callback() stays in image_cb_group, so MultiThreadedExecutor can overlap the model/config swap with an in-flight frame. camera_switch_in_progress only blocks the service path, not image processing, so one frame can see mixed old/new self.model, self.conf, self.iou, self.frame_id, or self.class_id_map. Use a shared lock around the camera state swap and reads, or move reconfigure onto the same mutually exclusive group as the image callback.
🤖 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/src/yolo_orientation.py` around lines 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.
This was referenced Jul 3, 2026
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