BT Tick takes too long for some condition nodes

[redvinaa]

Bug report

Required Info:

• Version or commit hash:
  • rolling head (13f728a)

Expected behavior

For all BT nodes, tick should return quickly.

Actual behavior

IsPathValid has spin_until_future_complete, which can take several ms to complete (that's a lot for a BT to be ticked at 100 Hz).

navigation2/nav2_behavior_tree/plugins/condition/is_path_valid_condition.cpp

Line 53 in 13f728a

if (rclcpp::spin_until_future_complete(node_, result, server_timeout_) ==

Because this calls an action, its behavior should be equivalent to the action nodes inheriting from BtActionNode, returning RUNNING while the action is calculating. I've checked, I didn't see such spinning in a tick function in other condition nodes. Should this be transformed into an action node to prevent code duplication?

[SteveMacenski]

This is a good point. We have a BT Service Node base class, but that is for action nodes, not service nodes [1] that takes care of this kind of thing. We spin the future proportional to the tick rate and spin multiple times until the timeout is completed in subsequent ticks.

I think it would be sensible to have a BT Service Node that is a condition base class so that it can be used in this case. We could just implement that behavior in the IsPathValid node, but it would be the exact same task, but this way we can re-use it later in the future for other nodes.

I don't think we should convert into an Action since it is a conditional we want to use in a tree to decide if we want to perform a replan or other action. A FAILURE vs SUCCESS for an action can be interpreted as the action itself failing or the action failing to make contact, so I don't think disambiguating that situation is a completely solvable problem without a bunch of debug output port variables (that yet another node would need to interpret).

What do you think - is that something you'd be open to doing?

[1] https://github.com/ros-navigation/navigation2/blob/13f728a67321102aadfe2843221f9da159c9d628/nav2_behavior_tree/include/nav2_behavior_tree/bt_service_node.hpp

[redvinaa]

My main source of confusion is that I don't know what the difference is between condition and action nodes. If I understand correctly, a condition node returns SUCCESS or FAILURE based on the condition. But if it fails to calculate the condition, then the situation is ambiguous. Also, so far (I think) condition nodes never returned running, but now with the async version they would.

I think it would be sensible to have a BT Service Node that is a condition base class so that it can be used in this case.

Wouldn't this be the exact copy of the BT service node now used for actions? I think this would only duplicate code.

I'm not suggesting converting all condition nodes to action nodes and breaking every imaginable kind of backwards compatibility, just saying my concerns.

[SteveMacenski]

I don't know if condition nodes cannot return running, I'm having a hard time finding any condition nodes that are remotely complex where it would be attempted. So my actual answer is 🤷 I'd have to test it out to see! Maybe that was true in BT.CPPv3, but from looking at the BT.CPP code in v4 branch for about ~5 minutes, I can't see any reason that it wouldn't be accepted (yet).

Wouldn't this be the exact copy of the BT service node now used for actions? I think this would only duplicate code.

It would have alot of similarities, yes. There may be clever arechitural ways to get around this, like having an impl object that does the core of it, that a BT action / condition node can share

[adivardi]

If I may join the discussion (I have been struggling with IsPathValidCondition and IsStoppedCondition which returns Running for a few days now):

I think Condition nodes should not return RUNNING. This is mentioned in the first BT.CPP tutorial, but more importantly, it prevents them from being effectively used inside a ReactiveSequence (or ReactiveFallback) and for backchaining.
As a result, using such conditions as checks for interrupting a long running task requires much more tree structuring.