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

Building melodic-devel on ROS Kinetic #1090

Merged

Conversation

Projects
None yet
3 participants
@IanTheEngineer
Copy link
Member

commented Oct 16, 2018

Background

As part of MoveIt's tf2 migration for melodic in #830, there were upstream changes required to RViz, tf1, and tf2. The changes to RViz and tf1 were not backwards compatible with Kinetic.

The moveit maintainers have a desire to have MoveIt compile on both ROS Kinetic and Melodic with the same branch, summed up in this ticket: #925 . The final step in #925 is to find an acceptable way to maintain tf2 buffers for interacting with RViz. In Kinetic, RViz doesn't easily expose its internal and protected tf2 buffers. I tried several ways to get at these buffers in Kinetic as described ros-visualization/rviz#1215, without any success.

Description

In the end, for Kinetic, it is simplest to have MoveIt's RViz plugin to just spin up a new tf2_ros::TransformListener and tf2_ros::Buffer when it would normally depend on RViz' internal TF Buffer. This is not necessary in ROS Melodic, as the buffers are properly exposed from RViz' API. That brings us to the conditional compile. As @davetcoleman mentioned, it might be acceptable to use ugly compile defs to build Kinetic and Melodic on the same branch. Despite them being ugly, that was the simplest and most run-time efficient solution: melodic-devel would not use extra tf2 listeners/buffers when built in a Melodic environment, and would compile and run in a Kinetic environment. I am using roscpp's release versioning to check for a "pre-Melodic" environment. Anything prior to release 1.14.x should fall into this category.

This code needs more testing in a Kinetic environment, but I figured I'd get it out there now to have eyeballs on it sooner rather than later. Everything builds on Melodic & Kinetic I think I had MoveIt/RViz running in a Kinetic docker container last night, but I didn't see any interactive markers. I'll continue to investigate and update this PR accordingly.

Checklist

  • Required by CI: Code is auto formatted using clang-format
  • Extended the tutorials / documentation, if necessary reference
  • (?) Decide if this should be cherry-picked to other current ROS branches
@rhaschke
Copy link
Collaborator

left a comment

In general this looks promising.
To avoid having two additional tf buffers in rviz, could you please make the MotionPlanningFrame reuse the tf buffer from the associated MotionPlanningDisplay (which inherits from PlanningSceneDisplay)?

Even better, you could go for singleton instances of these two new variables shared between all (potentially multiple) MoveIt display instances in rviz. Except from providing accessor functions, e.g. in planning_scene_display.cpp you don't need to touch existing (class) API.

rhaschke added some commits Oct 16, 2018

@davetcoleman
Copy link
Member

left a comment

Love it!

@@ -1,3 +1,7 @@
if(roscpp_VERSION VERSION_LESS 1.14) # Pre-Melodic release. REMOVE when Kinetic support is dropped

This comment has been minimized.

Copy link
@davetcoleman

davetcoleman Oct 17, 2018

Member
Suggested change
if(roscpp_VERSION VERSION_LESS 1.14) # Pre-Melodic release. REMOVE when Kinetic support is dropped
if(roscpp_VERSION VERSION_LESS 1.14) # Pre-Melodic release. TODO: Remove when Kinetic support is dropped.
@@ -1,5 +1,10 @@

set(MOVEIT_LIB_NAME moveit_planning_scene_rviz_plugin)

if(roscpp_VERSION VERSION_LESS 1.14) # Pre-Melodic release. REMOVE when Kinetic support is dropped

This comment has been minimized.

Copy link
@davetcoleman

davetcoleman Oct 17, 2018

Member
Suggested change
if(roscpp_VERSION VERSION_LESS 1.14) # Pre-Melodic release. REMOVE when Kinetic support is dropped
if(roscpp_VERSION VERSION_LESS 1.14) # Pre-Melodic release. TODO: Remove when Kinetic support is dropped
@IanTheEngineer

This comment has been minimized.

Copy link
Member Author

commented Oct 18, 2018

Thanks for the feedback and cleanup @rhaschke & @davetcoleman. Robert, could you expand upon your second recommendation here?

Even better, you could go for singleton instances of these two new variables shared between all (potentially multiple) MoveIt display instances in rviz. Except from providing accessor functions, e.g. in planning_scene_display.cpp you don't need to touch existing (class) API.

I'm not certain I know what that would look like in this context. Instead, I implemented what I understood to be your first recommendation. The downside is that I added a public function to the API to access at the internal tf2 buffer. Was that what you intended?

@davetcoleman

This comment has been minimized.

Copy link
Member

commented Oct 18, 2018

I tested this locally on Ubuntu 16.04 and it works! In celebration:
ros-planning/moveit.ros.org#205

note failed clang

@rhaschke

This comment has been minimized.

Copy link
Collaborator

commented Oct 18, 2018

@IanTheEngineer I committed the proposed singleton usage. Please approve and eventually merge.

@IanTheEngineer
Copy link
Member Author

left a comment

@rhaschke Github claims I'm not allowed to "Approve" of my own pull request, but I approve the changes you made :) Thanks for that! I've got small suggestions for which I'll push one more commit.

Show resolved Hide resolved .../visualization/motion_planning_rviz_plugin/src/motion_planning_frame.cpp
Show resolved Hide resolved .../visualization/planning_scene_rviz_plugin/src/planning_scene_display.cpp
@@ -46,6 +46,10 @@
#include <ros/ros.h>
#endif

#ifdef ROS_KINETIC
#include <tf2_ros/transform_listener.h>

This comment has been minimized.

Copy link
@IanTheEngineer

IanTheEngineer Oct 18, 2018

Author Member

This should now be

Suggested change
#include <tf2_ros/transform_listener.h>
#include <tf2_ros/buffer.h>

as the listener declaration is gone.

@IanTheEngineer

This comment has been minimized.

Copy link
Member Author

commented Oct 18, 2018

Ok! I think this PR is in good shape. Everything builds in Kinetic & Melodic environments, tf2_ros::Buffers are handled as efficiently as possible in Kinetic and Melodic, and we have some approvals. I would request one more set testing in a non-Docker Kinetic environment, just to make sure MoveIt/RViz is working as expected, but then we can squash-merge this guy and call it a day :) Thanks for the reviews!

@rhaschke rhaschke referenced this pull request Oct 21, 2018

Closed

2018 Fall Release Kinetic and Melodic #1083

11 of 11 tasks complete
@IanTheEngineer

This comment has been minimized.

Copy link
Member Author

commented Oct 22, 2018

Ok, I believe this PR is ready to squash-merge. @davetcoleman @rhaschke would you like to do the merge or should I?

@rhaschke

This comment has been minimized.

Copy link
Collaborator

commented Oct 22, 2018

@IanTheEngineer I didn't merged this yet, because you said you want to give it a real-world test in a non-docker Kinetic environment. If you did that successfully, we can squash-merge indeed.

@rhaschke rhaschke merged commit e32bb6c into ros-planning:melodic-devel Oct 22, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.