fix issue #525 when ReactiveSequence contains skipped children

Author: facontidavide

src/controls/reactive_fallback.cpp

@@ -50,7 +50,8 @@ NodeStatus ReactiveFallback::tick()
       }
 
       case NodeStatus::SKIPPED: {
-        // node skipped
+        // to allow it to be skipped again, we must reset the node
+        haltChild(index);
       }
       break;
 

src/controls/reactive_sequence.cpp

@@ -43,9 +43,13 @@ NodeStatus ReactiveSequence::tick()
         resetChildren();
         return NodeStatus::FAILURE;
       }
-      // do nothing if SUCCESS or SKIPPED
-      case NodeStatus::SUCCESS:
-      case NodeStatus::SKIPPED:
+      // do nothing if SUCCESS
+      case NodeStatus::SUCCESS: break;
+
+      case NodeStatus::SKIPPED:{
+        // to allow it to be skipped again, we must reset the node
+        haltChild(index);
+      }
       break;
 
       case NodeStatus::IDLE: {

src/decorators/repeat_node.cpp

@@ -86,9 +86,11 @@ NodeStatus RepeatNode::tick()
       }
 
       case NodeStatus::SKIPPED: {
+        // to allow it to be skipped again, we must reset the node
+        resetChild();
         // the child has been skipped. Skip the decorator too.
         // Don't reset the counter, though !
-        return NodeStatus::IDLE;
+        return NodeStatus::SKIPPED;
       }
       case NodeStatus::IDLE: {
         throw LogicError("[", name(), "]: A children should not return IDLE");

src/decorators/retry_node.cpp

@@ -94,6 +94,8 @@ NodeStatus RetryNode::tick()
       }
 
       case NodeStatus::SKIPPED: {
+        // to allow it to be skipped again, we must reset the node
+        resetChild();
         // the child has been skipped. Slip this too
         return NodeStatus::SKIPPED;
       }

tests/gtest_skipping.cpp

@@ -3,6 +3,7 @@
 #include "behaviortree_cpp/basic_types.h"
 #include "behaviortree_cpp/bt_factory.h"
 #include "test_helper.hpp"
+#include "action_test_node.h"
 
 using namespace BT;
 
@@ -121,39 +122,56 @@ TEST(SkippingLogic, SkippingReactiveSequence)
   std::array<int, 2> counters;
   RegisterTestTick(factory, "Test", counters);
 
-  const std::string xml_text = R"(
+  const std::string xml_text_noskip = R"(
     <root BTCPP_format="4" >
        <BehaviorTree>
-          <Sequence>
-            <ReactiveSequence>
-              <Script code=" value:=50 "/>
-              <TestA _skipIf="value < 25"/>
-            </ReactiveSequence>
-
-            <ReactiveSequence>
-              <Script code=" value:=10 "/>
-              <TestB _skipIf="value < 25"/>
-            </ReactiveSequence>
-         </Sequence>
+          <ReactiveSequence>
+            <Script code=" value:=50 "/>
+            <TestA _skipIf="value < 25"/>
+            <AsyncActionTest/>
+          </ReactiveSequence>
        </BehaviorTree>
     </root>)";
 
-  auto tree = factory.createTreeFromText(xml_text);
+  const std::string xml_text_skip = R"(
+    <root BTCPP_format="4" >
+       <BehaviorTree>
+          <ReactiveSequence>
+            <Script code=" value:=10 "/>
+            <TestB _skipIf="value < 25"/>
+            <AsyncActionTest/>
+          </ReactiveSequence>
+       </BehaviorTree>
+    </root>)";
+
+  factory.registerNodeType<AsyncActionTest>("AsyncActionTest");
+
+  int expected_test_A_ticks = 0;
 
-  BT::NodeStatus status;
-  try {
-    int runs = 0;
-    while(runs < 5)
+  for(auto const* xml_text: {&xml_text_noskip, &xml_text_skip})
+  {
+    auto tree = factory.createTreeFromText(*xml_text);
+
+    for(int repeat=0; repeat<3; repeat++)
     {
-      status = tree.tickOnce();
-      tree.sleep(std::chrono::milliseconds(10));
-      runs++;
+      NodeStatus status = NodeStatus::IDLE;
+      while(!StatusCompleted(status))
+      {
+        status = tree.tickOnce();
+
+        if(xml_text == &xml_text_noskip)
+        {
+          expected_test_A_ticks++;
+        }
+
+        tree.sleep(std::chrono::milliseconds{15});
+      }
+      ASSERT_EQ(status, NodeStatus::SUCCESS);
     }
-  } catch (BT::LogicError err) {
-    std::cout << err.what() << std::endl;
   }
+  // counters[0] contains the number ot times TestA was ticked
+  ASSERT_EQ(counters[0], expected_test_A_ticks);
 
-  ASSERT_EQ(status, NodeStatus::SUCCESS);
-  ASSERT_EQ(counters[0], 5);
+  // counters[1] contains the number ot times TestB was ticked
   ASSERT_EQ(counters[1], 0);
 }

tests/test_helper.hpp

@@ -11,7 +11,7 @@ inline BT::NodeStatus TestTick(int* tick_counter)
 
 template <size_t N> inline
 void RegisterTestTick(BT::BehaviorTreeFactory& factory, const std::string& name_prefix,
-                   std::array<int, N>& tick_counters)
+                      std::array<int, N>& tick_counters)
 {
     for(size_t i=0; i<tick_counters.size(); i++)
     {