Repository navigation
Fix/duplicate behavior tree id registration - #1133
facontidavide merged 5 commits into
Conversation
|
Hi @facontidavide, I'd love to get this merged if you think it's the right direction. The failing CI check on Windows is unrelated to this PR. Happy to fix that test as well if you'd like, or we could address it in a separate PR. Let me know how you'd like to proceed! |
| * duplicate-ID errors come from registerBehaviorTreeFrom* or | ||
| * createTreeFrom*, not from createTree(). |
There was a problem hiding this comment.
This comment struck me as odd. I would perhaps not add it. I don't think we should explain errors that can come from sub-function calls, that could get out of hand quickly.
|
@facontidavide FYI the author, Kanykei, works with me at Agile Robots. This was something that bugged us so we a proposing this fix 🙂 |
schornakj
left a comment
schornakj
left a comment
There was a problem hiding this comment.
When we have 2 files with the same BehaviorTree ID they get randomly chosen and executed without any warnings or errors.
I don't think this is quite accurate, based on what I've observed using this library. If you have 2 XML files which both define a tree using the same ID and call registerBehaviorTreeFromFile one one then the other, the definition provided in the second file overwrites the first.
Breaking (behavioral): Registering XML that defines an id already registered on the same factory throws where it previously overwrote silently. Callers who relied on overwrite must clearRegisteredBehaviorTrees() then register again (same pattern as #439).
I wouldn't want this to be a terminating error case, since I have some situations where I do this on purpose. One use case is to test a behavior tree with many subtrees: by overwriting the registration of some subtrees with fakes that have specific behavior (always failing, always outputting a specific value to a port, etc.) I can efficiently test the control flow through the top-level tree without having to replicate a lot of interaction with my robot and sensors. I wouldn't be able to use your suggested workaround of clearing and then reregistering because at the point where I want to replace a subtree with a fake one I no longer have detailed information about which tree came from which XML file, so I wouldn't be able to construct a subset of XML files that does not contain duplicates.
In my application we addressed this by writing a registration helper function that checks for duplicate tree IDs and BT nodes IDs (and also for trees with the same IDs as nodes, since that also causes problem) and reports a failure at that scope. The replace-a-tree-with-a-fake-during-testing situation is accessed directly through the BT factory.
I'd be OK with having a warning message about this situation, since it's confusing if it sneaks up on you.
Registering a BehaviorTree ID that already exists replaced the previous definition silently. Making it an error breaks legitimate uses of the deliberate last-wins behavior (a851c79): registering the same file twice, calling createTreeFromText repeatedly on one factory, diamond includes, and replacing a subtree with a test double. Keep last-wins and print a warning instead, only when the previous definition came from a different source or from the same document; reloading a file, including it from two places, or reusing the same inline text stays silent. Take the source from the include stack added by BehaviorTree#1250 instead of a second current_file variable, document the behavior in bt_factory.h, revert the clearRegisteredBehaviorTrees() calls added to existing tests, and turn the new tests into warning/last-wins checks, plus diamond-include and repeated-inline-text cases. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Thanks @KImanalieva, and @schornakj and @peter-mitrano-ar for the review. You're right that a silent overwrite is a footgun. But the overwrite itself is intentional (last definition wins, since a851c79), and people rely on it: registering the same file twice, calling To move this forward, I pushed a merge of master plus a commit that turns the throw into a warning and keeps last-wins. The warning is printed only when the previous definition came from a different source, or appears twice in the same document. Reloading a file, including it from two places, or reusing the same inline text stays silent. The source comes from the include stack added in #1250, so the separate |
Bug
When we have 2 files with the same BehaviorTree ID they get randomly chosen and executed without any warnings or errors.
Detect duplicate registrations in the shared XML parser and throw RuntimeError with both source locations (first registration vs conflicting registration). Silent map overwrite is removed; intentional reload uses BehaviorTreeFactory::clearRegisteredBehaviorTrees() before registering again.
Breaking (behavioral): Registering XML that defines an id already registered on the same factory throws where it previously overwrote silently. Callers who relied on overwrite must clearRegisteredBehaviorTrees() then register again (same pattern as PR #439).