replaced eigen_conversions with tf2_eigen in moveit_core #2472

petkovich · 2021-01-05T14:26:30Z

Description

Followed the tutorial and replaced eigen_conversions with tf2_eigen

Checklist

Required by CI: Code is auto formatted using clang-format
Extend the tutorials / documentation reference
Document API changes relevant to the user in the MIGRATION.md notes
Create tests, which fail without this PR reference
Include a screenshot if changing a GUI
While waiting for someone to review your request, please help review another open pull request to support the maintainers

welcome · 2021-01-05T14:26:32Z

Thanks for helping in improving MoveIt and open source robotics!

rhaschke

Thanks for this initial work. Could you, please, continue replacing eigen_conversions in all remaining packages as well (not only moveit_core)? I just pushed another commit removing eigen_conversions from various cmake files and package.xml (but did not yet adapted the actual conversion routines).
Note, that there are also many eigen_conversions in moveit_planners/pilz_industrial_motion_planner*.

codecov · 2021-01-05T16:26:37Z

Codecov Report

Merging #2472 (7c33bd0) into master (ea50801) will decrease coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #2472      +/-   ##
==========================================
- Coverage   60.30%   60.26%   -0.03%     
==========================================
  Files         351      351              
  Lines       26471    26475       +4     
==========================================
- Hits        15960    15953       -7     
- Misses      10511    10522      +11

Impacted Files	Coverage Δ
...anner/src/trajectory_blender_transition_window.cpp	`100.00% <ø> (ø)`
...strial_motion_planner/src/trajectory_generator.cpp	`100.00% <ø> (ø)`
..._motion_planner_testutils/cartesianconfiguration.h	`44.45% <ø> (ø)`
moveit_core/kinematic_constraints/src/utils.cpp	`30.04% <100.00%> (-0.50%)`	⬇️
...strial_motion_planner/src/trajectory_functions.cpp	`100.00% <100.00%> (ø)`
...l_motion_planner/src/trajectory_generator_circ.cpp	`100.00% <100.00%> (ø)`
...al_motion_planner/src/trajectory_generator_lin.cpp	`100.00% <100.00%> (ø)`
...al_motion_planner/src/trajectory_generator_ptp.cpp	`100.00% <100.00%> (ø)`
...n_planner_testutils/src/cartesianconfiguration.cpp	`63.83% <100.00%> (+1.33%)`	⬆️
moveit_core/robot_model/src/joint_model_group.cpp	`63.08% <0.00%> (-2.46%)`	⬇️
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4755105...24475ec. Read the comment docs.

gleichdick · 2021-01-05T23:48:53Z

Just for the record, this belongs to a first-timer-only issue I opened (#2437).
@petkovich thank you for contributing to this project. If you want to continue to replace eigen_conversions and friends (like Robert proposed) and have any questions, please don't hesitate to ask for help. I'm also happy to provide further patches for you to apply, if needed.

Basically, all methods in the tf namespace (quaternionEigenToMsg(), poseMsgToEigen(), transformKDLToEigen(), transformEigenToKDL(), vectorEigenToKDL(), poseMsgToEigen(), ...) have to be replaced. You can use tf2::convert() (defined in <tf2/convert.h>) for all of these functions. It takes two parameters, the first is the source and the second is the destination of the conversion. For all Eigen and/or KDL datatypes, you also have to include <tf2_eigen/tf2_eigen.h> resp. <tf2_kdl/tf2_kdl.h>. Please also make sure that the packages tf2_eigen and/or tf2_kdl are included in both package.xml and CMakeLists.txt.

So far, I found these methods in these files:

moveit_kinematics/ikfast_kinematics_plugin/templates/ikfast61_moveit_plugin_template.cpp
moveit_planners/ompl/ompl_interface/test/test_planning_context_manager.cpp
moveit_planners/pilz_industrial_motion_planner/src/trajectory_functions.cpp
moveit_planners/pilz_industrial_motion_planner/src/trajectory_generator_circ.cpp
moveit_planners/pilz_industrial_motion_planner/src/trajectory_generator_lin.cpp
moveit_planners/pilz_industrial_motion_planner_testutils/src/cartesianconfiguration.cpp
moveit_ros/planning_interface/test/move_group_interface_cpp_test.cpp
moveit_ros/planning_interface/test/subframes_test.cpp

I just used grep -r tf:: <path to moveit sources>.
Again, I'm happy to answer any questions and to supply further patches, if requested.

petkovich · 2021-01-06T19:59:11Z

No problem, @gleichdick. Are there any functionality tests or we just consider passed build as proof of well done migration? Also, is it convention to commit changes to the pull request multiple times (as one would on a normal coding project) or should it be done only once, when the whole issue is solved?

rhaschke · 2021-01-06T20:11:32Z

Thanks, for handling the remaining conversions as well. It's no problem, to push as many commits as you like to a PR branch.

petkovich · 2021-01-16T21:21:00Z

I have replaced
tf::transformKDLToEigen(trajectory.Pos(*time_iter), pose_sample);
with
tf2::convert(trajectory.Pos(*time_iter), pose_sample);
in moveit/moveit_planners/pilz_industrial_motion_planner/src/trajectory_functions.cpp

As result, my build fails with:

In file included from /opt/ros/noetic/include/tf2/convert.h:39,
from /home/tomislav/ws_moveit/src/moveit/moveit_planners/pilz_industrial_motion_planner/src/trajectory_functions.cpp:39:
/opt/ros/noetic/include/tf2/impl/convert.h: In instantiation of ‘static void tf2::impl::Converter<IS_MESSAGE_A, IS_MESSAGE_B>::convert(const A&, B&) [with A = KDL::Frame; B = Eigen::Transform<double, 3, 1>; bool IS_MESSAGE_A = false; bool IS_MESSAGE_B = false]’:
/opt/ros/noetic/include/tf2/convert.h:116:113: required from ‘void tf2::convert(const A&, B&) [with A = KDL::Frame; B = Eigen::Transform<double, 3, 1>]’
/home/tomislav/ws_moveit/src/moveit/moveit_planners/pilz_industrial_motion_planner/src/trajectory_functions.cpp:228:57: required from here
/opt/ros/noetic/include/tf2/impl/convert.h:83:16: error: ‘toMsg’ was not declared in this scope, and no declarations were found by argument-dependent lookup at the point of instantiation [-fpermissive]
83 | fromMsg(toMsg(a), b);
| ~~~~~^~~

Do you know what could be the cause of missed definition? As far as I can see, I have included all the relevant files as in moveit_planners/pilz_industrial_motion_planner_testutils/include/pilz_industrial_motion_planner_testutils/cartesianconfiguration.h, where tf2::convert raises no issue?
I have pushed my code in "toMsg_error" branch I hope that is ok?

gleichdick · 2021-01-17T23:20:03Z

Could you try to replace the tf2::convert() function with an explicit call to the conversion functions, this:

const geometry_msgs::Pose pose_msg = tf2::toMsg(trajectory.Pos(*time_iter));
tf2::fromMsg(pose_msg, pose_sample);

In this case, tf2::convert() sees that both parameters are not ROS message types and tries to deduce a message type (which is geometry_msgs::Pose). Currently, there are some issues in the tf2 package, AFAIK because of the mix of templated and non-templated functions and the header inclusion order.
PS. In general it's almost always a good idea to make separate branches, especially for helping other people reproduce (or fix) an error 😄

PPS. One small nitpick: In the header cartesianconfiguration.h, nothing (no types, no functions) from the tf2_eigen package is used so far. Only in the corresponding translation unit (cartesianconfiguration.cpp). To speed up compilation time and to tidy up the header file a bit, it'd be a good idea to move the #include <tf2_eigen/tf2_eigen.h> statement from the header into the .src file.

petkovich · 2021-01-18T10:46:13Z

@gleichdick thank you for your help. If I am not wrong, all tf:: occurrences are now replaced as well as eigen_conversions dependancies.

rhaschke

Thanks for this work. Let's see whether CI will be satisfied now.

rhaschke · 2021-01-18T11:49:49Z

moveit_planners/ompl/CMakeLists.txt

+  tf2
+  tf2_eigen


Are you sure tf2_eigen is required for building? This PR, only comprises changes to the testing code of OMPL.

I think tf2_ros is also not necessary

rhaschke · 2021-01-18T12:19:20Z

moveit_planners/pilz_industrial_motion_planner/CMakeLists.txt

@@ -8,7 +8,6 @@ add_definitions(-Wextra)
 add_definitions(-Wno-unused-parameter)

 find_package(catkin REQUIRED COMPONENTS
-  eigen_conversions
  kdl_conversions


What about kdl_conversions? Is this still needed, now that you use eigen_kdl?

gleichdick

Good work, you're almost done!

gleichdick · 2021-01-18T14:22:09Z

moveit_kinematics/ikfast_kinematics_plugin/templates/ikfast61_moveit_plugin_template.cpp

@@ -1387,7 +1386,7 @@ void IKFastKinematicsPlugin::transformToChainFrame(const geometry_msgs::Pose& ik
    if (base_transform_required_)
      ik_eigen_pose = chain_base_to_group_base_ * ik_eigen_pose;

-    tf::transformEigenToKDL(ik_eigen_pose, ik_pose_chain);
+    tf2::convert(ik_eigen_pose, ik_pose_chain);


Please use the explicit conversion (toMsg() / fromMsg()) for the same reason as before

gleichdick · 2021-01-18T14:34:12Z

moveit_planners/ompl/CMakeLists.txt

+  tf2
+  tf2_eigen


I think tf2_ros is also not necessary

gleichdick · 2021-01-18T16:17:01Z

This should fix the linking error in the CI:

--- a/moveit_planners/pilz_industrial_motion_planner/src/trajectory_generator_circ.cpp
+++ b/moveit_planners/pilz_industrial_motion_planner/src/trajectory_generator_circ.cpp
@@ -42,12 +42,12 @@
 #include <kdl/trajectory_segment.hpp>
 #include <kdl/utilities/error.h>
 #include <kdl/utilities/utility.h>
-#include <kdl_conversions/kdl_msg.h>
 #include <moveit/robot_state/conversions.h>
 #include <ros/ros.h>
 #include <tf2/convert.h>
 #include <tf2_eigen/tf2_eigen.h>
 #include <tf2_geometry_msgs/tf2_geometry_msgs.h>
+#include <tf2_kdl/tf2_kdl.h>
 
 namespace pilz_industrial_motion_planner
 {
@@ -222,9 +222,8 @@ std::unique_ptr<KDL::Path> TrajectoryGeneratorCIRC::setPathCIRC(const MotionPlan
   pose_msg = tf2::toMsg(info.goal_pose);
   tf2::fromMsg(pose_msg, goal_pose);
 
-  KDL::Vector path_point;
-  const geometry_msgs::Point point_msg = tf2::toMsg(info.circ_path_point.second);
-  tf2::fromMsg(point_msg, path_point);
+  const auto& eigen_path_point = info.circ_path_point.second;
+  const KDL::Vector path_point{ eigen_path_point.x(), eigen_path_point.y(), eigen_path_point.z() };
 
   // pass the ratio of translational by rotational velocity as equivalent radius
   // to get a trajectory with rotational speed, if no (or very little)
diff --git a/moveit_planners/pilz_industrial_motion_planner/src/trajectory_generator_lin.cpp b/moveit_planners/pilz_industrial_motion_planner/src/trajectory_generator_lin.cpp
index 67e5e939d..cd86b7b1c 100644
--- a/moveit_planners/pilz_industrial_motion_planner/src/trajectory_generator_lin.cpp
+++ b/moveit_planners/pilz_industrial_motion_planner/src/trajectory_generator_lin.cpp
@@ -45,6 +45,7 @@
 #include <kdl/trajectory_segment.hpp>
 #include <kdl/utilities/error.h>
 
+#include <tf2_kdl/tf2_kdl.h>
 #include <tf2/convert.h>
 #include <tf2_eigen/tf2_eigen.h>
 #include <tf2_geometry_msgs/tf2_geometry_msgs.h>

Unfortunately, one conversion function with KDL::Vector is missing, so we have do do it on our own...

petkovich · 2021-01-20T21:25:19Z

Is there any way to know whether CI will pass without pushing the code and waiting? Last few commits pass without warnings/errors locally but do not satisfy the CI check.

rhaschke · 2021-01-21T07:22:12Z

You can run the CI scripts locally as described here.
However, often it is more efficient to just look for the failing Travis command(s) and fix them, in your case, catkin_lint:
https://travis-ci.com/github/ros-planning/moveit/jobs/473550318

moveit_kinematics/ikfast_kinematics_plugin/templates/ikfast61_moveit_plugin_template.cpp

petkovich · 2021-01-25T11:58:19Z

It looks like the previous check passed, although, if I am correct, it did not at the time I have committed it. Is this PR now solved?

petkovich

Forgot to review them until now, fixed.

* Use tf2::toMsg / tf2::fromMsg where possible * Use tf2::convert when converting between two non-message types exception: conversion to/from KDL types: Due to a bad implementation of tf2_kdl (not using templates), tf2::convert cannot be used.

rhaschke

Thanks for your effort. I added another commit to even more unify usage of conversion functions:

Use tf2::toMsg / tf2::fromMsg where possible
Use tf2::convert when converting between two non-message types
exception: conversion to/from KDL types: Due to a bad implementation of tf2_kdl (not using templates), tf2::convert cannot be used.

Ready to merge if CI is happy.

welcome · 2021-02-17T12:07:46Z

Congrats on getting your first MoveIt pull request merged and improving open source robotics!

replaced eigen_conversions with tf2_eigen in moveit_core

7b6c025

petkovich requested review from rhaschke and v4hn as code owners January 5, 2021 14:26

Remove some more references to eigen_conversions

973e6b1

petkovich requested review from felixvd, gavanderhoorn, jrgnicho and mamoll as code owners January 5, 2021 16:02

rhaschke requested changes Jan 5, 2021

View reviewed changes

gleichdick mentioned this pull request Jan 9, 2021

Remove eigen_conversions from ompl JeroenDM/moveit#8

Merged

6 tasks

petkovich added 2 commits January 15, 2021 16:56

ikfast61 tf:: removed

fc15399

pilz_industrial_motion_planner_testutils replaced

6881e69

petkovich requested a review from jschleicher as a code owner January 16, 2021 18:47

petkovich added 3 commits January 18, 2021 11:31

pruned pilz_industrial_motion_planner/

84f60bd

removed eigen_conversions from moveit_planners

bb1776e

removed eigen_conversions from moveit_ros

1a90078

included tf2_eigen in ompl

57fe0ec

rhaschke reviewed Jan 18, 2021

View reviewed changes

gleichdick suggested changes Jan 18, 2021

View reviewed changes

petkovich added 2 commits January 20, 2021 10:58

removed dependancies and changed to explicit from/to msg in ikfast61

d5cff6d

linking dependancies, KDL::Vector conversion

a3676c2

removed build dep

2c96623

changed tf2_ros with tf2

e524ea1

gleichdick reviewed Jan 21, 2021

View reviewed changes

moveit_kinematics/ikfast_kinematics_plugin/templates/ikfast61_moveit_plugin_template.cpp Outdated Show resolved Hide resolved

fromMsg manual type in ifkast61

3bb7976

petkovich commented Jan 25, 2021

View reviewed changes

rhaschke approved these changes Jan 25, 2021

View reviewed changes

JeroenDM mentioned this pull request Jan 31, 2021

OMPL Constrained State Space Planning #2273

Open

6 tasks

Resolve conflicts with master

24475ec

rhaschke merged commit b7711c1 into moveit:master Feb 17, 2021

tylerjw mentioned this pull request Apr 9, 2021

Noetic Release 1.1.2 #2593

Closed

tylerjw mentioned this pull request Apr 29, 2021

Melodic Backports #2630

Merged

gleichdick mentioned this pull request Aug 4, 2021

Replace eigen_conversions with tf2_eigen in moveit_core #2437

Closed

8 tasks

sjahr pushed a commit to sjahr/moveit that referenced this pull request Jun 21, 2024

Fix typo in bullet function name (moveit#2472)

faa4795

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

replaced eigen_conversions with tf2_eigen in moveit_core #2472

replaced eigen_conversions with tf2_eigen in moveit_core #2472

petkovich commented Jan 5, 2021 •

edited

Loading

welcome bot commented Jan 5, 2021

rhaschke left a comment

codecov bot commented Jan 5, 2021 •

edited

Loading

gleichdick commented Jan 5, 2021

petkovich commented Jan 6, 2021

rhaschke commented Jan 6, 2021

petkovich commented Jan 16, 2021 •

edited

Loading

gleichdick commented Jan 17, 2021

petkovich commented Jan 18, 2021

rhaschke left a comment

rhaschke Jan 18, 2021

gleichdick Jan 18, 2021

rhaschke Jan 18, 2021

gleichdick left a comment

gleichdick Jan 18, 2021

gleichdick Jan 18, 2021

gleichdick commented Jan 18, 2021

petkovich commented Jan 20, 2021

rhaschke commented Jan 21, 2021

petkovich commented Jan 25, 2021

petkovich left a comment

rhaschke left a comment

welcome bot commented Feb 17, 2021

replaced eigen_conversions with tf2_eigen in moveit_core #2472

replaced eigen_conversions with tf2_eigen in moveit_core #2472

Conversation

petkovich commented Jan 5, 2021 • edited Loading

Description

Checklist

welcome bot commented Jan 5, 2021

rhaschke left a comment

Choose a reason for hiding this comment

codecov bot commented Jan 5, 2021 • edited Loading

Codecov Report

gleichdick commented Jan 5, 2021

petkovich commented Jan 6, 2021

rhaschke commented Jan 6, 2021

petkovich commented Jan 16, 2021 • edited Loading

gleichdick commented Jan 17, 2021

petkovich commented Jan 18, 2021

rhaschke left a comment

Choose a reason for hiding this comment

rhaschke Jan 18, 2021

Choose a reason for hiding this comment

gleichdick Jan 18, 2021

Choose a reason for hiding this comment

rhaschke Jan 18, 2021

Choose a reason for hiding this comment

gleichdick left a comment

Choose a reason for hiding this comment

gleichdick Jan 18, 2021

Choose a reason for hiding this comment

gleichdick Jan 18, 2021

Choose a reason for hiding this comment

gleichdick commented Jan 18, 2021

petkovich commented Jan 20, 2021

rhaschke commented Jan 21, 2021

petkovich commented Jan 25, 2021

petkovich left a comment

Choose a reason for hiding this comment

rhaschke left a comment

Choose a reason for hiding this comment

welcome bot commented Feb 17, 2021

petkovich commented Jan 5, 2021 •

edited

Loading

codecov bot commented Jan 5, 2021 •

edited

Loading

petkovich commented Jan 16, 2021 •

edited

Loading