Fix/duplicate behavior tree id registration#1133
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.
3 participants
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).