Update chooser composite (on Failure case of current child)

[GTedHa]

A duplicated issue of stoniers/py_trees issue #50.

[GTedHa]

This issue was concluded that it was intended, but I think this can make unpredictable or unexpected results. I'll update the examples of unexpected results soon.

[stonier]

@GTedHa I'd like to hear the examples for which you find it unpredictable/unexpected.

[GTedHa]

@stonier, sorry for late answer.
I summarized the possible cases of the 'unexpected results' here.
Please check the link and consider my suggestions. If you can't accept my opinion, please tell me the reason.
Thanks.

[nickvaras]

I agree with GTedHa, the current implementation just isn't faithful to what the Chooser is supposed to do according to the documentation. Definitely confused me and got me searching until I found these issues.

[stonier]

For reference, the Chooser documentation...

A variant of the selector class. Once a child is selected, it cannot be interrupted by higher priority siblings. As soon as the chosen child itself has finished it frees the chooser for an alternative selection. i.e. priorities only come into effect if the chooser wasn’t running in the previous tick.

I wouldn't say that it isn't faithful to the documentation, in particular note i.e. priorities only come into effect if the chooser wasn’t running in the previous tick. In the example given, when the children fail, it was running in the previous tick, so priorities and fallbacks should not be considered.

Having said that, I appreciate that there are multiple ways you might want a Chooser behaviour to function. None is really right or wrong, just a different logic. So what is needed in the reference implementation? a) It's logic should be crystal clear and to make it clear, this is a wonderful example (thanks @GTedHa!) I should add to the documentation/tests. It should also serve the most common use case or utilise policies for the different variants (like what is done for parallels).

Which brings us to the question - should the reference implementation change? I've never been a fan of Choosers - have often considered dropping them in later versions. Composites exponentially increase the complexity of following and programming behaviour trees - the less the better. So far any desired composite we've had can be implemented by some Selector-Sequence-Parallel-Behaviour pattern (when they're common, I bring them in as idioms). Choosers are no exception, they can be handled with blackboard setters and checkers.

Until I deprecate them though, I'm not against this change and created an issue over in the upstream repository for it. I won't be backporting it to any of the ROS1 releases though since it will be an API breaking change and might affect other users - feel free to make the patch in this fork until such a time you upgrade / switch to ROS2.

[nickvaras]

Hi Daniel, thanks for taking the time to look at the comment. Strictly speaking you are absolutely right, as far as the low level description of the logic goes. But at least on spirit, @GTedHa 's implementation is closer to "Choosers are Selectors with Commitment" , as a Selector would have ticked failing children in order, without jumping out and proceeding on the parent level... To be fair, like you say, as long as the documentation is clear there should be no confusion, and this amazing library is exquisitely done so huge thanks. Overriding the tick method with @GTedHa 's approach is simple enough . In an ideal world, instead of choosers there would be an "Uninterruptible" or "DoNotDisturb" decorator or something like that, but implementing that is probably cumbersome.

Thanks for the tips in your comments and thanks again for this amazing work!