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

Fetch cartographer to release-1.0 tag #7

Closed
wants to merge 16 commits into from

Conversation

Projects
None yet
6 participants
@routiful
Copy link

commented Feb 1, 2019

I fetched cartographer code to release-1.0 tag in googlecartographer/cartographer repo.
I have been tested this turtlebot3 and gazebo.

If you want to test this, please following below link
http://emanual.robotis.com/docs/en/platform/turtlebot3/ros2/#ros2

pifon2a and others added some commits Jun 1, 2018

Make cartographer compatible with ROS2. (#1)
* Make cartographer compatible with ROS2.

We have to disable all of the tests here because they all require
either gtest or gmock, neither of which is vendored cartographer.
Because of this, you would have to install a system
version of both of them, but that causes problems with all
other tests in the system.  Specifically, what happens is
that the system versions are (most likely) compiled with
different flags from the vendored versions in other tests.
When attempting to run the tests, the dynamic linker first
looks in the system places, finds the .so, and then doesn't
bother with the built one.  The tests typically crash in
bizarre ways after that.  For now, workaround all of this
mess just by disabling the tests here.

Signed-off-by: Chris Lalancette <clalancette@osrfoundation.org>

* Minor fixes to package.xml from review.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

@routiful routiful referenced this pull request Feb 1, 2019

Open

Port to ROS2 #1162

@clalancette

This comment has been minimized.

Copy link

commented Feb 7, 2019

@routiful It is great to get this up-to-date, so thanks for the PR. I'm actually wondering if it might make more sense to just force push this repository up to the latest cartographer code, then add the changes we need on top of that as a new rebase. That will reduce the history (since it is a bit confusing now), and get rid of the merge.

Separately, we should also look at pushing these changes into upstream cartographer, and seeing what their thoughts about it are, but that can wait until later.

What do you think?

@routiful

This comment has been minimized.

Copy link
Author

commented Feb 8, 2019

@clalancette
I agreed your opinion. In my thought, after this branch are going to be rebased from cartographer-1.0.0 tag, I would be modified the cmakelist to build using colcon. It might be make more sense.

Since I didn't modified this code except cmakelist to build in colcon, I don't think this code should be a reviewed.
But cartographer_ros(ros2/cartographer_ros#25) might be reviewed to cartographer developer. But I was wondering when they start, either.
I will write a comment to review PR at cartographer_ros to RFCs(https://github.com/googlecartographer/rfcs).

I have a plan to other nodes including cartographer_ros.
If I get more progress, I will let you know about it :)

@dirk-thomas

This comment has been minimized.

Copy link
Member

commented Feb 8, 2019

Since I didn't modified this code except cmakelist to build in colcon ...

Can you please describe what you mean with this? Or even better provide a link to the actual diff between the CMake code upstream and in this fork?

@routiful

This comment has been minimized.

Copy link
Author

commented Feb 8, 2019

@dirk-thomas

I've deleted google test line in CMakelists when I started fetch this repo.
bdf4a57
I assume that it was caused by dependency problem.

For now, I test again with activated test line I said, colcon build was successfully finished including one warning.

➜  ros2_overlay_ws colcon build --packages-select cartographer 
Starting >>> cartographer
[Processing: cartographer]                             
[Processing: cartographer]                                     
[Processing: cartographer]                                       
[Processing: cartographer]                                       
--- stderr: cartographer                                            
In file included from /usr/src/googletest/googlemock/include/gmock/gmock-spec-builders.h:75:0,
                 from /usr/src/googletest/googlemock/include/gmock/gmock-generated-function-mockers.h:43,
                 from /usr/src/googletest/googlemock/include/gmock/gmock.h:61,
                 from /home/ost/ros2_overlay_ws/src/cartographer/cartographer/io/serialization_format_migration_test.cc:27:
/usr/src/googletest/googlemock/include/gmock/gmock-matchers.h: In instantiation of ‘bool testing::internal::AnyEq::operator()(const A&, const B&) const [with A = unsigned int; B = int]’:
/usr/src/googletest/googlemock/include/gmock/gmock-matchers.h:908:18:   required from ‘bool testing::internal::ComparisonBase<D, Rhs, Op>::Impl<Lhs>::MatchAndExplain(Lhs, testing::MatchResultListener*) const [with Lhs = const unsigned int&; D = testing::internal::EqMatcher<int>; Rhs = int; Op = testing::internal::AnyEq]’
/home/ost/ros2_overlay_ws/src/cartographer/cartographer/io/serialization_format_migration_test.cc:120:1:   required from here
/usr/src/googletest/googlemock/include/gmock/gmock-matchers.h:204:60: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
   bool operator()(const A& a, const B& b) const { return a == b; }
                                                          ~~^~~~
---
Finished <<< cartographer [2min 24s]

Summary: 1 package finished [2min 25s]
  1 package had stderr output: cartographer
@dirk-thomas

This comment has been minimized.

Copy link
Member

commented Feb 8, 2019

colcon build was successfully finished including one warning.

I would assume that the same warning is also present when building the upstream package? If yes, I would suggest to propose a PR upstream to address the compiler warning and once that patch got accepted we can use upstream unmodified.

@routiful

This comment has been minimized.

Copy link
Author

commented Feb 11, 2019

@dirk-thomas
I've tested upstream packages(release-1.0) has same warning.
I agree your suggestion to upstream this package.

@clalancette

This comment has been minimized.

Copy link

commented Apr 26, 2019

I'm sorry for the long delay here. After looking at the difference between this PR as it is and upstream, it looks like the only difference is a single #include <array> in cartographer/mapping/pose_graph_interface.h, and the changing of the maintainers. Therefore, what I'm going to do is the following:

  1. Update the ros2 branch here by fetching from upstream cartographer 1.0.0 and force-pushing to the ros2 branch here.
  2. Add in a single commit on top changing the maintainer to me and @mjcarroll , with Pyo as an author.
  3. Close this PR.
  4. Release this package into crystal.

@clalancette clalancette force-pushed the ros2:ros2 branch from b429d1a to 4825445 Apr 26, 2019

@clalancette

This comment has been minimized.

Copy link

commented Apr 26, 2019

Done now, so closing this PR.

@routiful

This comment has been minimized.

Copy link
Author

commented Apr 27, 2019

@clalancette

Thanks you so much :)

@robotpilot robotpilot referenced this pull request Apr 29, 2019

Closed

ROS 2 Dashing Diademata #607

13 of 20 tasks complete
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.