simplify metadata, address review feedback

Author: adlarkin

include/behaviortree_cpp/basic_types.h

@@ -439,43 +439,6 @@ struct has_static_method_providedPorts<
 {
 };
 
-template <typename T, typename = void>
-struct has_static_method_description : std::false_type
-{
-};
-
-template <typename T>
-struct has_static_method_description<
-    T, typename std::enable_if<
-           std::is_same<decltype(T::description()), std::string>::value>::type>
-  : std::true_type
-{
-};
-
-/// Optional metadata for a TreeNodeManifest.
-/// The metadata can represent an xml element with text:
-///   <metadata_name>Descriptive text</metadata_name>
-/// Or an xml element with an attribute:
-///   <metadata_name attribute="value"/>
-struct ManifestMetadata
-{
-  /// A pair containing the <name, value> of a metadata's XML attribute.
-  using Attribute = std::pair<std::string, std::string>;
-
-  [[nodiscard]] bool representsText() const
-  {
-    return std::holds_alternative<std::string>(text_or_attribute);
-  }
-
-  [[nodiscard]] bool representsAttribute() const
-  {
-    return std::holds_alternative<Attribute>(text_or_attribute);
-  }
-
-  std::string name;
-  std::variant<std::string, Attribute> text_or_attribute;
-};
-
 template <typename T, typename = void>
 struct has_static_method_metadata : std::false_type
 {
@@ -484,7 +447,7 @@ struct has_static_method_metadata : std::false_type
 template <typename T>
 struct has_static_method_metadata<
     T, typename std::enable_if<
-           std::is_same<decltype(T::metadata()), std::vector<ManifestMetadata>>::value>::type>
+           std::is_same<decltype(T::metadata()), std::vector<std::pair<std::string, std::string>>>::value>::type>
   : std::true_type
 {
 };

include/behaviortree_cpp/bt_factory.h

@@ -22,6 +22,8 @@
 #include <unordered_map>
 #include <unordered_set>
 #include <set>
+#include <utility>
+#include <vector>
 
 #include "behaviortree_cpp/contrib/magic_enum.hpp"
 #include "behaviortree_cpp/behavior_tree.h"
@@ -44,20 +46,11 @@ template <typename T>
 inline TreeNodeManifest CreateManifest(const std::string& ID,
                                        PortsList portlist = getProvidedPorts<T>())
 {
-  if constexpr( has_static_method_description<T>::value && has_static_method_metadata<T>::value )
+  if constexpr( has_static_method_metadata<T>::value )
   {
-    return {getType<T>(), ID, portlist, T::description(), T::metadata()};
+    return {getType<T>(), ID, portlist, T::metadata()};
   }
-  else if constexpr( has_static_method_description<T>::value && !has_static_method_metadata<T>::value )
-  {
-    return {getType<T>(), ID, portlist, T::description(), {}};
-  }
-  else if constexpr( !has_static_method_description<T>::value && has_static_method_metadata<T>::value )
-  {
-    return {getType<T>(), ID, portlist, {}, T::metadata()};
-  }
-
-  return {getType<T>(), ID, portlist, {}, {}};
+  return {getType<T>(), ID, portlist, {}};
 }
 
 #ifdef BT_PLUGIN_EXPORT
@@ -432,10 +425,10 @@ class BehaviorTreeFactory
   Tree createTree(const std::string& tree_name,
                   Blackboard::Ptr blackboard = Blackboard::create());
 
-  /// Add a description to a specific manifest. This description will be added
+  /// Add metadata to a specific manifest. This metadata will be added
   /// to <TreeNodesModel> with the function writeTreeNodesModelXML()
-  void addDescriptionToManifest(const std::string& node_id,
-                                const std::string& description);
+  void addMetadataToManifest(const std::string& node_id,
+                             const std::vector<std::pair<std::string, std::string>>& metadata);
 
   /**
    * @brief Add an Enum to the scripting language.

include/behaviortree_cpp/tree_node.h

@@ -17,6 +17,7 @@
 #include <exception>
 #include <mutex>
 #include <map>
+#include <utility>
 #include <vector>
 
 #include "behaviortree_cpp/utils/signal.h"
@@ -39,8 +40,7 @@ struct TreeNodeManifest
   NodeType type;
   std::string registration_ID;
   PortsList ports;
-  std::string description;
-  std::vector<ManifestMetadata> metadata;
+  std::vector<std::pair<std::string, std::string>> metadata;
 };
 
 using PortsRemapping = std::unordered_map<std::string, std::string>;

src/bt_factory.cpp

@@ -157,7 +157,7 @@ void BehaviorTreeFactory::registerSimpleCondition(
     return std::make_unique<SimpleConditionNode>(name, tick_functor, config);
   };
 
-  TreeNodeManifest manifest = {NodeType::CONDITION, ID, std::move(ports), {}, {}};
+  TreeNodeManifest manifest = {NodeType::CONDITION, ID, std::move(ports), {}};
   registerBuilder(manifest, builder);
 }
 
@@ -170,7 +170,7 @@ void BehaviorTreeFactory::registerSimpleAction(
     return std::make_unique<SimpleActionNode>(name, tick_functor, config);
   };
 
-  TreeNodeManifest manifest = {NodeType::ACTION, ID, std::move(ports), {}, {}};
+  TreeNodeManifest manifest = {NodeType::ACTION, ID, std::move(ports), {}};
   registerBuilder(manifest, builder);
 }
 
@@ -183,7 +183,7 @@ void BehaviorTreeFactory::registerSimpleDecorator(
     return std::make_unique<SimpleDecoratorNode>(name, tick_functor, config);
   };
 
-  TreeNodeManifest manifest = {NodeType::DECORATOR, ID, std::move(ports), {}, {}};
+  TreeNodeManifest manifest = {NodeType::DECORATOR, ID, std::move(ports), {}};
   registerBuilder(manifest, builder);
 }
 
@@ -435,15 +435,15 @@ Tree BehaviorTreeFactory::createTree(const std::string& tree_name,
   return tree;
 }
 
-void BehaviorTreeFactory::addDescriptionToManifest(const std::string& node_id,
-                                                   const std::string& description)
+void BehaviorTreeFactory::addMetadataToManifest(const std::string& node_id,
+                                                const std::vector<std::pair<std::string, std::string>>& metadata)
 {
   auto it = _p->manifests.find(node_id);
   if (it == _p->manifests.end())
   {
-    throw std::runtime_error("addDescriptionToManifest: wrong ID");
+    throw std::runtime_error("addMetadataToManifest: wrong ID");
   }
-  it->second.description = description;
+  it->second.metadata = metadata;
 }
 
 void BehaviorTreeFactory::registerScriptingEnum(StringView name, int value)

src/xml_parsing.cpp

@@ -982,29 +982,18 @@ void addNodeModelToXML(const TreeNodeManifest& model,
     element->InsertEndChild(port_element);
   }
 
-  if (!model.description.empty())
+  if (!model.metadata.empty())
   {
-    auto description_element = doc.NewElement("description");
-    description_element->SetText(model.description.c_str());
-    element->InsertEndChild(description_element);
-  }
-
-  for (const auto& metadata : model.metadata)
-  {
-    auto metadata_element = doc.NewElement(metadata.name.c_str());
+    auto metadata_root = doc.NewElement("MetadataFields");
 
-    if (metadata.representsText())
-    {
-      const auto element_text = std::get<std::string>(metadata.text_or_attribute);
-      metadata_element->SetText(element_text.c_str());
-    }
-    else
+    for (const auto& [name, value] : model.metadata)
     {
-      const auto [attribute_name, attribute_val] = std::get<ManifestMetadata::Attribute>(metadata.text_or_attribute);
-      metadata_element->SetAttribute(attribute_name.c_str(), attribute_val.c_str());
+      auto metadata_element = doc.NewElement("Metadata");
+      metadata_element->SetAttribute(name.c_str(), value.c_str());
+      metadata_root->InsertEndChild(metadata_element);
     }
 
-    element->InsertEndChild(metadata_element);
+    element->InsertEndChild(metadata_root);
   }
 
   model_root->InsertEndChild(element);

tests/gtest_factory.cpp

@@ -391,49 +391,12 @@ TEST(BehaviorTreeReload, ReloadSameTree)
   }
 }
 
-class DescriptiveAction : public SyncActionNode
+std::vector<std::pair<std::string, std::string>> makeTestMetadata()
 {
-public:
-  DescriptiveAction(const std::string& name, const NodeConfig& config):
-    SyncActionNode(name, config) {}
-
-  BT::NodeStatus tick() override {
-    return NodeStatus::SUCCESS;
-  }
-
-  static PortsList providedPorts() {
-    return {};
-  }
-
-  static std::string description() {
-    return "THE DESCRIPTION";
-  }
-};
-
-TEST(BehaviorTreeFactory, DescriptionMethod)
-{
-  BehaviorTreeFactory factory;
-  factory.registerNodeType<DescriptiveAction>("DescriptiveAction");
-  const auto& manifest = factory.manifests().at("DescriptiveAction");
-  ASSERT_EQ(manifest.description, "THE DESCRIPTION");
-
-  auto xml = writeTreeNodesModelXML(factory, false);
-  std::cout << xml << std::endl;
-
-  ASSERT_NE(xml.find( "<description>THE DESCRIPTION</description>"), std::string::npos);
-}
-
-std::vector<ManifestMetadata> makeTestMetadata()
-{
-  ManifestMetadata text_metadata;
-  text_metadata.name = "text_metadata";
-  text_metadata.text_or_attribute = "text";
-
-  ManifestMetadata attribute_metadata;
-  attribute_metadata.name = "attribute_metadata";
-  attribute_metadata.text_or_attribute = std::make_pair<std::string, std::string>("attribute_name", "attribute_value");
-
-  return {text_metadata, attribute_metadata};
+  return {
+    std::make_pair<std::string, std::string>("foo", "hello"),
+    std::make_pair<std::string, std::string>("bar", "42"),
+  };
 }
 
 class ActionWithMetadata : public SyncActionNode
@@ -450,28 +413,39 @@ class ActionWithMetadata : public SyncActionNode
     return {};
   }
 
-  static std::vector<ManifestMetadata> metadata() {
+  static std::vector<std::pair<std::string, std::string>> metadata() {
     return makeTestMetadata();
   }
 };
 
 TEST(BehaviorTreeFactory, ManifestMethod)
 {
+  const char* expectedXML = R"(
+        <Action ID="ActionWithMetadata">
+            <MetadataFields>
+                <Metadata foo="hello"/>
+                <Metadata bar="42"/>
+            </MetadataFields>
+        </Action>)";
+
   BehaviorTreeFactory factory;
   factory.registerNodeType<ActionWithMetadata>("ActionWithMetadata");
   const auto& manifest = factory.manifests().at("ActionWithMetadata");
-  ASSERT_EQ(manifest.metadata.size(), 2u);
-  auto expected_metadata = makeTestMetadata();
-  EXPECT_EQ(manifest.metadata[0].name, expected_metadata[0].name);
-  EXPECT_EQ(manifest.metadata[0].text_or_attribute, expected_metadata[0].text_or_attribute);
-  EXPECT_TRUE(manifest.metadata[0].representsText());
-  EXPECT_EQ(manifest.metadata[1].name, expected_metadata[1].name);
-  EXPECT_EQ(manifest.metadata[1].text_or_attribute, expected_metadata[1].text_or_attribute);
-  EXPECT_TRUE(manifest.metadata[1].representsAttribute());
+  EXPECT_EQ(manifest.metadata, makeTestMetadata());
 
   auto xml = writeTreeNodesModelXML(factory, false);
   std::cout << xml << std::endl;
 
-  ASSERT_NE(xml.find("<text_metadata>text</text_metadata>"), std::string::npos);
-  ASSERT_NE(xml.find("<attribute_metadata attribute_name=\"attribute_value\"/>"), std::string::npos);
+  EXPECT_NE(xml.find(expectedXML), std::string::npos);
+}
+
+TEST(BehaviorTreeFactory, addMetadataToManifest)
+{
+  BehaviorTreeFactory factory;
+  factory.registerNodeType<DummyNodes::SaySomething>("SaySomething");
+  const auto& initial_manifest = factory.manifests().at("SaySomething");
+  EXPECT_TRUE(initial_manifest.metadata.empty());
+  factory.addMetadataToManifest("SaySomething", makeTestMetadata());
+  const auto& modified_manifest = factory.manifests().at("SaySomething");
+  EXPECT_EQ(modified_manifest.metadata, makeTestMetadata());
 }