diff --git a/include/behaviortree_cpp/bt_factory.h b/include/behaviortree_cpp/bt_factory.h index 9919b4307..3ac77ea11 100644 --- a/include/behaviortree_cpp/bt_factory.h +++ b/include/behaviortree_cpp/bt_factory.h @@ -320,6 +320,10 @@ class BehaviorTreeFactory * * where "tree_id" come from the XML attribute * + * If a tree with the same ID is already registered, the new definition + * replaces it. A warning is printed when the previous definition came from + * a different file (or from the same document); reloading the same file + * is silent. */ void registerBehaviorTreeFromFile(const std::filesystem::path& filename); diff --git a/src/xml_parsing.cpp b/src/xml_parsing.cpp index 02ee7fb3f..2470ee602 100644 --- a/src/xml_parsing.cpp +++ b/src/xml_parsing.cpp @@ -49,6 +49,7 @@ #include #include +#include #ifdef USING_ROS2 #include "ament_index_cpp/version.h" @@ -248,6 +249,7 @@ struct XMLParser::PImpl std::list > opened_documents; std::map tree_roots; + std::unordered_map tree_sources; const BehaviorTreeFactory* factory = nullptr; @@ -268,6 +270,7 @@ struct XMLParser::PImpl current_path = std::filesystem::current_path(); opened_documents.clear(); tree_roots.clear(); + tree_sources.clear(); } private: @@ -471,6 +474,9 @@ void XMLParser::PImpl::loadDocImpl(XMLDocument* doc, bool add_includes) loadSubtreeModel(xml_root); // Register each BehaviorTree within the XML + const std::string source = + include_stack.empty() ? "" : include_stack.back().string(); + std::unordered_set ids_in_document; for(auto bt_node = xml_root->FirstChildElement("BehaviorTree"); bt_node != nullptr; bt_node = bt_node->NextSiblingElement("BehaviorTree")) { @@ -484,7 +490,20 @@ void XMLParser::PImpl::loadDocImpl(XMLDocument* doc, bool add_includes) tree_name = "BehaviorTree_" + std::to_string(suffix_count++); } + // An ID that is already registered is replaced by the new definition. + // Warn when the previous one came from another source, or from this same + // document; reloading a file, or including it twice, stays silent. + const auto previous = tree_sources.find(tree_name); + const bool repeated_in_document = !ids_in_document.insert(tree_name).second; + if(repeated_in_document || + (previous != tree_sources.end() && previous->second != source)) + { + std::cout << "Warning: BehaviorTree ID [" << tree_name << "] in [" << source + << "] replaces the one previously registered from [" + << (repeated_in_document ? source : previous->second) << "]" << std::endl; + } tree_roots[tree_name] = bt_node; + tree_sources[tree_name] = source; } } diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 49fcce397..f0229596d 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -23,6 +23,7 @@ set(BT_TESTS gtest_blackboard.cpp gtest_coroutines.cpp gtest_decorator.cpp + gtest_duplicate_tree.cpp gtest_enums.cpp gtest_factory.cpp gtest_fallback.cpp diff --git a/tests/gtest_duplicate_tree.cpp b/tests/gtest_duplicate_tree.cpp new file mode 100644 index 000000000..591d6a3dc --- /dev/null +++ b/tests/gtest_duplicate_tree.cpp @@ -0,0 +1,253 @@ +/* Copyright (C) 2026 - duplicate BehaviorTree ID registration tests */ + +#include "behaviortree_cpp/bt_factory.h" + +#include +#include +#include +#include + +#include + +using namespace BT; + +namespace +{ +class TempSubdir +{ +public: + TempSubdir() + { + const auto stamp = std::chrono::steady_clock::now().time_since_epoch().count(); + path = std::filesystem::temp_directory_path() / + ("bt_duplicate_tree_" + std::to_string(stamp)); + std::filesystem::create_directories(path); + } + + ~TempSubdir() + { + std::error_code ec; + std::filesystem::remove_all(path, ec); + } + + std::filesystem::path path; + + TempSubdir(const TempSubdir&) = delete; + TempSubdir& operator=(const TempSubdir&) = delete; +}; + +void writeFile(const std::filesystem::path& p, const std::string& content) +{ + std::ofstream out(p.string()); + out << content; +} + +std::string minimalTreeXml(const std::string& tree_id, const std::string& node = "AlwaysS" + "uccess") +{ + return R"( + + + <)" + + node + + R"(/> + + + )"; +} + +// The parser reports included files by their canonical path (symlinks +// resolved, e.g. /var -> /private/var on macOS). +std::string canonicalPath(const std::filesystem::path& p) +{ + return std::filesystem::weakly_canonical(p).string(); +} + +// Runs the function and returns what it printed on stdout. +template +std::string captureStdout(Func&& func) +{ + testing::internal::CaptureStdout(); + func(); + return testing::internal::GetCapturedStdout(); +} + +bool hasWarning(const std::string& output) +{ + return output.find("Warning: BehaviorTree ID") != std::string::npos; +} + +} // namespace + +// A BehaviorTree ID registered again replaces the previous definition (last +// wins), with a warning when the two come from different sources. + +TEST(DuplicateBehaviorTreeId, DuplicateIdAcrossFilesWarnsAndLastWins) +{ + TempSubdir dir; + const auto path_a = dir.path / "a.xml"; + const auto path_b = dir.path / "b.xml"; + writeFile(path_a, minimalTreeXml("DupTree")); + writeFile(path_b, minimalTreeXml("DupTree", "AlwaysFailure")); + + BehaviorTreeFactory factory; + ASSERT_FALSE( + hasWarning(captureStdout([&] { factory.registerBehaviorTreeFromFile(path_a); }))); + const auto output = + captureStdout([&] { factory.registerBehaviorTreeFromFile(path_b); }); + EXPECT_TRUE(hasWarning(output)) << output; + EXPECT_NE(output.find(canonicalPath(path_a)), std::string::npos) << output; + EXPECT_NE(output.find(canonicalPath(path_b)), std::string::npos) << output; + + auto tree = factory.createTree("DupTree"); + EXPECT_EQ(tree.tickWhileRunning(), NodeStatus::FAILURE); +} + +TEST(DuplicateBehaviorTreeId, DuplicateIdWithinSameTextWarns) +{ + const std::string xml = R"( + + + + +)"; + + BehaviorTreeFactory factory; + const auto output = captureStdout([&] { factory.registerBehaviorTreeFromText(xml); }); + EXPECT_TRUE(hasWarning(output)) << output; + EXPECT_NE(output.find(""), std::string::npos) << output; + + auto tree = factory.createTree("foo"); + EXPECT_EQ(tree.tickWhileRunning(), NodeStatus::FAILURE); +} + +TEST(DuplicateBehaviorTreeId, DuplicateIdAcrossFileAndTextWarns) +{ + TempSubdir dir; + const auto path = dir.path / "one.xml"; + writeFile(path, minimalTreeXml("shared_id")); + + BehaviorTreeFactory factory; + factory.registerBehaviorTreeFromFile(path); + const auto output = captureStdout( + [&] { factory.registerBehaviorTreeFromText(minimalTreeXml("shared_id")); }); + EXPECT_TRUE(hasWarning(output)) << output; + EXPECT_NE(output.find(canonicalPath(path)), std::string::npos) << output; + EXPECT_NE(output.find(""), std::string::npos) << output; +} + +TEST(DuplicateBehaviorTreeId, ReloadingSameFileIsSilent) +{ + TempSubdir dir; + const auto path = dir.path / "reload.xml"; + writeFile(path, minimalTreeXml("same")); + + BehaviorTreeFactory factory; + factory.registerBehaviorTreeFromFile(path); + const auto output = captureStdout([&] { factory.registerBehaviorTreeFromFile(path); }); + EXPECT_FALSE(hasWarning(output)) << output; + ASSERT_NO_THROW(static_cast(factory.createTree("same"))); +} + +TEST(DuplicateBehaviorTreeId, RepeatedInlineTextIsSilent) +{ + BehaviorTreeFactory factory; + const std::string xml = minimalTreeXml("MainTree"); + const auto output = captureStdout([&] { + factory.createTreeFromText(xml); + factory.createTreeFromText(xml); + }); + EXPECT_FALSE(hasWarning(output)) << output; +} + +TEST(DuplicateBehaviorTreeId, DiamondIncludeIsSilent) +{ + // top includes left and right, which both include the same shared file + TempSubdir dir; + writeFile(dir.path / "shared.xml", minimalTreeXml("Shared")); + for(const char* side : { "left", "right" }) + { + writeFile(dir.path / (std::string(side) + ".xml"), std::string(R"( + + + +)"); + } + writeFile(dir.path / "top.xml", R"( + + + + + + +)"); + + BehaviorTreeFactory factory; + const auto output = + captureStdout([&] { factory.registerBehaviorTreeFromFile(dir.path / "top.xml"); }); + EXPECT_FALSE(hasWarning(output)) << output; + auto tree = factory.createTree("Top"); + EXPECT_EQ(tree.tickWhileRunning(), NodeStatus::SUCCESS); +} + +TEST(DuplicateBehaviorTreeId, ClearAllowsReload) +{ + TempSubdir dir; + const auto path = dir.path / "clear.xml"; + writeFile(path, minimalTreeXml("cleared")); + + BehaviorTreeFactory factory; + ASSERT_NO_THROW(factory.registerBehaviorTreeFromFile(path)); + factory.clearRegisteredBehaviorTrees(); + ASSERT_NO_THROW(factory.registerBehaviorTreeFromFile(path)); + ASSERT_NO_THROW(static_cast(factory.createTree("cleared"))); +} + +TEST(DuplicateBehaviorTreeId, DifferentIdsCoexist) +{ + TempSubdir dir; + const auto path_a = dir.path / "tree_a.xml"; + const auto path_b = dir.path / "tree_b.xml"; + writeFile(path_a, minimalTreeXml("TreeA")); + writeFile(path_b, minimalTreeXml("TreeB")); + + BehaviorTreeFactory factory; + const auto output = captureStdout([&] { + factory.registerBehaviorTreeFromFile(path_a); + factory.registerBehaviorTreeFromFile(path_b); + }); + EXPECT_FALSE(hasWarning(output)) << output; + + const auto ids = factory.registeredBehaviorTrees(); + ASSERT_EQ(ids.size(), 2); + ASSERT_NO_THROW(static_cast(factory.createTree("TreeA"))); + ASSERT_NO_THROW(static_cast(factory.createTree("TreeB"))); +} + +TEST(DuplicateBehaviorTreeId, IncludeDuplicateWarns) +{ + TempSubdir dir; + const auto inner_path = dir.path / "inner.xml"; + const auto outer_path = dir.path / "outer.xml"; + + writeFile(inner_path, minimalTreeXml("foo")); + writeFile(outer_path, R"( + + + + + + +)"); + + BehaviorTreeFactory factory; + const auto output = + captureStdout([&] { factory.registerBehaviorTreeFromFile(outer_path); }); + EXPECT_TRUE(hasWarning(output)) << output; + EXPECT_NE(output.find(canonicalPath(inner_path)), std::string::npos) << output; + + // the including file is registered after its includes, so it wins + auto tree = factory.createTree("foo"); + EXPECT_EQ(tree.tickWhileRunning(), NodeStatus::FAILURE); +}