Skip to content

Commit 5e085e5

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 5e085e5

2 files changed

Lines changed: 165 additions & 18 deletions

File tree

src/bt_factory.cpp

Lines changed: 130 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,8 @@
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"
@@ -21,6 +23,33 @@
2123
namespace BT
2224
{
2325

26+
// Extract the main tree ID from an XML root element.
27+
// Checks main_tree_to_execute attribute first, then falls back to the
28+
// single BehaviorTree ID if only one is defined.
29+
static std::string detectMainTreeId(const tinyxml2::XMLElement* xml_root)
30+
{
31+
if(auto attr = xml_root->Attribute("main_tree_to_execute"))
32+
{
33+
return attr;
34+
}
35+
int bt_count = 0;
36+
std::string single_id;
37+
for(auto* bt = xml_root->FirstChildElement("BehaviorTree"); bt != nullptr;
38+
bt = bt->NextSiblingElement("BehaviorTree"))
39+
{
40+
bt_count++;
41+
if(auto* id = bt->Attribute("ID"))
42+
{
43+
single_id = id;
44+
}
45+
}
46+
if(bt_count == 1 && !single_id.empty())
47+
{
48+
return single_id;
49+
}
50+
return {};
51+
}
52+
2453
bool WildcardMatch(std::string const& str, StringView filter)
2554
{
2655
return wildcards_match(str, { filter.data(), filter.size() });
@@ -342,17 +371,59 @@ const std::set<std::string>& BehaviorTreeFactory::builtinNodes() const
342371
Tree BehaviorTreeFactory::createTreeFromText(const std::string& text,
343372
Blackboard::Ptr blackboard)
344373
{
345-
if(!_p->parser->registeredBehaviorTrees().empty())
374+
// Determine the main tree from the XML text before loading into the
375+
// shared parser, which may already contain trees from previous
376+
// registerBehaviorTreeFrom*() or createTreeFrom*() calls.
377+
tinyxml2::XMLDocument doc;
378+
doc.Parse(text.c_str(), text.size());
379+
std::string main_tree_ID;
380+
if(auto* root = doc.RootElement())
381+
{
382+
main_tree_ID = detectMainTreeId(root);
383+
}
384+
385+
// When the main tree couldn't be determined from the raw XML
386+
// (e.g. <BehaviorTree> without an ID), snapshot registered trees
387+
// before loading so we can diff afterwards.
388+
std::set<std::string> before_set;
389+
if(main_tree_ID.empty())
390+
{
391+
auto before = _p->parser->registeredBehaviorTrees();
392+
before_set.insert(before.begin(), before.end());
393+
}
394+
395+
_p->parser->loadFromText(text);
396+
397+
// Try to identify the newly added tree by diffing.
398+
std::string resolved_ID = main_tree_ID;
399+
if(resolved_ID.empty())
400+
{
401+
auto after = _p->parser->registeredBehaviorTrees();
402+
std::string single_new_tree;
403+
int new_count = 0;
404+
for(const auto& name : after)
405+
{
406+
if(before_set.count(name) == 0)
407+
{
408+
single_new_tree = name;
409+
new_count++;
410+
}
411+
}
412+
if(new_count == 1)
413+
{
414+
resolved_ID = single_new_tree;
415+
}
416+
}
417+
418+
Tree tree;
419+
if(resolved_ID.empty())
420+
{
421+
tree = _p->parser->instantiateTree(blackboard);
422+
}
423+
else
346424
{
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;
425+
tree = _p->parser->instantiateTree(blackboard, resolved_ID);
352426
}
353-
XMLParser parser(*this);
354-
parser.loadFromText(text);
355-
auto tree = parser.instantiateTree(blackboard);
356427
tree.manifests = this->manifests();
357428
tree.remapManifestPointers();
358429
return tree;
@@ -361,18 +432,59 @@ Tree BehaviorTreeFactory::createTreeFromText(const std::string& text,
361432
Tree BehaviorTreeFactory::createTreeFromFile(const std::filesystem::path& file_path,
362433
Blackboard::Ptr blackboard)
363434
{
364-
if(!_p->parser->registeredBehaviorTrees().empty())
435+
// Determine the main tree from the file before loading into the
436+
// shared parser, which may already contain trees from previous
437+
// registerBehaviorTreeFrom*() or createTreeFrom*() calls.
438+
tinyxml2::XMLDocument doc;
439+
doc.LoadFile(file_path.string().c_str());
440+
std::string main_tree_ID;
441+
if(auto* root = doc.RootElement())
365442
{
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;
443+
main_tree_ID = detectMainTreeId(root);
371444
}
372445

373-
XMLParser parser(*this);
374-
parser.loadFromFile(file_path);
375-
auto tree = parser.instantiateTree(blackboard);
446+
// When the main tree couldn't be determined from the raw XML
447+
// (e.g. <BehaviorTree> without an ID), snapshot registered trees
448+
// before loading so we can diff afterwards.
449+
std::set<std::string> before_set;
450+
if(main_tree_ID.empty())
451+
{
452+
auto before = _p->parser->registeredBehaviorTrees();
453+
before_set.insert(before.begin(), before.end());
454+
}
455+
456+
_p->parser->loadFromFile(file_path);
457+
458+
// Try to identify the newly added tree by diffing.
459+
std::string resolved_ID = main_tree_ID;
460+
if(resolved_ID.empty())
461+
{
462+
auto after = _p->parser->registeredBehaviorTrees();
463+
std::string single_new_tree;
464+
int new_count = 0;
465+
for(const auto& name : after)
466+
{
467+
if(before_set.count(name) == 0)
468+
{
469+
single_new_tree = name;
470+
new_count++;
471+
}
472+
}
473+
if(new_count == 1)
474+
{
475+
resolved_ID = single_new_tree;
476+
}
477+
}
478+
479+
Tree tree;
480+
if(resolved_ID.empty())
481+
{
482+
tree = _p->parser->instantiateTree(blackboard);
483+
}
484+
else
485+
{
486+
tree = _p->parser->instantiateTree(blackboard, resolved_ID);
487+
}
376488
tree.manifests = this->manifests();
377489
tree.remapManifestPointers();
378490
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)