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

Change_map service to map_server [Rebase/Noetic] #1029

Merged
merged 5 commits into from
May 27, 2021
Merged

Change_map service to map_server [Rebase/Noetic] #1029

merged 5 commits into from
May 27, 2021

Conversation

DLu
Copy link
Member

@DLu DLu commented Sep 22, 2020

This is a redo of #461 for noetic, integrating changes that have been made in the last 4 years since the original PR. 🙄

The CI will fail until the changes from ros/common_msgs#152 are released, see ros/common_msgs#167

The return codes from the service are not ideal (in that it only returns success or failure, not the more nuanced failure codes described in the service) but I was trying to keep close to @sloretz's original PR structure.

@@ -4,6 +4,7 @@ project(map_server)
find_package(catkin REQUIRED
COMPONENTS
roscpp
roslib
Copy link
Contributor

Choose a reason for hiding this comment

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

this seems to only be in the Cmake/package.xml - where is it actually used/required?

map_server/src/main.cpp Show resolved Hide resolved
@EricWiener
Copy link

Any progress updates on this?

@DLu
Copy link
Member Author

DLu commented Mar 2, 2021

@ros-pull-request-builder retest this please

@EricWiener
Copy link

@DLu thanks for seeing if the tests pass. Does it look good to merge?

@DLu DLu requested a review from mikeferguson March 4, 2021 20:49
@DLu
Copy link
Member Author

DLu commented Mar 4, 2021

I think so. Let's get the +1 from @mikeferguson first since he had formally reviewed it earlier.

@EricWiener
Copy link

Any progress on this?

map_server/src/main.cpp Outdated Show resolved Hide resolved
@engineeve
Copy link

Any chance this will be merged within the next month?

sloretz and others added 4 commits May 26, 2021 15:35
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
@DLu DLu merged commit 2b807bd into noetic-devel May 27, 2021
@DLu DLu deleted the redo461 branch May 27, 2021 14:23
MatthijsBurgh pushed a commit to tue-robotics/navigation that referenced this pull request Aug 10, 2021
* Refactored map loading from constructor to three methods
* Added change_map service using LoadMap.srv
@HappySamuel
Copy link

Hi @DLu

I saw that there's an addition feature (change_map) to the map_server. Is this add-on feature available if install via apt?
$ sudo apt install ros-noetic-map-server
Because i check wiki of ros noetic map server, it remains outdated.

Best,
Samuel

MatthijsBurgh pushed a commit to tue-robotics/navigation that referenced this pull request Sep 28, 2021
* Refactored map loading from constructor to three methods
* Added change_map service using LoadMap.srv
@DLu
Copy link
Member Author

DLu commented Oct 7, 2021

No, and its been over a year since the last release, so its overdue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants