throw instead of wrapping the 16-bit UID counter in Tree::getUID (#1244)

Tree::getUID() incremented a uint16_t counter, so in a tree with more than
65535 nodes the UIDs wrapped around and collided (node 65537 got the UID of
node 1), confusing Groot2, the loggers and TreeObserver, which key on UID.
It now throws a RuntimeError when the counter is exhausted.

Behavior change: creating a tree with more than 65535 nodes now throws
instead of succeeding with duplicate UIDs.

Author: aysha-afrah26

src/bt_factory.cpp

@@ -20,6 +20,7 @@
 
 #include <filesystem>
 #include <functional>
+#include <limits>
 
 namespace BT
 {
@@ -755,6 +756,13 @@ void Tree::applyVisitor(const std::function<void(TreeNode*)>& visitor)
 
 uint16_t Tree::getUID()
 {
+  // UIDs are 16 bits wide (and serialized as such by Groot2 and the loggers):
+  // wrapping around would silently give two nodes the same UID.
+  if(uid_counter_ == std::numeric_limits<uint16_t>::max())
+  {
+    throw RuntimeError("Tree::getUID(): too many nodes, a tree can't have more than ",
+                       std::to_string(std::numeric_limits<uint16_t>::max()), " nodes");
+  }
   auto uid = ++uid_counter_;
   return uid;
 }

tests/gtest_factory.cpp

@@ -1,6 +1,7 @@
 #include "behaviortree_cpp/xml_parsing.h"
 
 #include <filesystem>
+#include <set>
 #include <string>
 #include <utility>
 #include <vector>
@@ -795,3 +796,18 @@ TEST(BehaviorTreeFactory, MalformedXML_UnknownNodeType)
   BehaviorTreeFactory factory;
   EXPECT_THROW((void)factory.createTreeFromText(xml), RuntimeError);
 }
+
+TEST(BehaviorTreeFactory, TooManyNodesForUID)
+{
+  // UIDs are 16 bits wide. Past 65535 nodes the counter used to wrap around
+  // and the next nodes got the same UID (and path) as the first ones.
+  Tree tree;
+  std::set<uint16_t> uids;
+  for(int i = 0; i < 65535; i++)
+  {
+    uids.insert(tree.getUID());
+  }
+  EXPECT_EQ(uids.size(), 65535u);
+  EXPECT_EQ(uids.count(0), 0u);
+  EXPECT_THROW((void)tree.getUID(), RuntimeError);
+}