Clean up VerifyXML logic (#1000)

* Refactor VerifyXML to clarify logic

- Reduces duplication in VerifyXML by handling the ID check for built-in
node types up front so they can then be definitively looked up in the
registered nodes.

- Enhances error messaging in VerifyXML by using *either* the node name
  *or* the ID, depending on which is appropriate, instead of leaving
users guessing "which Decorator is wrong"

- Fixes custom Action and Condition nodes using shorthand syntax not
  being properly verified

- Fixes `<Control ID="ReactiveSequence"/>` not being verified with the
  same logic as `<ReactiveSequence/>`

- Fixes `<Action ID="MyAction"/>` not triggering a behavior lookup when
  `<MyAction/>` would.

* fix tests that were failing due to bad assumptions

Author: EzraBrooks

src/xml_parsing.cpp

@@ -434,119 +434,76 @@ void VerifyXML(const std::string& xml_text,
     const std::string ID = node->Attribute("ID") ? node->Attribute("ID") : "";
     const int line_number = node->GetLineNum();
 
-    if(name == "Decorator")
+    // Precondition: built-in XML element types must define attribute [ID]
+    const bool is_builtin =
+        (name == "Decorator" || name == "Action" || name == "Condition" ||
+         name == "Control" || name == "SubTree");
+    if(is_builtin && ID.empty())
     {
-      if(children_count != 1)
-      {
-        ThrowError(line_number, "The tag <Decorator> must have exactly 1 "
-                                "child");
-      }
-      if(ID.empty())
-      {
-        ThrowError(line_number, "The tag <Decorator> must have the "
-                                "attribute [ID]");
-      }
-    }
-    else if(name == "Action")
-    {
-      if(children_count != 0)
-      {
-        ThrowError(line_number, "The tag <Action> must not have any "
-                                "child");
-      }
-      if(ID.empty())
-      {
-        ThrowError(line_number, "The tag <Action> must have the "
-                                "attribute [ID]");
-      }
+      ThrowError(line_number,
+                 std::string("The tag <") + name + "> must have the attribute [ID]");
     }
-    else if(name == "Condition")
+
+    if(name == "BehaviorTree")
     {
-      if(children_count != 0)
-      {
-        ThrowError(line_number, "The tag <Condition> must not have any "
-                                "child");
-      }
-      if(ID.empty())
+      if(ID.empty() && behavior_tree_count > 1)
       {
-        ThrowError(line_number, "The tag <Condition> must have the "
-                                "attribute [ID]");
+        ThrowError(line_number, "The tag <BehaviorTree> must have the attribute [ID]");
       }
-    }
-    else if(name == "Control")
-    {
-      if(children_count == 0)
+      if(registered_nodes.count(ID) != 0)
       {
-        ThrowError(line_number, "The tag <Control> must have at least 1 "
-                                "child");
+        ThrowError(line_number, "The attribute [ID] of tag <BehaviorTree> must not use "
+                                "the name of a registered Node");
       }
-      if(ID.empty())
+      if(children_count != 1)
       {
-        ThrowError(line_number, "The tag <Control> must have the "
-                                "attribute [ID]");
+        ThrowError(line_number, "The tag <BehaviorTree> with ID '" + ID +
+                                    "' must have exactly 1 child");
       }
     }
     else if(name == "SubTree")
     {
       if(children_count != 0)
       {
-        ThrowError(line_number, "<SubTree> should not have any child");
-      }
-      if(ID.empty())
-      {
-        ThrowError(line_number, "The tag <SubTree> must have the "
-                                "attribute [ID]");
+        ThrowError(line_number,
+                   "<SubTree> with ID '" + ID + "' should not have any child");
       }
       if(registered_nodes.count(ID) != 0)
       {
-        ThrowError(line_number, "The attribute [ID] of tag <SubTree> must "
-                                "not use the name of a registered Node");
-      }
-    }
-    else if(name == "BehaviorTree")
-    {
-      if(ID.empty() && behavior_tree_count > 1)
-      {
-        ThrowError(line_number, "The tag <BehaviorTree> must have the "
-                                "attribute [ID]");
-      }
-      if(children_count != 1)
-      {
-        ThrowError(line_number, "The tag <BehaviorTree> must have exactly 1 "
-                                "child");
-      }
-      if(registered_nodes.count(ID) != 0)
-      {
-        ThrowError(line_number, "The attribute [ID] of tag <BehaviorTree> "
-                                "must not use the name of a registered Node");
+        ThrowError(line_number, "The attribute [ID] of tag <SubTree> must not use the "
+                                "name of a registered Node");
       }
     }
     else
     {
-      // search in the factory and the list of subtrees
-      const auto search = registered_nodes.find(name);
+      // use ID for builtin node types, otherwise use the element name
+      const auto lookup_name = is_builtin ? ID : name;
+      const auto search = registered_nodes.find(lookup_name);
       bool found = (search != registered_nodes.end());
       if(!found)
       {
-        ThrowError(line_number, std::string("Node not recognized: ") + name);
+        ThrowError(line_number, std::string("Node not recognized: ") + lookup_name);
       }
 
-      if(search->second == NodeType::DECORATOR)
+      const auto node_type = search->second;
+      const std::string& registered_name = search->first;
+
+      if(node_type == NodeType::DECORATOR)
       {
         if(children_count != 1)
         {
-          ThrowError(line_number,
-                     std::string("The node <") + name + "> must have exactly 1 child");
+          ThrowError(line_number, std::string("The node '") + registered_name +
+                                      "' must have exactly 1 child");
         }
       }
-      else if(search->second == NodeType::CONTROL)
+      else if(node_type == NodeType::CONTROL)
       {
         if(children_count == 0)
         {
-          ThrowError(line_number,
-                     std::string("The node <") + name + "> must have 1 or more children");
+          ThrowError(line_number, std::string("The node '") + registered_name +
+                                      "' must have 1 or more children");
         }
-        if(name == "ReactiveSequence")
+        if(registered_name == "ReactiveSequence")
         {
           size_t async_count = 0;
           for(auto child = node->FirstChildElement(); child != nullptr;
@@ -568,13 +525,29 @@ void VerifyXML(const std::string& xml_text,
               ++async_count;
               if(async_count > 1)
               {
-                ThrowError(line_number, std::string("A ReactiveSequence cannot have more "
-                                                    "than one async child."));
+                ThrowError(line_number, std::string("A ReactiveSequence cannot have "
+                                                    "more than one async child."));
               }
             }
           }
         }
       }
+      else if(node_type == NodeType::ACTION)
+      {
+        if(children_count != 0)
+        {
+          ThrowError(line_number, std::string("The node '") + registered_name +
+                                      "' must not have any child");
+        }
+      }
+      else if(node_type == NodeType::CONDITION)
+      {
+        if(children_count != 0)
+        {
+          ThrowError(line_number, std::string("The node '") + registered_name +
+                                      "' must not have any child");
+        }
+      }
     }
     //recursion
     for(auto child = node->FirstChildElement(); child != nullptr;

tests/gtest_factory.cpp

@@ -82,7 +82,7 @@ static const char* xml_text_subtree_part1 = R"(
   <BehaviorTree ID="MainTree">
     <Fallback name="root_selector">
       <SubTree ID="DoorClosedSubtree" />
-      <Action ID="PassThroughWindow" />
+      <Action ID="PassThroughDoor" />
     </Fallback>
   </BehaviorTree>
 </root>  )";
@@ -93,11 +93,10 @@ static const char* xml_text_subtree_part2 = R"(
   <BehaviorTree ID="DoorClosedSubtree">
     <Sequence name="door_sequence">
       <Decorator ID="Inverter">
-        <Action ID="IsDoorLocked" />
+        <Action ID="IsDoorClosed" />
       </Decorator>
       <Action ID="OpenDoor" />
       <Action ID="PassThroughDoor" />
-      <Action ID="CloseDoor" />
     </Sequence>
   </BehaviorTree>
 </root>  )";