From b45d9edbf96e70555d62ba7f48036841c0695a68 Mon Sep 17 00:00:00 2001 From: Shane Copenhagen Date: Thu, 8 Oct 2026 14:04:35 -0600 Subject: [PATCH] fix(blackboard): follow subtree remapping when unsetting entries --- .../actions/unset_blackboard_node.h | 4 +- include/behaviortree_cpp/blackboard.h | 33 ++- tests/gtest_blackboard.cpp | 200 ++++++++++++++++++ 3 files changed, 229 insertions(+), 8 deletions(-) diff --git a/include/behaviortree_cpp/actions/unset_blackboard_node.h b/include/behaviortree_cpp/actions/unset_blackboard_node.h index 0a2836e59..f076c3b93 100644 --- a/include/behaviortree_cpp/actions/unset_blackboard_node.h +++ b/include/behaviortree_cpp/actions/unset_blackboard_node.h @@ -17,7 +17,9 @@ namespace BT { /** - * Action that removes an entry from the blackboard and return SUCCESS. + * Action that removes an entry from the blackboard and returns SUCCESS. + * The key follows SubTree remaps, automatic remapping, and the '@' root prefix. + * Missing entries are a no-op; private keys beginning with '_' are not automatically remapped. */ class UnsetBlackboardNode : public SyncActionNode { diff --git a/include/behaviortree_cpp/blackboard.h b/include/behaviortree_cpp/blackboard.h index b16b7f011..b45b07a96 100644 --- a/include/behaviortree_cpp/blackboard.h +++ b/include/behaviortree_cpp/blackboard.h @@ -126,6 +126,8 @@ class Blackboard template void set(const std::string& key, const T& value); + /// Remove the entry resolved by getEntry(), including SubTree remaps and '@' root keys. + /// A missing entry is a no-op. void unset(const std::string& key); [[nodiscard]] const TypeInfo* entryInfo(const std::string& key); @@ -266,17 +268,34 @@ inline T Blackboard::get(const std::string& key) const inline void Blackboard::unset(const std::string& key) { - std::unique_lock storage_lock(storage_mutex_); - - // check local storage - auto it = storage_.find(key); - if(it == storage_.end()) + if(StartWith(key, '@')) { - // No entry, nothing to do. + rootBlackboard()->unset(key.substr(1, key.size() - 1)); return; } - storage_.erase(it); + { + std::unique_lock storage_lock(storage_mutex_); + auto it = storage_.find(key); + if(it != storage_.end()) + { + storage_.erase(it); + return; + } + } + // Release the local storage lock before following the same remapping as getEntry(). + if(auto parent = parent_bb_.lock()) + { + auto remap_it = internal_to_external_.find(key); + if(remap_it != internal_to_external_.cend()) + { + parent->unset(remap_it->second); + } + else if(autoremapping_ && !StartWith(key, '_')) + { + parent->unset(key); + } + } } template diff --git a/tests/gtest_blackboard.cpp b/tests/gtest_blackboard.cpp index f4ce45689..7f155a35f 100644 --- a/tests/gtest_blackboard.cpp +++ b/tests/gtest_blackboard.cpp @@ -13,6 +13,8 @@ #include "behaviortree_cpp/blackboard.h" #include "behaviortree_cpp/bt_factory.h" +#include + #include #include "../sample_nodes/dummy_nodes.h" @@ -840,3 +842,201 @@ TEST(BlackboardTest, GetLockedPortContentWithDefault_Issue942) // The value should be accessible from the blackboard ASSERT_EQ(tree.rootBlackboard()->get("value"), 42); } + +TEST(BlackboardTest, UnsetMissingKeyIsIdempotent) +{ + auto parent = Blackboard::create(); + auto child = Blackboard::create(parent); + child->addSubtreeRemapping("inner", "outer"); + child->enableAutoRemapping(true); + parent->set("keep", 42); + + for(const auto& key : { "", "missing", "inner", "@missing", "_missing" }) + { + SCOPED_TRACE(key); + child->unset(key); + child->unset(key); + EXPECT_EQ(child->getEntry(key), nullptr); + } + EXPECT_EQ(parent->get("keep"), 42); +} + +TEST(BlackboardTest, UnsetLocalKeyDoesNotEraseParent) +{ + auto parent = Blackboard::create(); + auto child = Blackboard::create(parent); + parent->set("value", 42); + child->set("value", 7); + + child->unset("value"); + + EXPECT_EQ(child->getEntry("value"), nullptr); + EXPECT_EQ(parent->get("value"), 42); +} + +TEST(BlackboardTest, UnsetFollowsExplicitRemappingAndAllowsRewrite) +{ + auto parent = Blackboard::create(); + auto child = Blackboard::create(parent); + auto sibling = Blackboard::create(parent); + child->addSubtreeRemapping("inner", "outer"); + sibling->addSubtreeRemapping("alias", "outer"); + parent->set("outer", 42); + ASSERT_EQ(child->get("inner"), 42); + + child->unset("inner"); + + EXPECT_EQ(parent->getEntry("outer"), nullptr); + EXPECT_EQ(child->getEntry("inner"), nullptr); + EXPECT_EQ(sibling->getEntry("alias"), nullptr); + child->set("inner", 7); + EXPECT_EQ(parent->get("outer"), 7); + EXPECT_EQ(sibling->get("alias"), 7); +} + +TEST(BlackboardTest, UnsetExplicitRemappingOverridesAutoRemapping) +{ + auto parent = Blackboard::create(); + auto child = Blackboard::create(parent); + child->addSubtreeRemapping("inner", "outer"); + child->enableAutoRemapping(true); + parent->set("inner", 7); + parent->set("outer", 42); + + child->unset("inner"); + + EXPECT_EQ(parent->getEntry("outer"), nullptr); + EXPECT_EQ(parent->get("inner"), 7); +} + +TEST(BlackboardTest, UnsetAutoRemappingKeepsPrivateKeysLocal) +{ + auto parent = Blackboard::create(); + auto child = Blackboard::create(parent); + child->enableAutoRemapping(true); + parent->set("shared", 42); + parent->set("_private", 7); + child->set("_private", 1); + + child->unset("shared"); + child->unset("_private"); + child->unset("_private"); + + EXPECT_EQ(parent->getEntry("shared"), nullptr); + EXPECT_EQ(child->getEntry("shared"), nullptr); + EXPECT_EQ(child->getEntry("_private"), nullptr); + EXPECT_EQ(parent->get("_private"), 7); +} + +TEST(BlackboardTest, UnsetPrivateKeyFollowsExplicitRemapping) +{ + auto parent = Blackboard::create(); + auto child = Blackboard::create(parent); + child->addSubtreeRemapping("_inner", "outer"); + child->enableAutoRemapping(true); + parent->set("outer", 42); + + child->unset("_inner"); + + EXPECT_EQ(parent->getEntry("outer"), nullptr); + EXPECT_EQ(child->getEntry("_inner"), nullptr); +} + +TEST(BlackboardTest, UnsetFollowsNestedRemapping) +{ + auto root = Blackboard::create(); + auto middle = Blackboard::create(root); + auto leaf = Blackboard::create(middle); + middle->addSubtreeRemapping("middle_key", "root_key"); + leaf->addSubtreeRemapping("leaf_key", "middle_key"); + root->set("root_key", 42); + + leaf->unset("leaf_key"); + + EXPECT_EQ(root->getEntry("root_key"), nullptr); + EXPECT_EQ(middle->getEntry("middle_key"), nullptr); + EXPECT_EQ(leaf->getEntry("leaf_key"), nullptr); +} + +TEST(BlackboardTest, UnsetRootPrefixBypassesLocalKeysAndRemapping) +{ + auto root = Blackboard::create(); + auto middle = Blackboard::create(root); + auto leaf = Blackboard::create(middle); + root->set("value", 42); + middle->set("value", 7); + leaf->set("value", 1); + leaf->addSubtreeRemapping("value", "other"); + + leaf->unset("@value"); + + EXPECT_EQ(root->getEntry("value"), nullptr); + EXPECT_EQ(middle->get("value"), 7); + EXPECT_EQ(leaf->get("value"), 1); + root->set("value", 42); + root->unset("@value"); + EXPECT_EQ(root->getEntry("value"), nullptr); +} + +TEST(BlackboardTest, UnsetLocalEntryTakesPrecedenceOverRemapping) +{ + auto parent = Blackboard::create(); + auto child = Blackboard::create(parent); + child->set("inner", 7); + parent->set("outer", 42); + child->addSubtreeRemapping("inner", "outer"); + child->enableAutoRemapping(true); + ASSERT_EQ(child->get("inner"), 7); + + child->unset("inner"); + + EXPECT_EQ(parent->get("outer"), 42); + EXPECT_EQ(child->get("inner"), 42); +} + +TEST(BlackboardTest, UnsetWithExpiredParentIsNoOp) +{ + auto parent = Blackboard::create(); + auto child = Blackboard::create(parent); + child->addSubtreeRemapping("inner", "outer"); + child->enableAutoRemapping(true); + parent.reset(); + + child->unset("inner"); + child->unset("shared"); + child->unset("@root_key"); + + EXPECT_EQ(child->getEntry("inner"), nullptr); + EXPECT_EQ(child->getEntry("shared"), nullptr); + EXPECT_EQ(child->getEntry("root_key"), nullptr); +} + +TEST(BlackboardTest, UnsetBlackboardNodeClearsRemappedSubtreeVariable) +{ + for(const auto& [remapping, key] : + { std::pair{ "inner=\"{outer}\"", "inner" }, + std::pair{ "_autoremap=\"true\"", "outer" }, std::pair{ "", "@outer" } }) + { + SCOPED_TRACE(key); + BehaviorTreeFactory factory; + const std::string xml = StrCat(R"( + + + + + + + + + + + )"); + auto tree = factory.createTreeFromText(xml); + + EXPECT_EQ(tree.tickOnce(), NodeStatus::SUCCESS); + EXPECT_EQ(tree.rootBlackboard()->getEntry("outer"), nullptr); + EXPECT_EQ(tree.subtrees.at(1)->blackboard->getEntry(key), nullptr); + } +}