[Map server] Cleanup map server action items

[SteveMacenski]

• create lib for file IO that can be used as a basis for executable
• program for single time save, server to save on demand
• dramatically simplify the image loader in nav2_utils & pull into map_server

[AlexeyMerzlyakov]

From high-level point of view, the following refactoring changes seems are required in nav2_map_server:

• Move MapSaver code inside OccGridLoader. try_write_map_to_file()/mapCallback() methods to be renamed to saveMapToFile().
• map_saver_cli.cpp to be connected with OccGridLoader class instead. CLI functionality of map_saver binary to be untouchable.
• In OccGridLoader class uncover load_map_yaml(), loadMapFromFile(), loadMapFromYaml() methods and LoadParameters structure to be public. They already have all necessary functionality to load map from file or yaml.
• make saveMap()/loadMap*() API methods to be static. This seems to be possible since currently these methods does not use any class belonging variables except of parameter ones. This will allow to call save/loadMap() directly from libmap_server_core dynamic library without having a MapServer object initialized locally. This also necessary in Costmap Filters (keep out, preferred lanes, slow/safety zones) #1263 to call MapServer::loadMap() /MapServer::saveMap() without additionally map_server node initialized.
• Services GetMap "map" and LoadMap "load_map" are already present in OccGridLoader(). These services might be renamed if necessary in this ticket.
• nav_msgs::msg::OccupancyGrid loadMapFromFile() to be removed from nav2_util. It already was moved inside OccGridLoader by e57809b commit.

After all changed applied, nav2_map_server to be look as follows:

• map_server - is a node publishing nav_msgs::msg::OccupancyGrid topic taken from loaded map-file.
• map_saver - is a command-line executable allowing to save map-file from a currently publishing topic to a given file.
• GetMap ("map") and LoadMap ("load_map") are map_server node services (no changes here).
• libmap_server_core - is dynamic library (map_server and map_saver both are linking with it) with static OccGridLoader::saveMap()/OccGridLoader::loadMap*() API methods.

Note1: This refactor should not brake initial concept to have multiple mapLoaders for MapServer class. One of them will be OccGridLoader.
Note2: It should keep in mind further changes for multi-map activity like #392.

[SteveMacenski]

Is there also a map_saver_server node, ei something that can take a map being published and on a service call save it to a file? The single use map_saver is necessary, but I like the idea of multiple calls (often) having a single server that can be hosting it so we don't have nodes coming up and down alot for a necessary operation.

I see the output as needing 4 things:

• map server, as described, maintaining the change map capabilities
• map saver, as the commandline 1-time use
• map saver, as a server for continual use through a service call
• and the library that is core to the above to deal with IO save/get maps from files, with all the code from nav2_util pulled out and back into map_server package. And also linkable for other packages to get maps from files as well if necessary.

Some of the design intent was to be able to store other types of maps, but that should just be another package and another server. This should be dramatically simplified for dealing with image-based maps only (occupancy grids [free, unknown, occupied], probability grids [0-100 probabilities], vector grids [3-channel information]). I didn't follow all of your specific suggestions, but I'll default to your opinion.

[AlexeyMerzlyakov]

Is there also a map_saver_server node, ei something that can take a map being published and on a service call save it to a file?

Initially, I planned to merge map_server and map_saver into one node which can also save and load maps by input services. But if you consider that to remain architecture of separate map_server and map_saver is better choice, e.g. for some reasons like compatibility with code already using map_saver, there is not a problem to keep them individually.
Regarding map_saver_server with services to save the map vs. CLI map_saver: why do not to make one map_saver executable to acting as a ROS2 node and a CLI binary at the same time? I mean, depending on input parameters, it can either save a map and exit or going to infinite node loop waiting for incoming service calls.

The single use map_saver is necessary, but I like the idea of multiple calls (often) having a single server that can be hosting it so we don't have nodes coming up and down alot for a necessary operation.

I am not sure, do you mean the same as in previous paragraph: separate map_server and map_saver?

I see the output as needing 4 things:
...

I think here we are on the same wave.

Some of the design intent was to be able to store other types of maps, but that should just be another package and another server.

That is the big question I think. Is your vision - to remain map_server only for handling 2D OccupancyGrid maps; but for other maps (e.g. 3D maps, or something else) introduce new component, say nav2_3d_map_server?

[AlexeyMerzlyakov]

I've created WIP PR for intermediate review. Any comments/thoughts are welcome!
All action items from this ticket are completed in the following change: #1624.

Changes briefly:

• map_saver and map_server will remain to be separate.
• New map_saver_server with map saving from topic service.
• Dynamic library with static API methods for external calls from anywhere.
• Removed unnecessary nav2_util/map_loader.
• Code refactoring and clean-up.

Future works on this:

• Fix change after review
• Adopt existing testcases for the change
• Cover change by new testcases

[SteveMacenski]

Added comments - in addition I think something useful would be to remove the futures, I don't know what that really adds here. We give it a topic to subscribe to, it gets an image, it saves. We run on single threaded executor so only 1 request at a time. We can have a timeout to get the subscriber and fail. But I don't know what getting the future and then spinning it accomplishes other than making it more challenging to read a single-threaded blocking call for a map that we can't do anything in the mean-time while waiting for

[AlexeyMerzlyakov]

Thank you very much for detailed review!

As you recommended, I've moved all OccupancyGrid save/load functions to new MapIO library. Also, MapSaver class was completely re-worked:

• Switched MapSaver from rclcpp::Node to nav2_utils::LifecycleNode
• Revised and changed MapSaver node parameters model
• Added saveMapTopicToFile() method for listening map topic, receiving a message and saving it into a file
• Removed future-promise synchronization model as unnecessary

Also, there are other changes were made to fulfill all WIP review items. The new WIP change is: 84cff7d.

There is still required to fix PR testing and also fix & cover the change by ROS2 tests.