Merge pull request #1105 from BehaviorTree/fix/880-subtree-from-text

Fix #880: createTreeFromText now finds previously registered subtrees

Author: facontidavide

src/bt_factory.cpp

@@ -12,14 +12,87 @@
 
 #include "behaviortree_cpp/bt_factory.h"
 
+#include "tinyxml2.h"
+
 #include "behaviortree_cpp/utils/shared_library.h"
 #include "behaviortree_cpp/utils/wildcards.hpp"
 #include "behaviortree_cpp/xml_parsing.h"
 
 #include <filesystem>
+#include <functional>
 
 namespace BT
 {
+namespace
+{
+
+// Extract the main tree ID from an XML root element.
+// Checks main_tree_to_execute attribute first, then falls back to the
+// single BehaviorTree ID if only one is defined.
+std::string detectMainTreeId(const tinyxml2::XMLElement* xml_root)
+{
+  if(const auto* attr = xml_root->Attribute("main_tree_to_execute"))
+  {
+    return attr;
+  }
+  int bt_count = 0;
+  std::string single_id;
+  for(const auto* bt_elem = xml_root->FirstChildElement("BehaviorTree");
+      bt_elem != nullptr; bt_elem = bt_elem->NextSiblingElement("BehaviorTree"))
+  {
+    bt_count++;
+    if(const auto* tree_id = bt_elem->Attribute("ID"))
+    {
+      single_id = tree_id;
+    }
+  }
+  if(bt_count == 1 && !single_id.empty())
+  {
+    return single_id;
+  }
+  return {};
+}
+
+// Load XML into parser and resolve which tree to instantiate.
+// Returns the resolved tree ID (may be empty if parser should use default).
+std::string loadXmlAndResolveTreeId(Parser* parser, const std::string& main_tree_ID,
+                                    const std::function<void()>& load_func)
+{
+  // When the main tree couldn't be determined from the raw XML
+  // (e.g. <BehaviorTree> without an ID), snapshot registered trees
+  // before loading so we can diff afterwards.
+  std::set<std::string> before_set;
+  if(main_tree_ID.empty())
+  {
+    const auto before = parser->registeredBehaviorTrees();
+    before_set.insert(before.begin(), before.end());
+  }
+
+  load_func();
+
+  // Try to identify the newly added tree by diffing.
+  if(main_tree_ID.empty())
+  {
+    const auto after = parser->registeredBehaviorTrees();
+    std::string single_new_tree;
+    int new_count = 0;
+    for(const auto& name : after)
+    {
+      if(before_set.count(name) == 0)
+      {
+        single_new_tree = name;
+        new_count++;
+      }
+    }
+    if(new_count == 1)
+    {
+      return single_new_tree;
+    }
+  }
+  return main_tree_ID;
+}
+
+}  // namespace
 
 bool WildcardMatch(std::string const& str, StringView filter)
 {
@@ -342,17 +415,20 @@ const std::set<std::string>& BehaviorTreeFactory::builtinNodes() const
 Tree BehaviorTreeFactory::createTreeFromText(const std::string& text,
                                              Blackboard::Ptr blackboard)
 {
-  if(!_p->parser->registeredBehaviorTrees().empty())
+  // Determine the main tree from the XML before loading into the shared parser.
+  tinyxml2::XMLDocument doc;
+  doc.Parse(text.c_str(), text.size());
+  std::string main_tree_ID;
+  if(const auto* root = doc.RootElement())
   {
-    std::cout << "WARNING: You executed BehaviorTreeFactory::createTreeFromText "
-                 "after registerBehaviorTreeFrom[File/Text].\n"
-                 "This is NOT, probably, what you want to do.\n"
-                 "You should probably use BehaviorTreeFactory::createTree, instead"
-              << std::endl;
+    main_tree_ID = detectMainTreeId(root);
   }
-  XMLParser parser(*this);
-  parser.loadFromText(text);
-  auto tree = parser.instantiateTree(blackboard);
+
+  const std::string resolved_ID = loadXmlAndResolveTreeId(
+      _p->parser.get(), main_tree_ID, [&] { _p->parser->loadFromText(text); });
+
+  Tree tree = resolved_ID.empty() ? _p->parser->instantiateTree(blackboard) :
+                                    _p->parser->instantiateTree(blackboard, resolved_ID);
   tree.manifests = this->manifests();
   tree.remapManifestPointers();
   return tree;
@@ -361,18 +437,20 @@ Tree BehaviorTreeFactory::createTreeFromText(const std::string& text,
 Tree BehaviorTreeFactory::createTreeFromFile(const std::filesystem::path& file_path,
                                              Blackboard::Ptr blackboard)
 {
-  if(!_p->parser->registeredBehaviorTrees().empty())
+  // Determine the main tree from the XML before loading into the shared parser.
+  tinyxml2::XMLDocument doc;
+  doc.LoadFile(file_path.string().c_str());
+  std::string main_tree_ID;
+  if(const auto* root = doc.RootElement())
   {
-    std::cout << "WARNING: You executed BehaviorTreeFactory::createTreeFromFile "
-                 "after registerBehaviorTreeFrom[File/Text].\n"
-                 "This is NOT, probably, what you want to do.\n"
-                 "You should probably use BehaviorTreeFactory::createTree, instead"
-              << std::endl;
+    main_tree_ID = detectMainTreeId(root);
   }
 
-  XMLParser parser(*this);
-  parser.loadFromFile(file_path);
-  auto tree = parser.instantiateTree(blackboard);
+  const std::string resolved_ID = loadXmlAndResolveTreeId(
+      _p->parser.get(), main_tree_ID, [&] { _p->parser->loadFromFile(file_path); });
+
+  Tree tree = resolved_ID.empty() ? _p->parser->instantiateTree(blackboard) :
+                                    _p->parser->instantiateTree(blackboard, resolved_ID);
   tree.manifests = this->manifests();
   tree.remapManifestPointers();
   return tree;

tests/gtest_factory.cpp

@@ -572,6 +572,41 @@ TEST(BehaviorTreeFactory, FactoryDestroyedBeforeTick)
 // instantiation paths can still overflow the stack with deeply nested
 // input.  The fix adds a depth limit.
 
+// Regression test for issue #880:
+// createTreeFromText should be able to reference subtrees that were
+// previously registered via registerBehaviorTreeFromText/FromFile.
+TEST(BehaviorTreeFactory, CreateTreeFromTextFindsRegisteredSubtree)
+{
+  // Step 1: register a subtree definition
+  const char* subtree_xml = R"(
+  <root BTCPP_format="4">
+    <BehaviorTree ID="MyTree">
+      <AlwaysSuccess/>
+    </BehaviorTree>
+  </root>)";
+
+  BehaviorTreeFactory factory;
+  factory.registerBehaviorTreeFromText(subtree_xml);
+
+  // Verify "MyTree" is registered
+  ASSERT_EQ(factory.registeredBehaviorTrees().size(), 1);
+
+  // Step 2: use createTreeFromText with XML that references the
+  // registered subtree via <SubTree ID="MyTree"/>
+  const char* main_xml = R"(
+  <root BTCPP_format="4">
+    <BehaviorTree ID="TestTree">
+      <SubTree ID="MyTree"/>
+    </BehaviorTree>
+  </root>)";
+
+  // This should NOT throw. Before the fix it throws:
+  //   "Can't find a tree with name: MyTree"
+  Tree tree;
+  ASSERT_NO_THROW(tree = factory.createTreeFromText(main_xml));
+  ASSERT_EQ(NodeStatus::SUCCESS, tree.tickWhileRunning());
+}
+
 TEST(BehaviorTreeFactory, MalformedXML_InvalidRoot)
 {
   // XML that is not valid XML at all