From a4f0e1c3c3e4293b6f7d263b39613aa3841b6c84 Mon Sep 17 00:00:00 2001 From: David Vadovszki Date: Thu, 15 Oct 2026 12:05:55 -0600 Subject: [PATCH] fix(xml): accept SubTrees that share a name, as in 4.7.2 Upstream 4.8.4 throws "Duplicate SubTree path detected" when two SubTree nodes in one tree share a `name`. MoveIt Pro 10.1 ran on the fork's 4.7.2, which accepts them, and the MoveIt Pro editor writes name=ID on every SubTree it adds. Objectives that load on 10.1 failed to start on 10.2. Drop the check so duplicates share a path again, as in 4.7.2. Replace the upstream tests that expect a throw with tests for the accepted cases. Refs PickNikRobotics/moveit_pro#23867 Co-Authored-By: Claude Opus 5.5 --- src/xml_parsing.cpp | 17 ++------- tests/gtest_subtree.cpp | 82 ++++++++++++----------------------------- 2 files changed, 27 insertions(+), 72 deletions(-) diff --git a/src/xml_parsing.cpp b/src/xml_parsing.cpp index 8612f028b..f7b8a0cbd 100644 --- a/src/xml_parsing.cpp +++ b/src/xml_parsing.cpp @@ -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). recursivelyCreateSubtree(subtree_ID, subtree_path, // name subtree_path + "/", //prefix diff --git a/tests/gtest_subtree.cpp b/tests/gtest_subtree.cpp index 445dbecd2..f543de7bf 100644 --- a/tests/gtest_subtree.cpp +++ b/tests/gtest_subtree.cpp @@ -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"( - - + + + + + - + @@ -986,25 +988,27 @@ 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"( - - - - + - + @@ -1012,20 +1016,8 @@ TEST(SubTree, DuplicateSubTreeName_ErrorMessage) // 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 @@ -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"( - - - - - - - - - - - - -)"; - // clang-format on - - BehaviorTreeFactory factory; - - // Should throw RuntimeError because of duplicate SubTree names - ASSERT_THROW((void)factory.createTreeFromText(xml_text), RuntimeError); -}