search

ros / common_msgs

Commonly used messages in ROS. Includes messages for actions (actionlib_msgs), diagnostics (diagnostic_msgs), geometric primitives (geometry_msgs), robot navigation (nav_msgs), and common sensors (sensor_msgs), such as laser range finders, cameras, point clouds.
http://wiki.ros.org/common_msgs
177 stars 191 forks source link

Add LoadMap service definition #152

Closed jacobperron closed 4 years ago

jacobperron commented 4 years ago

Continuation of https://github.com/ros/common_msgs/pull/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).

mkhansenbot commented 4 years ago

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

jacobperron commented 4 years ago

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 :crossed_fingers:

mkhansenbot commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

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.

mkhansenbot commented 4 years ago

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

SteveMacenski commented 4 years ago

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 commented 4 years ago

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

mkhansenbot commented 4 years ago

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

SteveMacenski commented 4 years ago

That's also fine.

DLu commented 4 years ago

Alternative: We could put it in map_msgs

SteveMacenski commented 4 years ago

Also fine with that.

SteveMacenski commented 4 years ago

@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 commented 4 years ago

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

SteveMacenski commented 4 years ago

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

DLu commented 4 years ago

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 commented 4 years ago

Is there a potential to have an analog for ROS2?

jacobperron commented 4 years ago

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

SteveMacenski commented 4 years ago

https://github.com/ros2/common_interfaces/pull/129 done