Repository navigation
throw instead of wrapping the 16-bit UID counter in Tree::getUID - #1244
Merged
facontidavide merged 1 commit intoOct 24, 2026
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Tree::getUID() hands out node UIDs from a uint16_t counter, and the XML parser calls it once for every node it instantiates. Nothing stops the counter at 65535, so in a tree with more nodes it wraps to 0 and the following nodes get the same UID as the first ones. The node count comes straight from the tree XML, and SubTree expansion multiplies it, so a fairly small document can get there. Since the UID is the node identity for Groot2 (nodes_by_uid, the status buffer and the breakpoint/replace hooks), for FileLogger2, SqliteLogger and TreeObserver, and for the auto-generated path of unnamed nodes, aliased nodes end up sharing each other's hooks, transitions and statistics. I noticed it while building a Sequence with 65540 AlwaysSuccess children: five nodes came back with a UID that was already in use, and two of them even report the same fullPath(), AlwaysSuccess::2. Groot2 and the btlog format serialize the UID as 16 bits, so widening it isn't really an option, and getUID() now throws a RuntimeError once the counter is exhausted so tree creation fails with a clear message instead. Trees up to 65535 nodes behave exactly as before, there is no API or ABI change, and the new BehaviorTreeFactory.TooManyNodesForUID test fails on master and passes with the fix.