Skip to content

Commit c910871

Browse files
L4co77claude
L4co77
and
claude
committed
Preserve numeric types for literal subtree port values
When literal values are passed to SubTree ports (not blackboard remapping), detect numeric types (int64_t, double) before storing them in the child blackboard. Previously all literals were stored as std::string, which caused type-mismatch errors in Script expressions that tried to do arithmetic. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 parent c852132 commit c910871

2 files changed

Lines changed: 322 additions & 2 deletions

File tree

‎src/xml_parsing.cpp‎

Lines changed: 42 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
* WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
1111
*/
1212

13+
#include <charconv>
1314
#include <cstdio>
1415
#include <cstring>
1516
#include <functional>
@@ -1000,10 +1001,49 @@ void BT::XMLParser::PImpl::recursivelyCreateSubtree(const std::string& tree_ID,
10001001
}
10011002
else
10021003
{
1003-
// constant string: just set that constant value into the BB
1004+
// constant value: set it into the BB with appropriate type
10041005
// IMPORTANT: this must not be autoremapped!!!
10051006
new_bb->enableAutoRemapping(false);
1006-
new_bb->set(port_name, static_cast<std::string>(port_value));
1007+
const std::string str_value(port_value);
1008+
1009+
// Try to preserve numeric types so that Script expressions
1010+
// can perform arithmetic without type-mismatch errors.
1011+
// Use std::from_chars with strict full-string validation to avoid
1012+
// false positives on compound strings like "1;2;3" or "2.2;2.4".
1013+
bool stored = false;
1014+
if(!str_value.empty())
1015+
{
1016+
const char* begin = str_value.data();
1017+
const char* end = begin + str_value.size();
1018+
// Try integer first (no decimal point, no exponent notation)
1019+
if(str_value.find('.') == std::string::npos &&
1020+
str_value.find('e') == std::string::npos &&
1021+
str_value.find('E') == std::string::npos)
1022+
{
1023+
int64_t int_val = 0;
1024+
auto [ptr, ec] = std::from_chars(begin, end, int_val);
1025+
if(ec == std::errc() && ptr == end)
1026+
{
1027+
new_bb->set(port_name, int_val);
1028+
stored = true;
1029+
}
1030+
}
1031+
// Try double
1032+
if(!stored)
1033+
{
1034+
double dbl_val = 0;
1035+
auto [ptr, ec] = std::from_chars(begin, end, dbl_val);
1036+
if(ec == std::errc() && ptr == end)
1037+
{
1038+
new_bb->set(port_name, dbl_val);
1039+
stored = true;
1040+
}
1041+
}
1042+
}
1043+
if(!stored)
1044+
{
1045+
new_bb->set(port_name, str_value);
1046+
}
10071047
new_bb->enableAutoRemapping(do_autoremap);
10081048
}
10091049
}

‎tests/gtest_subtree.cpp‎

Lines changed: 280 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -757,3 +757,283 @@ TEST(SubTree, SubtreeNameNotRegistered)
757757
ASSERT_ANY_THROW(auto tree = factory.createTreeFromText(xml_text));
758758
ASSERT_ANY_THROW(factory.registerBehaviorTreeFromText(xml_text));
759759
}
760+
TEST(SubTree, RecursiveSubtree)
761+
{
762+
// clang-format off
763+
764+
static const char* xml_text = R"(
765+
<root BTCPP_format="4" >
766+
<BehaviorTree ID="MainTree">
767+
<Sequence name="root">
768+
<AlwaysSuccess/>
769+
<SubTree ID="MainTree" />
770+
</Sequence>
771+
</BehaviorTree>
772+
</root>
773+
)";
774+
775+
// clang-format on
776+
BehaviorTreeFactory factory;
777+
778+
ASSERT_ANY_THROW(auto tree = factory.createTreeFromText(xml_text));
779+
}
780+
781+
TEST(SubTree, RecursiveCycle)
782+
{
783+
// clang-format off
784+
785+
static const char* xml_text = R"(
786+
<root BTCPP_format="4" main_tree_to_execute="MainTree">
787+
<BehaviorTree ID="MainTree">
788+
<Sequence name="root">
789+
<AlwaysSuccess/>
790+
<SubTree ID="TreeA" />
791+
</Sequence>
792+
</BehaviorTree>
793+
794+
<BehaviorTree ID="TreeA">
795+
<Sequence name="root">
796+
<AlwaysSuccess/>
797+
<SubTree ID="TreeB" />
798+
</Sequence>
799+
</BehaviorTree>
800+
801+
<BehaviorTree ID="TreeB">
802+
<Sequence name="root">
803+
<AlwaysSuccess/>
804+
<SubTree ID="MainTree" />
805+
</Sequence>
806+
</BehaviorTree>
807+
</root>
808+
)";
809+
810+
// clang-format on
811+
BehaviorTreeFactory factory;
812+
813+
ASSERT_ANY_THROW(auto tree = factory.createTreeFromText(xml_text));
814+
}
815+
816+
TEST(SubTree, SubstringTreeIDsAreNotRecursive)
817+
{
818+
// Verify that tree IDs which are substrings of each other do NOT
819+
// incorrectly trigger the recursive cycle detection.
820+
// clang-format off
821+
822+
static const char* xml_text = R"(
823+
<root BTCPP_format="4" main_tree_to_execute="Tree">
824+
<BehaviorTree ID="Tree">
825+
<SubTree ID="TreeABC" />
826+
</BehaviorTree>
827+
828+
<BehaviorTree ID="TreeABC">
829+
<AlwaysSuccess/>
830+
</BehaviorTree>
831+
</root>
832+
)";
833+
834+
// clang-format on
835+
BehaviorTreeFactory factory;
836+
837+
ASSERT_NO_THROW(auto tree = factory.createTreeFromText(xml_text));
838+
}
839+
840+
// Test for Groot2 issue #56: duplicate _fullpath when multiple subtrees have the same name
841+
// https://github.com/BehaviorTree/Groot2/issues/56
842+
//
843+
// When two SubTree nodes under the same parent have the same "name" attribute,
844+
// tree creation should fail with a clear error message.
845+
TEST(SubTree, DuplicateSubTreeName_Groot2Issue56)
846+
{
847+
// clang-format off
848+
static const char* xml_text = R"(
849+
<root BTCPP_format="4" main_tree_to_execute="MainTree">
850+
<BehaviorTree ID="MainTree">
851+
<ParallelAll>
852+
<SubTree ID="Worker" name="my_worker"/>
853+
<SubTree ID="Worker" name="my_worker"/>
854+
</ParallelAll>
855+
</BehaviorTree>
856+
857+
<BehaviorTree ID="Worker">
858+
<AlwaysSuccess name="do_work"/>
859+
</BehaviorTree>
860+
</root>
861+
)";
862+
// clang-format on
863+
864+
BehaviorTreeFactory factory;
865+
866+
// Should throw RuntimeError because of duplicate SubTree names
867+
ASSERT_THROW((void)factory.createTreeFromText(xml_text), RuntimeError);
868+
}
869+
870+
// Additional test to verify the error message content
871+
TEST(SubTree, DuplicateSubTreeName_ErrorMessage)
872+
{
873+
// clang-format off
874+
static const char* xml_text = R"(
875+
<root BTCPP_format="4" main_tree_to_execute="MainTree">
876+
<BehaviorTree ID="MainTree">
877+
<Sequence>
878+
<SubTree ID="Task" name="my_task"/>
879+
<SubTree ID="Task" name="my_task"/>
880+
</Sequence>
881+
</BehaviorTree>
882+
883+
<BehaviorTree ID="Task">
884+
<AlwaysSuccess/>
885+
</BehaviorTree>
886+
</root>
887+
)";
888+
// clang-format on
889+
890+
BehaviorTreeFactory factory;
891+
892+
try
893+
{
894+
(void)factory.createTreeFromText(xml_text);
895+
FAIL() << "Expected RuntimeError to be thrown";
896+
}
897+
catch(const RuntimeError& e)
898+
{
899+
std::string msg = e.what();
900+
EXPECT_TRUE(msg.find("Duplicate SubTree path") != std::string::npos)
901+
<< "Error message should mention 'Duplicate SubTree path'. Got: " << msg;
902+
EXPECT_TRUE(msg.find("my_task") != std::string::npos)
903+
<< "Error message should mention the duplicate path 'my_task'. Got: " << msg;
904+
}
905+
}
906+
907+
// Test that unique names under the same parent work correctly
908+
TEST(SubTree, UniqueSubTreeNames_WorksCorrectly)
909+
{
910+
// clang-format off
911+
static const char* xml_text = R"(
912+
<root BTCPP_format="4" main_tree_to_execute="MainTree">
913+
<BehaviorTree ID="MainTree">
914+
<ParallelAll>
915+
<SubTree ID="Worker" name="worker_1"/>
916+
<SubTree ID="Worker" name="worker_2"/>
917+
</ParallelAll>
918+
</BehaviorTree>
919+
920+
<BehaviorTree ID="Worker">
921+
<AlwaysSuccess name="do_work"/>
922+
</BehaviorTree>
923+
</root>
924+
)";
925+
// clang-format on
926+
927+
BehaviorTreeFactory factory;
928+
Tree tree = factory.createTreeFromText(xml_text);
929+
930+
// Verify paths are unique
931+
std::set<std::string> all_paths;
932+
tree.applyVisitor([&](TreeNode* node) {
933+
EXPECT_EQ(all_paths.count(node->fullPath()), 0);
934+
all_paths.insert(node->fullPath());
935+
});
936+
937+
ASSERT_EQ(tree.subtrees.size(), 3);
938+
auto status = tree.tickWhileRunning();
939+
ASSERT_EQ(status, NodeStatus::SUCCESS);
940+
}
941+
942+
// Test that omitting name attribute auto-generates unique paths
943+
TEST(SubTree, NoNameAttribute_AutoGeneratesUniquePaths)
944+
{
945+
// clang-format off
946+
static const char* xml_text = R"(
947+
<root BTCPP_format="4" main_tree_to_execute="MainTree">
948+
<BehaviorTree ID="MainTree">
949+
<ParallelAll>
950+
<SubTree ID="Worker"/>
951+
<SubTree ID="Worker"/>
952+
</ParallelAll>
953+
</BehaviorTree>
954+
955+
<BehaviorTree ID="Worker">
956+
<AlwaysSuccess name="do_work"/>
957+
</BehaviorTree>
958+
</root>
959+
)";
960+
// clang-format on
961+
962+
BehaviorTreeFactory factory;
963+
Tree tree = factory.createTreeFromText(xml_text);
964+
965+
// Verify paths are unique (auto-generated with UID)
966+
std::set<std::string> all_paths;
967+
tree.applyVisitor([&](TreeNode* node) {
968+
EXPECT_EQ(all_paths.count(node->fullPath()), 0);
969+
all_paths.insert(node->fullPath());
970+
});
971+
972+
ASSERT_EQ(tree.subtrees.size(), 3);
973+
auto status = tree.tickWhileRunning();
974+
ASSERT_EQ(status, NodeStatus::SUCCESS);
975+
}
976+
977+
// Test nested subtrees - duplicate names at the same level should fail
978+
TEST(SubTree, NestedDuplicateNames_ShouldFail)
979+
{
980+
// clang-format off
981+
static const char* xml_text = R"(
982+
<root BTCPP_format="4" main_tree_to_execute="MainTree">
983+
<BehaviorTree ID="MainTree">
984+
<Sequence>
985+
<SubTree ID="Level1" name="task"/>
986+
<SubTree ID="Level1" name="task"/>
987+
</Sequence>
988+
</BehaviorTree>
989+
990+
<BehaviorTree ID="Level1">
991+
<AlwaysSuccess name="work"/>
992+
</BehaviorTree>
993+
</root>
994+
)";
995+
// clang-format on
996+
997+
BehaviorTreeFactory factory;
998+
999+
// Should throw RuntimeError because of duplicate SubTree names
1000+
ASSERT_THROW((void)factory.createTreeFromText(xml_text), RuntimeError);
1001+
}
1002+
1003+
// Regression test: literal numeric values passed to subtrees should preserve
1004+
// their numeric type so that Script expressions can do arithmetic.
1005+
TEST(SubTree, LiteralNumericPortsPreserveType)
1006+
{
1007+
// clang-format off
1008+
static const char* xml_text = R"(
1009+
<root BTCPP_format="4" main_tree_to_execute="MainTree">
1010+
1011+
<BehaviorTree ID="MainTree">
1012+
<Sequence>
1013+
<SubTree ID="DoMath" int_val="42" dbl_val="3.14" str_val="hello"
1014+
remapped_val="{from_parent}" />
1015+
</Sequence>
1016+
</BehaviorTree>
1017+
1018+
<BehaviorTree ID="DoMath">
1019+
<Sequence>
1020+
<ScriptCondition code=" int_val + 1 == 43 " />
1021+
<ScriptCondition code=" dbl_val > 3.0 " />
1022+
<ScriptCondition code=" remapped_val + 1 == 101 " />
1023+
</Sequence>
1024+
</BehaviorTree>
1025+
1026+
</root>
1027+
)";
1028+
// clang-format on
1029+
1030+
BehaviorTreeFactory factory;
1031+
1032+
auto tree = factory.createTreeFromText(xml_text);
1033+
1034+
// Set the remapped parent value as an integer
1035+
tree.rootBlackboard()->set("from_parent", 100);
1036+
1037+
const auto status = tree.tickWhileRunning();
1038+
ASSERT_EQ(status, NodeStatus::SUCCESS);
1039+
}

0 commit comments

Comments
 (0)