-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Nav2 single trigger node test fail-> halt does not does not convert state to idle #3310
Comments
@facontidavide this might point to a regression in BT.CPP. I looked to find where this started and the first PR that started to fail with this didn't change this file or anything related to it. I think it was just the first PR to turn over with a new release of BT.CPP. When did the last v3 release hit binaries? Looking over these files, they haven't changed in over a year. This error would point to the |
I've looked at the recent change to those files The first added
Which shouldn't be a problem since we do have a child node on that single trigger decorator. The second removes That only works resursively - not if the root node of the actual BT is a decorator / action / condition - that BT node then would not be reset back to idle - which is what our unit tests do. As a result, everyone's BTs will not have the root node reset to IDLE. In light of that @facontidavide, I think the @stevedanomodolor do you agree that's likely the issue? |
I will look at this in the next days. Thanks for reporting |
@SteveMacenski Pretty much yes, the issue was not from nav2 side but the halt not updating the bt root node. Yes, you can submit the PR when you can. |
Let me explain my reasoning, considering also that I am NOT AGAINST revert my changes. The concept I am trying to push is that:
This influence the way Decorators and Control nodes should be implemented. You should use:
Since "halt" is a bad name here, I just pushed this change in branch v3.8: BehaviorTree/BehaviorTree.CPP@87c3669 Summarizing, I think that your unit test is expecting something to happen that is not intended in the library. |
That sounds nice in theory, as long as all the default nodes properly reset all children when halt is called and the root node of the BT has some special logic to be halted (which is missing and causing this concern). But its also error prone if anyone makes a BT node without that same philosophy and/or comes to expect the previous behavior of BT.CPP that up until now has had the base nodes reset themselves as well for protection. I think that's wise defensive programming to have the nodes reset themselves especially for something exposed to users. It gives them the blueprint for doing that as well as they're using your BT nodes as models and removes a very subtle potential misunderstanding about the root nodes.
Yeah, that seems sensible - no argument here. But I'd ask "why not both?" especially for the default control / decorator nodes. Just cover your bases in case the user messes up with their custom nodes & provides a blueprint for success so the subtle nature of the root node (since there isn't special handling in the tree to halt that one -- yet?) isn't even something they need to worry about. There's been a handful of changes to BT.CPP like this (and the PR we asked to be reverted persisting state between executions) which create changes in behavior that makes it problematic to re-use behavior trees. We expect halting the tree to properly reset the state fully and we rely on that in Nav2. Up until recently, this has chugged along for us no problem. I don't think these are done intentionally to remove support for re-using behavior trees for multiple tasks sequentially, but seems like something that needs to be added to the review process of changes to make sure its not breaking that behavioral support. It may be being overlooked.
We call Note that separate of halting we also reset children / halt at the end of a cycle in the control nodes (ex I checked them all). We don't for decorators though. But regardless, the issue that we're having is that the root node - which is commonly going to be a decorator or a control flow node of some kind, isn't being reset in the BT because there is no parent to it for |
@SteveMacenski has any pr been pushed regarding this? |
Nope, I think we're still discussing this with @facontidavide to know what to do |
@facontidavide following up here on this |
the request changes where applied here: I need to "bloom" the new version 3.8.2 |
@facontidavide has it been bloomed up yet? We're still experiencing this error keeping our CI in the red 😢 |
This is still a bug impacting us |
I investigated this today. I found that the change that caused it was in the commit backporting a bunch of changes from V4 to V3.8 that we use BehaviorTree/BehaviorTree.CPP@cc77171. I went line by line and agree with most all of it except change removing the @facontidavide 's opinion was that it was the role of the parent nodes to reset the child's. I generally agree with that. The issue is resetting the root-most node, which is done in the tree's I'll follow up in an hour when I look more deeply at that commit and decide if (1) we just need to refactor the test or (2) this is still an outstanding problem in BT.CPP that is worth me opening a PR with a suggested repose so we can get our CI green again. I'm very confident I should have a solution for this by EOD |
OK... in summary, I think the test just needs to be updated. It would be nice if the root node was automatically reset in the state especially since I think the only change necessary is to move at least |
BehaviorTree/BehaviorTree.CPP#579 and #3605 resolve |
BT.CPP merged, just needs release |
@facontidavide can we get a release before the Rolling sync? |
Which version are you talking about? 3.8.3 or latest branch v3.8? I am a little worry about this: BehaviorTree/BehaviorTree.CPP#563 Solving it obliged me to do a change that breaks the ABI, and even if I think it is a good change, @Rid0n mentions that it doesn't fix the issue and introduces a regression. Can someone in Nav2 check if the latest version of branch v3.8 works as expected for them? |
I'm not sure how you do versioning, but yeah bumping a new subversion with that commit would be good. We merged the change needed to fix our tests (exposing the
Obviously I'm not in the trenches with you on that particular issue, but if those changes didn't fix the issue, why not simply revert it? Then there's no ABI issues + no more broken/fixed than before. I assume that your changes are closer in the right direction, but if it doesn't fully solve it that seems like a reasonable enough solution and re-apply the patch once a fuller solution is found. We don't make much use of subtrees TBH so I don't know that any of the changes made w.r.t. that would change one way or another to us |
Almost there, I should release 3.9 soon |
OK! Its not a huge rush or anything, but I would like the CI green as soon as practically possible. |
Bug report
Required Info:
Steps to reproduce issue
Expected behavior
All test should pass
Actual behavior
Additional information
This is bad debugging style but It shows what I mean, for example the test for the single node plugins,
As you will see below, the bt node is never able to back to idle.
The text was updated successfully, but these errors were encountered: