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

Added change_map service to map_server #461

Closed
wants to merge 3 commits into from

Conversation

sloretz
Copy link

@sloretz sloretz commented May 23, 2016

This PR adds a "change_map" service to map_server including a change_map rostest. It also resolves #279.

The change_map service expects a path to a yaml file, and responds with a Boolean indicating if the map was successfully loaded. The new map and metadata are published to the appropriate topics.

If change_map is called with the currently loaded map, the current map is reloaded as if it were a brand new map, resolving #279.

Added
MapServer::loadMapFromYaml()
MapServer::loadMapFromParams()
MapServer::loadMapFromValues()
of causing the map_server_node to exit

Changed methods
MapServer::loadMapFromValues()
MapServer::loadMapFromParams()
MapServer::loadMapFromYaml()
Added second test map and test constants
Added rostest for change_map service
@sloretz
Copy link
Author

sloretz commented May 23, 2016

CI failed with an unusual error. Is the build system misconfigured?

$ rake
rake aborted!
No Rakefile found (looking for: rakefile, Rakefile, rakefile.rb, Rakefile.rb)
/home/travis/.rvm/gems/ruby-1.9.3-p551/bin/ruby_executable_hooks:15:in `eval'
/home/travis/.rvm/gems/ruby-1.9.3-p551/bin/ruby_executable_hooks:15:in `<main>'
(See full trace by running task with --trace)


The command "rake" exited with 1.

@@ -0,0 +1,3 @@
string map_path
---
bool success
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd really rather we not put messages in the packages in navigation -- we have a separate repo (https://github.com/ros-planning/navigation_msgs) that is for messages, this has the advantage of making the interface itself (the messages) lightweight to build, especially if someone is on an alternate platform where they can't actually build/install the full navigation stack, but they just want to communicate with it

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, the right place for this message is probably nav_msgs, given that is where GetMap is, etc (although I do realize that is in common_msgs, meaning getting a review/release is likely going to take longer)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can submit a PR to common_msgs/nav_msgs for LoadMap.srv. Do you think the service would pass scrutiny in that repo?

Would that mean a path for this change to be released is to wait for the service to be released in common_msgs, then modify this PR to not contain the LoadMap service?

sloretz added a commit to sloretz/common_msgs that referenced this pull request Oct 16, 2016
When called map_server will load a new map (or reload the old) when requested
This would allow completion of pull request ros-planning/navigation#461
Which would close ros-planning/navigation#279
@DLu DLu added the map_server label May 30, 2017
@DLu DLu added the jade label Apr 30, 2018
@DLu
Copy link
Member

DLu commented Feb 22, 2019

I'm digging up old PRs. This is the oldest. Can you rebase this on melodic?

@DLu
Copy link
Member

DLu commented Feb 26, 2019

Actually, this is blocked by ros/common_msgs#95

@DLu
Copy link
Member

DLu commented Nov 25, 2019

Now blocked by ros/common_msgs#152

@DLu
Copy link
Member

DLu commented Aug 14, 2020

No longer blocked.

@sloretz
Copy link
Author

sloretz commented Oct 19, 2020

Closing in favor of #1029 :)

@sloretz sloretz closed this Oct 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants