Don't restart SequenceStar on halt

[Affonso-Gui]

This might need some discussion, but I am pretty sure it's a bug.

The problem:
SequenceStar resets progress when halted https://github.com/BehaviorTree/BehaviorTree.CPP/blob/master/src/controls/sequence_star_node.cpp#L77

So in reactive formulations such as the example image below (also used in the documentation), if the battery check fails during GoTo=B, GoTo=A will be re-executed in the next sequence tick. Since we are using SequenceStar to avoid repeating actions which have already returned SUCESS, the execution should be resumed from GoTo=B even after halted.

[facontidavide]

I think you are correct and it is a bug. Thanks a lot for taking the time to add unit tests too!!
I will review and see if there is any corner case we are missing here.

I hope not too many people were relying on this buggy behavior.

[facontidavide]

thanks

[tgreier]

@facontidavide

Question on this new behavior.

In ROS2 navigation2, when a navigation is canceled, all of the behavior tree nodes are 'halted'. The assumption is that by halting all of the nodes, the behavior tree is reset and can be called again.
With this new behavior, when the behavior tree is called, a previously active sequence-star node will not begin from child idx 0, but instead will tick at the last active child prior to the last navigation cancellation.

Given this behavior, what is the proper method for truly resetting all behavior tree nodes?
Is the only way to truly reset a behavior tree is to destroy and recreate it?
Thank you,
Tom

[Affonso-Gui]

@tgreier I think a normal Sequence node will do what you are looking for (ticking consecutive nodes without going back for as long as the node is running, and then ticking from start after the node has been reset/halted).

[tgreier]

@Affonso-Gui Thank you, yes I agree that will solve my specific issue. But, is there a way to reset all behavior tree nodes without destroying and recreating the behavior tree instance?

[SteveMacenski]

@facontidavide I tend to agree with @tgreier assuming my intepretation of "halt" is correct in BT language (@miccol, thoughts?). When I call halt, I expect the behavior tree to reset all of the nodes' states (as it does now), that includes the control flow nodes. So I think idx should be set back to zero, why would the control flow nodes be some special case relative to the other nodes? The idx is still a state variable, if all else is reset, why would this be excluded?

The memory of a "star" node is for intra-execution, I don't think having memory inter-execution is sensible (or if there are inter-execution states, blackboard may be a good choice). When halt is called, the interpretation I have is to reset the behavior tree so that a new, fresh execution may start. Retaining idx in the "star" memory nodes seems counter to that.

Edit: This does actually cause a regression in our BT's in Nav2 https://github.com/ros-planning/navigation2/blob/main/nav2_bt_navigator/behavior_trees/navigate_to_pose_w_replanning_goal_patience_and_recovery.xml

[miccol]

Hi,

I would also reset the index after a halt. I usually use the Sequence Node when I need a node with memory. Coupling an action with its "success condition" (when possible) helps skip actions that are not required to be re-executed.

[facontidavide]

@Affonso-Gui

But, is there a way to reset all behavior tree nodes without destroying and recreating the behavior tree instance?
That never, ever happens. No Tree instance is destroyed.

@tgreier and @SteveMacenski

With this new behavior, when the behavior tree is called, a previously active sequence-star node will not begin from child idx 0, but instead will tick at the last active child prior to the last navigation cancellation.

Why and in which context are you using SequenceStar? What is your motivating example?

[SteveMacenski]

If I have a BT with some SequenceStar branch in it to iterate over recovery behaviors -- or do any number of things as part of a task that is modeled as behavior trees (there's innumerable reasons even outside of robotics), that should be properly reset when I halt the tree so I can do another run of it later. I also gave a link to a specific BT in Nav2 using SequenceStar in the comment above. The memory provided by a control flow node should be during the execution of the behavior tree, not retained between completely different execution cycles marked by a request to reset the tree (halt).

After this PR, it breaks that behavior by not resetting the node to be ticked next so it no longer starts with the first child like it would in a "fresh" start of the tree, which halt should be providing. @miccol has the same thought as me: halt means to reset the state of the tree/node(s) so you can have a fresh execution of that behavior tree/node(s). Do you agree with that? If so, then all of the BT nodes would need to be reset to its initial state so it can be executed again. That includes the control flow nodes states (e.g. this current_child_idx_). Its incredibly bizarre that only SequenceStar is the only node that doesn't have its state fully reset on a halt call and is asking for future bug reports / complaints.

Also moreover that I think you knew this was incorrect initially when you developed it. This is a behavior Nav2 relies on so that we can execute multiple navigation tasks using the same tree. I don't really see the sensibility in this PR change.

tl;dr: I strongly encourage you to revert this PR, I don't see any reason why halt should not reset the state of all BT nodes. This seems like a regression and happy that The BT King, Mr. Michele Colledanchise himself agrees 😆

[Affonso-Gui]

My main motivation for this change was:

1. To better match the (now outdated?) documentation https://github.com/BehaviorTree/BehaviorTree.CPP/blob/3.6.0/docs/SequenceNode.md#sequencestar
2. To skip having to write recognition nodes / add blackboard entries for every action, especially in early development stages. My specific use case was to look around if an object is lost or re-plan a motion if an object was moved during a pick-and-place task. I agree that adding explicit success conditions (either as recognition nodes or as state variables) would be a more "mature" implementation, but using SequenceStar helped me manage the complexity at first developments.
3. To ensure that initialization nodes are not repeated. E.g. loading the robot model, setting subscribers.

However, I do agree with the following.

I don't think having memory inter-execution is sensible (or if there are inter-execution states, blackboard may be a good choice)

Hopefully state management has also become easier with the new version 4.0.

On the other hand, the halt method invocation may be a little too opaque to rely on (?). For example, in the following tree if OtherAction is a SyncAction that returns SUCCESS the SequenceStar will not be halted. If OtherAction is an AsyncAction, the SequenceStar will be halted as soon as the action starts (i.e. returns RUNNING).
EDIT: Actually, standard Sequence nodes would also have different behavior depending on the OtherAction implementation, so this might be the topic for another discussion.

Overall I agree with reverting this, but I would also like to have a more detailed documentation entry on this behavior.

[SteveMacenski]

(now outdated?) documentation

Yeah, I ran into alot of pages on google with deadlinks to BT.CPP's website, I'm not sure what's up with that.

For your other points, I think those are situations perhaps that need some different designs in your BT to handle. I could give you some off the cuff suggestions, but obviously I don't know alot about your situation so they may be off base. I would strongly argue though that relying on a non-resetting SequenceStar child counter between executions is the wrong (and very fragile) thing to do.

2 suggestions though:

• Have your own SequenceStarGlobal BT node with "global" state stored on the blackboard that stores between runs (not best)
• Have an initialization branch that runs on a condition custom node that checks if_initialized on the blackboard, that is set once initialization is complete (better). That way if that branch is done, it won't be tried again, ever, even between executions. Given that the initialization bit is "task dependent" that feels very kosher and cleanly encapsulated into a single trivial, reusable node (heck, even a node that might be useful in Nav2).

Overall I agree with reverting this

🥳 🎆

[Affonso-Gui]

Thanks for the insights. Currently I am using a version of the 1st approach (custom control node), but having a condition guard for initialization code also seems nice.

A little more clarification, though, just in case.

The memory provided by a control flow node should be during the execution of the behavior tree, not retained between completely different execution cycles marked by a request to reset the tree (halt).

My main use case was not to retain states across different executions, but across possible action shifts ("interruptions") within the same execution. More concretely, to avoid re-execution of completed actions when the SEARCH action is triggered below (i.e. implicit calls to the halt method).

In my particular case, I would only tick the tree until the root returned SUCCESS or FAILURE. When I wanted to take another run, I would re-run the program and reconstruct a new tree, as hinted by @tgreier.

[Affonso-Gui]

Taking a quick look into the Nav2 package, it seems like you are focusing more on the explicit calls to the halt method (https://github.com/ros-planning/navigation2/blob/main/nav2_behavior_tree/src/behavior_tree_engine.cpp#L86-L104), while I was more concerned with the implicit calls.
(I completely agree that it makes no sense to retain states even after explicit calls to clean up the tree).

This makes me wonder if it would be the case to simply overwride the halt method of the root to clean up all SequenceStar memory.
Does the example of the switching actions above (or the battery example in the original comment) classify as intra-execution (and is therefore sensible), or should it be considered as inter-execution as long as the triggered action is outside the SequenceStar child?

[SteveMacenski]

@facontidavide can this be reverted?

[facontidavide]

reverted

[SteveMacenski]

Thanks! Do you know when you might run a release to get this out into binary format?