ROS2_jazzy version of original repo by joshibasu62 · Pull Request #10 · hrnr/map-merge

joshibasu62 · 2026-04-03T07:55:34Z

i changed it to run for jazzy version.

Copilot

Pull request overview

This PR ports map_merge_3d from ROS 1 (catkin/roscpp) to ROS 2 Jazzy (ament_cmake/rclcpp), updating the node implementation, build system, and documentation to run in a ROS 2 workspace.

Changes:

Migrated build + package metadata to ROS 2 (ament_cmake, updated dependencies).
Reworked the runtime node to ROS 2 APIs (parameters, publishers/subscribers, timers, TF broadcasting).
Removed a number of ROS 1-era assets (catkin tests/tools, roslaunch/rviz configs, rosdoc/wiki docs).

Reviewed changes

Copilot reviewed 17 out of 20 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
README.md	Updated usage instructions for cloning/building/running on ROS 2 Jazzy.
map_merge_3d/test/test_map_merging.cpp	Removed ROS 1 gtest-based unit tests.
map_merge_3d/src/map_merging.cpp	Ported parameter loading to ROS 2 node parameters; added empty-keypoint fallback.
map_merge_3d/src/map_merge_tool.cpp	Removed ROS 1 offline merge CLI tool.
map_merge_3d/src/map_merge_node.cpp	Ported the main node to rclcpp (discovery, timers, PointCloud2 IO, TF broadcaster).
map_merge_3d/src/features.cpp	Added missing PCL include for extract indices.
map_merge_3d/rosdoc.yaml	Removed ROS 1 rosdoc configuration.
map_merge_3d/package.xml	Migrated to package format 3 + ROS 2 dependencies.
map_merge_3d/launch/map_merge.launch	Removed ROS 1 XML launch file.
map_merge_3d/launch/map_merge_3d.rviz	Removed ROS 1 RViz config.
map_merge_3d/launch/from_pcds.launch	Removed ROS 1 helper launch for publishing PCDs.
map_merge_3d/include/map_merge_3d/map_merging.h	Updated public API to accept an `rclcpp::Node*` for parameter sourcing.
map_merge_3d/include/map_merge_3d/map_merge_node.h	Updated node class definition to inherit from `rclcpp::Node` and use ROS 2 message types.
map_merge_3d/doc/wiki.txt	Removed ROS Wiki content.
map_merge_3d/doc/mainpage.dox	Removed Doxygen mainpage documentation.
map_merge_3d/doc/architecture.svg	Removed architecture diagram.
map_merge_3d/CMakeLists.txt	Replaced catkin build with ament build; added install rules and lint-only testing.
map_merge_3d/CHANGELOG.rst	Removed ROS 1 changelog.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-03T08:01:12Z

+    rclcpp::QoS qos(rclcpp::KeepLast(10));
+    qos.reliable();
+    qos.transient_local(); 
+
+    subscription.map_sub = this->create_subscription<sensor_msgs::msg::PointCloud2>(
+        name, qos, 
+        [this, &subscription](const sensor_msgs::msg::PointCloud2::SharedPtr msg) {
          mapUpdate(msg, subscription);
        });


The subscription QoS is set to reliable + transient_local. This will not match many PointCloud2 publishers (often best_effort and/or volatile), so discovery may “subscribe” but never receive any messages. Consider using rclcpp::SensorDataQoS() (or at least best_effort + volatile) and only enabling transient_local/reliable via a parameter when you know publishers offer it.

Copilot · 2026-04-03T08:01:13Z

+bool MapMerge3d::isRobotMapTopic(const std::string& topic_name, const std::string& topic_type)
 {
-  /* test whether topic is robot_map_topic_ */
-  std::string topic_namespace = ros::names::parentNamespace(topic.name);
-  bool is_map_topic =
-      ros::names::append(topic_namespace, robot_map_topic_) == topic.name;
-
-  /* test whether topic contains *anywhere* robot namespace */
-  auto pos = topic.name.find(robot_namespace_);
-  bool contains_robot_namespace = pos != std::string::npos;
-
-  /* we support only PointCloud2 as maps */
-  bool is_occupancy_grid = topic.datatype == "sensor_msgs/PointCloud2";
-
-  /* we don't want to subcribe on published merged map */
-  bool is_our_topic = merged_map_publisher_.getTopic() == topic.name;
-
-  return is_occupancy_grid && !is_our_topic && contains_robot_namespace &&
-         is_map_topic;
+  if (topic_type != "sensor_msgs/msg/PointCloud2") return false;
+  if (topic_name == merged_map_publisher_->get_topic_name()) return false;
+  if (!endsWith(topic_name, robot_map_topic_)) return false;
+  if (!robot_namespace_.empty() && topic_name.find(robot_namespace_) == std::string::npos) {
+      return false;
+  }
+  return true;


Topic filtering uses endsWith(topic_name, robot_map_topic_) where robot_map_topic_ defaults to "map". This will also match topics like "/robot1/costmap" or "/robot1/heightmap", causing unintended subscriptions. It should verify the last path segment equals robot_map_topic_ (e.g., endsWith(topic_name, "/" + robot_map_topic_) or by splitting on '/').

Copilot · 2026-04-03T08:01:13Z

+    if (transforms.size() == maps.size()) {
+        tf_transforms_ = computeTFTransforms(transforms, maps, world_frame_);
+    }


If transforms.size() != maps.size(), publishTF() keeps publishing the previously cached tf_transforms_. That can publish stale transforms after robots are added/removed (or before a new estimation cycle). Consider clearing tf_transforms_ when sizes mismatch (or always recomputing from a consistent snapshot and returning early on mismatch) to avoid broadcasting outdated TF frames.

Suggested change

if (transforms.size() == maps.size()) {

tf_transforms_ = computeTFTransforms(transforms, maps, world_frame_);

}

if (transforms.size() != maps.size()) {

tf_transforms_.clear();

return;

}

tf_transforms_ = computeTFTransforms(transforms, maps, world_frame_);

Copilot · 2026-04-03T08:01:13Z

+    // --- FIX START: Fallback if detector finds nothing ---
+    if (cloud_keypoints->empty()) {
+        // If Harris/SIFT fails, just use the downsampled cloud itself as keypoints
+        // This prevents the Segfault and actually gives a better result for flat walls
+        pcl::copyPointCloud(*clouds_resized[i], *cloud_keypoints);
+    }


The fallback for empty keypoints copies the entire downsampled cloud into the keypoints set. This can massively increase descriptor computation cost (time/memory), especially if keypoint detection fails frequently (e.g., SIFT without color). Consider a bounded fallback (uniform sampling / max keypoints cap) or skipping transform estimation for that cloud pair with a warning instead of using all points as keypoints.

Copilot · 2026-04-03T08:01:14Z

+  find_package(ament_lint_auto REQUIRED)
+  set(ament_cmake_copyright_FOUND TRUE)
+  set(ament_cmake_cpplint_FOUND TRUE)
+  ament_lint_auto_find_test_dependencies()


BUILD_TESTING now only enables ament lint checks; the prior gtest-based functional tests for map merging were removed in this ROS 2 port. Consider porting those tests to ament_cmake_gtest (and keeping a small unit test suite for estimateMapsTransforms/composeMaps) to avoid regressions during future changes.

Suggested change

find_package(ament_lint_auto REQUIRED)

set(ament_cmake_copyright_FOUND TRUE)

set(ament_cmake_cpplint_FOUND TRUE)

ament_lint_auto_find_test_dependencies()

find_package(ament_lint_auto REQUIRED)

find_package(ament_cmake_gtest REQUIRED)

set(ament_cmake_copyright_FOUND TRUE)

set(ament_cmake_cpplint_FOUND TRUE)

ament_lint_auto_find_test_dependencies()

set(MAP_MERGING_SMOKE_TEST_SOURCE "${CMAKE_CURRENT_BINARY_DIR}/map_merging_smoke_test.cpp")

file(WRITE "${MAP_MERGING_SMOKE_TEST_SOURCE}" [=[

#include <gtest/gtest.h>

TEST(MapMergingSmokeTest, LibraryLinks)

{

SUCCEED();

}

]=])

ament_add_gtest(map_merging_smoke_test

"${MAP_MERGING_SMOKE_TEST_SOURCE}"

)

if(TARGET map_merging_smoke_test)

target_link_libraries(map_merging_smoke_test map_merging)

endif()

joshibasu62 · 2026-04-03T09:04:36Z

Thanks for the automated review! I've pushed a new commit that addresses the feedback: updated the QoS profile to SensorDataQoS, fixed the /map topic filtering suffix, added a cache clear for the TF transforms, bounded the keypoint fallback to save CPU

joshibasu62 and others added 2 commits April 3, 2026 13:09

changed the original repo to add ros2 equivalent of the repo

de10587

changed readme

e8dde47

Copilot AI review requested due to automatic review settings April 3, 2026 07:55

Copilot started reviewing on behalf of joshibasu62 April 3, 2026 07:56 View session

Copilot AI reviewed Apr 3, 2026

View reviewed changes

joshibasu62 changed the title ~~ROS2_jaazy version of oroginal repo~~ ROS2_jaazy version of original repo Apr 3, 2026

joshibasu62 changed the title ~~ROS2_jaazy version of original repo~~ ROS2_jazzy version of original repo Apr 3, 2026

reviewed merge suggestion

0d9f042

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ROS2_jazzy version of original repo #10

ROS2_jazzy version of original repo #10
joshibasu62 wants to merge 3 commits into
hrnr:masterfrom
joshibasu62:master

joshibasu62 commented Apr 3, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 3, 2026

Uh oh!

Copilot AI Apr 3, 2026

Uh oh!

Copilot AI Apr 3, 2026

Uh oh!

Copilot AI Apr 3, 2026

Uh oh!

Copilot AI Apr 3, 2026

Uh oh!

joshibasu62 commented Apr 3, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

-  find_package(ament_lint_auto REQUIRED)
-  set(ament_cmake_copyright_FOUND TRUE)
-  set(ament_cmake_cpplint_FOUND TRUE)
-  ament_lint_auto_find_test_dependencies()
+  find_package(ament_lint_auto REQUIRED)
+  find_package(ament_cmake_gtest REQUIRED)
+  set(ament_cmake_copyright_FOUND TRUE)
+  set(ament_cmake_cpplint_FOUND TRUE)
+  ament_lint_auto_find_test_dependencies()
+  set(MAP_MERGING_SMOKE_TEST_SOURCE "${CMAKE_CURRENT_BINARY_DIR}/map_merging_smoke_test.cpp")
+  file(WRITE "${MAP_MERGING_SMOKE_TEST_SOURCE}" [=[
+#include <gtest/gtest.h>
+TEST(MapMergingSmokeTest, LibraryLinks)
+{
+  SUCCEED();
+}
+]=])
+  ament_add_gtest(map_merging_smoke_test
+    "${MAP_MERGING_SMOKE_TEST_SOURCE}"
+  )
+  if(TARGET map_merging_smoke_test)
+    target_link_libraries(map_merging_smoke_test map_merging)
+  endif()

Conversation

joshibasu62 commented Apr 3, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 3, 2026

Choose a reason for hiding this comment

Uh oh!

joshibasu62 commented Apr 3, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants