Pre-comp dev merge#36
Conversation
Ghost pos fix
|
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 updates RViz visualization and marker assets, adds TreeStack-based autonomy handling, mapping controls, ghost TF publication, DPI-aware overlays, array-parameter tracking, pinger controls, and new rosbag and service panels. ChangesRViz GUI and control panel changes
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant TreeStackTopic
participant ControlPanel
participant InteractiveMarkerServer
TreeStackTopic->>ControlPanel: treeStackCallback(TreeStack msg)
alt stack not empty
ControlPanel->>InteractiveMarkerServer: erase setpoint marker
else stack empty
ControlPanel->>InteractiveMarkerServer: reinsert and apply setpoint marker
end
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
riptide_rviz/control_config_talos.rviz (3)
1109-1126: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win"Table" entry in Mapping Objects points to a dummy-detection topic, not a mapping topic.
Every sibling entry in this "Mapping Objects" group (Gate, Pill, Plug, Bandage, Warning, Helmet, Sos, Hammer and Wrench, Buoy, Compass, Torpedo, Bin Target 1/2) subscribes to
/talos/mapping/*. This "Table" entry instead uses/talos/dummydetections/table— the exact same topic used by the separate "Table" entry in the "Dummy Detections" group (line 2125). This looks like a copy-paste error; the fused mapping pose for the table will never be displayed here.🔧 Suggested fix
Topic: Depth: 5 Durability Policy: Volatile Filter size: 10 History Policy: Keep Last Reliability Policy: Reliable - Value: /talos/dummydetections/table + Value: /talos/mapping/table🤖 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_rviz/control_config_talos.rviz` around lines 1109 - 1126, The “Table” item in the Mapping Objects group is subscribed to the wrong topic and should match the other mapping entries. Update the Topic value for the Table entry in control_config_talos.rviz so it uses the mapping namespace instead of the dummy-detections topic, aligning it with the other Mapping Objects entries and avoiding duplication with the separate Table item in the Dummy Detections group.
2605-2651: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDuplicate topic: "Fire Hole Small" and "Fire Hole Large" both subscribe to the same "_large" topic.
Name: Fire Hole Small(line 2605) andName: Fire Hole Large(line 2640) both useValue: /talos/dummydetections/torpedo_fire_hole_large. Compare with the correctly-differentiated sibling pair "Blood Hole Large"/"Blood Hole Small", which use distinct_large/_smalltopics. The "Fire Hole Small" display will never show actual small-hole detections, and both panels will duplicate the "large" hole data.🐛 Suggested fix
Topic: Depth: 5 Durability Policy: Volatile Filter size: 10 History Policy: Keep Last Reliability Policy: Reliable - Value: /talos/dummydetections/torpedo_fire_hole_large + Value: /talos/dummydetections/torpedo_fire_hole_small Value: true(applies to the "Fire Hole Small" entry at line 2615; the "Fire Hole Large" entry at line 2650 can keep the
_largetopic)🤖 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_rviz/control_config_talos.rviz` around lines 2605 - 2651, The “Fire Hole Small” display is subscribed to the same detection topic as “Fire Hole Large,” so the small and large views duplicate the large-hole data. Update the topic used by the Fire Hole Small entry in the rviz config to the corresponding small-hole topic, following the pattern already used by the Blood Hole Small/Large displays. Keep the Fire Hole Large entry on the _large topic and verify the two Fire Hole displays reference distinct topics.
730-741: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winRename the gate mapping displays to match the new object names
Gate Shark/Gate Sawstill use/talos/mapping/gate_sharkand/talos/mapping/gate_sawhere, whilemarkers.yamland the dummy-detections entries already useGate Repair/Gate Rescuewithgate_repair/gate_rescue. Update these two RViz entries to keep the mapping config consistent. [lines 730-741, 764-776]🤖 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_rviz/control_config_talos.rviz` around lines 730 - 741, The RViz gate mapping display entries are still using the old object names and topics, so update the two display blocks in control_config_talos.rviz for the Gate Shark/Gate Saw mappings to match the new naming used elsewhere. In the relevant RViz entries, rename the display labels to Gate Repair and Gate Rescue and change their Topic Value fields from the old gate_shark/gate_saw mappings to the corresponding gate_repair/gate_rescue topics so the configuration stays consistent with markers.yaml and the dummy-detections data.
🧹 Nitpick comments (3)
riptide_rviz/riptide_rviz/src/OverlayDisplay.cpp (2)
115-116: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTypo in comment.
"JLog" should be "Log".
🤖 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_rviz/riptide_rviz/src/OverlayDisplay.cpp` around lines 115 - 116, The comment on logUiScale has a typo: “JLog” should be “Log”. Update the nearby comment in OverlayDisplay.cpp so it correctly says “Log info about the ui scaling,” keeping the function name logUiScale unchanged.
266-267: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUnused
QFontMetrics fmleft after removingtext_rectcomputation.
fmis declared but no longer used since the bounding-rect computation was removed. This will trigger an unused-variable warning (and potential-Werrorbuild failure depending on compiler flags).🧹 Proposed fix
- QStaticText only_wrapped_text(color_wrapped_text.c_str()); - QFontMetrics fm(painter.fontMetrics()); + QStaticText only_wrapped_text(color_wrapped_text.c_str()); painter.drawStaticText(config.x_ + 1, config.y_ + 1, static_shadow);🤖 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_rviz/riptide_rviz/src/OverlayDisplay.cpp` around lines 266 - 267, The OverlayDisplay::drawText path has an unused QFontMetrics fm declaration left behind after removing the text_rect calculation, which can trigger warnings or break builds with -Werror. Remove the redundant fm variable from the block that creates only_wrapped_text, and keep the wrapping/rendering logic in OverlayDisplay.cpp using only the symbols that are still needed.riptide_rviz/riptide_rviz/src/MissionPanel.cpp (1)
29-31: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
Qt::NoFocusremoves keyboard access to the tree selector.Setting
btSelect(aQComboBox) toQt::NoFocusprevents it from receiving keyboard focus at all — it can no longer be tabbed to or operated via keyboard, only by mouse click. If the intent is just to suppress the focus-rectangle highlight rather than block keyboard interaction entirely, considerQt::ClickFocuscombined with a stylesheet override (e.g.,QComboBox:focus { outline: none; }) instead ofQt::NoFocus.♻️ Suggested alternative preserving keyboard access
- // dont want random highlighting of the tree list - uiPanel->btSelect->setFocusPolicy(Qt::NoFocus); + // dont want random highlighting of the tree list, but still allow keyboard use + uiPanel->btSelect->setFocusPolicy(Qt::ClickFocus); + uiPanel->btSelect->setStyleSheet("QComboBox:focus { outline: none; }");🤖 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_rviz/riptide_rviz/src/MissionPanel.cpp` around lines 29 - 31, The focus setting on btSelect in MissionPanel is blocking keyboard navigation by using Qt::NoFocus. Update the MissionPanel setup so the QComboBox can still receive keyboard focus, and use a styling approach or a less restrictive focus policy to suppress the visual focus highlight instead. Keep the change localized to the btSelect configuration in MissionPanel.cpp.
🤖 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_rviz/riptide_rviz/python/MarkerPublisher.py`:
- Around line 302-324: Avoid mutating the aliased pose in place in the marker
publishing path: in the logic around ghost pose handling, `ghost.pose` is shared
with `self.latestOdom.pose.pose` / `self.latestSetpt`, so subtracting the
rotated base-link offset directly changes the stored source message and causes
repeated drift on later ticks. Update `MarkerPublisher` so it copies the pose
into a fresh `Pose`/`Point` object before applying the `v_world` offset, and
then append that independent marker pose instead of modifying the original.
In `@riptide_rviz/riptide_rviz/src/OverlayDisplay.cpp`:
- Around line 228-234: The font sizing in OverlayDisplay::paintText is not using
the same DPI scaling as the overlay dimensions, so text stays visually smaller
on high-DPI displays. Update the font setup in paintText to apply
getUiScaleFactor() consistently when calling QFont::setPixelSize, using the same
scale logic already used by updateWidth() and updateHeight(), and make sure the
other paintText occurrence uses the same adjusted sizing path.
- Around line 102-113: The UI scaling logic in getUiScaleFactor() is using
QGuiApplication::primaryScreen() and combining devicePixelRatio with
logicalDotsPerInch(), which can mis-scale overlays on multi-monitor setups.
Update the scale source to use the RViz render window’s owning screen/window
instead of the primary screen, and adjust the calculation so DPR and logical DPI
are not blindly merged unless there’s a platform-specific need. Keep the change
localized to getUiScaleFactor() in OverlayDisplay.cpp and preserve the existing
high-DPI fallback behavior.
---
Outside diff comments:
In `@riptide_rviz/control_config_talos.rviz`:
- Around line 1109-1126: The “Table” item in the Mapping Objects group is
subscribed to the wrong topic and should match the other mapping entries. Update
the Topic value for the Table entry in control_config_talos.rviz so it uses the
mapping namespace instead of the dummy-detections topic, aligning it with the
other Mapping Objects entries and avoiding duplication with the separate Table
item in the Dummy Detections group.
- Around line 2605-2651: The “Fire Hole Small” display is subscribed to the same
detection topic as “Fire Hole Large,” so the small and large views duplicate the
large-hole data. Update the topic used by the Fire Hole Small entry in the rviz
config to the corresponding small-hole topic, following the pattern already used
by the Blood Hole Small/Large displays. Keep the Fire Hole Large entry on the
_large topic and verify the two Fire Hole displays reference distinct topics.
- Around line 730-741: The RViz gate mapping display entries are still using the
old object names and topics, so update the two display blocks in
control_config_talos.rviz for the Gate Shark/Gate Saw mappings to match the new
naming used elsewhere. In the relevant RViz entries, rename the display labels
to Gate Repair and Gate Rescue and change their Topic Value fields from the old
gate_shark/gate_saw mappings to the corresponding gate_repair/gate_rescue topics
so the configuration stays consistent with markers.yaml and the dummy-detections
data.
---
Nitpick comments:
In `@riptide_rviz/riptide_rviz/src/MissionPanel.cpp`:
- Around line 29-31: The focus setting on btSelect in MissionPanel is blocking
keyboard navigation by using Qt::NoFocus. Update the MissionPanel setup so the
QComboBox can still receive keyboard focus, and use a styling approach or a less
restrictive focus policy to suppress the visual focus highlight instead. Keep
the change localized to the btSelect configuration in MissionPanel.cpp.
In `@riptide_rviz/riptide_rviz/src/OverlayDisplay.cpp`:
- Around line 115-116: The comment on logUiScale has a typo: “JLog” should be
“Log”. Update the nearby comment in OverlayDisplay.cpp so it correctly says “Log
info about the ui scaling,” keeping the function name logUiScale unchanged.
- Around line 266-267: The OverlayDisplay::drawText path has an unused
QFontMetrics fm declaration left behind after removing the text_rect
calculation, which can trigger warnings or break builds with -Werror. Remove the
redundant fm variable from the block that creates only_wrapped_text, and keep
the wrapping/rendering logic in OverlayDisplay.cpp using only the symbols that
are still needed.
🪄 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: 9f9c43fc-f061-48a7-aa0d-7f9fea5fc22b
⛔ Files ignored due to path filters (42)
riptide_meshes/meshes/bin/Task03_DropBRUVS.pngis excluded by!**/*.pngriptide_meshes/meshes/bin/model.daeis excluded by!**/*.daeriptide_meshes/meshes/bin_magnet/model.daeis excluded by!**/*.daeriptide_meshes/meshes/bin_vinyl/Task3_Blood_Fixed.pngis excluded by!**/*.pngriptide_meshes/meshes/bin_vinyl/Task3_Fire_Fixed.pngis excluded by!**/*.pngriptide_meshes/meshes/bin_vinyl/model.daeis excluded by!**/*.daeriptide_meshes/meshes/gate_repair/Task1_SurveyRepair_Fixed.pngis excluded by!**/*.pngriptide_meshes/meshes/gate_repair/model.daeis excluded by!**/*.daeriptide_meshes/meshes/gate_rescue/Task1_SearchRescue_Fixed.pngis excluded by!**/*.pngriptide_meshes/meshes/gate_rescue/model.daeis excluded by!**/*.daeriptide_meshes/meshes/octagon_buoy/Task5_Search_Fixed.pngis excluded by!**/*.pngriptide_meshes/meshes/octagon_buoy/model.daeis excluded by!**/*.daeriptide_meshes/meshes/octagon_compass/Task5_Survey_Fixed.pngis excluded by!**/*.pngriptide_meshes/meshes/octagon_compass/model.daeis excluded by!**/*.daeriptide_meshes/meshes/octagon_hammer_and_wrench/Task5_Repair_Fixed.pngis excluded by!**/*.pngriptide_meshes/meshes/octagon_hammer_and_wrench/model.daeis excluded by!**/*.daeriptide_meshes/meshes/octagon_sos/Task5_Rescue_Fixed.pngis excluded by!**/*.pngriptide_meshes/meshes/octagon_sos/model.daeis excluded by!**/*.daeriptide_meshes/meshes/reefshark/model.daeis excluded by!**/*.daeriptide_meshes/meshes/sawfish/model.daeis excluded by!**/*.daeriptide_meshes/meshes/table/model.daeis excluded by!**/*.daeriptide_meshes/meshes/table_bandage/Task5_BandAid_Fixed.pngis excluded by!**/*.pngriptide_meshes/meshes/table_bandage/model.daeis excluded by!**/*.daeriptide_meshes/meshes/table_basket_helmet/Task5_RedCross_Fixed.pngis excluded by!**/*.pngriptide_meshes/meshes/table_basket_helmet/model.daeis excluded by!**/*.daeriptide_meshes/meshes/table_basket_pink/model.daeis excluded by!**/*.daeriptide_meshes/meshes/table_basket_warning/Task5_Warning_Fixed.pngis excluded by!**/*.pngriptide_meshes/meshes/table_basket_warning/model.daeis excluded by!**/*.daeriptide_meshes/meshes/table_basket_yellow/model.daeis excluded by!**/*.daeriptide_meshes/meshes/table_bottle_pink/model.daeis excluded by!**/*.daeriptide_meshes/meshes/table_bottle_yellow/model.daeis excluded by!**/*.daeriptide_meshes/meshes/table_nut_and_bolt/Task5_NutBolt_Fixed.pngis excluded by!**/*.pngriptide_meshes/meshes/table_nut_and_bolt/model.daeis excluded by!**/*.daeriptide_meshes/meshes/table_pill/Task5_Pill_Fixed.pngis excluded by!**/*.pngriptide_meshes/meshes/table_pill/model.daeis excluded by!**/*.daeriptide_meshes/meshes/table_plug/Task5_Electric_Fixed.pngis excluded by!**/*.pngriptide_meshes/meshes/table_plug/model.daeis excluded by!**/*.daeriptide_meshes/meshes/table_spoon_pink/model.daeis excluded by!**/*.daeriptide_meshes/meshes/table_spoon_yellow/model.daeis excluded by!**/*.daeriptide_meshes/meshes/torpedo/Task04_Tagging_01.pngis excluded by!**/*.pngriptide_meshes/meshes/torpedo/Task4_ver1_Fixed.pngis excluded by!**/*.pngriptide_meshes/meshes/torpedo/model.daeis excluded by!**/*.dae
📒 Files selected for processing (11)
riptide_rviz/control_config_talos.rvizriptide_rviz/riptide_rviz/config/markers.yamlriptide_rviz/riptide_rviz/include/riptide_rviz/ControlPanel.hppriptide_rviz/riptide_rviz/include/riptide_rviz/MappingPanel.hppriptide_rviz/riptide_rviz/python/MarkerPublisher.pyriptide_rviz/riptide_rviz/src/ControlPanel.cppriptide_rviz/riptide_rviz/src/MappingPanel.cppriptide_rviz/riptide_rviz/src/MappingPanel.uiriptide_rviz/riptide_rviz/src/MissionPanel.cppriptide_rviz/riptide_rviz/src/OverlayDisplay.cppriptide_rviz/riptide_rviz/src/ParamPanel.cpp
|
|
||
| ghostBaseLinkPose = Pose() | ||
| ghostBaseLinkPose.position.x = ghost.pose.position.x | ||
| ghostBaseLinkPose.position.y = ghost.pose.position.y | ||
| ghostBaseLinkPose.position.z = ghost.pose.position.z | ||
| ghostBaseLinkPose.orientation.w = ghost.pose.orientation.w | ||
| ghostBaseLinkPose.orientation.x = ghost.pose.orientation.x | ||
| ghostBaseLinkPose.orientation.y = ghost.pose.orientation.y | ||
| ghostBaseLinkPose.orientation.z = ghost.pose.orientation.z | ||
|
|
||
| # Rotate the base_link offset vector from the robot frame | ||
| # into the world frame using the robot’s orientation. | ||
| q = [ghost.pose.orientation.w, ghost.pose.orientation.x, ghost.pose.orientation.y, ghost.pose.orientation.z] | ||
| v = [float(self.baseLinkOffset[0]), float(self.baseLinkOffset[1]), float(self.baseLinkOffset[2])] | ||
| v_as_quat = [0.0, v[0], v[1], v[2]] | ||
| q_conj = [q[0], -q[1], -q[2], -q[3]] | ||
| v_world = qmult(qmult(q, v_as_quat), q_conj) | ||
|
|
||
| # translates the ghost pose so the base_link is correctly positioned relative to origin | ||
| ghost.pose.position.x = ghost.pose.position.x - v_world[1] | ||
| ghost.pose.position.y = ghost.pose.position.y - v_world[2] | ||
| ghost.pose.position.z = ghost.pose.position.z - v_world[3] | ||
| array.markers.append(ghost) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm that assigning a message sub-pose does not deep-copy (mutation propagates back to source).
python - <<'PY'
try:
from nav_msgs.msg import Odometry
from visualization_msgs.msg import Marker
o = Odometry()
m = Marker()
m.pose = o.pose.pose # same assignment pattern as MarkerPublisher line 278
m.pose.position.x = 5.0
print("source mutated:", o.pose.pose.position.x == 5.0)
except Exception as e:
print("env unavailable:", e)
PYRepository: osu-uwrt/riptide_gui
Length of output: 202
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the target file and inspect the relevant section with line numbers.
git ls-files 'riptide_rviz/riptide_rviz/python/MarkerPublisher.py'
wc -l riptide_rviz/riptide_rviz/python/MarkerPublisher.py
sed -n '250,340p' riptide_rviz/riptide_rviz/python/MarkerPublisher.py
# Find related usages of latestOdom/latestSetpt and pose assignment patterns.
rg -n "latestOdom|latestSetpt|ghost\.pose|baseLinkOffset|ghostBaseLinkPose" riptide_rviz/riptide_rviz/python/MarkerPublisher.pyRepository: osu-uwrt/riptide_gui
Length of output: 7357
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path('riptide_rviz/riptide_rviz/python/MarkerPublisher.py')
text = p.read_text()
for needle in [
'ghost.pose = self.latestOdom.pose.pose',
'ghost.pose = self.latestSetpt',
'ghostBaseLinkPose = Pose()',
'ghost.pose.position.x = ghost.pose.position.x - v_world[1]',
]:
print(needle, '=>', needle in text)
PYRepository: osu-uwrt/riptide_gui
Length of output: 345
🌐 Web query:
ROS 2 Python generated message field assignment stores a reference not a deep copy rosidl_generator_py message semantics
💡 Result:
In ROS 2 Python, message field assignment directly via class properties does not perform a deep copy; it stores a reference to the assigned object [1][2][3]. The generated Python code for ROS 2 messages uses standard Python property setters [1][2][3]. These setters enforce type constraints (such as verifying that a provided value matches the expected ROS message type) [1][2][3], but they do not internally clone or deep-copy the object being assigned [2][3]. Consequently, if you assign a mutable object (like a list or a nested message) to a field, modifying that original object elsewhere in your code will also affect the data stored inside the ROS 2 message [2][3]. Regarding helper utilities: - The common utility function set_message_fields, located in the rosidl_runtime_py package, is frequently used to populate message fields from dictionaries [4]. Historically, this function has been noted for modifying input dictionaries or objects during the population process [5][6]. - Recent updates (e.g., Pull Request #34 in rosidl_runtime_py) have addressed safety concerns by implementing deepcopy operations within set_message_fields to prevent unintended side effects on user-provided input data [5][6]. In summary, direct assignment to a message attribute is a reference assignment, and developers should be mindful of mutability when handling nested messages or arrays [1][2]. When using utility functions like set_message_fields, verify the version and behavior of your environment regarding deep copying to ensure data integrity [5][6].
Citations:
- 1: https://github.com/ros2/design/blob/gh-pages/articles/114_generated_interfaces_python.md
- 2: https://github.com/ros2/rosidl_python/blob/master/rosidl_generator_py/resource/_msg.py.em
- 3: https://github.com/ros2/rosidl_python/blob/0f5c8f360be92566ad86f4b29f3db1febfca2242/rosidl_generator_py/resource/_msg.py.em
- 4: https://github.com/ros2/rosidl_runtime_py/blob/63a9c99ad735ef08b9cfda69ba35322b5f8b75f3/rosidl_runtime_py/set_message.py
- 5: use deepcopy in set_message_fields for safety. ros2/rosidl_runtime_py#34
- 6: use deepcopy in set_message_fields for safety. ros2/rosidl_runtime_py#34
Avoid mutating the aliased pose in place. ghost.pose points at self.latestOdom.pose.pose / self.latestSetpt, so subtracting v_world here also changes the stored message. That makes the offset accumulate on subsequent timer ticks and drifts both the marker and the published ghost/base_link transform. Assign a fresh Pose/Point before applying the offset.
🧰 Tools
🪛 Ruff (0.15.20)
[warning] 313-313: Comment contains ambiguous ’ (RIGHT SINGLE QUOTATION MARK). Did you mean ``` (GRAVE ACCENT)?
(RUF003)
🤖 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_rviz/riptide_rviz/python/MarkerPublisher.py` around lines 302 - 324,
Avoid mutating the aliased pose in place in the marker publishing path: in the
logic around ghost pose handling, `ghost.pose` is shared with
`self.latestOdom.pose.pose` / `self.latestSetpt`, so subtracting the rotated
base-link offset directly changes the stored source message and causes repeated
drift on later ticks. Update `MarkerPublisher` so it copies the pose into a
fresh `Pose`/`Point` object before applying the `v_world` offset, and then
append that independent marker pose instead of modifying the original.
Source: Linters/SAST tools
| // For high dpi displays | ||
| static float getUiScaleFactor() | ||
| { | ||
| QScreen* screen = QGuiApplication::primaryScreen(); | ||
| if (!screen) return 1.0f; | ||
|
|
||
| const float dpr = static_cast<float>(screen->devicePixelRatio()); | ||
| const float dpi = static_cast<float>(screen->logicalDotsPerInch() / 96.0); | ||
|
|
||
| const float s = std::max(dpr, dpi); | ||
| return std::max(1.0f, s); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Qt QScreen devicePixelRatio vs logicalDotsPerInch relationship for HiDPI scaling on X11/Wayland/Windows
💡 Result:
In Qt, logicalDotsPerInch and devicePixelRatio serve distinct purposes in the framework's HiDPI scaling architecture [1][2][3]. Relationship and Definitions Qt operates on a coordinate system of device-independent pixels (DIPs) [4][3]. These are mapped to physical screen pixels using a scale factor, known as the device pixel ratio (DPR) [4][3]. - devicePixelRatio (QScreen::devicePixelRatio or QWindow::devicePixelRatio): This is the effective scale factor representing the ratio between physical screen pixels and device-independent pixels [1][5][3]. It is often a product of the native OS scale factor (provided by the platform) and any Qt-specific scale factor (e.g., set via QT_SCALE_FACTOR) [2][3][6]. - logicalDotsPerInch: This value is used primarily to convert font sizes and user interface element dimensions (expressed in points or other logical units) into device-independent pixels [1][5]. It is often derived from user-settable desktop environment settings [1]. Key Interactions Qt calculates the necessary devicePixelRatio by comparing the target display's logical DPI to a reference "base" DPI (typically 96 on X11) [2][3]. When an OS reports a scale factor via DPI (e.g., 144 DPI), Qt translates this into a devicePixelRatio [2][3]. Platform-Specific Behavior - X11: Often provides only global logical DPI or per-screen physical DPI [2][3]. Because neither is a direct native scale factor, HiDPI configuration on X11 is frequently more complex than on other platforms [2][3]. QT_USE_PHYSICAL_DPI can be used to force Qt to use physical DPI if logical DPI is unavailable or incorrect [2][3]. - Wayland/Windows: These platforms generally provide native scaling information that Qt integrates directly into its devicePixelRatio calculation [3][6]. On Wayland, QWindow::devicePixelRatio may incorporate fractional scaling, which may differ from QScreen::devicePixelRatio depending on window-specific surface resolution settings [1][5]. Best Practices - Prefer QWindow::devicePixelRatio over QScreen::devicePixelRatio when a window instance is available [1][5]. QWindow::devicePixelRatio accurately tracks the DPR as the window moves between displays with different properties [2][3]. - logicalDotsPerInch is appropriate for UI layout and font scaling logic, whereas devicePixelRatio is intended for low-level drawing (e.g., OpenGL) and mapping coordinate systems to physical buffers [2][3].
Citations:
- 1: https://doc.qt.io/qt-6.11/qscreen.html
- 2: https://doc.qt.io/QT-6/highdpi.html
- 3: https://doc.qt.io/qt-6.11/highdpi.html
- 4: https://forum.qt.io/topic/127446/how-to-relate-logical-dpi-physical-dpi-ppi-and-device-pixel-ratio/2
- 5: https://doc.qt.io/qt-6.10/qscreen.html
- 6: https://github.com/qt/qtbase/blob/d0787745aa43e5baf49de876f917946df6aceca5/src/gui/kernel/qhighdpiscaling.cpp
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== File outline ==\n'
ast-grep outline riptide_rviz/riptide_rviz/src/OverlayDisplay.cpp --view expanded || true
printf '\n== Relevant excerpt ==\n'
sed -n '1,220p' riptide_rviz/riptide_rviz/src/OverlayDisplay.cpp | cat -n
printf '\n== Search for getUiScaleFactor usages and screen/window APIs ==\n'
rg -n "getUiScaleFactor|primaryScreen|screen\(|windowHandle\(|QScreen|devicePixelRatio|logicalDotsPerInch" riptide_rviz/riptide_rviz/src riptide_rviz/riptide_rviz/include || trueRepository: osu-uwrt/riptide_gui
Length of output: 11723
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Call sites around getUiScaleFactor ==\n'
sed -n '330,400p' riptide_rviz/riptide_rviz/src/OverlayDisplay.cpp | cat -n
printf '\n== OverlayDisplay declaration ==\n'
ast-grep outline riptide_rviz/riptide_rviz/include/riptide_rviz/OverlayDisplay.hpp --view expanded || true
sed -n '1,260p' riptide_rviz/riptide_rviz/include/riptide_rviz/OverlayDisplay.hpp | cat -n
printf '\n== Search for window/screen access in overlay-related code ==\n'
rg -n "windowHandle\(|screen\(\)|QWidget|QWindow|QGuiApplication::primaryScreen|QScreen" riptide_rviz/riptide_rviz/include riptide_rviz/riptide_rviz/src || trueRepository: osu-uwrt/riptide_gui
Length of output: 17239
Scale from the RViz render window, not the primary screen. primaryScreen() can mis-size the overlay on multi-monitor setups, and max(devicePixelRatio(), logicalDotsPerInch()/96) can over-scale on platforms where Qt already folds DPI scaling into DPR. Use the screen/window that owns the RViz surface, and keep DPR and logical DPI separate unless there’s a platform-specific reason to combine 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_rviz/riptide_rviz/src/OverlayDisplay.cpp` around lines 102 - 113, The
UI scaling logic in getUiScaleFactor() is using QGuiApplication::primaryScreen()
and combining devicePixelRatio with logicalDotsPerInch(), which can mis-scale
overlays on multi-monitor setups. Update the scale source to use the RViz render
window’s owning screen/window instead of the primary screen, and adjust the
calculation so DPR and logical DPI are not blindly merged unless there’s a
platform-specific need. Keep the change localized to getUiScaleFactor() in
OverlayDisplay.cpp and preserve the existing high-DPI fallback behavior.
| if (config.font_size_ != 0) { | ||
| // QFont font = painter.font(); | ||
| QFont font(config.font_name_.length() > 0 ? config.font_name_.c_str() : "Liberation Sans"); | ||
| font.setPointSize(config.font_size_); | ||
| font.setPixelSize(std::max(config.font_size_, 1)); | ||
| font.setBold(true); | ||
| painter.setFont(font); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Font pixel size is not scaled while texture dimensions are — inconsistent DPI scaling.
updateWidth()/updateHeight() multiply the configured width/height by getUiScaleFactor(), but paintText() still uses config.font_size_ unscaled via setPixelSize(). Since the overlay canvas grows on high-DPI displays but the font size doesn't, text will render visually smaller relative to the overlay/anchors than before this change, undermining the intent of the DPI-scaling feature.
💡 Proposed fix: scale font size consistently with texture dimensions
if (config.font_size_ != 0) {
// QFont font = painter.font();
QFont font(config.font_name_.length() > 0 ? config.font_name_.c_str() : "Liberation Sans");
- font.setPixelSize(std::max(config.font_size_, 1));
+ font.setPixelSize(std::max(static_cast<int>(std::lround(config.font_size_ * getUiScaleFactor())), 1));
font.setBold(true);
painter.setFont(font);
}Also applies to: 368-378
🤖 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_rviz/riptide_rviz/src/OverlayDisplay.cpp` around lines 228 - 234, The
font sizing in OverlayDisplay::paintText is not using the same DPI scaling as
the overlay dimensions, so text stays visually smaller on high-DPI displays.
Update the font setup in paintText to apply getUiScaleFactor() consistently when
calling QFont::setPixelSize, using the same scale logic already used by
updateWidth() and updateHeight(), and make sure the other paintText occurrence
uses the same adjusted sizing path.
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)
riptide_rviz/control_config_talos.rviz (1)
2621-2634: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win"Fire Hole Small" subscribes to the
_largetopic.Both
Fire Hole Small(Line 2634) andFire Hole Large(Line 2669) subscribe to/talos/dummydetections/torpedo_fire_hole_large. The small variant almost certainly should point totorpedo_fire_hole_small(matching the Blood Hole pattern at Lines 2564/2599). As written, the small-hole marker will never render its own detection.🐛 Proposed fix
- Value: /talos/dummydetections/torpedo_fire_hole_large + Value: /talos/dummydetections/torpedo_fire_hole_small Value: true(apply to the
Fire Hole Smallentry ending at Line 2634)🤖 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_rviz/control_config_talos.rviz` around lines 2621 - 2634, Update the Topic Value in the “Fire Hole Small” marker entry to subscribe to /talos/dummydetections/torpedo_fire_hole_small, while leaving the “Fire Hole Large” entry pointed at the large topic.
🤖 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 `@riptide_rviz/control_config_talos.rviz`:
- Around line 2621-2634: Update the Topic Value in the “Fire Hole Small” marker
entry to subscribe to /talos/dummydetections/torpedo_fire_hole_small, while
leaving the “Fire Hole Large” entry pointed at the large topic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 56c79325-e15c-4516-aac1-981a6d80c0c0
📒 Files selected for processing (3)
riptide_rviz/control_config_talos.rvizriptide_rviz/riptide_rviz/include/riptide_rviz/MissionPanel.hppriptide_rviz/riptide_rviz/src/MissionPanel.cpp
There was a problem hiding this comment.
🧹 Nitpick comments (1)
riptide_rviz/riptide_rviz/src/ElectricalPanel.ui (1)
481-937: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
pingerFreq5carries stray custom styling that the other four buttons don't.Only
pingerFreq5defines a full custompalette(blue button, black text) plus an italicfont, whilepingerFreq1–pingerFreq4use defaults. This appears to be an accidental Qt Designer artifact and will render the fifth frequency button inconsistently with its siblings. Consider dropping the palette/font overrides so all five buttons match.🤖 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_rviz/riptide_rviz/src/ElectricalPanel.ui` around lines 481 - 937, Remove the custom palette and font properties from the pingerFreq5 widget, including its blue palette and italic font override. Keep its text and checkable properties unchanged so it uses the same default styling as pingerFreq1–pingerFreq4.
🤖 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.
Nitpick comments:
In `@riptide_rviz/riptide_rviz/src/ElectricalPanel.ui`:
- Around line 481-937: Remove the custom palette and font properties from the
pingerFreq5 widget, including its blue palette and italic font override. Keep
its text and checkable properties unchanged so it uses the same default styling
as pingerFreq1–pingerFreq4.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2764eebb-170e-4556-b778-5dba8f02e0d2
📒 Files selected for processing (3)
riptide_rviz/riptide_rviz/include/riptide_rviz/ElectricalPanel.hppriptide_rviz/riptide_rviz/src/ElectricalPanel.cppriptide_rviz/riptide_rviz/src/ElectricalPanel.ui
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 `@riptide_rviz/riptide_rviz/src/RosbagPanel.cpp`:
- Around line 210-213: Update RosbagPanel::refreshTopics to store the result of
getRosNodeAbstraction().lock() and return early when it is null, before calling
get_raw_node(). Preserve the existing topic refresh flow when the ROS node
abstraction is available.
In `@riptide_rviz/riptide_rviz/src/ServicePanel.cpp`:
- Around line 113-116: Update ServicePanel::refreshServices to store the result
of getRosNodeAbstraction().lock(), check it for null, and return before calling
get_raw_node when unavailable. Preserve the existing service discovery flow when
the abstraction is valid, following the guard pattern used by
RosbagPanel::refreshTopics.
🪄 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: c699200c-8628-4d95-a6f8-f2c2bd4f5be0
📒 Files selected for processing (7)
riptide_rviz/riptide_rviz/config/rosbag_presets.yamlriptide_rviz/riptide_rviz/include/riptide_rviz/RosbagPanel.hppriptide_rviz/riptide_rviz/include/riptide_rviz/ServicePanel.hppriptide_rviz/riptide_rviz/src/RosbagPanel.cppriptide_rviz/riptide_rviz/src/RosbagPanel.uiriptide_rviz/riptide_rviz/src/ServicePanel.cppriptide_rviz/riptide_rviz/src/ServicePanel.ui
| void RosbagPanel::refreshTopics() | ||
| { | ||
| auto node = getDisplayContext()->getRosNodeAbstraction().lock()->get_raw_node(); | ||
| const auto topicsAndTypes = node->get_topic_names_and_types(); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Guard against a null ROS node abstraction.
getRosNodeAbstraction().lock() can return a null shared_ptr (e.g. during teardown or before the node is bound), and dereferencing it via ->get_raw_node() would crash the panel. A cheap guard avoids the segfault.
🛡️ Proposed guard
void RosbagPanel::refreshTopics()
{
- auto node = getDisplayContext()->getRosNodeAbstraction().lock()->get_raw_node();
+ auto nodeAbstraction = getDisplayContext()->getRosNodeAbstraction().lock();
+ if (!nodeAbstraction)
+ {
+ setStatus("ROS node not available yet.", true);
+ return;
+ }
+ auto node = nodeAbstraction->get_raw_node();
const auto topicsAndTypes = node->get_topic_names_and_types();📝 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.
| void RosbagPanel::refreshTopics() | |
| { | |
| auto node = getDisplayContext()->getRosNodeAbstraction().lock()->get_raw_node(); | |
| const auto topicsAndTypes = node->get_topic_names_and_types(); | |
| void RosbagPanel::refreshTopics() | |
| { | |
| auto nodeAbstraction = getDisplayContext()->getRosNodeAbstraction().lock(); | |
| if (!nodeAbstraction) | |
| { | |
| setStatus("ROS node not available yet.", true); | |
| return; | |
| } | |
| auto node = nodeAbstraction->get_raw_node(); | |
| const auto topicsAndTypes = node->get_topic_names_and_types(); |
🤖 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_rviz/riptide_rviz/src/RosbagPanel.cpp` around lines 210 - 213, Update
RosbagPanel::refreshTopics to store the result of getRosNodeAbstraction().lock()
and return early when it is null, before calling get_raw_node(). Preserve the
existing topic refresh flow when the ROS node abstraction is available.
| void ServicePanel::refreshServices() | ||
| { | ||
| auto node = getDisplayContext()->getRosNodeAbstraction().lock()->get_raw_node(); | ||
| const auto namesAndTypes = node->get_service_names_and_types(); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Guard against a null ROS node abstraction.
Same pattern as RosbagPanel::refreshTopics: getRosNodeAbstraction().lock() may return null and dereferencing it here would crash the panel.
🛡️ Proposed guard
void ServicePanel::refreshServices()
{
- auto node = getDisplayContext()->getRosNodeAbstraction().lock()->get_raw_node();
+ auto nodeAbstraction = getDisplayContext()->getRosNodeAbstraction().lock();
+ if (!nodeAbstraction)
+ {
+ setStatus("ROS node not available yet.", true);
+ return;
+ }
+ auto node = nodeAbstraction->get_raw_node();
const auto namesAndTypes = node->get_service_names_and_types();📝 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.
| void ServicePanel::refreshServices() | |
| { | |
| auto node = getDisplayContext()->getRosNodeAbstraction().lock()->get_raw_node(); | |
| const auto namesAndTypes = node->get_service_names_and_types(); | |
| void ServicePanel::refreshServices() | |
| { | |
| auto nodeAbstraction = getDisplayContext()->getRosNodeAbstraction().lock(); | |
| if (!nodeAbstraction) | |
| { | |
| setStatus("ROS node not available yet.", true); | |
| return; | |
| } | |
| auto node = nodeAbstraction->get_raw_node(); | |
| const auto namesAndTypes = node->get_service_names_and_types(); |
🤖 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_rviz/riptide_rviz/src/ServicePanel.cpp` around lines 113 - 116,
Update ServicePanel::refreshServices to store the result of
getRosNodeAbstraction().lock(), check it for null, and return before calling
get_raw_node when unavailable. Preserve the existing service discovery flow when
the abstraction is valid, following the guard pattern used by
RosbagPanel::refreshTopics.
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 (2)
riptide_rviz/control_config_talos.rviz (2)
2163-2174: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Fire Hole Smallsubscribes to thetorpedo_fire_hole_largetopic.The display is named "Fire Hole Small" but its topic (line 2173) is
/talos/dummydetections/torpedo_fire_hole_large— identical to the "Fire Hole Large" display below it (line 2208). TheBlood Hole Small/Blood Hole Largepair confirms the expected pattern: small should use_small. Both displays will render identical data.🔧 Proposed fix
Name: Fire Hole Small Shaft Length: 1 Shaft Radius: 0.05000000074505806 Shape: Arrow Topic: Depth: 5 Durability Policy: Volatile Filter size: 10 History Policy: Keep Last Reliability Policy: Reliable - Value: /talos/dummydetections/torpedo_fire_hole_large + Value: /talos/dummydetections/torpedo_fire_hole_small Value: true🤖 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_rviz/control_config_talos.rviz` around lines 2163 - 2174, Update the `Fire Hole Small` display’s `Topic.Value` to subscribe to `/talos/dummydetections/torpedo_fire_hole_small`, while leaving the `Fire Hole Large` topic unchanged.
673-684: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Tablein Mapping Objects subscribes to the wrong topic namespace.All other displays in the Mapping Objects group use
/talos/mapping/...topics, butTableuses/talos/dummydetections/table— the same topic as theTabledisplay in the Dummy Detections group (line 1683). This is likely a copy-paste error that will show dummy detection data in the mapping section.🔧 Proposed fix
Name: Table Shaft Length: 1 Shaft Radius: 0.05000000074505806 Shape: Arrow Topic: Depth: 5 Durability Policy: Volatile Filter size: 10 History Policy: Keep Last Reliability Policy: Reliable - Value: /talos/dummydetections/table + Value: /talos/mapping/table Value: true🤖 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_rviz/control_config_talos.rviz` around lines 673 - 684, Update the Mapping Objects `Table` display’s `Topic.Value` from `/talos/dummydetections/table` to the corresponding `/talos/mapping/...` namespace, matching the topic pattern used by the other displays in that group; leave the separate Dummy Detections `Table` unchanged.
🤖 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 `@riptide_rviz/control_config_talos.rviz`:
- Around line 2163-2174: Update the `Fire Hole Small` display’s `Topic.Value` to
subscribe to `/talos/dummydetections/torpedo_fire_hole_small`, while leaving the
`Fire Hole Large` topic unchanged.
- Around line 673-684: Update the Mapping Objects `Table` display’s
`Topic.Value` from `/talos/dummydetections/table` to the corresponding
`/talos/mapping/...` namespace, matching the topic pattern used by the other
displays in that group; leave the separate Dummy Detections `Table` unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: be7afa1d-b50c-4604-9aa9-d3167ed51474
📒 Files selected for processing (2)
riptide_rviz/control_config_talos.rvizriptide_rviz/riptide_rviz/plugins_description.xml
3 participants
Summary by CodeRabbit