Refine behavior of QgsGeometry equals tests

nyalldawson · nyalldawson · commit 2d43dac0b509 · 2018-01-05T09:59:22.000+10:00
Before we had two checks - equals() and isGeosEqual() which
performed the exact same check (since equals() called the geos
equality test)

Since the geos equality test is a slow, topological test, which
considers two geometries equal if their component edges overlap,
but disregards ordering of vertices this is not always what we
want. There's also the issue that geos cannot consider m values
when testing the geometries, so two geometries with different
m values would be reported equal.

So, now calling QgsGeometry::equals performs a very fast, strict
equality test where geometries are only equal if the have exactly
the same vertices, type, and order.

And swap most code which was calling the slow geos test to instead
use the fast strict native test.
diff --git a/doc/api_break.dox b/doc/api_break.dox
@@ -1354,6 +1354,8 @@ maintains Z or M dimensions from the input points and is more efficient.
 - exportToWkt() was renamed to asWkt()
 - exportToGeoJSON() was renamed to asJson()
 - closestSegmentWithContext() now returns an extra value, indicating whether the point is to the left of the geometry
+- equals() now performs a fast, strict equality check, where geometries are considered equal only if they have the
+exact same type, vertices and order. The slower topological test can be performed by calling isGeosEqual instead.
 
 
 QgsGeometryAnalyzer        {#qgis_api_break_3_0_QgsGeometryAnalyzer}
diff --git a/python/core/geometry/qgsgeometry.sip b/python/core/geometry/qgsgeometry.sip
@@ -247,13 +247,46 @@ return true for isEmpty().
     bool isMultipart() const;
 %Docstring
 Returns true if WKB of the geometry is of WKBMulti* type
+%End
+
+    bool equals( const QgsGeometry &geometry ) const;
+%Docstring
+Test if this geometry is exactly equal to another ``geometry``.
+
+This is a strict equality check, where the underlying geometries must
+have exactly the same type, component vertices and vertex order.
+
+Calling this method is dramatically faster than the topological
+equality test performed by isGeosEqual().
+
+.. note::
+
+   Comparing two null geometries will return false.
+
+.. versionadded:: 1.5
+
+.. seealso:: :py:func:`isGeosEqual()`
 %End
 
     bool isGeosEqual( const QgsGeometry & ) const;
 %Docstring
-Compares the geometry with another geometry using GEOS
+Compares the geometry with another geometry using GEOS.
+
+This method performs a slow, topological check, where geometries
+are considered equal if all of the their component edges overlap. E.g.
+lines with the same vertex locations but opposite direction will be
+considered equal by this method.
+
+Consider using the much faster, stricter equality test performed
+by equals() instead.
+
+.. note::
+
+   Comparing two null geometries will return false.
 
 .. versionadded:: 1.5
+
+.. seealso:: :py:func:`equals()`
 %End
 
     bool isGeosValid() const;
@@ -749,13 +782,6 @@ Tests for if geometry is contained in another (uses GEOS)
 %Docstring
 Tests for if geometry is disjoint of another (uses GEOS)
 
-.. versionadded:: 1.5
-%End
-
-    bool equals( const QgsGeometry &geometry ) const;
-%Docstring
-Test for if geometry equals another (uses GEOS)
-
 .. versionadded:: 1.5
 %End
 
diff --git a/src/core/geometry/qgsgeometry.cpp b/src/core/geometry/qgsgeometry.cpp
@@ -1147,9 +1147,12 @@ bool QgsGeometry::equals( const QgsGeometry &geometry ) const
     return false;
   }
 
-  QgsGeos geos( d->geometry.get() );
-  mLastError.clear();
-  return geos.isEqual( geometry.d->geometry.get(), &mLastError );
+  // fast check - are they shared copies of the same underlying geometry?
+  if ( d == geometry.d )
+    return true;
+
+  // slower check - actually test the geometries
+  return *d->geometry.get() == *geometry.d->geometry.get();
 }
 
 bool QgsGeometry::touches( const QgsGeometry &geometry ) const
diff --git a/src/core/geometry/qgsgeometry.h b/src/core/geometry/qgsgeometry.h
@@ -307,8 +307,36 @@ class CORE_EXPORT QgsGeometry
     bool isMultipart() const;
 
     /**
-     * Compares the geometry with another geometry using GEOS
+     * Test if this geometry is exactly equal to another \a geometry.
+     *
+     * This is a strict equality check, where the underlying geometries must
+     * have exactly the same type, component vertices and vertex order.
+     *
+     * Calling this method is dramatically faster than the topological
+     * equality test performed by isGeosEqual().
+     *
+     * \note Comparing two null geometries will return false.
+     *
+     * \since QGIS 1.5
+     * \see isGeosEqual()
+     */
+    bool equals( const QgsGeometry &geometry ) const;
+
+    /**
+     * Compares the geometry with another geometry using GEOS.
+     *
+     * This method performs a slow, topological check, where geometries
+     * are considered equal if all of the their component edges overlap. E.g.
+     * lines with the same vertex locations but opposite direction will be
+     * considered equal by this method.
+     *
+     * Consider using the much faster, stricter equality test performed
+     * by equals() instead.
+     *
+     * \note Comparing two null geometries will return false.
+     *
      * \since QGIS 1.5
+     * \see equals()
      */
     bool isGeosEqual( const QgsGeometry & ) const;
 
@@ -790,12 +818,6 @@ class CORE_EXPORT QgsGeometry
      */
     bool disjoint( const QgsGeometry &geometry ) const;
 
-    /**
-     * Test for if geometry equals another (uses GEOS)
-     *  \since QGIS 1.5
-     */
-    bool equals( const QgsGeometry &geometry ) const;
-
     /**
      * Test for if geometry touch another (uses GEOS)
      *  \since QGIS 1.5
diff --git a/src/core/qgsvectorlayer.cpp b/src/core/qgsvectorlayer.cpp
@@ -985,7 +985,7 @@ bool QgsVectorLayer::updateFeature( const QgsFeature &updatedFeature, bool skipD
     bool hasChanged = false;
     bool hasError = false;
 
-    if ( ( updatedFeature.hasGeometry() || currentFeature.hasGeometry() ) && !updatedFeature.geometry().isGeosEqual( currentFeature.geometry() ) )
+    if ( ( updatedFeature.hasGeometry() || currentFeature.hasGeometry() ) && !updatedFeature.geometry().equals( currentFeature.geometry() ) )
     {
       if ( changeGeometry( updatedFeature.id(), updatedFeature.geometry(), true ) )
       {
diff --git a/tests/src/core/testqgsgeometry.cpp b/tests/src/core/testqgsgeometry.cpp
@@ -80,8 +80,10 @@ class TestQgsGeometry : public QObject
     void asVariant(); //test conversion to and from a QVariant
     void isEmpty();
     void operatorBool();
+    void equality();
     void vertexIterator();
 
+
     // geometry types
     void point(); //test QgsPointV2
     void lineString(); //test QgsLineString
@@ -431,6 +433,43 @@ void TestQgsGeometry::operatorBool()
   QVERIFY( !geom );
 }
 
+void TestQgsGeometry::equality()
+{
+  // null geometries
+  QVERIFY( !QgsGeometry().equals( QgsGeometry() ) );
+
+  // compare to null
+  QgsGeometry g1( qgis::make_unique< QgsPoint >( 1.0, 2.0 ) );
+  QVERIFY( !g1.equals( QgsGeometry() ) );
+  QVERIFY( !QgsGeometry().equals( g1 ) );
+
+  // compare implicitly shared copies
+  QgsGeometry g2( g1 );
+  QVERIFY( g2.equals( g1 ) );
+  QVERIFY( g1.equals( g2 ) );
+  QVERIFY( g1.equals( g1 ) );
+
+  // equal geometry, but different internal data
+  g2 = QgsGeometry::fromWkt( "Point( 1.0 2.0 )" );
+  QVERIFY( g2.equals( g1 ) );
+  QVERIFY( g1.equals( g2 ) );
+
+  // different dimensionality
+  g2 = QgsGeometry::fromWkt( "PointM( 1.0 2.0 3.0)" );
+  QVERIFY( !g2.equals( g1 ) );
+  QVERIFY( !g1.equals( g2 ) );
+
+  // different type
+  g2 = QgsGeometry::fromWkt( "LineString( 1.0 2.0, 3.0 4.0 )" );
+  QVERIFY( !g2.equals( g1 ) );
+  QVERIFY( !g1.equals( g2 ) );
+
+  // different direction
+  g1 = QgsGeometry::fromWkt( "LineString( 3.0 4.0, 1.0 2.0 )" );
+  QVERIFY( !g2.equals( g1 ) );
+  QVERIFY( !g1.equals( g2 ) );
+}
+
 void TestQgsGeometry::vertexIterator()
 {
   QgsGeometry geom;

Original file line number	Diff line number	Diff line change
`@@ -1147,9 +1147,12 @@ bool QgsGeometry::equals( const QgsGeometry &geometry ) const`
`1147`	`1147`	`return false;`
`1148`	`1148`	`}`
`1149`	`1149`
`1150`		`- QgsGeos geos( d->geometry.get() );`
`1151`		`- mLastError.clear();`
`1152`		`- return geos.isEqual( geometry.d->geometry.get(), &mLastError );`
	`1150`	`+ // fast check - are they shared copies of the same underlying geometry?`
	`1151`	`+ if ( d == geometry.d )`
	`1152`	`+ return true;`
	`1153`	`+`
	`1154`	`+ // slower check - actually test the geometries`
	`1155`	`+ return d->geometry.get() == geometry.d->geometry.get();`
`1153`	`1156`	`}`
`1154`	`1157`
`1155`	`1158`	`bool QgsGeometry::touches( const QgsGeometry &geometry ) const`
Original file line number	Diff line number	Diff line change
`@@ -985,7 +985,7 @@ bool QgsVectorLayer::updateFeature( const QgsFeature &updatedFeature, bool skipD`
`985`	`985`	`bool hasChanged = false;`
`986`	`986`	`bool hasError = false;`
`987`	`987`
`988`		`- if ( ( updatedFeature.hasGeometry() \|\| currentFeature.hasGeometry() ) && !updatedFeature.geometry().isGeosEqual( currentFeature.geometry() ) )`
	`988`	`+ if ( ( updatedFeature.hasGeometry() \|\| currentFeature.hasGeometry() ) && !updatedFeature.geometry().equals( currentFeature.geometry() ) )`
`989`	`989`	`{`
`990`	`990`	`if ( changeGeometry( updatedFeature.id(), updatedFeature.geometry(), true ) )`
`991`	`991`	`{`