v0.2.54..v0.2.55 changeset ElementComparer.cpp

diff --git a/hoot-core/src/main/cpp/hoot/core/elements/ElementComparer.cpp b/hoot-core/src/main/cpp/hoot/core/elements/ElementComparer.cpp
index e25663b..d6d66ed 100644
--- a/hoot-core/src/main/cpp/hoot/core/elements/ElementComparer.cpp
+++ b/hoot-core/src/main/cpp/hoot/core/elements/ElementComparer.cpp
@@ -29,111 +29,58 @@
 #include <hoot/core/elements/Node.h>
 #include <hoot/core/elements/Way.h>
 #include <hoot/core/elements/Relation.h>
-#include <hoot/core/util/GeometryUtils.h>
 #include <hoot/core/util/Log.h>
 #include <hoot/core/schema/MetadataTags.h>
-#include <hoot/core/visitors/CalculateHashVisitor2.h>
+#include <hoot/core/visitors/ElementHashVisitor.h>
 
 namespace hoot
 {
 
-ElementComparer::ElementComparer(Meters threshold) :
-_threshold(threshold),
-_ignoreElementId(false)
+ElementComparer::ElementComparer() :
+_ignoreElementId(false),
+_ignoreVersion(false)
 {
 }
 
-void ElementComparer::_removeTagsNotImportantForComparison(Tags& tags) const
+QString ElementComparer::toHashString(const ConstElementPtr& e) const
 {
-  LOG_TRACE("Removing tags...");
-
-  // having trouble making this work here in place of the two following lines due to the
-  // ServiceChangesetReplacement* tests...will revisit
-  //tags.removeMetadata();
-
-  tags.remove(MetadataTags::HootStatus());
-  //this is ok b/c we have the actual id to compare to, but it should still probably be fixed to
-  //carry along the hoot:id tag for consistency's sake when that is desired
-  tags.remove(MetadataTags::HootId());
-
-  // not sure where "status" is coming from...should be "hoot:status"...bug somewhere?
-  tags.remove("status");
-  tags.remove(MetadataTags::SourceDateTime());
+  ElementHashVisitor hashVis;
+  hashVis.setOsmMap(_map.get());
+  // TODO: This doesn't seem right, but its been working this way for awhile now...remove later?
+  hashVis.setIncludeCircularError(true);
+  return hashVis.toHashString(e);
 }
 
 bool ElementComparer::isSame(ElementPtr e1, ElementPtr e2) const
 {
-  if (_ignoreElementId && !_map.get())
-  {
-    throw IllegalArgumentException(
-      "If ignoring element IDs in ElementComparer a map must be passed in.");
-  }
-
-  LOG_VART(e1->getElementId());
-  LOG_VART(e2->getElementId());
+  //LOG_VART(e1->getElementId());
+  //LOG_VART(e2->getElementId());
 
+  // different types?
   if (e1->getElementType() != e2->getElementType())
   {
+    LOG_TRACE("compare failed on type: " << e1->getElementId() << ", " << e2->getElementId());
     return false;
   }
 
-  if (e1->getElementType() == ElementType::Node)
+  // different IDs?
+  if (!_ignoreElementId && (e1->getElementId() != e2->getElementId()))
   {
-    // Set the hoot hash tag here if it doesn't exist, since its required for node comparisons.
-    CalculateHashVisitor2 hashVis;
-    if (!e1->getTags().contains(MetadataTags::HootHash()))
-    {
-      hashVis.visit(e1);
-    }
-    if (!e2->getTags().contains(MetadataTags::HootHash()))
-    {
-      hashVis.visit(e2);
-    }
+    LOG_TRACE("compare failed on ID: " << e1->getElementId() << ", " << e2->getElementId());
+    return false;
   }
-  //only nodes have been converted over to use hash comparisons so far
-  else
-  {
-    //create modified copies of the tags for comparing, as we don't care if some tags are identical
-    Tags tags1 = e1->getTags();
-    _removeTagsNotImportantForComparison(tags1);
-    Tags tags2 = e2->getTags();
-    _removeTagsNotImportantForComparison(tags2);
 
-    // not checking status here b/c if only the status changed on the element and no other tags or
-    // geometries, there's no point in detecting a change
-    if ((!_ignoreElementId && e1->getElementId() != e2->getElementId()) ||
-        !(tags1 == tags2) ||
-        (e1->getVersion() != e2->getVersion()) ||
-        fabs(e1->getCircularError() - e2->getCircularError()) > _threshold)
-    {
-      if (Log::getInstance().getLevel() == Log::Trace)
-      {
-        if ( e1->getElementId() != e2->getElementId())
-        {
-          LOG_TRACE("compare failed on IDs");
-        }
-        else if (!(tags1 == tags2))
-        {
-          LOG_TRACE("compare failed on tags");
-        }
-        else if (e1->getVersion() != e2->getVersion())
-        {
-          LOG_TRACE("compare failed on version");
-        }
-        else if (fabs(e1->getCircularError() - e2->getCircularError()) > _threshold)
-        {
-          LOG_TRACE("compare failed on circular error:");
-          LOG_VART(fabs(e1->getCircularError() - e2->getCircularError()));
-          LOG_VART(_threshold);
-        }
-
-        LOG_TRACE("elements failed comparison:");
-        LOG_VART(tags1);
-        LOG_VART(tags2);
-      }
+  // different versions?
+  if (!_ignoreVersion && (e1->getVersion() != e2->getVersion()))
+  {
+    LOG_TRACE("compare failed on version: " << e1->getElementId() << ", " << e2->getElementId());
+    return false;
+  }
 
-      return false;
-    }
+  if (_ignoreElementId && !_map.get())
+  {
+    throw IllegalArgumentException(
+      "If ignoring element IDs in ElementComparer a map must be passed in.");
   }
 
   switch (e1->getElementType().getEnum())
@@ -149,109 +96,169 @@ bool ElementComparer::isSame(ElementPtr e1, ElementPtr e2) const
   }
 }
 
-bool ElementComparer::_compareNode(const std::shared_ptr<const Element>& re,
-                                   const std::shared_ptr<const Element>& e) const
+void ElementComparer::_setHash(ElementPtr element) const
 {
-  if (!re->getTags().contains(MetadataTags::HootHash()) ||
-      !e->getTags().contains(MetadataTags::HootHash()))
+  // Set the hoot hash tag here if it doesn't exist.
+  ElementHashVisitor hashVis;
+  hashVis.setOsmMap(_map.get());
+  // TODO: This doesn't seem right, but its been working this way for awhile now...remove later?
+  hashVis.setIncludeCircularError(true);
+  if (!element->getTags().contains(MetadataTags::HootHash()))
   {
-    throw HootException(
-      QString("ElementComparer requires the %1 tag be set for node comparison. Nodes: %2, %3")
-      .arg(MetadataTags::HootHash())
-      .arg(re->getElementId().toString())
-      .arg(e->getElementId().toString()));
+    hashVis.visit(element);
   }
+}
 
-  if (re->getElementId().getId() == DEBUG_ID || e->getElementId().getId() == DEBUG_ID)
-  {
-    LOG_VARD(re);
-    LOG_VARD(e);
-  }
+bool ElementComparer::_compareNode(ElementPtr re, ElementPtr e) const
+{
+  _setHash(re);
+  _setHash(e);
 
-  bool same = false;
-  if (re->getTags()[MetadataTags::HootHash()] == e->getTags()[MetadataTags::HootHash()])
-  {
-    same = true;
-  }
-  else
+  if (re->getElementId().getId() == DEBUG_ID || e->getElementId().getId() == DEBUG_ID)
   {
-    LOG_TRACE("Compare failed:");
     LOG_VART(re);
     LOG_VART(e);
   }
 
-  return same;
+  return _haveSameHash(re, e);
 }
 
-bool ElementComparer::_compareWay(const std::shared_ptr<const Element>& re,
-                                  const std::shared_ptr<const Element>& e) const
+bool ElementComparer::_compareWay(ElementPtr re, ElementPtr e) const
 {
   ConstWayPtr rw = std::dynamic_pointer_cast<const Way>(re);
   ConstWayPtr w = std::dynamic_pointer_cast<const Way>(e);
 
-  if (rw->getNodeIds().size() != w->getNodeIds().size())
+  // optimization; tag count comparison isn't a good opt b/c you need to count metadata tags only
+  // and that count isn't available without extra computation
+  if (rw->getNodeCount() != w->getNodeCount())
   {
     LOG_TRACE(
       "Ways " << rw->getElementId() << " and " << w->getElementId() <<
-      " failed comparison on way node count: " << rw->getNodeIds().size() << " and " <<
-      w->getNodeIds().size());
+      " failed comparison on way node count: " << rw->getNodeCount() << " and " <<
+      w->getNodeCount());
     return false;
   }
 
   if (!_ignoreElementId)
   {
-    for (size_t i = 0; i < rw->getNodeIds().size(); ++i)
+    // If we're not ignoring the element IDs of the way nodes, then we'll perform just these checks
+    // instead of computing a hash and avoid having to use a passed in map for callers that don't
+    // have one.
+
+    if (!rw->hasSameNodes(*w))
     {
-      if (rw->getNodeIds()[i] != w->getNodeIds()[i])
-      {
-        LOG_TRACE(
-          "Ways " << rw->getElementId() << " and " << w->getElementId() <<
-          " failed comparison on way nodes: " << rw->getNodeIds() << " and " << w->getNodeIds());
-        return false;
-      }
+      LOG_TRACE(
+        "Ways " << rw->getElementId() << " and " << w->getElementId() <<
+        " failed comparison on way nodes: " << rw->getNodeIds() << " and " << w->getNodeIds());
+      return false;
     }
+
+    if (!rw->hasSameNonMetadataTags(*w))
+    {
+      LOG_TRACE(
+        "Ways " << rw->getElementId() << " and " << w->getElementId() <<
+        " failed comparison on tags.");
+      return false;
+    }
+
+    LOG_TRACE("Ways " << re->getElementId() << " and " << e->getElementId() << " are the same.");
+    return true;
   }
   else
   {
-    for (size_t i = 0; i < rw->getNodeIds().size(); ++i)
-    {
-      NodePtr nodeRw = _map->getNode(ElementId(ElementType::Node, rw->getNodeIds()[i]));
-      NodePtr nodeW = _map->getNode(ElementId(ElementType::Node, w->getNodeIds()[i]));
-      if (!nodeRw || !nodeW || !isSame(nodeRw, nodeW))
-      {
-        LOG_TRACE(
-          "Ways " << rw->getElementId() << " and " << w->getElementId() <<
-          " failed comparison on way nodes: " << rw->getNodeIds() << " and " << w->getNodeIds());
-        return false;
-      }
-    }
+    // If we're ignoring element ID, then we have to dig deeper and compare each way node, which the
+    // hashing takes care of. Hashing also takes tags into account.
+    _setHash(re);
+    _setHash(e);
+    return _haveSameHash(re, e);
   }
-
-  return true;
 }
 
-bool ElementComparer::_compareRelation(const std::shared_ptr<const Element>& re,
-                                       const std::shared_ptr<const Element>& e) const
+bool ElementComparer::_compareRelation(ElementPtr re, ElementPtr e) const
 {
   ConstRelationPtr rr = std::dynamic_pointer_cast<const Relation>(re);
   ConstRelationPtr r = std::dynamic_pointer_cast<const Relation>(e);
 
-  if (rr->getType() != r->getType() ||
-      rr->getMembers().size() != r->getMembers().size())
+  // optimization; tag count comparison isn't a good opt b/c you need to count metadata tags only
+  // and that count isn't available without extra computation
+  if (rr->getType() != r->getType())
   {
+    LOG_TRACE(
+      "Relations " << rr->getElementId() << " and " << r->getElementId() <<
+      " failed comparison on type: " << rr->getType() << " and " << r->getType());
+    return false;
+  }
+  if (rr->getMemberCount() != r->getMemberCount())
+  {
+    LOG_TRACE(
+      "Relations " << rr->getElementId() << " and " << r->getElementId() <<
+      " failed comparison on relation member count: " << rr->getMemberCount() << " and " <<
+      r->getMemberCount());
     return false;
   }
 
-  for (size_t i = 0; i < rr->getMembers().size(); i++)
+  if (!_ignoreElementId)
   {
-    if (rr->getMembers()[i].role != r->getMembers()[i].role ||
-        rr->getMembers()[i].getElementId() != r->getMembers()[i].getElementId())
+    // If we're not ignoring the element IDs of the relation members, then we'll perform just these
+    // checks instead of computing a hash and avoid having to use a passed in map for callers that
+    // don't have one.
+
+    if (!rr->hasSameNonMetadataTags(*r))
     {
+      LOG_TRACE(
+        "Relations " << rr->getElementId() << " and " << r->getElementId() <<
+        " failed comparison on tags.");
       return false;
     }
-  }
 
+    for (size_t i = 0; i < rr->getMembers().size(); i++)
+    {
+      if (rr->getMembers()[i].role != r->getMembers()[i].role ||
+          rr->getMembers()[i].getElementId() != r->getMembers()[i].getElementId())
+      {
+        LOG_TRACE(
+          "Relations " << rr->getElementId() << " and " << r->getElementId() <<
+          " failed comparison on relation members.");
+        return false;
+      }
+    }
     return true;
+  }
+  else
+  {
+    // If we're ignoring element ID, then we have to dig deeper and compare each member element,
+    // which the hashing takes care of. Hashing also takes tags into account.
+    _setHash(re);
+    _setHash(e);
+    return _haveSameHash(re, e);
+  }
+}
+
+bool ElementComparer::_haveSameHash(ElementPtr re, ElementPtr e) const
+{
+  if (!re->getTags().contains(MetadataTags::HootHash()) ||
+      !e->getTags().contains(MetadataTags::HootHash()))
+  {
+    throw HootException(
+      QString("ElementComparer requires the %1 tag be set for element comparison. Elements: %2, %3")
+        .arg(MetadataTags::HootHash())
+        .arg(re->getElementId().toString())
+        .arg(e->getElementId().toString()));
+  }
+
+  bool same = false;
+  if (re->getTags()[MetadataTags::HootHash()] == e->getTags()[MetadataTags::HootHash()])
+  {
+    LOG_TRACE(re->getElementId() << " and " << e->getElementId() << " are the same.");
+    same = true;
+  }
+  else
+  {
+    LOG_TRACE("Compare failed:");
+    LOG_VART(re);
+    LOG_VART(e);
+  }
+  return same;
 }
 
 }