Feature: Load config service

[Crola1702]

Description

These changes implement the feature requested in #1594, by creating the service when the visualization frame is initialized.

Changes:

• Refactored loadDisplayConfig which now receives a string that can be a path to a configuration file or the configuration yaml string
• Created new service file in rviz_resource_interfaces that receives a config_string as a request and responds with a trigger (success and message). The service callback just calls loadDisplayConfig
• rviz_common now depends on rviz_resource_interfaces

[ahcorde]

Suggested change

	#include <rclcpp/service.hpp>
	#include "rviz_common/config.hpp"
	#include "rviz_common/window_manager_interface.hpp"
	#include "rviz_common/ros_integration/ros_node_abstraction_iface.hpp"
	#include "rviz_rendering/render_window.hpp"
	#include <rviz_resource_interfaces/srv/load_config.hpp>

[ahcorde]

include <memory>

[ahcorde]

Suggested change

	#include "rclcpp/clock.hpp"
	#include <rclcpp/service.hpp>
	#include "tf2_ros/buffer.hpp"
	#include "tf2_ros/transform_listener.hpp"
	#include "rviz_common/load_resource.hpp"
	#include "rviz_common/logging.hpp"
	#include "rviz_common/panel.hpp"
	#include "rviz_common/panel_dock_widget.hpp"
	#include "rviz_common/render_panel.hpp"
	#include "rviz_common/tool.hpp"
	#include "rviz_common/yaml_config_reader.hpp"
	#include "rviz_common/yaml_config_writer.hpp"
	#include "rviz_rendering/render_window.hpp"
	#include <rviz_resource_interfaces/srv/load_config.hpp>

[ahcorde]

update docs

[Crola1702]

Comments addressed @ahcorde. Thank you!

[ahcorde]

cpplint is failing /tmp/ws/src/rviz/rviz_common/src/rviz_common/visualization_frame.cpp:734: At least two spaces is best between code and comments [whitespace/comments] [2]

[clalancette]

I have two large questions about this feature.

1. Do we need this at all? I know the original issue asked for a way to reload configurations without restarting rviz, but I think we have that already. If you click on File->Open and open a config, it will replace the last config. That said, there currently isn't a programmatic way to do it. But before we add more features, I think it would be good to ask the original reporter whether using the GUI would work for them.
2. If we do need this, I think that we should split up the implementation of loading the config more. In particular, I think we should have a method loadDisplayConfigFromYaml which takes in a YamlConfigReader. The service can setup a YamlConfigReader from the string, then call this function. We would leave the existing overload of loadDisplayConfig that takes a filename, and this would open a YamlConfigReader with the file, then call loadDisplayConfigFromYaml. Does that make sense?

[mjcarroll]

I think that we should split up the implementation of loading the config more. In particular, I think we should have a method loadDisplayConfigFromYaml which takes in a YamlConfigReader. The service can setup a YamlConfigReader from the string, then call this function. We would leave the existing overload of loadDisplayConfig that takes a filename, and this would open a YamlConfigReader with the file, then call loadDisplayConfigFromYaml.

I think that we discussed this in person at the hackathon, but I hadn't had a chance to follow-up on a review here yet.

[SamuelFoo]

Hi @Crola1702 @mjcarroll, I'm interested in continuing to develop this feature. May I ask if it's still open? Thanks!

[christophebedard]

@Crola1702 do you want to continue working on this, or can someone else take over?

[Crola1702]

Hi @Crola1702 @mjcarroll, I'm interested in continuing to develop this feature. May I ask if it's still open? Thanks!

Yeah sure!

@Crola1702 do you want to continue working on this, or can someone else take over?

Someone else can take over. No problem!