Skip to content

Commit 6ed3dbe

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 1a21a78 commit 6ed3dbe

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
@@ -481,6 +481,49 @@ TEST(BehaviorTreeFactory, ManifestMethod)
481481
EXPECT_NE(xml.find(expectedXML), std::string::npos);
482482
}
483483

484+
class ActionWithManyPorts : public SyncActionNode
485+
{
486+
public:
487+
ActionWithManyPorts(const std::string& name, const NodeConfig& config)
488+
: SyncActionNode(name, config)
489+
{}
490+
491+
BT::NodeStatus tick() override
492+
{
493+
return NodeStatus::SUCCESS;
494+
}
495+
496+
static PortsList providedPorts()
497+
{
498+
return { OutputPort<int>("out_b"), InputPort<int>("in_c"),
499+
BidirectionalPort<int>("io_a"), InputPort<int>("in_a"),
500+
OutputPort<int>("out_a"), InputPort<int>("in_b") };
501+
}
502+
};
503+
504+
// Issue #851: PortsList is an unordered_map, so the model must sort the ports
505+
// itself, otherwise Groot2 shows them in a different order on every build.
506+
TEST(BehaviorTreeFactory, ModelPortsAreSorted)
507+
{
508+
BehaviorTreeFactory factory;
509+
factory.registerNodeType<ActionWithManyPorts>("ActionWithManyPorts");
510+
511+
const auto xml = writeTreeNodesModelXML(factory, false);
512+
const std::vector<std::string> expected_order = {
513+
R"(<input_port name="in_a")", R"(<input_port name="in_b")",
514+
R"(<input_port name="in_c")", R"(<output_port name="out_a")",
515+
R"(<output_port name="out_b")", R"(<inout_port name="io_a")"
516+
};
517+
size_t prev_pos = 0;
518+
for(const auto& port : expected_order)
519+
{
520+
const auto pos = xml.find(port);
521+
ASSERT_NE(pos, std::string::npos) << port;
522+
EXPECT_GT(pos, prev_pos) << port;
523+
prev_pos = pos;
524+
}
525+
}
526+
484527
TEST(BehaviorTreeFactory, ReturnByValue)
485528
{
486529
// Issue #937: BehaviorTreeFactory cannot be returned by value from

0 commit comments

Comments
 (0)