Pre-comp dev merge#36
Conversation
Ghost pos fix
📝 WalkthroughWalkthroughThis PR updates RViz scene/marker configuration for the Talos robot, adds autonomy-aware interactive marker suppression in ControlPanel, introduces mapping reset/capture image services and UI in MappingPanel, adds TF broadcasting with base-link offset correction in MarkerPublisher, adds DPI-aware scaling to OverlayDisplay, fixes MissionPanel focus behavior, and reworks ParamPanel array-parameter change detection using stored widget properties. ChangesRViz GUI and control panel changes
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant TreeStackTopic
participant ControlPanel
participant InteractiveMarkerServer
TreeStackTopic->>ControlPanel: treeStackCallback(TreeStack msg)
alt stack not empty
ControlPanel->>ControlPanel: autonomyActive = true
ControlPanel->>InteractiveMarkerServer: erase setpoint marker
else stack empty
ControlPanel->>ControlPanel: autonomyActive = false
ControlPanel->>InteractiveMarkerServer: reinsert + apply setpoint marker
end
sequenceDiagram
participant User
participant MappingPanel
participant ROSService
User->>MappingPanel: click Reset Mapping button
MappingPanel->>ROSService: resetMappingClient Trigger request
User->>MappingPanel: click Capture Image button
MappingPanel->>ROSService: captureImageClient Trigger request
🚥 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: 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.
3 participants
Summary by CodeRabbit
New Features
Bug Fixes
Style