Feature/dynamic configs #25
Conversation
Schmluk
commented
Aug 18, 2024
Schmluk
commented
- Initial implementation of dynamic configs and dynamic config servers.
- Also adds a ROS server to set configs via topics (not via params for now, although we could think of this too)
- Add utests
- Add demo with two GUIs to set configs (Plain YAML and specialized fields, although experimental)
- Add docs
- Minor refactor and cleanup of settings. This will be breaking interfaces though (should be easy to fix if used)
Schmluk
force-pushed
the
feature/dynamic_configs
branch
from
c82f6d7 to
ebac5c1
Compare
Schmluk
force-pushed
the
feature/dynamic_configs
branch
from
c6ef9d9 to
27f2f79
Compare
There was a problem hiding this comment.
LGTM, thanks for putting this together! The general structure seems very intuitive!
We'll need to coordinate though on when we actually merge this given the changes to the settings struct (it'll break Hydra). Alternatively we could split this into two PRs (one with the settings, one with the dynamic configs) but I think that's more work than it's worth at this point
| @@ -40,7 +40,7 @@ | |||
| #include <typeinfo> | |||
| #include <vector> | |||
|
|
|||
| #include "config_utilities/internal/meta_data.h" | |||
| #include <yaml-cpp/yaml.h> | |||
There was a problem hiding this comment.
(minor) This is maybe my fault, but dataToString should probably live elsewhere (and this file could probably just be standard library includes and the gnu demangling)
| */ | ||
| template <typename ConfigT> | ||
| struct DynamicConfig { | ||
| using Callback = std::function<void()>; |
There was a problem hiding this comment.
(minor) I feel like I often find myself writing callbacks that are something like
[&]() {
const auto& config = dynamic_config_.get();
// do stuff with config
}which is slightly simpler as
[&](const DynamicConfig<ConfigT>& dynamic_config) {
const auto& config = dynamic_config.get();
}| /** | ||
| * @brief Parse a single value to the yaml node. | ||
| * @param value The value to parse. | ||
| * @param error Optional: Where to store the error message if conversion fails. | ||
| */ | ||
| template <typename T> | ||
| static YAML::Node toYaml(const T& value, std::string* error = nullptr) { | ||
| YAML::Node node; | ||
| std::string err; | ||
| try { | ||
| node = toYamlImpl("tmp", value, err); | ||
| } catch (const std::exception& e) { | ||
| err = std::string(e.what()); | ||
| } | ||
| if (!err.empty()) { | ||
| if (error) { | ||
| *error = err; | ||
| } | ||
| return YAML::Node(YAML::NodeType::Null); | ||
| } | ||
| return node["tmp"]; | ||
| } | ||
|
|
There was a problem hiding this comment.
It took me a while to parse why this overload existed, mostly because of the "tmp" namespace. I think it might make more sense to have toYamlImpl not namespace the returned node and have the previous toYaml version handle namespacing
There was a problem hiding this comment.
Agreed! I've been meaning to clean this up but didn't want to touch everything, should be better now 👍
Schmluk
force-pushed
the
feature/dynamic_configs
branch
from
99d4887 to
222a82a
Compare
|
Many thanks for the pointers @nathanhhughes! Agreed, probably good to time this. I'm not in a rush with this and also wanted to expose which modules are registered, for the main use of easier visualization switches (as Marcus correctly noted, currently it's a bit hard to figure out which colorizer goes where in the config 😇). Although these things are easy to add later if we find a good time slot to merge the first batch. |
