Maintain & expose tf2 Buffer in shared_ptr for tf

ros / geometry

Packages for common geometric calculations including the ROS transform library, "tf". Also includes ROS bindings for "bullet" physics engine and "kdl" kinematics/dynamics package.

173 stars 274 forks source link

Maintain & expose tf2 Buffer in shared_ptr for tf #163

Closed IanTheEngineer closed 6 years ago

IanTheEngineer commented 6 years ago

This PR is an implementation of the Melodic API discussed in https://github.com/ros-visualization/rviz/issues/1215#issuecomment-385124873 which would make the RViz PR https://github.com/ros-visualization/rviz/pull/1224 obsolete. With this change added to tf, no (or minimal) modifications to rviz are necessary in order to accomplish tf2 migration for Moveit

Adds a tf2_ros::Buffer via a public accessor method to expose to customers of Transformer
Maintains the tf2_ros::Buffer in a shared_ptr to safely share access to the Buffer object
As this is targeting Melodic, adds c++11 compile flags to grant access to std::shared_ptr's
Reorders the include_directories in the CMakeLists to ensure the headers exposed in this package are read before the system installed catkin_INCLUDE_DIRS (otherwise changes to tf source headers are never detected during a catkin_make on a system with ros-*-geometry installed)

tfoote commented 6 years ago

Thanks this looks good. I'm going to need to fork and retarget this for melodic. I'll do that shortly and get a release out.

wjwwood commented 6 years ago

Thanks @IanTheEngineer, I was just about to push on this myself, do you need help with the rviz integration or is that already in flight on your end?

wjwwood commented 6 years ago

Or I guess you don't need to change rviz now...

IanTheEngineer commented 6 years ago

@tfoote and @wjwwood do you see any way this PR can be backported to Kinetic? The other MoveIt Maintainers have requested that that our melodic-devel branch builds on ROS Kinetic.

This commit doesn't break API, but it does introduce the usage of C++11 shared_ptrs and likely breaks ABI with the restructuring of how the tf2 buffer is maintained. With that said, backporting this change to Kinetic would be incredibly helpful in maintaining these two MoveIt branches. Your input would be appreciated here.

tfoote commented 6 years ago

This definitely breaks the ABI due to memory layout changes. The c++11 isms would be ok in Kinetic if used internally, but this is also in the public interface so it would have to export c++11 which we chose not to do in Kinetic. And there's also not a fork for separate indigo and kinetic development yet. With all that combined I'm generally not in favor of backporting this to kinetic. Melodic and Kinetic are targeting two different LTS Ubuntus, which is something that we have learned is generally quite hard to do with a single code base.