Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Maintain & expose tf2 Buffer in shared_ptr for tf #163

Merged
merged 1 commit into from
May 2, 2018

Conversation

IanTheEngineer
Copy link
Contributor

@IanTheEngineer IanTheEngineer commented May 1, 2018

This PR is an implementation of the Melodic API discussed in ros-visualization/rviz#1215 (comment) which would make the RViz PR ros-visualization/rviz#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)

- 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
Copy link
Member

tfoote commented May 2, 2018

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.

@tfoote tfoote changed the base branch from indigo-devel to melodic-devel May 2, 2018 22:59
@tfoote tfoote merged commit 42995de into ros:melodic-devel May 2, 2018
IanTheEngineer added a commit to IanTheEngineer/moveit that referenced this pull request May 3, 2018
Based on the new API changes to the TF
Transformer Library, that allows for access
to a shared pointer into the internal
tf2_ros::Buffer object:

ros/geometry#163
IanTheEngineer added a commit to IanTheEngineer/moveit that referenced this pull request May 3, 2018
Based on the new API changes to the TF
Transformer Library, that allows for access
to a shared pointer into the internal
tf2_ros::Buffer object:

ros/geometry#163
@wjwwood
Copy link
Member

wjwwood commented May 3, 2018

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
Copy link
Member

wjwwood commented May 3, 2018

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

moriarty added a commit to moriarty/navigation that referenced this pull request May 14, 2018
Change required due to changes in upstream dependencies:
ros/geometry#163: "Maintain & expose tf2 Buffer in shared_ptr for tf"

fixes ros-planning#717 (for compile errors at least.)
moriarty added a commit to moriarty/navigation that referenced this pull request May 14, 2018
Change required due to changes in upstream dependencies:
ros/geometry#163: "Maintain & expose tf2 Buffer in shared_ptr for tf"

fixes ros-planning#717 (for compile errors at least.)
IanTheEngineer added a commit to IanTheEngineer/moveit that referenced this pull request May 16, 2018
Based on the new API changes to the TF
Transformer Library, that allows for access
to a shared pointer into the internal
tf2_ros::Buffer object:

ros/geometry#163
IanTheEngineer added a commit to IanTheEngineer/moveit that referenced this pull request May 17, 2018
- All type conversions now depend on geometry2 ROS packages, rather
  than geometry (see ros/geometry2#292 and
  ros/geometry2#294 for details of the new
  conversions)
- Removes all boost::shared_ptr<tf::TransformListener> from the API,
  and replaced them with std::shared_ptr<tf2_ros::Buffer>'s
- Piped tf2_ros::Buffer's everywhere where tf::TransformListeners were
  used
- Utilizes the new tf2 API in the tf::Transformer library to access
  the internal tf2::Buffer object used by RViz
  (see ros/geometry#163 for details of the
  new API)
- Removes the prepending of forward slashes ('/') for Transforms frames
  as this is deprecated in tf2
rhaschke pushed a commit to moveit/moveit that referenced this pull request May 18, 2018
migration from tf to tf2 API, resolves issue #745

- All type conversions now depend on geometry2 ROS packages, rather than geometry
  (see ros/geometry2#292 and
  ros/geometry2#294 for details of the new conversions)
- Removes all boost::shared_ptr<tf::TransformListener> from the API,
  and replaced them with std::shared_ptr<tf2_ros::Buffer>'s
- Utilize new tf2 API in the tf::Transformer library to access the internal tf2::Buffer of RViz
  (see ros/geometry#163 for details of the new API)
- Removes prepending of forward slashes ('/') for transforms frames as this is deprecated in tf2
- Replaced deprecated tf2 _getLatestCommonTime
@IanTheEngineer
Copy link
Contributor Author

@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
Copy link
Member

tfoote commented Jul 26, 2018

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants