Refactored Map Server by bpwilcox · Pull Request #31 · ros-navigation/navigation2

bpwilcox · 2018-08-24T23:12:56Z

This is a refactored map server from ROS Navigation. It provides machinery for an extensible framework for loading map files into available map representations (currently just nav_msgs/msg/OccupancyGrid) and provide as services/topics.

lucbettaieb · 2018-08-28T23:55:21Z

Mind if I give this a review, too? @bpwilcox
No worries if you'd rather not have the potential extra noise.

bpwilcox · 2018-08-29T19:47:57Z

Feel free to offer your comments/review @lucbettaieb

mkhansenbot

@bpwilcox - a few comments. I haven't done a full review yet though. First, unless this has a dependency on the code in the ros2_devel branch, it should be a PR to the master branch. I don't think this has such a dependency.

Second, for code we have ported, if we started with the ROS file, we need to keep the copyright and license in the header. For the others we need to add our copyright and Apache 2.0 license info. Let me know if you want help with that. Otherwise please make those changes and submit the PR to the master branch.

Thanks, Matt

mjeronimo · 2018-08-30T16:40:37Z

src/map_server/include/map_server/base_map_loader.h

@@ -0,0 +1,37 @@
+#ifndef MAP_SERVER_BASE_MAP_LOADER_H


Please run cpplint and uncrustify on all of the source files (a requirement for integration of a PR).

mjeronimo · 2018-08-30T16:41:23Z

src/map_server/include/map_server/base_map_loader.h

@@ -0,0 +1,37 @@
+#ifndef MAP_SERVER_BASE_MAP_LOADER_H


You'll have to make some changes to accomodate the new directory structure. For example, map_server becomes nav2_map_server, affecting include directories, header macro guards, etc.

mjeronimo · 2018-08-30T16:41:48Z

src/map_server/include/map_server/base_map_loader.h

+#include <fstream>
+#include "rclcpp/rclcpp.hpp"
+
+


Please remove extra whitespace. See other source files for example source formatting.

mjeronimo · 2018-08-30T16:43:47Z

src/map_server/package.xml

@@ -0,0 +1,30 @@
+<?xml version="1.0"?>


We're using two space indentation on package.xml and CMakeLists.txt files too.

src/map_server/CMakeLists.txt

mjeronimo · 2018-08-30T16:52:08Z

src/map_server/CMakeLists.txt

+	${SDL_INCLUDE_DIR}
+	${SDL_IMAGE_INCLUDE_DIRS}
+	${YAML_CPP_ROS2_INCLUDE_DIRS}
+


Remove extra lines in this file (and similar files).

mjeronimo · 2018-08-30T16:53:43Z

src/map_server/include/map_server/map_reps/gridmap.h

+#include <grid_map_msgs/GridMap.h>
+#include <grid_map_ros/grid_map_ros.hpp>
+#include <grid_map_cv/grid_map_cv.hpp>
+#include <cv_bridge/cv_bridge.h>


There seem to be several headers included that aren't required by the definition of this interface. Only include header files required by the interface. The implementation file can include additional headers, if required by the implementation.

mjeronimo · 2018-08-30T16:54:30Z

src/map_server/include/map_server/map_reps/gridmap.h

+{
+    public:
+        GridMap gridMap;
+        grid_map_msgs::GridMap map_msg;


Why is this public?

mjeronimo · 2018-08-30T16:54:47Z

src/map_server/include/map_server/map_reps/gridmap.h

+        GridMap gridMap;
+        grid_map_msgs::GridMap map_msg;
+
+        cv::Mat MapImage;


Why is MapImage public?

mjeronimo · 2018-08-30T16:55:04Z

src/map_server/include/map_server/map_reps/map_reps.h

+// Add headers for any used map packages 
+
+#include "map_server/map_reps/occupancy_grid.h"
+//#include "map_server/map_reps/gridmap.h"


Should remove commented-out code.

mjeronimo · 2018-08-30T16:56:11Z

src/map_server/package.xml

@@ -0,0 +1,30 @@
+<?xml version="1.0"?>
+<?xml-model href="http://download.ros.org/schema/package_format2.xsd" schematypens="http://www.w3.org/2001/XMLSchema"?>


Please follow the formatting conventions for the CMakeLists.txt files (see other examples in the source tree).

mjeronimo

Please address all cpplint, uncrustify, and other formatting issues. Also, accomodate the new directory organization and related naming conventions. Remove dead code and extra lines/whitespace.

moriarty · 2018-09-02T05:49:52Z

Not to bring up an age old debate, but it looks like when you addressed the whitespace and formatting in 1f16881 maybe you ran an auto-format tool and there are some oddities... Unless ROS2 is using a different style guide than ROS1, you've gone from spaces to tabs, and all/most of the * and & were moved from Type& variable_name --> Type &variable_name.

src/mapping/nav2_map_server/src/map_reps/occupancy_grid.cpp

orduno · 2018-09-04T19:02:14Z

src/mapping/nav2_map_server/src/map_reps/occupancy_grid.cpp

+	}
+}
+
+void OccGridLoader::loadMapFromFile(std::string mapfname)


Should this function be instead on the utils folder?
On the planner_tester, I also have code for reading an image file and generating an occupancy grid msg.

nav_msgs::msg::OccupancyGrid loadMapFromFile( const std::string image_file_name, const double resolution, const bool negate, const double occupancy_threshold, const double free_threshold, const geometry_msgs::msg::Twist origin, const MapMode mode)

Yes, we should be on the lookout for utility code to factor it out into a utility library.

mjeronimo · 2018-09-05T21:46:09Z

src/mapping/nav2_map_server/CMakeLists.txt

+find_package(Bullet REQUIRED)
+find_package(SDL REQUIRED)
+find_package(SDL_image REQUIRED)
+#find_package(yaml-cpp REQUIRED)


Remove commented-out lines

mjeronimo · 2018-09-05T21:46:22Z

src/mapping/nav2_map_server/CMakeLists.txt

+add_library(map_reps
+  src/map_reps/occupancy_grid.cpp
+  src/map_reps/map_loader.cpp
+  #src/map_reps/gridmap.cpp


Remove commented-out lines

mjeronimo · 2018-09-05T21:48:25Z

src/mapping/nav2_map_server/include/nav2_map_server/base_map_loader.hpp

+class BaseMapLoader
+{
+public:
+  std::string fname;


Do these need to be public? Also, do they need to be in the abstract base class. I'd rather see a set of virtual methods and a virtual destructor (only).

mjeronimo · 2018-09-05T21:49:48Z

src/mapping/nav2_map_server/include/nav2_map_server/map_reps/occupancy_grid.hpp

+
+  // Occupancy Grid Specific //
+
+  OccGridLoader() {}


By convention, I'd put constructors and destructors first in the class definition. The logical flow is that someone needs to first know how to create one of the objects, and then would be interested in the methods it provides.

mjeronimo · 2018-09-05T21:51:21Z

src/mapping/nav2_map_server/include/nav2_map_server/map_reps/occupancy_grid.hpp

+
+  ~OccGridLoader() {}
+
+  void OccMapCallback(


Should be private (or protected).

Does the callback work when it is private?

src/mapping/nav2_map_server/package.xml

This file is unused / hasn't been ported to ROS2 yet

* adding map_server tests & starting port of costmap_2d package * adding integration test node * + integration tests * ++ integration test * create branch for tests

…d tests etc.

…nd server class, fixed yaml-cpp path, throwing exceptions up from yaml parser, cleaned cmakelist, added gtest assert throws, fixes naming conventions, added namespace

…Map, adhered to project cmakelist style and build_test, deleted unused packages

…int tests, uncrustify, etc.

mkhansenbot · 2018-09-21T02:30:18Z

I approve 👍

@mjeronimo - were their any further changes you were requesting or can you change your status to approved also?

mkhansenbot · 2018-09-21T16:22:41Z

Squashing and merging map_server

…av2 controller (#31)

…t_configurable_timeout Backport configurable service BT wait timeout

* Adding disengagement threshold to rotation shim controller (backport ros-navigation#4699) (ros-navigation#4702) * Adding disengagement threshold to rotation shim controller (ros-navigation#4699) * adding disengagement threshold to rotation shim controller Signed-off-by: Steve Macenski <stevenmacenski@gmail.com> * change default to 22.5 deg Signed-off-by: Steve Macenski <stevenmacenski@gmail.com> --------- Signed-off-by: Steve Macenski <stevenmacenski@gmail.com> (cherry picked from commit fc7e086) * Update nav2_rotation_shim_controller/src/nav2_rotation_shim_controller.cpp Signed-off-by: Steve Macenski <stevenmacenski@gmail.com> --------- Signed-off-by: Steve Macenski <stevenmacenski@gmail.com> Co-authored-by: Steve Macenski <stevenmacenski@gmail.com> * fix(nav2_theta_star_planner) Fix crash on Humble when goal is outside map bounds (ros-navigation#4706) * Humble sync 13: Nov 8, 2024 (ros-navigation#4748) * [DWB] Option to limit velocity commands in trajectory generator (ros-navigation#4663) * Option to limit vel cmd through traj generator Signed-off-by: huiyulhy <lhyleonghuiyu@gmail.com> * Cleanup Signed-off-by: huiyulhy <lhyleonghuiyu@gmail.com> * fix linting Signed-off-by: huiyulhy <lhyleonghuiyu@gmail.com> * Update linting Signed-off-by: huiyulhy <lhyleonghuiyu@gmail.com> * uncrustify Signed-off-by: huiyulhy <lhyleonghuiyu@gmail.com> * uncrustify Signed-off-by: huiyulhy <lhyleonghuiyu@gmail.com> --------- Signed-off-by: huiyulhy <lhyleonghuiyu@gmail.com> * fix to bt action server logging before bt execution result being ready (ros-navigation#4677) Signed-off-by: DreamWest <sirjamestsao@gmail.com> * fix(simple-action-server): info log instead of warn on cancel (ros-navigation#4684) Cancelling a goal is nominal behavior and therefore it should not log warning. Signed-off-by: Rein Appeldoorn <rein.appeldoorn@nobleo.nl> * [RotationShimController] fix: rotate to goal heading (ros-navigation#4724) Add frame_id to goal when rotating towards goal heading, otherwise the transform would fail. This bug was introduced in 30e2cde by not setting the frame_id. Signed-off-by: agennart <antoine.gennart@quimesis.be> Co-authored-by: agennart <antoine.gennart@quimesis.be> * Fix incorrect doxygen comment (ros-navigation#4741) Signed-off-by: Ryan Friedman <25047695+Ryanf55@users.noreply.github.com> * [map_io] Replace std logs by rclcpp logs (ros-navigation#4720) * replace std logs by rclcpp logs Signed-off-by: Guillaume Doisy <guillaume@dexory.com> * RCLCPP_DEBUG to RCLCPP_INFO for visibility Signed-off-by: Guillaume Doisy <guillaume@dexory.com> --------- Signed-off-by: Guillaume Doisy <guillaume@dexory.com> Co-authored-by: Guillaume Doisy <guillaume@dexory.com> * bump to 1.1.17 for humble sync Signed-off-by: Steve Macenski <stevenmacenski@gmail.com> --------- Signed-off-by: huiyulhy <lhyleonghuiyu@gmail.com> Signed-off-by: DreamWest <sirjamestsao@gmail.com> Signed-off-by: Rein Appeldoorn <rein.appeldoorn@nobleo.nl> Signed-off-by: agennart <antoine.gennart@quimesis.be> Signed-off-by: Ryan Friedman <25047695+Ryanf55@users.noreply.github.com> Signed-off-by: Guillaume Doisy <guillaume@dexory.com> Signed-off-by: Steve Macenski <stevenmacenski@gmail.com> Co-authored-by: Huiyu Leong <26198479+huiyulhy@users.noreply.github.com> Co-authored-by: DreamWest <sirjamestsao@gmail.com> Co-authored-by: Rein Appeldoorn <rein.appeldoorn@nobleo.nl> Co-authored-by: Saitama <gennartan@users.noreply.github.com> Co-authored-by: agennart <antoine.gennart@quimesis.be> Co-authored-by: Ryan <25047695+Ryanf55@users.noreply.github.com> Co-authored-by: Guillaume Doisy <doisyg@users.noreply.github.com> Co-authored-by: Guillaume Doisy <guillaume@dexory.com> --------- Signed-off-by: Steve Macenski <stevenmacenski@gmail.com> Signed-off-by: huiyulhy <lhyleonghuiyu@gmail.com> Signed-off-by: DreamWest <sirjamestsao@gmail.com> Signed-off-by: Rein Appeldoorn <rein.appeldoorn@nobleo.nl> Signed-off-by: agennart <antoine.gennart@quimesis.be> Signed-off-by: Ryan Friedman <25047695+Ryanf55@users.noreply.github.com> Signed-off-by: Guillaume Doisy <guillaume@dexory.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Co-authored-by: Steve Macenski <stevenmacenski@gmail.com> Co-authored-by: brayanpa <brayanspallares@gmail.com> Co-authored-by: Huiyu Leong <26198479+huiyulhy@users.noreply.github.com> Co-authored-by: DreamWest <sirjamestsao@gmail.com> Co-authored-by: Rein Appeldoorn <rein.appeldoorn@nobleo.nl> Co-authored-by: Saitama <gennartan@users.noreply.github.com> Co-authored-by: agennart <antoine.gennart@quimesis.be> Co-authored-by: Ryan <25047695+Ryanf55@users.noreply.github.com> Co-authored-by: Guillaume Doisy <doisyg@users.noreply.github.com> Co-authored-by: Guillaume Doisy <guillaume@dexory.com>

* added parameters and initial layout * update page name Co-authored-by: Steve Macenski <stevenmacenski@gmail.com> * split parameters into multiple files * some formatting fixes * more formatting fixes * added some descriptions * fixed RotateToGoalCritic params namespaces * Update configuration/packages/dwb-params/controller.rst Co-authored-by: Steve Macenski <stevenmacenski@gmail.com> * Fixed namings * removed tunable parameters table * fixed formatting error Co-authored-by: Steve Macenski <stevenmacenski@gmail.com>

bpwilcox requested review from a user, mhpanah, mjeronimo, mkhansenbot and orduno August 24, 2018 23:12

mkhansenbot requested changes Aug 30, 2018

View reviewed changes

mjeronimo reviewed Aug 30, 2018

View reviewed changes

src/map_server/CMakeLists.txt Outdated Show resolved Hide resolved

mjeronimo reviewed Aug 30, 2018

View reviewed changes

mjeronimo suggested changes Aug 30, 2018

View reviewed changes

moriarty reviewed Sep 2, 2018

View reviewed changes

src/mapping/nav2_map_server/src/map_reps/occupancy_grid.cpp Outdated Show resolved Hide resolved

orduno reviewed Sep 4, 2018

View reviewed changes

bpwilcox changed the base branch from ros2_devel to master September 5, 2018 16:39

mjeronimo reviewed Sep 5, 2018

View reviewed changes

src/mapping/nav2_map_server/package.xml Outdated Show resolved Hide resolved

Brian added 17 commits September 20, 2018 11:14

deleted random file

c6f970c

cleaned up map server code

7d042e8

pull request for map_server only

7ca19e2

Delete gridmap.h

397380b

This file is unused / hasn't been ported to ROS2 yet

Map server tests (#1)

629904b

* adding map_server tests & starting port of costmap_2d package * adding integration test node * + integration tests * ++ integration test * create branch for tests

added license info, addressed formatting and white spaces, implemente…

12a0ee2

…d tests etc.

restructure of map_server

c439b6b

additional directory refactor

b28ea55

clean up package

cf7d461

all test passing (cpplint, uncrustify, unit, integration, etc.)

53fad59

added TODO for publisher

3e57e5e

dropping yaml-cpp backwards support, changed service/topic name

6524297

addressing feedback: changed all fprint to RCLCPP_ERROR, split main a…

5263a9d

…nd server class, fixed yaml-cpp path, throwing exceptions up from yaml parser, cleaned cmakelist, added gtest assert throws, fixes naming conventions, added namespace

changed class/file names, replaced fprints, took out redundant create…

81fb42c

…Map, adhered to project cmakelist style and build_test, deleted unused packages

changed name integration to component test, passes unit, component, l…

b836950

…int tests, uncrustify, etc.

some additional cleanup

dbdb39d

cleanup

2ade04e

bpwilcox force-pushed the map_server-devel branch from afd2def to 2ade04e Compare September 20, 2018 20:44

bpwilcox mentioned this pull request Sep 20, 2018

Fix linter failures in nav2_map_server #92

Closed

mkhansenbot approved these changes Sep 21, 2018

View reviewed changes

mjeronimo approved these changes Sep 21, 2018

View reviewed changes

mkhansenbot merged commit e57809b into ros-navigation:master Sep 21, 2018

ghost referenced this pull request in logivations/navigation2 Mar 7, 2022

WMO-57151: adapt behavior tree to retry TEB, adapt error logging in n…

1ab6efb

…av2 controller (#31)

mrmara mentioned this pull request Apr 29, 2022

AMCL change map / first_map_only = false #2931

Closed

jonipol mentioned this pull request Feb 23, 2024

SmacHybrid planner can crash to segmentation fault #4141

Closed

doisyg pushed a commit to doisyg/navigation2 that referenced this pull request Apr 30, 2024

Merge pull request ros-navigation#31 from botsandus/AUTO-2302_backpor…

95b428a

…t_configurable_timeout Backport configurable service BT wait timeout

		@@ -0,0 +1,30 @@
		<?xml version="1.0"?>
		<?xml-model href="http://download.ros.org/schema/package_format2.xsd" schematypens="http://www.w3.org/2001/XMLSchema"?>

		#include <fstream>
		#include "rclcpp/rclcpp.hpp"

Conversation

bpwilcox commented Aug 24, 2018

Uh oh!

lucbettaieb commented Aug 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bpwilcox commented Aug 29, 2018

Uh oh!

mkhansenbot left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjeronimo left a comment

Choose a reason for hiding this comment

Uh oh!

moriarty commented Sep 2, 2018

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mkhansenbot commented Sep 21, 2018

Uh oh!

mkhansenbot commented Sep 21, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

lucbettaieb commented Aug 28, 2018 •

edited

Loading