Skip to content

Commit 1716a60

Browse files
facontidavideclaude
andcommitted
Fix #880: createTreeFromText now finds previously registered subtrees
The issue was that createTreeFromText() and createTreeFromFile() used a temporary XMLParser that couldn't see subtrees registered via registerBehaviorTreeFromText() or registerBehaviorTreeFromFile(). This fix changes these methods to use the factory's shared parser instead, allowing them to resolve references to previously registered subtrees. Key changes: - createTreeFromText/createTreeFromFile now use the shared parser - Added main tree ID detection to identify which tree to instantiate - Maintains API and ABI compatibility (no header changes) - Added regression test for the reported use case Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
1 parent c8f92dc commit 1716a60

File tree

2 files changed

+131
-18
lines changed

2 files changed

+131
-18
lines changed

src/bt_factory.cpp

Lines changed: 96 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -12,14 +12,87 @@
1212

1313
#include "behaviortree_cpp/bt_factory.h"
1414

15+
#include "tinyxml2.h"
16+
1517
#include "behaviortree_cpp/utils/shared_library.h"
1618
#include "behaviortree_cpp/utils/wildcards.hpp"
1719
#include "behaviortree_cpp/xml_parsing.h"
1820

1921
#include <filesystem>
22+
#include <functional>
2023

2124
namespace BT
2225
{
26+
namespace
27+
{
28+
29+
// Extract the main tree ID from an XML root element.
30+
// Checks main_tree_to_execute attribute first, then falls back to the
31+
// single BehaviorTree ID if only one is defined.
32+
std::string detectMainTreeId(const tinyxml2::XMLElement* xml_root)
33+
{
34+
if(const auto* attr = xml_root->Attribute("main_tree_to_execute"))
35+
{
36+
return attr;
37+
}
38+
int bt_count = 0;
39+
std::string single_id;
40+
for(const auto* bt_elem = xml_root->FirstChildElement("BehaviorTree");
41+
bt_elem != nullptr; bt_elem = bt_elem->NextSiblingElement("BehaviorTree"))
42+
{
43+
bt_count++;
44+
if(const auto* tree_id = bt_elem->Attribute("ID"))
45+
{
46+
single_id = tree_id;
47+
}
48+
}
49+
if(bt_count == 1 && !single_id.empty())
50+
{
51+
return single_id;
52+
}
53+
return {};
54+
}
55+
56+
// Load XML into parser and resolve which tree to instantiate.
57+
// Returns the resolved tree ID (may be empty if parser should use default).
58+
std::string loadXmlAndResolveTreeId(Parser* parser, const std::string& main_tree_ID,
59+
const std::function<void()>& load_func)
60+
{
61+
// When the main tree couldn't be determined from the raw XML
62+
// (e.g. <BehaviorTree> without an ID), snapshot registered trees
63+
// before loading so we can diff afterwards.
64+
std::set<std::string> before_set;
65+
if(main_tree_ID.empty())
66+
{
67+
const auto before = parser->registeredBehaviorTrees();
68+
before_set.insert(before.begin(), before.end());
69+
}
70+
71+
load_func();
72+
73+
// Try to identify the newly added tree by diffing.
74+
if(main_tree_ID.empty())
75+
{
76+
const auto after = parser->registeredBehaviorTrees();
77+
std::string single_new_tree;
78+
int new_count = 0;
79+
for(const auto& name : after)
80+
{
81+
if(before_set.count(name) == 0)
82+
{
83+
single_new_tree = name;
84+
new_count++;
85+
}
86+
}
87+
if(new_count == 1)
88+
{
89+
return single_new_tree;
90+
}
91+
}
92+
return main_tree_ID;
93+
}
94+
95+
} // namespace
2396

2497
bool WildcardMatch(std::string const& str, StringView filter)
2598
{
@@ -342,17 +415,20 @@ const std::set<std::string>& BehaviorTreeFactory::builtinNodes() const
342415
Tree BehaviorTreeFactory::createTreeFromText(const std::string& text,
343416
Blackboard::Ptr blackboard)
344417
{
345-
if(!_p->parser->registeredBehaviorTrees().empty())
418+
// Determine the main tree from the XML before loading into the shared parser.
419+
tinyxml2::XMLDocument doc;
420+
doc.Parse(text.c_str(), text.size());
421+
std::string main_tree_ID;
422+
if(const auto* root = doc.RootElement())
346423
{
347-
std::cout << "WARNING: You executed BehaviorTreeFactory::createTreeFromText "
348-
"after registerBehaviorTreeFrom[File/Text].\n"
349-
"This is NOT, probably, what you want to do.\n"
350-
"You should probably use BehaviorTreeFactory::createTree, instead"
351-
<< std::endl;
424+
main_tree_ID = detectMainTreeId(root);
352425
}
353-
XMLParser parser(*this);
354-
parser.loadFromText(text);
355-
auto tree = parser.instantiateTree(blackboard);
426+
427+
const std::string resolved_ID = loadXmlAndResolveTreeId(
428+
_p->parser.get(), main_tree_ID, [&] { _p->parser->loadFromText(text); });
429+
430+
Tree tree = resolved_ID.empty() ? _p->parser->instantiateTree(blackboard) :
431+
_p->parser->instantiateTree(blackboard, resolved_ID);
356432
tree.manifests = this->manifests();
357433
tree.remapManifestPointers();
358434
return tree;
@@ -361,18 +437,20 @@ Tree BehaviorTreeFactory::createTreeFromText(const std::string& text,
361437
Tree BehaviorTreeFactory::createTreeFromFile(const std::filesystem::path& file_path,
362438
Blackboard::Ptr blackboard)
363439
{
364-
if(!_p->parser->registeredBehaviorTrees().empty())
440+
// Determine the main tree from the XML before loading into the shared parser.
441+
tinyxml2::XMLDocument doc;
442+
doc.LoadFile(file_path.string().c_str());
443+
std::string main_tree_ID;
444+
if(const auto* root = doc.RootElement())
365445
{
366-
std::cout << "WARNING: You executed BehaviorTreeFactory::createTreeFromFile "
367-
"after registerBehaviorTreeFrom[File/Text].\n"
368-
"This is NOT, probably, what you want to do.\n"
369-
"You should probably use BehaviorTreeFactory::createTree, instead"
370-
<< std::endl;
446+
main_tree_ID = detectMainTreeId(root);
371447
}
372448

373-
XMLParser parser(*this);
374-
parser.loadFromFile(file_path);
375-
auto tree = parser.instantiateTree(blackboard);
449+
const std::string resolved_ID = loadXmlAndResolveTreeId(
450+
_p->parser.get(), main_tree_ID, [&] { _p->parser->loadFromFile(file_path); });
451+
452+
Tree tree = resolved_ID.empty() ? _p->parser->instantiateTree(blackboard) :
453+
_p->parser->instantiateTree(blackboard, resolved_ID);
376454
tree.manifests = this->manifests();
377455
tree.remapManifestPointers();
378456
return tree;

tests/gtest_factory.cpp

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -572,6 +572,41 @@ TEST(BehaviorTreeFactory, FactoryDestroyedBeforeTick)
572572
// instantiation paths can still overflow the stack with deeply nested
573573
// input. The fix adds a depth limit.
574574

575+
// Regression test for issue #880:
576+
// createTreeFromText should be able to reference subtrees that were
577+
// previously registered via registerBehaviorTreeFromText/FromFile.
578+
TEST(BehaviorTreeFactory, CreateTreeFromTextFindsRegisteredSubtree)
579+
{
580+
// Step 1: register a subtree definition
581+
const char* subtree_xml = R"(
582+
<root BTCPP_format="4">
583+
<BehaviorTree ID="MyTree">
584+
<AlwaysSuccess/>
585+
</BehaviorTree>
586+
</root>)";
587+
588+
BehaviorTreeFactory factory;
589+
factory.registerBehaviorTreeFromText(subtree_xml);
590+
591+
// Verify "MyTree" is registered
592+
ASSERT_EQ(factory.registeredBehaviorTrees().size(), 1);
593+
594+
// Step 2: use createTreeFromText with XML that references the
595+
// registered subtree via <SubTree ID="MyTree"/>
596+
const char* main_xml = R"(
597+
<root BTCPP_format="4">
598+
<BehaviorTree ID="TestTree">
599+
<SubTree ID="MyTree"/>
600+
</BehaviorTree>
601+
</root>)";
602+
603+
// This should NOT throw. Before the fix it throws:
604+
// "Can't find a tree with name: MyTree"
605+
Tree tree;
606+
ASSERT_NO_THROW(tree = factory.createTreeFromText(main_xml));
607+
ASSERT_EQ(NodeStatus::SUCCESS, tree.tickWhileRunning());
608+
}
609+
575610
TEST(BehaviorTreeFactory, MalformedXML_InvalidRoot)
576611
{
577612
// XML that is not valid XML at all

0 commit comments

Comments
 (0)