[WIP] ROS2 Port #101

Abishalini · 2021-08-04T19:55:24Z

Taking over Andy's work #100
I had to clone rviz_visual_tools, moveit_visual_tools, moveit_resources and graph_msgs from source. The rest can be installed using rosdep

How to use moveit_calibration

AndyZe · 2021-08-04T19:56:33Z

Nice. It should target the foxy branch

henningkayser

I will finish reviewing the rest later today

moveit_calibration_gui/CMakeLists.txt

henningkayser · 2021-08-09T10:59:21Z

moveit_calibration_gui/CMakeLists.txt

+find_package(rviz_common REQUIRED)
+find_package(rviz_default_plugins REQUIRED)
+find_package(rviz_rendering REQUIRED)
+find_package(rviz_visual_tools REQUIRED)

 find_package(Eigen3 REQUIRED)


Not entirely sure if this is still required (I think it is), but we used to depend on eigen3_cmake_module for Eigen3 to work. See https://github.com/ros2/eigen3_cmake_module

henningkayser · 2021-08-09T11:00:27Z

moveit_calibration_gui/CMakeLists.txt


 find_package(Eigen3 REQUIRED)
+find_package(OpenGL REQUIRED)
+find_package(GLUT REQUIRED)


Are we directly depending on GLUT or is this only added because of moveit_ros_perception not exporting it correctly? If so, please add a comment + issue link

moveit_calibration_gui/CMakeLists.txt

henningkayser · 2021-08-09T11:04:47Z

moveit_calibration_gui/CMakeLists.txt

-  DEPENDS
-    EIGEN3
+    rviz_rendering
+    tf2_eigen


Eigen3 and eigen3_cmake_module should be included as well.

moveit_calibration_gui/CMakeLists.txt

henningkayser · 2021-08-09T11:13:41Z

moveit_calibration_gui/handeye_calibration_rviz_plugin/CMakeLists.txt

 add_library(${MOVEIT_LIB_NAME}_core ${SOURCE_FILES} ${HEADERS})
 set_target_properties(${MOVEIT_LIB_NAME}_core PROPERTIES VERSION "${${PROJECT_NAME}_VERSION}")
 target_link_libraries(${MOVEIT_LIB_NAME}_core
-  ${catkin_LIBRARIES} ${OpenCV_LIBS} ${rviz_DEFAULT_PLUGIN_LIBRARIES} ${OGRE_LIBRARIES} ${QT_LIBRARIES} ${Boost_LIBRARIES})
+  ${OpenCV_LIBS} ${rviz_DEFAULT_PLUGIN_LIBRARIES} ${OGRE_LIBRARIES} ${QT_LIBRARIES} ${Boost_LIBRARIES} ${OPENGL_LIBRARIES} ${GLUT_LIBRARY})


Is rviz_DEFAULT_PLUGIN_LIBRARIES still a thing in ROS 2?
Also, all targets providing _LIBRARIES and _INCLUDE_DIRS can just be passed to ament_target_dependencies() which also supports the SYSTEM keyword. For readability, I would opt for using it, possibly even in combination with a SYSTEM_DEPENDS variable or similar.

...ation_rviz_plugin/include/moveit/handeye_calibration_rviz_plugin/handeye_calibration_frame.h

henningkayser · 2021-08-09T11:17:36Z

...ation_rviz_plugin/include/moveit/handeye_calibration_rviz_plugin/handeye_calibration_frame.h

@@ -83,7 +83,7 @@ class HandEyeCalibrationFrame : public QWidget
  ControlTabWidget* tab_control_;

 private:
-  rviz::DisplayContext* context_;
+  rviz_common::DisplayContext* context_;


not a high priority until things are working, but I would strongly advocate replacing all unnecessary raw pointers with smart pointers if possible

henningkayser · 2021-08-09T11:19:23Z

...ibration_rviz_plugin/include/moveit/handeye_calibration_rviz_plugin/handeye_context_widget.h

  {
-    robot_model_loader_.reset(new robot_model_loader::RobotModelLoader("robot_description"));
-    frame_manager_.reset(new rviz::FrameManager());
+    robot_model_loader_.reset(new robot_model_loader::RobotModelLoader(node_, "robot_description"));


imo, it's a general best practice to replace the new operator with std::make_shared() or std::make_unique()

...libration_rviz_plugin/include/moveit/handeye_calibration_rviz_plugin/handeye_target_widget.h

henningkayser · 2021-08-09T12:27:34Z

moveit_calibration_gui/handeye_calibration_rviz_plugin/src/handeye_target_widget.cpp

-            camerainfo_sub_.shutdown();
-            camerainfo_sub_ = nh_.subscribe(topic_name.toStdString(), 1, &TargetTabWidget::cameraInfoCallback, this);
+            // camerainfo_sub_.shutdown();
+            camerainfo_sub_.reset();


reset() is redundant if the pointer is reassigned in the next line

moveit_calibration_plugins/handeye_calibration_target/src/handeye_target_charuco.cpp

tylerjw · 2021-08-09T19:49:11Z

Please don't port without tests

AndyZe · 2021-08-10T12:58:13Z

Please don't port without tests

Disagree. Some port is better than no port at all. The first PR probably won't be perfect. I mean, it's 650 lines already.

We will test this eventually because it's needed on a project.

sjahr

Thanks for your work! I had issues with building this PR (Code did not compile). Maybe I did something wrong when I've tried to install this. Could you add some instructions in the PR description (or even the README)? Nonetheless, I've added a couple of comments with suggestions that I've received recently :)

sjahr · 2021-10-25T10:05:53Z

moveit_calibration_gui/CMakeLists.txt

+
+install(
+  TARGETS ${THIS_PACKAGE_LIBRARIES}
+  EXPORT export_${PROJECT_NAME}


I was recently advised by @tylerjw that this is more standard cmake than the export_ prefix

Suggested change

EXPORT export_${PROJECT_NAME}

EXPORT ${PROJECT_NAME}Targets

sjahr · 2021-10-25T10:12:21Z

moveit_calibration_gui/handeye_calibration_rviz_plugin/CMakeLists.txt

@@ -21,17 +19,26 @@ set(SOURCE_FILES
 )


I think you can remove the comment above that is related to catkin_lint

sjahr · 2021-10-25T10:13:32Z

moveit_calibration_gui/handeye_calibration_rviz_plugin/CMakeLists.txt

@@ -21,17 +19,26 @@ set(SOURCE_FILES
 )

 set(MOVEIT_LIB_NAME moveit_handeye_calibration_rviz_plugin)
-add_library(${MOVEIT_LIB_NAME}_core ${SOURCE_FILES} ${HEADERS})


Nit: Do you know why this is built with headers and source files instead of using only the source files and including + exporting the headers?

sjahr · 2021-10-25T10:20:30Z

...ibration_rviz_plugin/include/moveit/handeye_calibration_rviz_plugin/handeye_context_widget.h

@@ -48,21 +48,25 @@
 #include <QRadioButton>



Can you add a description?

sjahr · 2021-10-25T10:28:23Z

moveit_calibration_gui/handeye_calibration_rviz_plugin/src/handeye_calibration_frame.cpp

 #include <cmath>

 namespace moveit_rviz_plugin
 {
-HandEyeCalibrationFrame::HandEyeCalibrationFrame(HandEyeCalibrationDisplay* pdisplay, rviz::DisplayContext* context,
+static const rclcpp::Logger LOGGER = rclcpp::get_logger("handeye_calibration_frame");


Nit: Another way to do this would be to use an anonymous namespace instead

Suggested change

static const rclcpp::Logger LOGGER = rclcpp::get_logger("handeye_calibration_frame");

namespace {

const rclcpp::Logger LOGGER = rclcpp::get_logger("handeye_calibration_frame");

}

You can find the related discussion here

sjahr · 2021-10-25T10:56:34Z

...s/handeye_calibration_target/include/moveit/handeye_calibration_target/handeye_target_base.h

-    geometry_msgs::TransformStamped transform_stamped;
-    transform_stamped.header.stamp = ros::Time::now();
+    geometry_msgs::msg::TransformStamped transform_stamped;
+    transform_stamped.header.stamp = rclcpp::Clock(RCL_ROS_TIME).now(); //Not sure if this is the right approach


I guess this is still an open issue in ROS 2 FOXY to get the current time without a node . Maybe you can use the cpp API to get the current time

sjahr · 2021-10-25T11:04:04Z

...s/handeye_calibration_target/include/moveit/handeye_calibration_target/handeye_target_base.h

@@ -412,6 +420,7 @@ class HandEyeTargetBase
  }

 protected:
+  static const rclcpp::Logger LOGGER;


Does this need to be a member of this class? I think it would be better to just have one per source file like in the rest of this PR

sjahr · 2021-10-25T11:05:19Z

moveit_calibration_plugins/handeye_calibration_target/src/handeye_target_aruco.cpp

@@ -39,6 +39,9 @@
 namespace moveit_handeye_calibration
 {
 const std::string LOGNAME = "handeye_aruco_target";
+static const rclcpp::Logger LOGGER = rclcpp::get_logger(LOGNAME);


You could merge line 41 and 42 as LOGNAME is only used here

sjahr · 2021-10-25T11:06:10Z

moveit_calibration_plugins/handeye_calibration_target/src/handeye_target_charuco.cpp

@@ -38,7 +38,10 @@

 namespace moveit_handeye_calibration
 {
-const std::string LOGNAME = "handeye_charuco_target";
+const std::string LOGNAME = "moveit_calibration.plugins.handeye_charuco_target";
+static const rclcpp::Logger LOGGER = rclcpp::get_logger(LOGNAME);


Same as above

sjahr · 2021-10-25T11:07:42Z

moveit_calibration_plugins/package.xml

  <license>BSD</license>

  <url type="website">http://moveit.ros.org</url>
  <url type="bugtracker">https://github.com/ros-planning/moveit_calibration/issues</url>
  <url type="repository">https://github.com/ros-planning/moveit_calibration</url>

-  <buildtool_depend>catkin</buildtool_depend>
+  <buildtool_depend>ament_cmake</buildtool_depend>


Add moveit_common as build_depend

Fix tf visual tools, add image transport dependency

Signed-off-by: Andrej Orsula <orsula.andrej@gmail.com>

hamsadatta · 2023-07-17T11:45:03Z

@Abishalini @AndyZe
Hello, any updates on this? I am interested in using this package in one of our projects.

Abishalini added 2 commits August 4, 2021 13:41

Inital port of handeye_calibration_target

2e6b0af

handeye_calibration_target compiles

47b88ca

Abishalini changed the base branch from master to foxy August 4, 2021 19:58

Initial port of handeye_calibration_solver

8384400

Abishalini changed the title ~~ROS2 Port~~ [WIP] ROS2 Port Aug 4, 2021

Abishalini added 5 commits August 5, 2021 09:10

Initial port of moveit_calibration_gui

1c764fa

Fix CMakeLists for moveit_calibration_gui

1c63c60

Port headers in moveit_calibration_gui

e78726c

Initial port of source files

ed75892

Some more changes

772e87d

henningkayser reviewed Aug 9, 2021

View reviewed changes

Some more fixes

7ba71c5

Abishalini added 3 commits August 9, 2021 13:50

Remove LOGNAME

7a99b24

moveit_calibration_gui compiles without errors

f36358d

Initial port of tests

bcd3b4d

Loads display in RViz

a6cca4d

sjahr reviewed Oct 25, 2021

View reviewed changes

henningkayser mentioned this pull request Nov 2, 2021

Port to ROS 2 #99

Open

henningkayser linked an issue Nov 2, 2021 that may be closed by this pull request

Port to ROS 2 #99

Open

henningkayser assigned Abishalini Nov 2, 2021

Vatan Aksoy Tezer and others added 3 commits December 1, 2021 23:26

Add image transport dependency

668929b

Add node for tf_visual_tools

3f70f54

Merge pull request #1 from vatanaksoytezer/pr-fix_ros2

8b1d0c0

Fix tf visual tools, add image transport dependency

AndyZe mentioned this pull request Jan 28, 2022

[WIP] Begin porting handeye_calibration_target to ROS2 #100

Closed

AndrejOrsula mentioned this pull request Jun 1, 2022

WMD Track: Tutorial cleanup moveit/moveit2#1293

Closed

Refactor CMake config and fix exporting of pluginlib classes

1a81445

Signed-off-by: Andrej Orsula <orsula.andrej@gmail.com>

AndrejOrsula added 7 commits June 6, 2022 16:18

Fix compilation of tests (not yet functional)

968a750

Signed-off-by: Andrej Orsula <orsula.andrej@gmail.com>

Fix compilation of handeye_calibration_target as a shared library

e91de73

Signed-off-by: Andrej Orsula <orsula.andrej@gmail.com>

Use a unique rclcpp::logger in handeye_calibration_solver lib

1f64994

Signed-off-by: Andrej Orsula <orsula.andrej@gmail.com>

Fix moveit_calibration_gui for ROS 2

76727a2

Signed-off-by: Andrej Orsula <orsula.andrej@gmail.com>

Update CHANGELOGs

294ec4c

Signed-off-by: Andrej Orsula <orsula.andrej@gmail.com>

Update README with instructions

b1d24cb

Signed-off-by: Andrej Orsula <orsula.andrej@gmail.com>

Run pre-commit on all files

b8f2408

Signed-off-by: Andrej Orsula <orsula.andrej@gmail.com>

AndrejOrsula mentioned this pull request Jun 6, 2022

ROS 2 port & Gazebo demos #118

Open

Works on rolling

010310f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] ROS2 Port #101

[WIP] ROS2 Port #101

Abishalini commented Aug 4, 2021 •

edited

AndyZe commented Aug 4, 2021

henningkayser left a comment

henningkayser Aug 9, 2021

henningkayser Aug 9, 2021

henningkayser Aug 9, 2021

henningkayser Aug 9, 2021

henningkayser Aug 9, 2021

henningkayser Aug 9, 2021

henningkayser Aug 9, 2021

tylerjw commented Aug 9, 2021

AndyZe commented Aug 10, 2021

sjahr left a comment •

edited

sjahr Oct 25, 2021

sjahr Oct 25, 2021

sjahr Oct 25, 2021

sjahr Oct 25, 2021

sjahr Oct 25, 2021

sjahr Oct 25, 2021

sjahr Oct 25, 2021

sjahr Oct 25, 2021

sjahr Oct 25, 2021

sjahr Oct 25, 2021

hamsadatta commented Jul 17, 2023

[WIP] ROS2 Port #101

Are you sure you want to change the base?

[WIP] ROS2 Port #101

Conversation

Abishalini commented Aug 4, 2021 • edited

AndyZe commented Aug 4, 2021

henningkayser left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tylerjw commented Aug 9, 2021

AndyZe commented Aug 10, 2021

sjahr left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hamsadatta commented Jul 17, 2023

Abishalini commented Aug 4, 2021 •

edited

sjahr left a comment •

edited