ROS2 bridge: publish geographic poses too - #99
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (18)
📝 WalkthroughWalkthroughAdds explicit static-analysis settings and editor dictionary entries; tightens ROS2 CMake/package dependencies; and implements geo-referenced pose publishing plus refactored localization and per-layer map publishing in the ROS2 bridge (BridgeROS2), including new georeferencing storage, publishing timers, and related helpers. Many MRPT initializer registrations were expanded with NOLINT annotations and minor internal refactors. Changes
Sequence Diagram(s)sequenceDiagram
participant LocalizationSource
participant MapSource
participant BridgeROS2
participant TF_Broadcaster
participant ROS_Topics
LocalizationSource->>BridgeROS2: LocalizationUpdate(pose, quality, stamp)
BridgeROS2->>BridgeROS2: timerPubLocalization() -> delegate helpers
BridgeROS2->>TF_Broadcaster: timerPubLocalizationTf(pose)
TF_Broadcaster-->>ROS_Topics: publish TF transform
BridgeROS2->>ROS_Topics: timerPubLocalizationOdom(pose) -> publish Odometry
BridgeROS2->>ROS_Topics: timerPubLocalizationQuality(quality) -> publish quality
BridgeROS2->>ROS_Topics: timerPubLocalizationGeoRef(pose, georef) -> publish GeoPoseStamped
MapSource->>BridgeROS2: MapUpdate(layer, data)
BridgeROS2->>BridgeROS2: timerPubMap() -> timerPubMapLayer(layer, data)
BridgeROS2->>ROS_Topics: publish per-layer map + georeferencing metadata
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.vscode/settings.json (1)
43-67: Consider scoping/removing overly-genericcSpell.words.
CPose3DPDFGaussian,relocalize, andTUTMCoordslook appropriately project-specific, butPosePDF) or accept it only if you’re seeing frequent false positives in this repo.mola_input_mulran_dataset/src/MulranDataset.cpp (1)
269-277: CI blocker: clang-format job is failing for this file (reported at Line 270).
Please run the repo’s clang-format (CI mentionsclang-format-14 --style=file -i) and re-commit. Also, this erase can be simplified (and is typically clang-format-friendly):Proposed simplification
- lstPointCloudFiles_.erase( - std::next( - lstPointCloudFiles_.begin(), - static_cast<std::vector<std::string>::difference_type>(idx))); + lstPointCloudFiles_.erase( + lstPointCloudFiles_.begin() + + static_cast<std::vector<std::string>::difference_type>(idx));
🤖 Fix all issues with AI agents
In @.clang-tidy:
- Around line 64-70: The CheckOptions entry
misc-non-private-member-variables-in-classes.IgnoreClassesWithAllMemberVariablesBeingPublic
is ineffective because the Checks list disables that check with
'-misc-non-private-member-variables-in-classes'; either remove the CheckOptions
line entirely if you don't want the check, or re-enable the check by deleting
the '-' from the Checks entry so misc-non-private-member-variables-in-classes is
active and the IgnoreClassesWithAllMemberVariablesBeingPublic option will take
effect.
- Around line 60-62: HeaderFilterRegex currently set to ".*" causes external
headers to be included; update the HeaderFilterRegex setting in .clang-tidy to
restrict linting to your project's headers (e.g., match your include/src
directories or namespace prefix) by replacing the ".*" value with a narrower
regex that targets only internal headers (for example a pattern matching your
project's include/ or src/ layout or your project namespace) so ROS2/MRPT
external headers are excluded.
In `@mola_bridge_ros2/src/BridgeROS2.cpp`:
- Around line 1530-1535: The early-return condition is inverted: it returns when
the incoming transform IS map->base_link instead of skipping other frames.
Update the if in BridgeROS2.cpp to return only when geo-referenced publishing is
disabled OR the transform is NOT map->base_link by checking l.child_frame !=
params_.base_link_frame || l.reference_frame != params_.reference_frame (or
equivalently negate the current frame-equality test); keep the publish toggle
check params_.publish_geo_referenced_poses_from_slam as-is so publishing occurs
for matching map->base_link updates.
- Around line 1849-1851: Reformat the Odometry subscription to match the file's
subscription style: move the lambda onto its own indented lines and align the
capture, parameter, and body like the other create_subscription calls; locate
the subsOdometry_.emplace_back call that uses
rosNode_->create_subscription<nav_msgs::msg::Odometry> and ensure the lambda
invoking this->callbackOnOdometry(o, output_sensor_label) follows the same
spacing/line breaks as the other subscriptions to satisfy clang-format.
In `@mola_viz/src/MolaViz.cpp`:
- Around line 1624-1626: The fade-out calculation wrongly divides (decay_time -
threshold_time) by DECAY_FADE_OUT_TIME which equals 1; replace that term with
the actual time-since-threshold (delta_time) so alpha =
clamp(decay_cloud.initial_alpha * (1.0f - delta_time / DECAY_FADE_OUT_TIME),
0.0f, 1.0f). Locate the expression using decay_cloud.initial_alpha, decay_time,
threshold_time and DECAY_FADE_OUT_TIME and use the delta_time (or compute
time_since_threshold = current_time - threshold_time) as the numerator before
clamping.
- Around line 387-390: minmax_ignore_nan() can return a pair of end iterators
when the buffer is all-NaNs, but the code immediately dereferences the returned
iterators (itMin/itMax) causing UB; change each call (the ones that build
additionalMsgs with field, i.e., where itMin/itMax are used) to capture the
returned iterators into a named pair/auto, check whether the iterators indicate
the empty/all-NaN case (e.g., itMin==itMax or itMin==field.end()), and only
dereference when valid; for the all-NaN case push a safe message like "range:
[all-NaN]" or skip the range entry. Apply the same guard to the other
occurrences mentioned (the blocks around the other additionalMsgs pushes).
🧹 Nitpick comments (2)
.clang-tidy (2)
47-59:WarningsAsErrorslooks syntactically OK, but includes patterns for checks not enabled inChecks.
E.g.,cert-*is listed as error, butcert-*isn’t enabled (only-cert-err58-cppis explicitly disabled). If intentional “future-proofing”, consider a short comment; otherwise, trim to reduce confusion.
35-42: Consider toning down the wording in comments (repo professionalism / downstream reuse).
“This is too much of a pain…” may be fine locally, but tends to age poorly in shared configs.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
.clang-tidy.vscode/settings.jsonmola_bridge_ros2/CMakeLists.txtmola_bridge_ros2/include/mola_bridge_ros2/BridgeROS2.hmola_bridge_ros2/package.xmlmola_bridge_ros2/src/BridgeROS2.cppmola_input_euroc_dataset/src/EurocDataset.cppmola_input_kitti360_dataset/src/Kitti360Dataset.cppmola_input_kitti_dataset/src/KittiOdometryDataset.cppmola_input_lidar_bin_dataset/src/BinFileDataset.cppmola_input_mulran_dataset/src/MulranDataset.cppmola_input_paris_luco_dataset/src/ParisLucoDataset.cppmola_input_rawlog/src/RawlogDataset.cppmola_input_rosbag2/src/Rosbag2Dataset.cppmola_kernel/src/register.cppmola_metric_maps/src/register.cppmola_viz/src/MolaViz.cpp
🧰 Additional context used
🪛 GitHub Actions: CI clang-format
mola_input_mulran_dataset/src/MulranDataset.cpp
[error] 270-270: clang-format formatting diff detected. To fix, run '/usr/bin/clang-format-14 --style=file -i' on the file. Code style mismatch in MulranDataset.cpp.
mola_bridge_ros2/src/BridgeROS2.cpp
[error] 1847-1847: clang-format formatting diff detected. To fix, run '/usr/bin/clang-format-14 --style=file -i' on the file. Found diff in BridgeROS2.cpp around the Odometry subscription lambdas.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build_docker (ubuntu:noble, rolling, true)
- GitHub Check: build_docker (ubuntu:noble, jazzy, true)
- GitHub Check: build_docker (ubuntu:jammy, humble, true)
🔇 Additional comments (32)
mola_bridge_ros2/package.xml (1)
35-35: LGTM!The
geographic_msgsdependency is correctly added and aligns with the CMakeLists.txt changes and the new geo-referenced pose publishing feature in BridgeROS2.cpp.mola_bridge_ros2/CMakeLists.txt (3)
26-29: LGTM!Good restructuring of the MOLA core dependencies section with clearer comments and explicit REQUIRED constraints.
35-37: LGTM!The
geographic_msgsdependency is correctly added as REQUIRED andrclcppis now explicitly marked REQUIRED, improving build-time error detection.
62-62: LGTM!Correctly exposing
geographic_msgs_TARGETSin PUBLIC_LINK_LIBRARIES to make the dependency available to downstream users.mola_bridge_ros2/src/BridgeROS2.cpp (9)
65-65: LGTM!The
geographic_msgs/msg/geo_pose_stamped.hppheader is correctly included to support the new geo-referenced pose publishing feature.
78-81: LGTM!The NOLINT annotation for
misc-use-anonymous-namespaceis appropriate for the MRPT module registration pattern.
681-708: LGTM!Good optimization to add early returns after handling each observation type. This avoids unnecessary
dynamic_pointer_castchecks after a match is found.
1424-1476: LGTM!Clean extraction of TF publishing logic into a dedicated helper. The REP-105 handling and mutex locking are correctly preserved.
1479-1511: LGTM!Clean helper function for odometry message publishing with correct early-return logic for filtering by source.
1513-1524: LGTM!Clean helper for publishing localization quality metrics.
1553-1567: LGTM!Good exception handling around the coordinate conversion that could fail with invalid georeferencing data. Logging a warning rather than propagating is appropriate here.
1609-1702: LGTM!Well-structured helper function for per-layer map publishing. It correctly handles multiple map types and stores georeferencing info in a thread-safe manner.
736-742: LGTM!Consistent renaming of TF message variable improves readability.
mola_bridge_ros2/include/mola_bridge_ros2/BridgeROS2.h (4)
111-116: LGTM!Appropriate to make
BridgeROS2non-copyable and non-movable given it manages threads and shared resources like ROS nodes.
184-186: LGTM!New configuration parameter
publish_geo_referenced_poses_from_slamwith sensible default value enables the new geo-referenced pose publishing feature.
362-370: LGTM!Thread-safe storage and accessor for georeferencing information. The accessor correctly uses
std::lock_guardto protect the read operation.
373-381: LGTM!New helper function declarations align with the refactored implementation in BridgeROS2.cpp, providing a clean separation of localization and map publishing concerns.
.clang-tidy (1)
1-45: The.clang-tidyconfiguration is valid. The YAML array syntax forChecksis officially supported by clang-tidy since at least LLVM 12, and is documented in clang-tidy documentation through current releases (14, 17, 22, 23). The clang-tidy source explicitly states "the list of globs can be specified as a list instead of a string." The configuration will parse and function correctly without modification.Likely an incorrect or invalid review comment.
mola_input_kitti_dataset/src/KittiOdometryDataset.cpp (1)
45-48: MRPT initializer refactor is behavior-preserving and improves consistency.
The explicit block +// NOLINT(misc-use-anonymous-namespace)keeps module registration intact while aligning with the project-wide pattern.mola_kernel/src/register.cpp (1)
23-23: NOLINT addition is a safe, no-op lint suppression.
No behavioral or linkage impact; just silences the targeted clang-tidy check.mola_input_euroc_dataset/src/EurocDataset.cpp (1)
45-48: Initializer formatting change looks good and maintains registration behavior.
KeepsMOLA_REGISTER_MODULE(EurocDataset);execution at library load while matching the repo’s updated style.mola_input_rawlog/src/RawlogDataset.cpp (1)
39-42: Registration initializer refactor is fine (no semantic change).
The explicit braces + NOLINT keep the same load-time registration and reduce lint noise.mola_input_rosbag2/src/Rosbag2Dataset.cpp (2)
76-79: Initializer block + NOLINT is consistent with the rest of the PR and keeps behavior unchanged.
327-332: Rotating-scan callback reformat is behavior-neutral.
The callback still routes throughcatchExceptions()totoRotatingScan(); the added braces just improve readability/consistency.mola_input_lidar_bin_dataset/src/BinFileDataset.cpp (1)
36-39: Initializer reformat + NOLINT looks fine.
No behavior change; the registration call is preserved.mola_input_kitti360_dataset/src/Kitti360Dataset.cpp (1)
50-53: Initializer reformat + NOLINT looks fine.mola_viz/src/MolaViz.cpp (2)
616-619: Pure reformatting is OK here.Also applies to: 693-697, 705-709
720-737: Module registration block refactor looks fine.mola_metric_maps/src/register.cpp (1)
36-36: NOLINT annotation on initializer is fine.mola_input_mulran_dataset/src/MulranDataset.cpp (1)
48-51: Initializer reformat + NOLINT looks fine.mola_input_paris_luco_dataset/src/ParisLucoDataset.cpp (2)
51-54: Initializer refactor looks safe; NOLINT intent is clear.Multi-line
MRPT_INITIALIZER(do_register_ParisLucoDataset)keeps the same registration behavior and theNOLINT(misc-use-anonymous-namespace)documents the suppression rationale.
274-278: Whitespace-only change in points map creation.No semantic change here; improves readability/consistency.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
3fbef90 to
e4e950f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
mola_bridge_ros2/src/BridgeROS2.cpp (2)
1221-1242: Bug: Wrong collection checked in guard condition.The function checks
molaSubs_.relocalization.empty()but operates onmolaSubs_.mapServers. This will incorrectly reject valid map load requests when relocalization modules aren't present, and incorrectly accept requests when map servers aren't present.🐛 Proposed fix
void BridgeROS2::service_map_load( const std::shared_ptr<mola_msgs::srv::MapLoad::Request> request, // NOLINT(performance-unnecessary-value-param) std::shared_ptr<mola_msgs::srv::MapLoad::Response> response) // NOLINT(performance-unnecessary-value-param) { auto lck = mrpt::lockHelper(rosPubsMtx_); - if (molaSubs_.relocalization.empty()) + if (molaSubs_.mapServers.empty()) { response->success = false; response->error_message = "No MOLA module with MapServer interface is running."; MRPT_LOG_WARN(response->error_message); return; } const auto& m = *molaSubs_.mapServers.begin();
1244-1265: Bug: Same wrong collection checked in guard condition.Same issue as
service_map_load- checkingrelocalizationinstead ofmapServers.🐛 Proposed fix
void BridgeROS2::service_map_save( const std::shared_ptr<mola_msgs::srv::MapSave::Request> request, // NOLINT(performance-unnecessary-value-param) std::shared_ptr<mola_msgs::srv::MapSave::Response> response) // NOLINT(performance-unnecessary-value-param) { auto lck = mrpt::lockHelper(rosPubsMtx_); - if (molaSubs_.relocalization.empty()) + if (molaSubs_.mapServers.empty()) { response->success = false; response->error_message = "No MOLA module with MapServer interface is running."; MRPT_LOG_WARN(response->error_message); return; } const auto& m = *molaSubs_.mapServers.begin();
♻️ Duplicate comments (1)
mola_bridge_ros2/src/BridgeROS2.cpp (1)
1849-1851: Fix clang-format error flagged by CI.The pipeline failure indicates a formatting inconsistency in the Odometry subscription lambda. The lambda should be formatted consistently with other subscription patterns in this file.
🔧 Proposed fix
subsOdometry_.emplace_back(rosNode_->create_subscription<nav_msgs::msg::Odometry>( - topic_name, qos, [this, output_sensor_label](const nav_msgs::msg::Odometry& o) + topic_name, qos, + [this, output_sensor_label](const nav_msgs::msg::Odometry& o) { this->callbackOnOdometry(o, output_sensor_label); }));
🧹 Nitpick comments (1)
.clang-tidy (1)
4-19: Consider removing redundant specific check entries.Lines 4-11 enable specific checks that are already covered by the wildcard patterns on lines 12-19 (e.g.,
readability-*andcppcoreguidelines-*). These entries are harmless but redundant since clang-tidy processes entries in order and the wildcards would enable them anyway.If these are intentional for documentation purposes, consider adding a comment to clarify. Otherwise, they can be removed for conciseness.
♻️ Optional cleanup
Checks: [ "-*", "bugprone-*", - "readability-braces-around-statements", - "readability-else-after-return", - "cppcoreguidelines-interfaces-global-init", - "cppcoreguidelines-pro-type-member-init", - "cppcoreguidelines-pro-type-static-cast-downcast", - "cppcoreguidelines-pro-type-union-access", - "cppcoreguidelines-slicing", - "cppcoreguidelines-special-member-functions", "clang-analyzer-*", "concurrency-*", "cppcoreguidelines-*",
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.clang-tidy.vscode/settings.jsonmola_bridge_ros2/include/mola_bridge_ros2/BridgeROS2.hmola_bridge_ros2/src/BridgeROS2.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- .vscode/settings.json
🧰 Additional context used
🪛 GitHub Actions: CI clang-format
mola_bridge_ros2/src/BridgeROS2.cpp
[error] 1847-1848: clang-format detected a formatting mismatch in BridgeROS2.cpp. Found diff indicating code style changes needed. Run '/usr/bin/clang-format-14 --style=file -i mola_bridge_ros2/src/BridgeROS2.cpp' to fix.
[error] 1847-1848: Code Style does not match coding style. clang-format exited with an error.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build_docker (ubuntu:noble, jazzy, true)
- GitHub Check: build_docker (ubuntu:jammy, humble, true)
- GitHub Check: build_docker (ubuntu:noble, rolling, true)
🔇 Additional comments (9)
.clang-tidy (3)
47-58: LGTM!The WarningsAsErrors configuration is well-structured with appropriate exclusions for checks that may produce excessive noise (
concurrency-mt-unsafe) or need relaxed settings (readability-function-cognitive-complexitywhich hasIgnoreMacrosconfigured below).
60-61: Previous feedback addressed - HeaderFilterRegex now properly scoped.The regex
"(mola_[^/]*/include|mola_[^/]*/src)/"correctly limits linting to project headers only, excluding external dependencies like ROS2 and MRPT.
63-66: Previous feedback addressed - dead CheckOptions entry removed.The configuration now only includes the valid
readability-function-cognitive-complexity.IgnoreMacrosoption, which correctly applies since the check is enabled viareadability-*.mola_bridge_ros2/include/mola_bridge_ros2/BridgeROS2.h (3)
111-116: LGTM! Properly deleted copy/move operations.Disabling copy and move semantics is appropriate for this class given its use of threads (
rosNodeThread_), mutexes, and shared ROS2 resources that cannot be safely copied or moved.
362-370: LGTM! Thread-safe georeferencing accessor.The mutex-protected accessor pattern is correctly implemented for thread-safe access to the optional georeferencing info.
373-381: LGTM! Well-structured refactoring of publish methods.The decomposition of
timerPubLocalizationandtimerPubMapinto smaller, focused helper methods improves maintainability and follows the single responsibility principle.mola_bridge_ros2/src/BridgeROS2.cpp (3)
1526-1585: LGTM! Geo-referenced pose publishing implementation.The logic correctly:
- Guards against disabled feature or non-matching frames (line 1531-1532 now correctly uses
!())- Safely handles missing georeferencing info
- Converts local pose to ENU coordinates, then to geodetic coordinates via geocentric transformation
- Publishes as
GeoPoseStampedwith proper latitude/longitude/altitude and orientationThe exception handling around the coordinate conversion (lines 1555-1567) provides resilience against edge cases in topographic calculations.
1410-1421: LGTM! Clean refactoring of localization publishing.The decomposition into dedicated helper methods (
timerPubLocalizationTf,timerPubLocalizationOdom,timerPubLocalizationQuality,timerPubLocalizationGeoRef) improves readability and maintainability while keeping the publishing flow clear.
1609-1702: LGTM! Well-organized per-layer map publishing.The
timerPubMapLayerfunction cleanly handles different map types (point clouds, occupancy grids, voxel maps) with appropriate fallbacks, and properly publishes georeferencing and metadata when available.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@mola_bridge_ros2/src/BridgeROS2.cpp`:
- Around line 1555-1584: The catch in timerPubLocalizationGeoRef currently logs
exceptions from ENU->Geodetic conversion (MRPT_LOG_WARN_STREAM) but then
continues and publishes default-initialized pose_coords; change the control flow
to return early from timerPubLocalizationGeoRef immediately after logging the
exception in the catch block so the subsequent publishing code (creation of msg,
setting msg.pose.position.latitude/longitude/altitude and
pubGeoPose->publish(msg)) is not executed with invalid defaults.
♻️ Duplicate comments (2)
mola_viz/src/MolaViz.cpp (1)
1629-1632: Bug: fade-out alpha computation is constant and drops to ~0 immediately.The formula
(decay_time - threshold_time) / DECAY_FADE_OUT_TIMEalways equals1.0sincethreshold_time = decay_time - DECAY_FADE_OUT_TIME. This causes alpha to immediately become ~0 instead of gradually fading.Should use
delta_time(elapsed time) instead ofdecay_time(the configured decay duration):Proposed fix
const float new_alpha = mrpt::saturate_val( decay_cloud.initial_alpha * - (1.0f - (decay_time - threshold_time) / DECAY_FADE_OUT_TIME), + (1.0f - (delta_time - threshold_time) / DECAY_FADE_OUT_TIME), 0.0f, 1.0f);mola_bridge_ros2/src/BridgeROS2.cpp (1)
1849-1851: Fix clang-format error flagged by CI.The pipeline failure indicates a formatting inconsistency in the Odometry subscription lambda.
🔧 Proposed fix
subsOdometry_.emplace_back(rosNode_->create_subscription<nav_msgs::msg::Odometry>( - topic_name, qos, [this, output_sensor_label](const nav_msgs::msg::Odometry& o) + topic_name, qos, + [this, output_sensor_label](const nav_msgs::msg::Odometry& o) { this->callbackOnOdometry(o, output_sensor_label); }));
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.clang-tidymola_bridge_ros2/src/BridgeROS2.cppmola_viz/include/mola_viz/MolaViz.hmola_viz/src/MolaViz.cpp
✅ Files skipped from review due to trivial changes (1)
- mola_viz/include/mola_viz/MolaViz.h
🧰 Additional context used
🪛 GitHub Actions: CI clang-format
mola_bridge_ros2/src/BridgeROS2.cpp
[error] 1847-1847: clang-format failed: Code Style does not match coding style. Run 'clang-format-14 --style=file -i mola_bridge_ros2/src/BridgeROS2.cpp' to fix formatting.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build_docker (ubuntu:jammy, humble, true)
- GitHub Check: build_docker (ubuntu:noble, rolling, true)
- GitHub Check: build_docker (ubuntu:noble, jazzy, true)
🔇 Additional comments (9)
mola_viz/src/MolaViz.cpp (3)
118-127: LGTM! Good fix for the all-NaN edge case.This helper properly guards against dereferencing end iterators when all values are NaN. The check at line 122 ensures safe behavior.
Note:
std::to_stringproduces different precision output than the previous%.02fformat (e.g.,"0.123456"instead of"0.12"). If consistent precision is desired, consider usingmrpt::formatfor the values as well.
133-134: LGTM! Good defensive initialization.The explicit
auto*type with nullptr check, and the initialization ofshowHzat line 192 andglControlat line 246, improve code safety and clarity.
395-400: LGTM!The switch to
minmax_ignore_nan_strproperly addresses the potential UB when buffers contain only NaN values..clang-tidy (1)
1-64: LGTM! Previous review concerns have been addressed.The configuration is well-organized with:
- Narrowed
HeaderFilterRegexto project headers only (line 58)- Removed the dead
CheckOptionsentry for the disabledmisc-non-private-member-variables-in-classescheck- Clear comments explaining the rationale for disabled checks
Minor observation: Lines 5-10 explicitly enable specific
cppcoreguidelines-*checks that are already covered by the wildcard at line 13. This redundancy is harmless but could be cleaned up for clarity.mola_bridge_ros2/src/BridgeROS2.cpp (5)
1526-1542: Logic inversion fix verified - condition now correct.The early-return condition properly skips publishing when:
- The feature is disabled, OR
- The update is NOT
map -> base_linkThe geo-referenced pose will now be published only for valid
map -> base_linklocalization updates when the feature is enabled.
1388-1421: Clean refactoring of localization publishing pipeline.The decomposition into focused helper functions (
timerPubLocalizationTf,timerPubLocalizationOdom,timerPubLocalizationQuality,timerPubLocalizationGeoRef) improves code organization and maintainability. The mutex is properly released before processing the copied data.
1609-1702: Well-structured per-layer map publishing with appropriate null checks.The function handles multiple map types gracefully with proper fallbacks. The georeferencing storage (line 1679-1680) correctly uses mutex protection.
Note: The
std::const_pointer_castat line 1625 is a code smell, but acceptable here for reusinginternalOnwith fake observations. Consider documenting this workaround inline.
78-81: Appropriate NOLINT annotation for MRPT initializer macro.The
misc-use-anonymous-namespacesuppression is correct here asMRPT_INITIALIZERrequires specific linkage behavior.
1168-1172: NOLINT annotations correctly applied for ROS2 service callback signatures.ROS2 service callbacks require
std::shared_ptrparameters by value per the rclcpp API. Theperformance-unnecessary-value-paramsuppressions are appropriate.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
…ternally, and publish georef poses merge of these commits: - Enable many more clang-tidy checks - lint clean - implement publishing georeferenced poses - mola-viz: fix potential crash on edge case with all points having NaN value - FIX: potential crash if no MapServer is present and map services are called
d2df084 to
276392f
Compare
1 participant
Summary by CodeRabbit
New Features
Improvements
Style
Chores
✏️ Tip: You can customize this high-level summary in your review settings.