missing symbol on convert(tf2::Quaternion, Eigen::Quaternion)

[gleichdick]

I build moveit and geometry2 from source, importing moveit_commander python module throws an ImportError due to missing symbol. See ros-planning/moveit#1785. Reverting #367 fixes the issue.

With HEAD pointed to cd55e45, following snippet (extracted from here) builds fine:

#include <tf2/LinearMath/Quaternion.h>
#include <Eigen/Geometry>
#include <tf2_geometry_msgs/tf2_geometry_msgs.h>
#include <tf2_eigen/tf2_eigen.h>
#include <iostream>

int main() {
  Eigen::Quaterniond eq;
  const tf2::Quaternion tq(0,1,0,1);

  tf2::convert(tq, eq);
  std::cout << eq.toRotationMatrix() << '\n';
}

With current melodic-devel (#367 applied, 1cbda1e14), the symbol is also missing:

$ g++ -I/usr/include/eigen3/ -I/ws_moveit/install/include/ -I/opt/ros/melodic/include/ test.cpp 
/tmp/ccQipwpW.o: In function `void tf2::impl::Converter<false, false>::convert<tf2::Quaternion, Eigen::Quaternion<double, 0> >(tf2::Quaternion const&, Eigen::Quaternion<double, 0>&)':
test.cpp:(.text._ZN3tf24impl9ConverterILb0ELb0EE7convertINS_10QuaternionEN5Eigen10QuaternionIdLi0EEEEEvRKT_RT0_[_ZN3tf24impl9ConverterILb0ELb0EE7convertINS_10QuaternionEN5Eigen10QuaternionIdLi0EEEEEvRKT_RT0_]+0x41): undefined reference to `void tf2::fromMsg<geometry_msgs::Quaternion_<std::allocator<void> >, Eigen::Quaternion<double, 0> >(geometry_msgs::Quaternion_<std::allocator<void> > const&, Eigen::Quaternion<double, 0>&)'
collect2: error: ld returned 1 exit status

The function is declared in tf2_geometry_msgs.h as inline, but not template<>. If I got it right, the declaration order is:

• template<class A, class B> fromMsg(const A&, B&);
• template<class A, class B> void convert(const A&, B&) {...}
• inline void fromMsg(const geometry_msgs::Quaternion &, tf2::Quaternion&) {...}

So, in impl::convert the correct overload is not found because it is defined as a stand-alone function afterwards (I guess).

When template<> is added to the fromMsg overloads, the snippet compiles.

[rhaschke]

Hint: If you can provide a PR (and I believe you can based on your analysis of the issue), the issue has higher chances to be fixed soon.

[gleichdick]

Well, this whole template stuff isn't that easy. I tried adding template<> on each fromMsg() and toMsg() overload (in general the bug also applies to toMsg), but that does not work because one template parameter is the return value toMsg (return values are not taken into account for template parameter deduction). GCC complains:

   out.orientation = toMsg(in.getRotation());
                                           ^
In file included from /ws_moveit/install/include/tf2/convert.h:35:0,
                 from /ws_moveit/install/include/tf2_geometry_msgs/tf2_geometry_msgs.h:35,
                 from test.cpp:3:
/ws_moveit/install/include/tf2/transform_functions.h:90:5: note: candidate: template<class A, class B> B tf2::toMsg(const A&)
   B toMsg(const A& a);
     ^~~~~
/ws_moveit/install/include/tf2/transform_functions.h:90:5: note:   template argument deduction/substitution failed:
In file included from test.cpp:3:0:
/ws_moveit/install/include/tf2_geometry_msgs/tf2_geometry_msgs.h:427:43: note:   couldn't deduce template parameter ‘B’
   out.orientation = toMsg(in.getRotation());
                                           ^

possible solutions:

• rearrange header includes in tf2_geometry_msgs.h
• add forward declarations before includes in tf2_geometry_msgs.h
• template wrappers introduced in #254 like https://github.com/ros/geometry2/blob/1cbda1e145a1214920c9448ffd8b753bf866df67/tf2_geometry_msgs/include/tf2_geometry_msgs/tf2_geometry_msgs.h#L384-L395
• template<> everything

1 and 2 still rely on the correct order of header includes (tf2_geometry_msgs.h before tf2/convert.h), so these options aren't bullet proof. Number 3 clutters up the headers but keeps backward compatibility and does not rely on the include order. Number 4 breaks everything, as toMsg has to be specified manually on each call. Maybe in the next release, the signature could be changed so that every parameter could be deducted.

[gleichdick]

433 is ready for a review, downstream projects have to update their A tf2::toMsg(const B&) calls to either A& tf2::toMsg(const B&, A&) or tf2::convert(const B&, A&). The latter one is backwards compatible (except for three-way conversions).

@rhaschke moveit melodic-devel compiles with these changes (with moveit_visual_tools updated as well). Should I open a PR for it to make a review easier?