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

Merge together the indigo, jade, and kinetic branches #443

Merged
merged 164 commits into from Jun 15, 2017

Conversation

pjreed
Copy link
Contributor

@pjreed pjreed commented May 9, 2017

Maintaining three different branches for different ROS distributions is a pain and also unnecessary, so I've merged all of them together. This branch builds and all tests pass on indigo, jade, kinetic, and lunar. Don't look too hard at that commit list, it's ugly.

This PR targets the indigo-devel branch, but after it is merged, we should rename that branch to master, lock down all of the distro-specific branches, and then release a new version (0.3.0) to all distros.

Not very much needed to be changed to make everything compatible, but some notable things are:

  1. Packages that use OpenCV (swri_geometry_util, swri_image_util, swri_opencv_util, and swri_transform_util) have a rosdep on cv_bridge and don't specify a version when calling find_package in their CMakeLists.txt; this means they'll use OpenCV 2 on Indigo and Jade and OpenCV 3 on Kinetic and Lunar.
  2. swri_image_util depends on Qt 5.
  3. Nodes and utilities in swri_transform_util previously used the gps_common/GPSFix message on Indigo and sensor_msgs/NavSatFix (or Pose, or GeoPose, or whatever else was appropriate) on Jade and later. Now they will all handle all of them. This is a little ugly in initialize_origin.py because it was basically completely rewritten for NavSatFix support. The right thing to do would be to rewrite it again to handle every case gracefully, but that's a bit beyond the scope of this PR, so instead it just detects whether your ${ROS_DISTRO} is "indigo" or not and changes behavior appropriately.

Edward Venator and others added 30 commits September 28, 2015 16:29
The gps_common package was removed in ROS Jade, so a different message
type is needed for the local XY origin message. (Issue swri-robotics#246).

This replaces the gps_common/GPSFix message with a
geometry_msgs/PoseStamped message. The latitude is stored in
pose.position.y, the longitude is stored in pose.position.x, and the
altitude is stored in pose.position.z. As before, the local xy frame is
fixed in rotation such that the Z axis points away from the center of
the Earth and the Y axis points north. However, the choice of
geometry_msgs/PoseStamped allows for headings to be added in the future.
Mark single argument constructor explicit.
The main problem was that reshape was being incorrectly, causing the
points to get shuffled around.  Once that was fixed, it was clear that
the rotation should not be inverted.  Also added a comment to clarify
the significance of the returned transform.
 * The reference heading has been renamed to reference angle.
 * It's not recommended to set a non-zero reference angle.
 * A parameter is provided to ignore the reference heading for backwards compatibility.

Conflicts:
	swri_transform_util/src/local_xy_util.cpp
pjreed and others added 6 commits April 20, 2017 17:44
swri_geometry_util used OpenCV but didn't declare its dependency.
…y-kinetic

Add OpenCV dependency (Kinetic)
* Enable blending with transparency mask

Instead of blending an entire image, mask out portions that are a
particular color (e.g., for a black-and-white top image, only blend the
white portions, so the black portions don't darken the entire image).

Adds three parameters to the swri_image_util blend_images_nodelet (mask
red, green, and blue components).

* Remove old debug code

* Remove unnecessary include
@evenator
Copy link
Contributor

I recommend we soak test this in SwRI-owned vehicles for a while before merging. A major change to marti_common affects a lot of projects. We should also verify that the kinetic packages still work as expected. I'm not aware of any projects using Jade, so I'm not too concerned about that.

${YamlCpp_INCLUDE_DIR}
${catkin_INCLUDE_DIRS}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a rule, I prefer to include in the order:

  1. package-local
  2. catkin
  3. system

I do this on the principal that a catkin workspace overlay should take precedence over system packages.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. Both swri_yaml_util and swri_transform_util didn't have ${catkin_INCLUDE_DIRS} second, so I fixed them.

@pjreed
Copy link
Contributor Author

pjreed commented May 17, 2017

I found one hitch on Indigo; Mapviz uses swri_image_util, which means that if swri_image_util is switched to Qt 5, Mapviz would need to use Qt 5, and anything that provides a plugin for Mapviz would also need to switch. In addition, anything that links in OpenCV's highgui module would also require Qt 4.

It's easy to make swri_image_util switch which version of Qt it links in, but because there's no way in package.xml to make dependencies conditional, that means a cross-distro compatible version has to say it depends on both Qt 4 and 5. It doesn't hurt anything, but it does mean you'd end up installing a version of Qt you don't need.

@@ -20,6 +20,8 @@
<depend>geometry_msgs</depend>
<depend>image_geometry</depend>
<depend>image_transport</depend>
<depend>libqt4-opengl-dev</depend>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will having both qt4 and qt5 as deps cause a conflict?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They don't conflict at all, and in fact work fine when installed side-by-side. It's mostly just annoying that rosdep is guaranteed to install a version of Qt that you may not be using at all.

It was only being used to find the intersection of two polygons, and
swri_geometry_util can be used for that instead.
@pjreed
Copy link
Contributor Author

pjreed commented May 17, 2017

Easier solution: remove Qt entirely. swri_image_util was only using its QPolygonF class to find the intersection of two polygons, but swri_geometry_util can do that even more easily, so Qt is entirely unnecessary in marti_common. All unit tests pass.

Copy link
Contributor

@elliotjo elliotjo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this a great idea. We need a documented process on how to do future updates to avoid diverging again in the future.

@evenator
Copy link
Contributor

I'm going to set a target to get the necessary tests in on this next week, with the goal to merge by June 2.

@elliotjo
Copy link
Contributor

elliotjo commented Jun 7, 2017

What tests are needed? I would love to get this merged as soon as possible.

@evenator
Copy link
Contributor

evenator commented Jun 8, 2017

This totally got pushed off my plate. Sorry about that. @pjreed tested it in Kinetic. I don't care about Jade, but it needs to be tested for Indigo. I'd hate to see a major government client 😉 have problems.

@nick-alton
Copy link
Contributor

Tested on vehicle, there seems to be an issue with initialize_origin.py in swri_transform_util. When it gets to line 118 it complains that global name '_gps_fix' is not defined.

Initialize `_gps_fix` to `None`

As reported by @nick-alton in swri-robotics#443, initialize_origin.py would crash with a `NameError` on line 118 because `_gps_fix` is undefined. `_gps_fix` was previously defined and initialized as `None`
@evenator
Copy link
Contributor

evenator commented Jun 9, 2017

Ok, @nick-alton, try it again now.

@evenator evenator merged commit 2162ca9 into swri-robotics:indigo-devel Jun 15, 2017
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.

None yet

9 participants