Minor fixes for Cartographer from ROS #1705

clalancette · 2020-06-17T13:29:14Z

This PR is a "port" of the Cartographer code to ROS 2. I put the words port in quotes because the port doesn't require very much; it is mostly changes to compile on Ubuntu 20.04, plus some changes to the package.xml to make it a pure cmake package rather than a catkin one.

As-is, this PR doesn't apply to master. That's because this "port" was originally done against the 1.0.0 tag of this repository. However, given the rather simple nature of this PR, it should be pretty easy to move forward.

I'm opening this PR as a starting point for the discussion on how to move this code into the upstream. Some steps that seem obvious to me:

~~Update this to apply against master.~~ This is now done.
~~Create a ros2 branch for this to target~~ We decided to try to merge this all into master.

Some steps that are less obvious, and/or need discussion:

~~Do we want to try to support ROS 1 and ROS 2 on the same branch? It might be possible here, but I'm not sure.~~ We decided we do.
Is the cartographer project willing to take over maintenance of this branch and release it into newer ROS 2 versions as they come along?

Finally, I'll note that I don't have too much of time to devote to this. I'm doing this on discretionary time, so while I'm glad to do some simple work here, I won't have the time to do any major refactoring.

Comments, questions, thoughts all welcome!

This was originally PR #1701, but I had to close that one and move it to a new source branch. Below are comments from that PR.

@wohe said:

@clalancette I opened #1704 for the remaining focal related fixes. Some of the changes in this PR have already been fixed. I'll send a PR which adds focal to CI later.

Since this is the library without a dependency on ROS it should be possible to have one version for both ROS 1 and 2. The only ROS related file is the package.xml which to my understanding allows configuring dependencies differently based on the ROS version. Should we do this? Can you have a look? I dont't think anything would speak against merging a PR which just fixes up the package.xml for ROS 2 support.

@clalancette said:

@clalancette I opened #1704 for the remaining focal related fixes. Some of the changes in this PR have already been fixed. I'll send a PR which adds focal to CI later.

Great, thanks!

Since this is the library without a dependency on ROS it should be possible to have one version for both ROS 1 and 2. The only ROS related file is the package.xml which to my understanding allows configuring dependencies differently based on the ROS version. Should we do this? Can you have a look? I dont't think anything would speak against merging a PR which just fixes up the package.xml for ROS 2 support.

Yep, in theory we should be able to do that. I'm going to rebase this on top of master, and then see if I can get that working.

googlebot · 2020-06-17T13:29:20Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

clalancette · 2020-06-17T13:30:29Z

I've now rebased this on top of master, and done some additional fixes to make this compatible with both ROS 1 and ROS 2. At this point, I think it is ready for review.

wohe · 2020-06-18T12:47:24Z

Could you rebase again? I added some changes for Ubuntu 20.04 support in #1706 and it is now also included in CI.

clalancette · 2020-06-18T13:07:23Z

Rebased again now.

cmake/functions.cmake

package.xml

MichaelGrupp · 2020-07-09T09:01:13Z

Can we just use python3-sphinx in all cases? It's available also for previous Ubuntu distros and we just need its executable as a build dependency for documentation. It doesn't interfere with the actual ROS stuff in Cartographer.

clalancette · 2020-07-09T14:11:37Z

Can we just use python3-sphinx in all cases? It's available also for previous Ubuntu distros and we just need its executable as a build dependency for documentation. It doesn't interfere with the actual ROS stuff in Cartographer.

Sure, that works for me. See e1e5212

MichaelGrupp · 2020-07-09T15:54:38Z

cmake/modules/FindGMock.cmake

@@ -77,6 +77,7 @@ if(NOT GMock_FOUND)
          EXCLUDE_FROM_ALL)
      endif()
      set(GMOCK_LIBRARIES gmock_main)
+      set(GMOCK_INCLUDE_DIRS ${GMOCK_SRC_DIR}/include)


+1
I'm working on migrating our fork to Noetic, this was needed there too.

Did you try installing the libgmock-dev package? I added Ubuntu Focal (which Noetic is primarily targeted at) to CI recently and it passes.

So installing libgmock-dev does make it work. But I actually still think this is a bug, and installing libgmock-dev just works around it.

When you install libgmock-dev, it means you actually don't come in the if(NOT GMOCK_LIBRARIES) block at all, and hence don't build gmock from source. If for some reason you do need to build gmock from source, then this block still doesn't export the GMOCK_INCLUDE_DIRS properly.

Thus, I think we should either keep this fix, or we should remove the fallback to build from source completely.

cmake/functions.cmake

wohe · 2020-07-15T15:10:31Z

cmake/modules/FindGMock.cmake

@@ -77,6 +77,7 @@ if(NOT GMock_FOUND)
          EXCLUDE_FROM_ALL)
      endif()
      set(GMOCK_LIBRARIES gmock_main)
+      set(GMOCK_INCLUDE_DIRS ${GMOCK_SRC_DIR}/include)


Did you try installing the libgmock-dev package? I added Ubuntu Focal (which Noetic is primarily targeted at) to CI recently and it passes.

package.xml

This changes cartographer_ros to no longer build Ceres and instead uses libceres-dev for which a suitable version is provided by ROS for Kinetic. Related to cartographer-project#1705. Signed-off-by: Wolfgang Hess <[email protected]>

This changes cartographer_ros to no longer build Ceres and instead uses libceres-dev for which a suitable version is provided by ROS for Kinetic. Related to #1705. Signed-off-by: Wolfgang Hess <[email protected]>

clalancette · 2020-09-24T19:37:43Z

Thanks for pushing forward with this. I've now rebased this branch. This still has a few fixes that would be good to get in, so I'll still suggest that we look into merging this as well.

clalancette · 2020-10-12T15:03:22Z

Friendly ping on this one; can we get the fixes in this PR reviewed? Thanks.

MichaelGrupp

So, we now have 3 lines left to review. One big discussion about libceres-dev is already resolved. Everything else looks fine to me, so I would prefer to merge. Since he was more involved into the discussion, I would ask @wohe to take a look please.

MichaelGrupp · 2020-10-23T09:52:11Z

Please rebase / update branch and then let's get this merged. This patch is also needed for users that are building ROS1 from source, see my experiment in cartographer-project/cartographer_ros#1531. As we can see from the reply I got there it really affects users.

So unless @wohe has strong objections I would proceed to get things fixed.

wohe · 2020-10-23T11:44:17Z

@MichaelGrupp I will have a look today. We need to make sure that this does not break anything ROS1 related.

* restrict boost dependencies to the ones used Signed-off-by: Mikael Arguedas <[email protected]>

Signed-off-by: Chris Lalancette <[email protected]>

clalancette · 2020-10-23T12:24:40Z

Please rebase / update branch and then let's get this merged. This patch is also needed for users that are building ROS1 from source, see my experiment in cartographer-project/cartographer_ros#1531. As we can see from the reply I got there it really affects users.

I've rebased onto the latest.

@MichaelGrupp I will have a look today. We need to make sure that this does not break anything ROS1 related.

Agreed. I think that CI should show this, however.

wohe · 2020-10-23T13:11:43Z

@clalancette Unfortunately, CI for this repo is unaffected by the file you changed. All ROS related code is in cartographer_ros. There, CI of course would fail when we introduce issues, but only after we merged the PR. I now ran this CI manually with your branch and it all succeeded.

clalancette · 2020-10-23T15:20:53Z

@clalancette Unfortunately, CI for this repo is unaffected by the file you changed. All ROS related code is in cartographer_ros. There, CI of course would fail when we introduce issues, but only after we merged the PR. I now ran this CI manually with your branch and it all succeeded.

Ah, good call. Thanks for kicking off the tests and merging!

googlebot added the cla: no label Jun 17, 2020

clalancette mentioned this pull request Jun 17, 2020

ROS 2 Cartographer port #1701

Closed

clalancette force-pushed the ros2-upstream branch from 2be33aa to 330a947 Compare June 18, 2020 13:07

wohe reviewed Jun 18, 2020

View reviewed changes

cmake/functions.cmake Outdated Show resolved Hide resolved

MichaelGrupp reviewed Jun 25, 2020

View reviewed changes

package.xml Outdated Show resolved Hide resolved

MichaelGrupp approved these changes Jul 9, 2020

View reviewed changes

MichaelGrupp mentioned this pull request Jul 15, 2020

Changes for ROS Noetic support cartographer-project/cartographer_ros#1494

Merged

wohe reviewed Jul 15, 2020

View reviewed changes

wohe mentioned this pull request Jul 22, 2020

Update package.xml for ROS Noetic. #1723

Merged

clalancette force-pushed the ros2-upstream branch from 864edf6 to 2f0315a Compare July 23, 2020 17:58

wohe mentioned this pull request Sep 24, 2020

Depend on libceres-dev in package.xml for ROS. #1750

Merged

clalancette force-pushed the ros2-upstream branch from 2f0315a to 8f6ef41 Compare September 24, 2020 19:36

MichaelGrupp approved these changes Oct 19, 2020

View reviewed changes

MichaelGrupp removed the cla: no label Oct 19, 2020

MichaelGrupp mentioned this pull request Oct 22, 2020

Build instructions are not up to date cartographer-project/cartographer_ros#1531

Closed

4 tasks

mikaelarguedas and others added 3 commits October 23, 2020 12:08

restrict boost dependencies to the ones used (#14)

0334fc7

* restrict boost dependencies to the ones used Signed-off-by: Mikael Arguedas <[email protected]>

Also add googletest as a build dependency.

55eb0a9

Signed-off-by: Chris Lalancette <[email protected]>

Set GMOCK_INCLUDE_DIRS when doing src fallback.

e30f75a

Signed-off-by: Chris Lalancette <[email protected]>

clalancette force-pushed the ros2-upstream branch from 8f6ef41 to e30f75a Compare October 23, 2020 12:22

clalancette changed the title ~~ROS 2 Cartographer port~~ Minor fixes for Cartographer from ROS Oct 23, 2020

wohe approved these changes Oct 23, 2020

View reviewed changes

wohe merged commit 1f69b83 into cartographer-project:master Oct 23, 2020

clalancette deleted the ros2-upstream branch October 23, 2020 15:20

MichaelGrupp mentioned this pull request Oct 29, 2020

Added GMOCK_LIBRARY check #1769

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Minor fixes for Cartographer from ROS #1705

Minor fixes for Cartographer from ROS #1705

clalancette commented Jun 17, 2020 •

edited

Loading

googlebot commented Jun 17, 2020

clalancette commented Jun 17, 2020

wohe commented Jun 18, 2020

clalancette commented Jun 18, 2020

MichaelGrupp commented Jul 9, 2020

clalancette commented Jul 9, 2020

MichaelGrupp Jul 9, 2020

wohe Jul 15, 2020

clalancette Jul 17, 2020

wohe Jul 15, 2020

clalancette commented Sep 24, 2020

clalancette commented Oct 12, 2020

MichaelGrupp left a comment

MichaelGrupp commented Oct 23, 2020

wohe commented Oct 23, 2020

clalancette commented Oct 23, 2020

wohe commented Oct 23, 2020

clalancette commented Oct 23, 2020

Minor fixes for Cartographer from ROS #1705

Minor fixes for Cartographer from ROS #1705

Conversation

clalancette commented Jun 17, 2020 • edited Loading

googlebot commented Jun 17, 2020

clalancette commented Jun 17, 2020

wohe commented Jun 18, 2020

clalancette commented Jun 18, 2020

MichaelGrupp commented Jul 9, 2020

clalancette commented Jul 9, 2020

MichaelGrupp Jul 9, 2020

Choose a reason for hiding this comment

wohe Jul 15, 2020

Choose a reason for hiding this comment

clalancette Jul 17, 2020

Choose a reason for hiding this comment

wohe Jul 15, 2020

Choose a reason for hiding this comment

clalancette commented Sep 24, 2020

clalancette commented Oct 12, 2020

MichaelGrupp left a comment

Choose a reason for hiding this comment

MichaelGrupp commented Oct 23, 2020

wohe commented Oct 23, 2020

clalancette commented Oct 23, 2020

wohe commented Oct 23, 2020

clalancette commented Oct 23, 2020

clalancette commented Jun 17, 2020 •

edited

Loading