Add LoadMap service definition #152

jacobperron · 2019-11-25T21:50:37Z

Continuation of #95. Now targeting the default branch.

Original description:

The service is for loading a map as an nav_msgs/OccupancyGrid given a resource ID. It also features a result member to encode errors. I tried to make the result codes generic enough for support of arbitrary backends and underlying data structures.

This could be implemented in a map_loader node or directly in map_server seeing how it already "loads" a map on startup.

I've updated the definition to take a URL as suggested by @jack-oquin (774d96f) and fixed an error (a12d806).

Signed-off-by: Jacob Perron <[email protected]>

mkhansenbot · 2019-11-26T18:45:33Z

If you merge this for Eloquent, I'll change my Nav2 PR to be based on this service.

jacobperron · 2019-11-26T23:15:25Z

If you merge this for Eloquent, I'll change my Nav2 PR to be based on this service.

@mkhansen-intel This PR is targeting ROS 1. But if it is approved and merged, I'll port it to ROS 2.
Since this may take some time, I would go ahead with the nav2 version of the service definition and we can make the change to the common type in the future. Hopefully it's as easy as swapping the package name 🤞

mkhansenbot · 2019-11-27T16:01:11Z

@jacobperron - so I started changing my PR to use this format by temporarily adding this to our nav2_msgs until you merge this for Eloquent.

However, there is a minor issue to discuss. Inside each map.yaml file is a path to the image file:

image: testmap.png
resolution: 0.1
origin: [0.0, 0.0, 0.0]
occupied_thresh: 0.65
free_thresh: 0.196
negate: 0

So, do we now require that every image is a full path?
file://home/user/ros/testmap.png
https:/server/ros/testmap.png

If so it means we're not backwards compatible with existing map.yaml files, they'll need to be updated.

Thoughts on this?

jacobperron · 2019-11-27T17:41:03Z

So, do we now require that every image is a full path?

Good question. I figured that the path to image file is assumed relative to the location of the YAML file. I'd say no, we shouldn't use URLs for the image files. I think we should be compatible with whatever map saver is doing.

mkhansenbot · 2019-11-28T01:07:50Z

I think we should be compatible with whatever map saver is doing.

I agree, I'll see what I can do to stay compatible. I think it's reasonable to assume the yaml file and pgm file are in the same relative path. I'll see if using that assumption can keep this simple.

jacobperron · 2019-12-03T22:36:15Z

nav_msgs/srv/LoadMap.srv

+# URL of map resource
+# Can be an absolute path to a file: file:///path/to/maps/floor1.yaml
+# Or, relative to a ROS package: package://my_ros_package/maps/floor2.yaml
+string map_url


I'd like to posit that how the map is loaded or what type of file the URL points to is up to the node implementing the service. E.g. it might point to a yaml file containing the metadata, or maybe an image file directly.

See ros-navigation/navigation2#1303 (comment) for a relevant discussion.

cc @SteveMacenski

@jacobperron this is also for ROS1, I'm not as concerned about it.

However you think we can change this to map_uri? I typically think of URLs as links to a server rather than a URI which can be a local path.

A URL can also be a local path. It describes a way to locate a resource.
I'm okay with changing this to a more general URI, but would like to hear what others think. It might make the semantics more ambiguous.

I know a URL 'is' that, but in vernacular we usually talk about servers. A URL is a subclass of URI. 100% agree its semantics. I just think it makes it more clear actually because of the vernacular uses and URI is what is used in SDF for getting models/packages/files in the same way this is attempting to so its consistent with other ROS-y tools.

I don't care that much one way or another since I know what it means, but I could see someone just looking at a message definition in ros2 msg show ... and thinking it had to be a web link. If no one else agrees, I'm not going to push it.

SteveMacenski · 2019-12-05T17:05:16Z

nav_msgs/srv/LoadMap.srv

+# URL of map resource
+# Can be an absolute path to a file: file:///path/to/maps/floor1.yaml
+# Or, relative to a ROS package: package://my_ros_package/maps/floor2.yaml
+string map_url


@jacobperron this is also for ROS1, I'm not as concerned about it.

However you think we can change this to map_uri? I typically think of URLs as links to a server rather than a URI which can be a local path.

SteveMacenski · 2019-12-05T17:06:12Z

nav_msgs/srv/LoadMap.srv

+string map_url
+---
+# Result code defintions
+uint8 RESULT_SUCCESS=0


I'd prefer if these were just SUCCESS etc without the RESULT_ prefix since in use it would be nav_msgs::msg::LoadMap::Result::RESULT_MAP_DOES_NOT_EXIST and we see result twice in a row.

Actually, its not:
nav_msgs::msg::LoadMap::Result::RESULT_MAP_DOES_NOT_EXIST
It's
nav_msgs::msg::LoadMap::Response::RESULT_MAP_DOES_NOT_EXIST

Subtle difference.

ah, yes sorry.

Its not something I care deeply about, just a comment that it seems related enough with the namespace the additional wording just uses linespace. If no one else agrees, I'm certainly not going to push it.

mkhansenbot · 2020-05-08T16:49:44Z

What happened with this PR? I merged my Nav2 PR awhile back assuming this would be completed shortly. Who needs to approve it?

SteveMacenski · 2020-05-08T19:15:09Z

I believe we're using the Nav2 msgs for now on this: https://github.com/ros-planning/navigation2/blob/master/nav2_msgs/srv/LoadMap.srv

I'm not too concerned personally with this being merged (at this point, I think the PR should be closed). In the medium term future, I want nav2_msgs put into nav_msgs, but I want to wait until the APIs settle down. We're still working on adding result and feedback types to all actions.

jacobperron · 2020-05-13T16:21:11Z

I think @tfoote is the maintainer of this package and should be the one to review / merge.

mkhansenbot · 2020-05-14T22:02:53Z

@SteveMacenski - I'm still hoping that the nav2_msgs LoadMap.srv will be replaced by this.

SteveMacenski · 2020-05-14T22:09:35Z

That's also fine.

DLu · 2020-07-03T18:57:45Z

Alternative: We could put it in map_msgs

SteveMacenski · 2020-07-03T20:35:59Z

Also fine with that.

SteveMacenski · 2020-07-10T19:05:33Z

@jacobperron what's the blocker from this being merged? It looks like Matt, David, and I all approve it. I don't know if David has admin here to merge it but I don't. What's the reason this isn't in / does this just need someone to hit the button?

jacobperron · 2020-07-13T18:38:34Z

@SteveMacenski I'm not the maintainer of this package. I believe the responsibility lies on @tfoote.

tfoote

We can add this here. Especially since it's been proven out in the nav2_msgs already.

It's purely an addition so it will be backwards compatible. What distributions are most important to target? By default I'm going to retarget this to noetic-devel to merge.

SteveMacenski · 2020-07-21T23:01:48Z

Throw it into Foxy as well, I'll file a ticket then to migrate to this message uses' over nav2_msgs

DLu · 2020-07-24T13:44:08Z

This PR is for the jade-devel branch, but confusingly, that branch is used for kinetic and melodic, so merging this should update both those distros. I've cherry-picked the commits for noetic in #164

SteveMacenski · 2020-08-14T01:25:45Z

Is there a potential to have an analog for ROS2?

jacobperron · 2020-08-14T18:12:33Z

If you propose a PR to the default branch of https://github.com/ros2/common_interfaces I can review it.

SteveMacenski · 2020-08-14T18:22:15Z

ros2/common_interfaces#129 done

jacobperron and others added 3 commits November 25, 2019 13:45

Add LoadMap service

bd5af35

Use a URL

774d96f

Signed-off-by: Jacob Perron <[email protected]>

Make result codes unique

a12d806

Signed-off-by: Jacob Perron <[email protected]>

This was referenced Nov 25, 2019

Add LoadMap service #95

Closed

Add LoadMap service to map_server ros-navigation/navigation2#1303

Merged

Move result code definitions into response message

9a35027

Signed-off-by: Jacob Perron <[email protected]>

DLu mentioned this pull request Nov 25, 2019

Added change_map service to map_server ros-planning/navigation#461

Closed

jacobperron commented Dec 3, 2019

View reviewed changes

SteveMacenski reviewed Dec 5, 2019

View reviewed changes

Rayman mentioned this pull request Jun 17, 2020

[map_server] load_map and save_map interfaces ros-planning/navigation#1001

Closed

DLu mentioned this pull request Jul 10, 2020

Load Map service definition ros-planning/navigation_msgs#20

Closed

tfoote approved these changes Jul 21, 2020

View reviewed changes

tfoote self-assigned this Jul 21, 2020

DLu mentioned this pull request Jul 24, 2020

[noetic] Add LoadMap service #164

Merged

tfoote merged commit 10bcfe7 into jade-devel Aug 13, 2020

tfoote deleted the jacob/load_map branch August 13, 2020 20:48

SteveMacenski mentioned this pull request Aug 14, 2020

Add LoadMap service definition ros2/common_interfaces#129

Merged

DLu mentioned this pull request Sep 22, 2020

Change_map service to map_server [Rebase/Noetic] ros-planning/navigation#1029

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add LoadMap service definition #152

Add LoadMap service definition #152

jacobperron commented Nov 25, 2019

mkhansenbot commented Nov 26, 2019

jacobperron commented Nov 26, 2019

mkhansenbot commented Nov 27, 2019

jacobperron commented Nov 27, 2019 •

edited

Loading

mkhansenbot commented Nov 28, 2019

jacobperron Dec 3, 2019

SteveMacenski Dec 5, 2019

jacobperron Dec 5, 2019

SteveMacenski Dec 5, 2019 •

edited

Loading

SteveMacenski Dec 5, 2019

SteveMacenski Dec 5, 2019

mkhansenbot Dec 5, 2019

SteveMacenski Dec 5, 2019

mkhansenbot commented May 8, 2020

SteveMacenski commented May 8, 2020

jacobperron commented May 13, 2020

mkhansenbot commented May 14, 2020

SteveMacenski commented May 14, 2020

DLu commented Jul 3, 2020

SteveMacenski commented Jul 3, 2020

SteveMacenski commented Jul 10, 2020

jacobperron commented Jul 13, 2020

tfoote left a comment

SteveMacenski commented Jul 21, 2020

DLu commented Jul 24, 2020

SteveMacenski commented Aug 14, 2020

jacobperron commented Aug 14, 2020

SteveMacenski commented Aug 14, 2020

Add LoadMap service definition #152

Add LoadMap service definition #152

Conversation

jacobperron commented Nov 25, 2019

mkhansenbot commented Nov 26, 2019

jacobperron commented Nov 26, 2019

mkhansenbot commented Nov 27, 2019

jacobperron commented Nov 27, 2019 • edited Loading

mkhansenbot commented Nov 28, 2019

jacobperron Dec 3, 2019

Choose a reason for hiding this comment

SteveMacenski Dec 5, 2019

Choose a reason for hiding this comment

jacobperron Dec 5, 2019

Choose a reason for hiding this comment

SteveMacenski Dec 5, 2019 • edited Loading

Choose a reason for hiding this comment

SteveMacenski Dec 5, 2019

Choose a reason for hiding this comment

SteveMacenski Dec 5, 2019

Choose a reason for hiding this comment

mkhansenbot Dec 5, 2019

Choose a reason for hiding this comment

SteveMacenski Dec 5, 2019

Choose a reason for hiding this comment

mkhansenbot commented May 8, 2020

SteveMacenski commented May 8, 2020

jacobperron commented May 13, 2020

mkhansenbot commented May 14, 2020

SteveMacenski commented May 14, 2020

DLu commented Jul 3, 2020

SteveMacenski commented Jul 3, 2020

SteveMacenski commented Jul 10, 2020

jacobperron commented Jul 13, 2020

tfoote left a comment

Choose a reason for hiding this comment

SteveMacenski commented Jul 21, 2020

DLu commented Jul 24, 2020

SteveMacenski commented Aug 14, 2020

jacobperron commented Aug 14, 2020

SteveMacenski commented Aug 14, 2020

jacobperron commented Nov 27, 2019 •

edited

Loading

SteveMacenski Dec 5, 2019 •

edited

Loading