Skip to content

fix(xml): accept SubTrees that share a name, as in 4.7.2 #41

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
david-vadovszki merged 1 commit into from
Oct 15, 2026
Merged

fix(xml): accept SubTrees that share a name, as in 4.7.2 #41

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 3 additions & 14 deletions src/xml_parsing.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1114,20 +1114,9 @@ void BT::XMLParser::PImpl::recursivelyCreateSubtree(
subtree_path += subtree_ID + "::" + std::to_string(node->UID());
}

// Check if the path already exists - duplicate paths cause issues in Groot2
// and TreeObserver (see Groot2 issue #56)
for(const auto& sub : output_tree.subtrees)
{
if(sub->instance_name == subtree_path)
{
throw RuntimeError("Duplicate SubTree path detected: '", subtree_path,
"'. Multiple SubTree nodes with the same 'name' attribute "
"under the same parent are not allowed. "
"Please use unique names or omit the 'name' attribute "
"to auto-generate unique paths.");
}
}

// Upstream throws here when two SubTrees share a path (Groot2 issue #56). The fork
// keeps its 4.7.2 behavior and accepts them, because the MoveIt Pro editor writes
// name=ID on every SubTree (PickNikRobotics/moveit_pro#23867).
Comment thread
coderabbitai[bot] marked this conversation as resolved.
recursivelyCreateSubtree(subtree_ID,
subtree_path, // name
subtree_path + "/", //prefix
Expand Down
82 changes: 24 additions & 58 deletions tests/gtest_subtree.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -961,21 +961,23 @@ TEST(SubTree, SubstringTreeIDsAreNotRecursive)
ASSERT_NO_THROW(auto tree = factory.createTreeFromText(xml_text));
}

// Test for Groot2 issue #56: duplicate _fullpath when multiple subtrees have the same name
// https://github.com/BehaviorTree/Groot2/issues/56
//
// When two SubTree nodes under the same parent have the same "name" attribute,
// tree creation should fail with a clear error message.
TEST(SubTree, DuplicateSubTreeName_Groot2Issue56)
// PickNik fork: MoveIt Pro 10.1 and earlier ran on 4.7.2, which accepts SubTree nodes that share a
// `name`, and the MoveIt Pro editor writes name=ID on every SubTree it adds. Upstream rejects them
// since 4.8.4 (Groot2 issue #56). The fork accepts them until MoveIt Pro 11.0
// (PickNikRobotics/moveit_pro#23867). The instances share a path, as they did in 4.7.2.
TEST(SubTree, DuplicateSubTreeName_AcceptedAsIn472)
{
// clang-format off
static const char* xml_text = R"(
<root BTCPP_format="4" main_tree_to_execute="MainTree">
<BehaviorTree ID="MainTree">
<ParallelAll>
<SubTree ID="Worker" name="my_worker"/>
<Fallback>
<Sequence>
<SubTree ID="Worker" name="my_worker"/>
<SubTree ID="Worker" name="my_worker"/>
</Sequence>
<SubTree ID="Worker" name="my_worker"/>
</ParallelAll>
</Fallback>
</BehaviorTree>

<BehaviorTree ID="Worker">
Expand All @@ -986,46 +988,36 @@ TEST(SubTree, DuplicateSubTreeName_Groot2Issue56)
// clang-format on

BehaviorTreeFactory factory;
Tree tree = factory.createTreeFromText(xml_text);

// Should throw RuntimeError because of duplicate SubTree names
ASSERT_THROW((void)factory.createTreeFromText(xml_text), RuntimeError);
ASSERT_EQ(tree.subtrees.size(), 4);
for(size_t i = 1; i < tree.subtrees.size(); i++)
{
EXPECT_EQ(tree.subtrees[i]->instance_name, "my_worker");
}
ASSERT_EQ(tree.tickWhileRunning(), NodeStatus::SUCCESS);
}

// Additional test to verify the error message content
TEST(SubTree, DuplicateSubTreeName_ErrorMessage)
// An empty name at the top of the main tree gives the SubTree the root's empty path.
TEST(SubTree, EmptySubTreeNameAtRoot_AcceptedAsIn472)
{
// clang-format off
static const char* xml_text = R"(
<root BTCPP_format="4" main_tree_to_execute="MainTree">
<BehaviorTree ID="MainTree">
<Sequence>
<SubTree ID="Task" name="my_task"/>
<SubTree ID="Task" name="my_task"/>
</Sequence>
<SubTree ID="Worker" name=""/>
</BehaviorTree>

<BehaviorTree ID="Task">
<BehaviorTree ID="Worker">
<AlwaysSuccess/>
</BehaviorTree>
</root>
)";
// clang-format on

BehaviorTreeFactory factory;

try
{
(void)factory.createTreeFromText(xml_text);
FAIL() << "Expected RuntimeError to be thrown";
}
catch(const RuntimeError& e)
{
std::string msg = e.what();
EXPECT_TRUE(msg.find("Duplicate SubTree path") != std::string::npos)
<< "Error message should mention 'Duplicate SubTree path'. Got: " << msg;
EXPECT_TRUE(msg.find("my_task") != std::string::npos)
<< "Error message should mention the duplicate path 'my_task'. Got: " << msg;
}
Tree tree = factory.createTreeFromText(xml_text);
ASSERT_EQ(tree.tickWhileRunning(), NodeStatus::SUCCESS);
}

// Test that unique names under the same parent work correctly
Expand Down Expand Up @@ -1097,29 +1089,3 @@ TEST(SubTree, NoNameAttribute_AutoGeneratesUniquePaths)
auto status = tree.tickWhileRunning();
ASSERT_EQ(status, NodeStatus::SUCCESS);
}

// Test nested subtrees - duplicate names at the same level should fail
TEST(SubTree, NestedDuplicateNames_ShouldFail)
{
// clang-format off
static const char* xml_text = R"(
<root BTCPP_format="4" main_tree_to_execute="MainTree">
<BehaviorTree ID="MainTree">
<Sequence>
<SubTree ID="Level1" name="task"/>
<SubTree ID="Level1" name="task"/>
</Sequence>
</BehaviorTree>

<BehaviorTree ID="Level1">
<AlwaysSuccess name="work"/>
</BehaviorTree>
</root>
)";
// clang-format on

BehaviorTreeFactory factory;

// Should throw RuntimeError because of duplicate SubTree names
ASSERT_THROW((void)factory.createTreeFromText(xml_text), RuntimeError);
}
Loading