diff --git a/include/behaviortree_cpp/bt_factory.h b/include/behaviortree_cpp/bt_factory.h index 0ba3c86bb..cdc76146d 100644 --- a/include/behaviortree_cpp/bt_factory.h +++ b/include/behaviortree_cpp/bt_factory.h @@ -40,15 +40,31 @@ inline NodeBuilder CreateBuilder(Args... args) }; } +template +inline constexpr bool IsManifestAsync() +{ + return std::is_base_of_v || + std::is_base_of_v || + std::is_base_of_v; +} + template inline TreeNodeManifest CreateManifest(const std::string& ID, PortsList portlist = getProvidedPorts()) { + TreeNodeManifest manifest; + manifest.type = getType(); + manifest.registration_ID = ID; + manifest.ports = std::move(portlist); if constexpr(has_static_method_metadata::value) { - return { getType(), ID, portlist, T::metadata() }; + manifest.metadata = T::metadata(); } - return { getType(), ID, portlist, {} }; + if constexpr(IsManifestAsync()) + { + SetNodeManifestAsync(manifest); + } + return manifest; } #ifdef BT_PLUGIN_EXPORT @@ -472,6 +488,9 @@ class BehaviorTreeFactory /// to with the function writeTreeNodesModelXML() void addMetadataToManifest(const std::string& node_id, const KeyValueVector& metadata); + /// Mark a registered node as asynchronous for XML validation. + void markNodeAsAsynchronous(const std::string& node_id, bool is_async = true); + /** * @brief Add an Enum to the scripting language. * For instance if you do: diff --git a/include/behaviortree_cpp/tree_node.h b/include/behaviortree_cpp/tree_node.h index 9fe48568d..0866cffd8 100644 --- a/include/behaviortree_cpp/tree_node.h +++ b/include/behaviortree_cpp/tree_node.h @@ -41,6 +41,63 @@ struct TreeNodeManifest KeyValueVector metadata; }; +[[nodiscard]] inline bool IsReservedNodeMetadataField(StringView key) +{ + return key == "__bt_async"; +} + +inline void SetNodeManifestAsync(TreeNodeManifest& manifest, bool is_async = true) +{ + auto async_it = manifest.metadata.end(); + for(auto it = manifest.metadata.begin(); it != manifest.metadata.end();) + { + if(IsReservedNodeMetadataField(it->first)) + { + if(async_it == manifest.metadata.end()) + { + async_it = it; + ++it; + } + else + { + it = manifest.metadata.erase(it); + } + } + else + { + ++it; + } + } + + if(is_async) + { + if(async_it == manifest.metadata.end()) + { + manifest.metadata.emplace_back("__bt_async", "true"); + } + else + { + async_it->second = "true"; + } + } + else if(async_it != manifest.metadata.end()) + { + manifest.metadata.erase(async_it); + } +} + +[[nodiscard]] inline bool IsNodeManifestAsync(const TreeNodeManifest& manifest) +{ + for(const auto& [key, value] : manifest.metadata) + { + if(IsReservedNodeMetadataField(key)) + { + return value == "true"; + } + } + return false; +} + using PortsRemapping = std::unordered_map; using NonPortAttributes = std::unordered_map; diff --git a/include/behaviortree_cpp/xml_parsing.h b/include/behaviortree_cpp/xml_parsing.h index d815ef85b..3595e28a7 100644 --- a/include/behaviortree_cpp/xml_parsing.h +++ b/include/behaviortree_cpp/xml_parsing.h @@ -44,7 +44,8 @@ class XMLParser : public Parser }; void VerifyXML(const std::string& xml_text, - const std::unordered_map& registered_nodes); + const std::unordered_map& + registered_nodes); /** * @brief writeTreeNodesModelXML generates an XMl that contains the manifests in the diff --git a/src/bt_factory.cpp b/src/bt_factory.cpp index 9040e80f1..ef385d960 100644 --- a/src/bt_factory.cpp +++ b/src/bt_factory.cpp @@ -119,6 +119,8 @@ BehaviorTreeFactory::BehaviorTreeFactory() : _p(new PImpl) registerNodeType("AsyncFallback", true); registerNodeType("Sequence"); registerNodeType("AsyncSequence", true); + markNodeAsAsynchronous("AsyncFallback"); + markNodeAsAsynchronous("AsyncSequence"); registerNodeType("SequenceWithMemory"); #ifdef USE_BTCPP3_OLD_NAMES @@ -485,7 +487,23 @@ void BehaviorTreeFactory::addMetadataToManifest(const std::string& node_id, { throw std::runtime_error("addMetadataToManifest: wrong ID"); } + const bool is_async = IsNodeManifestAsync(it->second); it->second.metadata = metadata; + if(is_async) + { + SetNodeManifestAsync(it->second); + } +} + +void BehaviorTreeFactory::markNodeAsAsynchronous(const std::string& node_id, + bool is_async) +{ + auto it = _p->manifests.find(node_id); + if(it == _p->manifests.end()) + { + throw std::runtime_error("markNodeAsAsynchronous: wrong ID"); + } + SetNodeManifestAsync(it->second, is_async); } void BehaviorTreeFactory::registerScriptingEnum(StringView name, int value) diff --git a/src/xml_parsing.cpp b/src/xml_parsing.cpp index e37836d0f..90dbd50e8 100644 --- a/src/xml_parsing.cpp +++ b/src/xml_parsing.cpp @@ -439,18 +439,12 @@ void XMLParser::PImpl::loadDocImpl(XMLDocument* doc, bool add_includes) } // Collect the names of all nodes registered with the behavior tree factory - std::unordered_map registered_nodes; - for(const auto& it : factory->manifests()) - { - registered_nodes.insert({ it.first, it.second.type }); - } - XMLPrinter printer; doc->Print(&printer); auto xml_text = std::string(printer.CStr(), size_t(printer.CStrSize())); // Verify the validity of the XML before adding any behavior trees to the parser's list of registered trees - VerifyXML(xml_text, registered_nodes); + VerifyXML(xml_text, factory->manifests()); loadSubtreeModel(xml_root); @@ -473,7 +467,8 @@ void XMLParser::PImpl::loadDocImpl(XMLDocument* doc, bool add_includes) } void VerifyXML(const std::string& xml_text, - const std::unordered_map& registered_nodes) + const std::unordered_map& + registered_nodes) { XMLDocument doc; auto xml_error = doc.Parse(xml_text.c_str(), xml_text.size()); @@ -629,7 +624,7 @@ void VerifyXML(const std::string& xml_text, ThrowError(line_number, std::string("Node not recognized: ") + lookup_name); } - const auto node_type = search->second; + const auto node_type = search->second.type; const std::string& registered_name = search->first; if(node_type == NodeType::DECORATOR) @@ -665,11 +660,12 @@ void VerifyXML(const std::string& xml_text, ThrowError(child->GetLineNum(), std::string("Unknown node type: ") + child_name); } - const auto child_type = child_search->second; - if(child_type == NodeType::CONTROL && - ((child_name == "ThreadedAction") || - (child_name == "StatefulActionNode") || - (child_name == "CoroActionNode") || (child_name == "AsyncSequence"))) + const auto& child_manifest = child_search->second; + const bool is_async_child = + IsNodeManifestAsync(child_manifest) && + (child_manifest.type == NodeType::ACTION || + child_manifest.type == NodeType::CONTROL); + if(is_async_child) { ++async_count; if(async_count > 1) @@ -1340,12 +1336,19 @@ void addNodeModelToXML(const TreeNodeManifest& model, XMLDocument& doc, for(const auto& [name, value] : model.metadata) { + if(IsReservedNodeMetadataField(name)) + { + continue; + } auto metadata_element = doc.NewElement("Metadata"); metadata_element->SetAttribute(name.c_str(), value.c_str()); metadata_root->InsertEndChild(metadata_element); } - element->InsertEndChild(metadata_root); + if(metadata_root->FirstChildElement() != nullptr) + { + element->InsertEndChild(metadata_root); + } } model_root->InsertEndChild(element); diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 49fcce397..fde6559de 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -12,6 +12,14 @@ set_target_properties(plugin_issue953 PROPERTIES ) target_link_libraries(plugin_issue953 ${BTCPP_LIBRARY}) +add_library(plugin_issue1184 SHARED plugin_issue1184/plugin_issue1184.cpp) +target_compile_definitions(plugin_issue1184 PRIVATE BT_PLUGIN_EXPORT) +set_target_properties(plugin_issue1184 PROPERTIES + PREFIX "" + LIBRARY_OUTPUT_DIRECTORY "${CMAKE_CURRENT_BINARY_DIR}" +) +target_link_libraries(plugin_issue1184 ${BTCPP_LIBRARY}) + ###################################################### set(BT_TESTS @@ -55,6 +63,7 @@ set(BT_TESTS gtest_simple_string.cpp gtest_polymorphic_ports.cpp gtest_plugin_issue953.cpp + gtest_plugin_issue1184.cpp gtest_blackboard_thread_safety.cpp gtest_xml_null_subtree_id.cpp @@ -123,7 +132,8 @@ endif() target_compile_definitions(behaviortree_cpp_test PRIVATE BT_TEST_FOLDER="${CMAKE_CURRENT_SOURCE_DIR}") # Ensure plugin is built before tests run, and tests can find it -add_dependencies(behaviortree_cpp_test plugin_issue953) +add_dependencies(behaviortree_cpp_test plugin_issue953 plugin_issue1184) target_compile_definitions(behaviortree_cpp_test PRIVATE BT_PLUGIN_ISSUE953_PATH="$" + BT_PLUGIN_ISSUE1184_PATH="$" ) diff --git a/tests/gtest_basic_types.cpp b/tests/gtest_basic_types.cpp index 680c45ce3..aa426bdf0 100644 --- a/tests/gtest_basic_types.cpp +++ b/tests/gtest_basic_types.cpp @@ -408,6 +408,19 @@ TEST(BasicTypes, TreeNodeManifest) ASSERT_EQ(manifest.ports.size(), 2u); } +TEST(BasicTypes, TreeNodeManifestAsyncMetadata) +{ + TreeNodeManifest manifest; + EXPECT_FALSE(IsNodeManifestAsync(manifest)); + + SetNodeManifestAsync(manifest); + EXPECT_TRUE(IsNodeManifestAsync(manifest)); + EXPECT_TRUE(IsReservedNodeMetadataField("__bt_async")); + + SetNodeManifestAsync(manifest, false); + EXPECT_FALSE(IsNodeManifestAsync(manifest)); +} + // ============ Result type tests ============ TEST(BasicTypes, Result_Success) diff --git a/tests/gtest_factory.cpp b/tests/gtest_factory.cpp index 2c257b5b2..2df701b23 100644 --- a/tests/gtest_factory.cpp +++ b/tests/gtest_factory.cpp @@ -1,5 +1,7 @@ #include "behaviortree_cpp/xml_parsing.h" +#include "action_test_node.h" + #include #include #include @@ -495,6 +497,19 @@ TEST(BehaviorTreeFactory, addMetadataToManifest) EXPECT_EQ(modified_manifest.metadata, makeTestMetadata()); } +TEST(BehaviorTreeFactory, addMetadataToManifestPreservesAsyncMarker) +{ + BehaviorTreeFactory factory; + factory.registerNodeType("AsyncActionTest"); + + factory.addMetadataToManifest("AsyncActionTest", makeTestMetadata()); + + const auto& manifest = factory.manifests().at("AsyncActionTest"); + EXPECT_TRUE(IsNodeManifestAsync(manifest)); + EXPECT_EQ(manifest.metadata[0], makeTestMetadata()[0]); + EXPECT_EQ(manifest.metadata[1], makeTestMetadata()[1]); +} + // Action node used to reproduce issue #1046 (use-after-free on // manifest pointer). It calls getInput() for a port name that is // NOT in the XML, so getInputStamped falls through to the @@ -777,3 +792,42 @@ TEST(BehaviorTreeFactory, MalformedXML_UnknownNodeType) BehaviorTreeFactory factory; EXPECT_THROW((void)factory.createTreeFromText(xml), RuntimeError); } + +TEST(BehaviorTreeFactory, VerifyXMLRejectsManualAsyncControlInReactiveSequence) +{ + const char* xml_text_issue = R"( + + + + + + + + + + + + + )"; + + BehaviorTreeFactory factory; + + TreeNodeManifest manifest{ NodeType::CONTROL, "ManualAsyncFallback", {}, {} }; + SetNodeManifestAsync(manifest); + factory.registerBuilder( + manifest, [](const std::string& name, const NodeConfig&) -> std::unique_ptr { + return std::make_unique(name, true); + }); + + EXPECT_THROW((void)factory.createTreeFromText(xml_text_issue), RuntimeError); +} + +TEST(BehaviorTreeFactory, WriteTreeNodesModelXMLSkipsInternalAsyncMetadata) +{ + BehaviorTreeFactory factory; + factory.registerNodeType("AsyncActionTest"); + + const auto xml = writeTreeNodesModelXML(factory, false); + + EXPECT_EQ(xml.find("__bt_async"), std::string::npos); +} diff --git a/tests/gtest_plugin_issue1184.cpp b/tests/gtest_plugin_issue1184.cpp new file mode 100644 index 000000000..bcdf9d9f9 --- /dev/null +++ b/tests/gtest_plugin_issue1184.cpp @@ -0,0 +1,52 @@ +#include "behaviortree_cpp/bt_factory.h" + +#include + +#include + +using namespace BT; + +#ifndef BT_PLUGIN_ISSUE1184_PATH +#define BT_PLUGIN_ISSUE1184_PATH "plugin_issue1184.so" +#endif + +class PluginIssue1184Test : public testing::Test +{ +protected: + void SetUp() override + { + plugin_path_ = BT_PLUGIN_ISSUE1184_PATH; + + if(!std::filesystem::exists(plugin_path_)) + { + GTEST_SKIP() << "Plugin not found at: " << plugin_path_ << ". " + << "Make sure it's built before running this test."; + } + } + + std::string plugin_path_; +}; + +TEST_F(PluginIssue1184Test, VerifyXMLRejectsPluginAsyncControlInReactiveSequence) +{ + const char* xml_text = R"( + + + + + + + + + + + + + + )"; + + BehaviorTreeFactory factory; + factory.registerFromPlugin(plugin_path_); + + EXPECT_THROW((void)factory.createTreeFromText(xml_text), RuntimeError); +} diff --git a/tests/gtest_reactive.cpp b/tests/gtest_reactive.cpp index 55c9176de..c92ab1858 100644 --- a/tests/gtest_reactive.cpp +++ b/tests/gtest_reactive.cpp @@ -1,5 +1,6 @@ #include "test_helper.hpp" +#include "action_test_node.h" #include "behaviortree_cpp/bt_factory.h" #include "behaviortree_cpp/loggers/bt_observer.h" @@ -187,6 +188,51 @@ TEST(Reactive, TwoAsyncNodesInReactiveSequence) EXPECT_ANY_THROW(auto tree = factory.createTreeFromText(reactive_xml_text)); } +TEST(Reactive, TwoAsyncActionNodesInReactiveSequence) +{ + static const char* reactive_xml_text = R"( + + + + + + + + +)"; + + BT::BehaviorTreeFactory factory; + factory.registerNodeType("AsyncActionTest"); + + EXPECT_ANY_THROW(auto tree = factory.createTreeFromText(reactive_xml_text)); +} + +TEST(Reactive, AsyncFallbackAndAsyncSequenceInReactiveSequence) +{ + static const char* reactive_xml_text = R"( + + + + + + + + + + + + + + +)"; + + BT::BehaviorTreeFactory factory; + std::array counters{}; + RegisterTestTick(factory, "Test", counters); + + EXPECT_ANY_THROW(auto tree = factory.createTreeFromText(reactive_xml_text)); +} + // ============ Phase 4: Additional Reactive Tests ============ TEST(Reactive, ReactiveSequence_FirstChildFails) diff --git a/tests/plugin_issue1184/plugin_issue1184.cpp b/tests/plugin_issue1184/plugin_issue1184.cpp new file mode 100644 index 000000000..b2605f506 --- /dev/null +++ b/tests/plugin_issue1184/plugin_issue1184.cpp @@ -0,0 +1,13 @@ +#include "behaviortree_cpp/bt_factory.h" +#include "behaviortree_cpp/controls/fallback_node.h" + +BT_REGISTER_NODES(factory) +{ + BT::TreeNodeManifest manifest{ BT::NodeType::CONTROL, "PluginAsyncFallback", {}, {} }; + BT::SetNodeManifestAsync(manifest); + factory.registerBuilder( + manifest, + [](const std::string& name, const BT::NodeConfig&) -> std::unique_ptr { + return std::make_unique(name, true); + }); +}