ReactiveSequence with multiple asynch children

[facontidavide]

Hi @miccol

as you know, I was unhappy with the logic of V2 about Action not being "tickable" unless set to IDLE.
Removinf that affect the ability of a ReactiveSequence (and ReactiveFallback) to have multiple asynch child (not just actions, but entire branches).

I propose this solution (separate branch):

https://github.com/BehaviorTree/BehaviorTree.CPP/blob/reactive_stuff/src/controls/reactive_sequence.cpp

[facontidavide]

Take into account that a "child" could be an AsyncAction of an entire branch that contains at least a single AsyncAction.

The idea a RUNNING state from an AsyncAction should always be propagated back to the root of the tree. Whenever a ControlNode or DecoratorNode receive a RUNNING from a child, it must return RUNNING too.

For this reason, asych children should be easy to identify if RUNNING was returned at least once.

https://github.com/BehaviorTree/BehaviorTree.CPP/blob/reactive_stuff/src/controls/reactive_sequence.cpp#L45

[miccol]

Just to recap (useful for a user that is reading this for the first time and to check if we are all on the same page). Having reactive control flow nodes (i.e. control flow nodes that keep re-evaluating the status of the children) with asynchronous actions as children have some problems:

Consider the following BT (Fallback and Sequence are reactive. Actions are asynchronous):

Let's say that at the initial situation the Battery is OK. Hence the BT will execute the asynchronous action "Goto Kitchen", as shown below(Gray border means that the node returns RUNNING, Green SUCCESS, and Red FAILURE):

After some time, while the robot moves into the kitchen (e.g. Goto Kitchen is running), the battery is no longer ok. When the Fallback ticks the condition "Battery is OK", this returns FAILURE and it ticks the action "Goto Charging Station" but... OUCH!! the action "Goto Kitchen" is still RUNNING and we can have undetermined behaviors, as shown below :

Eventually the action "Goto Charging Station" will return RUNNING, so will do the Fallback and the Sequence will Halt the action "Goto Kitchen". as shown below:

But, for a small amount of time, there are two actions running concurrently.

The solution you propose solves this problem as we cannot have more than one "Asynchronous sub-BT" as children of a Reactive Control Flow node. Probably it is a bit too restrictive.

Another possible solution is to assume that Asynchronous Actions always return RUNNING at the first tick (i.e. they cannot return Success or Failure with only one tick). Hence we could implement that, before ticking an Asynchronous action, the reactive control flow nodes will halt the other asynchronous action that is running, if any (of course taking into consideration that the Parallel control flow now as an exception). After the other actionis halted, the control flow node goes forward with ticking the action.

What do you think?

[facontidavide]

Very good point. Have you checked the change I propose?
https://github.com/BehaviorTree/BehaviorTree.CPP/blob/reactive_stuff/src/controls/reactive_sequence.cpp

• Let's assume that, as you mentioned: "that Asynchronous Actions always return RUNNING at the first tick". This is exactly what happens with AsyncAction (but not CoroAction yet).

• You are apparently right (unless I miss something) when you say: "But, for a small amount of time, there are two actions running concurrently." Ouch +1

• By the way, I believe that version 2.x was affected by exactly the same problem... correct me if I am wrong.

I can fix the first. but I can hardly address the second.

Another possible solution is to add a method/property to TreeNode:

   virtual bool TreeNode::isAsynch() const; 

If at least one of your children is Asynch, you are Asynch too.
Knowing this property in advance, i.e before the first tick(), solves both the two problems mentioned earlier!!!

[miccol]

Very good point. Have you checked the change I propose?
https://github.com/BehaviorTree/BehaviorTree.CPP/blob/reactive_stuff/src/controls/reactive_sequence.cpp

Yep. I am thinking about the effects of this:

BehaviorTree.CPP/src/controls/reactive_sequence.cpp

Lines 31 to 37 in b58b802

	TreeNode* current_child_node = children_nodes_[index];
	if( is_asynch_child_[index] &&
	current_child_node->status() == NodeStatus::SUCCESS )
	{
	success_count++;
	continue; // skip already executed asynch children
	}

There are some cases in which we want to re-evaluate async children. For example, the BT above, the Sequence must re-evaluate the Fallback to check if the robot has to move to the charging stations. The second and the third figures above show an example where the async child (the Fallback) should be re-evaluated even if it returns SUCCESS.

Let's assume that, as you mentioned: "that Asynchronous Actions always return RUNNING at the first tick". This is exactly what happens with AsyncAction (but not CoroAction yet).

Yep.

By the way, I believe that version 2.x was affected by exactly the same problem... correct me if I am wrong.

Yes, this was present even in Version 1. I was avoiding funny behaviors manually in the actions.

I can fix the first. but I can hardly address the second.

I can try to propose a solution to the second (i.e. haling other running async action before ticking a new one) and open a PR.

Another possible solution is to add a method/property to TreeNode:virtual bool TreeNode::isAsynch() const;
If at least one of your children is Asynch, you are Asynch too.
Knowing this property in advance, i.e before the first tick(), solves both the two problems mentioned earlier!!!

Right, in that case a Reactive control flow node will have only one asych child, right?

[facontidavide]

There are some cases in which we want to re-evaluate async children. For example, the BT above, the Sequence must re-evaluate the Fallback to check if the robot has to move to the charging stations. The second and the third figures above show an example where the async child (the Fallback) should be re-evaluated even if it returns SUCCESS.

I realized that you are right. In my proposed solution, we would not re-evaluate the left branch :(

My question now is: is the tree in the example semantically correct? Can't it be that we are trying to make work a tree that is conceptually wrong?
After all, this tree does not provide any explicit cleanup /halting of GotoKitchen.

But anyway, please propose your solution, if you already have an idea :)

[miccol]

But anyway, please propose your solution, if you already have an idea :)

I am planning to show you the solution I have in mind in a separate branch. Then we can discuss it.

[hlzl]

@miccol @facontidavide
Any update on this?

[miccol]

There is an open PR on this.
#93

[facontidavide]

I think it is time to address this, eventually.

[msadowski]

Hi everyone!

I just came across this issue and I'm wondering if it has been addressed? The reason I'm asking is that I followed the tutorials, added some custom BT Nodes and ended up with a tree like this:

<root>
     <BehaviorTree>
        <Fallback>
            <ReactiveSequence>
                <BatteryCheck/>
                <TimedWait      seconds="5"/>
                <TriggerWait    service_name="/start_run"/>
                <BackOffCmd     distance="1.0"/>
                <MoveBase       goal="-10;9;3.14"/>
                <MoveBase       goal="-9;9;0"/>
                <MoveBase       goal="-8;8;3"/>
                <MoveBase       dock_approach="true"/>
            </ReactiveSequence>
            <!-- Fallback: if anything fails, go back to dock-->
            <MoveBase       dock_approach="true"/> <!-- let's say 0,0 is charging point-->
        </Fallback>
     </BehaviorTree>
 </root>

Every single node that appear in this tree below BatteryCheck inherits from AsyncActionNode and the tree is executed as I would expect (continuously tick BatteryCheck, and then tick every node one after another).

Just thought I would signal this. I'm currently looking to refactor my approach and use something akin to:

[facontidavide]

Hi @msadowski

just to be sure I understand correctly.
The BT you shared IS working as expected or IS NOT working as expected, from the reactivity point of view?

The refactoring you mentioned can be helpful if AFTER a failure (MoveTo docking station), you don't want to repeat one of the steps of the previous sequence

[msadowski]

@facontidavide it IS working as I would expect. I created my comment as I was not sure if I missed something and went past some limitation without noticing it. All seems good on this side.

[facontidavide]

yes, this is probably fixed