Skip to content

Commit c1a7ce9

Browse files
facontidavideclaude
andcommitted
Sort ports in the TreeNodesModel XML (#851)
PortsList is an unordered_map, so writeTreeNodesModelXML() emitted the ports in hash order, which differs across builds and shows up in Groot2. Sort them by direction (input, output, inout), then by name, as was done before d5ea17b. Keeping the order of providedPorts() would need to change the PortsList type (#870), which breaks API/ABI; this is the compatible subset. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
1 parent a9d6e56 commit c1a7ce9

2 files changed

Lines changed: 60 additions & 1 deletion

File tree

‎src/xml_parsing.cpp‎

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1309,8 +1309,24 @@ void addNodeModelToXML(const TreeNodeManifest& model, XMLDocument& doc,
13091309
XMLElement* element = doc.NewElement(toStr(model.type).c_str());
13101310
element->SetAttribute("ID", model.registration_ID.c_str());
13111311

1312-
for(const auto& [port_name, port_info] : model.ports)
1312+
// PortsList is unordered: sort by direction, then by name, so that the
1313+
// output (and the order shown in Groot2) is deterministic.
1314+
std::vector<const PortsList::value_type*> sorted_ports;
1315+
sorted_ports.reserve(model.ports.size());
1316+
for(const auto& port : model.ports)
13131317
{
1318+
sorted_ports.push_back(&port);
1319+
}
1320+
std::sort(sorted_ports.begin(), sorted_ports.end(),
1321+
[](const auto* lhs, const auto* rhs) {
1322+
const auto lhs_dir = lhs->second.direction();
1323+
const auto rhs_dir = rhs->second.direction();
1324+
return lhs_dir != rhs_dir ? lhs_dir < rhs_dir : lhs->first < rhs->first;
1325+
});
1326+
1327+
for(const auto* port : sorted_ports)
1328+
{
1329+
const auto& [port_name, port_info] = *port;
13141330
XMLElement* port_element = nullptr;
13151331
switch(port_info.direction())
13161332
{

‎tests/gtest_factory.cpp‎

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -505,6 +505,49 @@ TEST(BehaviorTreeFactory, WrittenXMLHasLibraryVersion)
505505
EXPECT_EQ(reloaded.tickWhileRunning(), NodeStatus::SUCCESS);
506506
}
507507

508+
class ActionWithManyPorts : public SyncActionNode
509+
{
510+
public:
511+
ActionWithManyPorts(const std::string& name, const NodeConfig& config)
512+
: SyncActionNode(name, config)
513+
{}
514+
515+
BT::NodeStatus tick() override
516+
{
517+
return NodeStatus::SUCCESS;
518+
}
519+
520+
static PortsList providedPorts()
521+
{
522+
return { OutputPort<int>("out_b"), InputPort<int>("in_c"),
523+
BidirectionalPort<int>("io_a"), InputPort<int>("in_a"),
524+
OutputPort<int>("out_a"), InputPort<int>("in_b") };
525+
}
526+
};
527+
528+
// Issue #851: PortsList is an unordered_map, so the model must sort the ports
529+
// itself, otherwise Groot2 shows them in a different order on every build.
530+
TEST(BehaviorTreeFactory, ModelPortsAreSorted)
531+
{
532+
BehaviorTreeFactory factory;
533+
factory.registerNodeType<ActionWithManyPorts>("ActionWithManyPorts");
534+
535+
const auto xml = writeTreeNodesModelXML(factory, false);
536+
const std::vector<std::string> expected_order = {
537+
R"(<input_port name="in_a")", R"(<input_port name="in_b")",
538+
R"(<input_port name="in_c")", R"(<output_port name="out_a")",
539+
R"(<output_port name="out_b")", R"(<inout_port name="io_a")"
540+
};
541+
size_t prev_pos = 0;
542+
for(const auto& port : expected_order)
543+
{
544+
const auto pos = xml.find(port);
545+
ASSERT_NE(pos, std::string::npos) << port;
546+
EXPECT_GT(pos, prev_pos) << port;
547+
prev_pos = pos;
548+
}
549+
}
550+
508551
TEST(BehaviorTreeFactory, ReturnByValue)
509552
{
510553
// Issue #937: BehaviorTreeFactory cannot be returned by value from

0 commit comments

Comments
 (0)