Design of BT non-leaf (decorator and control) nodes

[tonynajjar]

Steps to reproduce issue

Expected behavior

1- If the first child of the RecoveryNode fails, I would expect the second child to be ticked after the specified bt_loop_duration (aka at the next tick).

2- If the second child of the RecoveryNode succeeds (after the first child failed), I would expect the first child to be (re)ticked after the specified bt_loop_duration (aka at the next tick).

Actual behavior

In both cases 1 and 2, the next child is ticked immediately (at the next iteration of the internal while loop)

Generalization

Both Nav2's (e.g. RecoveryNode, PipelineSequence, RoundRobinNode) and BT.CPP's non-leaf nodes do not return RUNNING after their children reach a terminal state (e.g SUCCESS, FAILURE); they directly tick the next child according to their control logic.

I don't have any theoretical proof but I feel like control nodes should not have their independent logic to execute children multiple times at their own rate; they should instead be saving a certain state (in the case of RecoveryNode that's with current_child_idx_) and executing according to it only at the next top-level tick. @facontidavide is this the philosophy you follow?

Additional information

[SteveMacenski]

I don't disagree - will look at PR :-)

[tonynajjar]

@facontidavide is this the philosophy you follow?

Actually, looking at the SequenceNode I don't think this is the followed philosophy.

In the SequenceNode, as implemented, if a child succeeds, the next child is immediately ticked (not after a tick/sleep period). Either we break and go in another iteration of the while loop to tick the next child, or we return RUNNING after emitWakeUpSignal() which will abort the next sleep. In both cases, the next child is ticked immediately.

I think it's important to get on the same page on how to design parent nodes (decorator, control, etc...)

[tonynajjar]

I'll reopen the issue until the last comment is addressed just so we don't forget about it

[SteveMacenski]

@tonynajjar should we consider reverting the PR changes then?

[tonynajjar]

@tonynajjar should we consider reverting the PR changes then?

Yeah let's do it until we have a design consensus

I edited the description to consolidate with #4782

[SteveMacenski]

I'll ping Davide in another thread to get his take

The crux of this ticket is that Tony thinks the Nav2 BT control nodes should return RUNNING on its tick if a child returns and should trigger another child -- rather than immediately ticking the next child. That would be reasonable to me so that the behavior is fully introspectible. We don't do this currently in Nav2 and it was pointed out that BT.CPP doesn't either.

[SteveMacenski]

Davide says that this is fine and expected. @tonynajjar should we close this out? I agree that it doesn't make things fully introspectable if looking at the tree output states, but it is in line with the library's use and other built-in nodes. We could change them here in Nav2, but it doesn't seem like updates to changing BT.CPP's to do that as well is wanted

[tonynajjar]

Some more information on why this is fine and expected would have been helpful since this is a subjective design question.
This design decision has the significant drawback of making the BT "less reactive", especially with BT nodes with a retry behavior: since the control node does not return before all the retries are done, other parts of the tree that may want to preempt the retrying (or do something else) are not executed. It essentially acts similar to a blocking function call in a high-rate loop.

[jplapp]

We keep stumbling on this issue - a simple example on how this is non-intuitive is this:
<Timeout><RetryUntilSuccessful num_attempts="-1"><AlwaysFailure/></RetryUntilSuccessful></Timeout>

Which will never time out because Retry has its "own" loop. Unfortunately this seems to be a design decision.

[facontidavide]

I will be happy to modify Retry as I did for Sequence

https://github.com/BehaviorTree/BehaviorTree.CPP/blob/master/include/behaviortree_cpp/controls/sequence_node.h#L37

Will that help?