Avoid firing up a thread for blocking GEOS based validity checks

It's an unnecessary expense. Also fix duplicate code. (cherry picked from commit 35f613d)
qgis · Mar 5, 2019 · 82f8eb0 · 82f8eb0
1 parent 111f357
commit 82f8eb0
Show file tree

Hide file tree

Showing 9 changed files with 128 additions and 63 deletions.
diff --git a/python/core/auto_generated/geometry/qgsgeometry.sip.in b/python/core/auto_generated/geometry/qgsgeometry.sip.in
@@ -1793,6 +1793,8 @@ True if the location available from :py:func:`where` is valid.
         sipRes = PyUnicode_FromString( str.toUtf8().data() );
 %End
 
+        bool operator==( const QgsGeometry::Error &other ) const;
+
     };
 
     enum ValidationMethod

diff --git a/python/core/auto_generated/geometry/qgsgeometryengine.sip.in b/python/core/auto_generated/geometry/qgsgeometryengine.sip.in
@@ -208,7 +208,7 @@ pattern.
     virtual double area( QString *errorMsg = 0 ) const = 0;
     virtual double length( QString *errorMsg = 0 ) const = 0;
 
-    virtual bool isValid( QString *errorMsg = 0, bool allowSelfTouchingHoles = false ) const = 0;
+    virtual bool isValid( QString *errorMsg = 0, bool allowSelfTouchingHoles = false, QgsGeometry *errorLoc = 0 ) const = 0;
 %Docstring
 Returns true if the geometry is valid.
 
@@ -217,6 +217,8 @@ If the geometry is invalid, ``errorMsg`` will be filled with the reported geomet
 The ``allowSelfTouchingHoles`` argument specifies whether self-touching holes are permitted.
 OGC validity states that self-touching holes are NOT permitted, whilst other vendor
 validity checks (e.g. ESRI) permit self-touching holes.
+
+If ``errorLoc`` is specified, it will be set to the geometry of the error location.
 %End
 
     virtual bool isEqual( const QgsAbstractGeometry *geom, QString *errorMsg = 0 ) const = 0;

diff --git a/src/core/geometry/qgsgeometry.cpp b/src/core/geometry/qgsgeometry.cpp
@@ -2459,7 +2459,42 @@ QgsGeometry QgsGeometry::forceRHR() const
 
 void QgsGeometry::validateGeometry( QVector<QgsGeometry::Error> &errors, ValidationMethod method ) const
 {
-  QgsGeometryValidator::validateGeometry( *this, errors, method );
+  errors.clear();
+  if ( !d->geometry )
+    return;
+
+  // avoid expensive calcs for trivial point geometries
+  if ( QgsWkbTypes::geometryType( d->geometry->wkbType() ) == QgsWkbTypes::PointGeometry )
+  {
+    return;
+  }
+
+  switch ( method )
+  {
+    case ValidatorQgisInternal:
+      QgsGeometryValidator::validateGeometry( *this, errors, method );
+      return;
+
+    case ValidatorGeos:
+    {
+      QgsGeos geos( d->geometry.get() );
+      QString error;
+      QgsGeometry errorLoc;
+      if ( !geos.isValid( &error, true, &errorLoc ) )
+      {
+        if ( errorLoc.isNull() )
+        {
+          errors.append( QgsGeometry::Error( error ) );
+        }
+        else
+        {
+          const QgsPointXY point = errorLoc.asPoint();
+          errors.append( QgsGeometry::Error( error, point ) );
+        }
+        return;
+      }
+    }
+  }
 }
 
 bool QgsGeometry::isGeosValid( const QgsGeometry::ValidityFlags flags ) const
@@ -2477,7 +2512,7 @@ bool QgsGeometry::isGeosValid( const QgsGeometry::ValidityFlags flags ) const
 
   QgsGeos geos( d->geometry.get() );
   mLastError.clear();
-  return geos.isValid( &mLastError, flags & FlagAllowSelfTouchingHoles );
+  return geos.isValid( &mLastError, flags & FlagAllowSelfTouchingHoles, nullptr );
 }
 
 bool QgsGeometry::isSimple() const

diff --git a/src/core/geometry/qgsgeometry.h b/src/core/geometry/qgsgeometry.h
@@ -1851,6 +1851,11 @@ class CORE_EXPORT QgsGeometry
         % End
 #endif
 
+        bool operator==( const QgsGeometry::Error &other ) const
+        {
+          return other.mMessage == mMessage && other.mHasLocation == mHasLocation && other.mLocation == mLocation;
+        }
+
       private:
         QString mMessage;
         QgsPointXY mLocation;

diff --git a/src/core/geometry/qgsgeometryengine.h b/src/core/geometry/qgsgeometryengine.h
@@ -226,8 +226,10 @@ class CORE_EXPORT QgsGeometryEngine
      * The \a allowSelfTouchingHoles argument specifies whether self-touching holes are permitted.
      * OGC validity states that self-touching holes are NOT permitted, whilst other vendor
      * validity checks (e.g. ESRI) permit self-touching holes.
+     *
+     * If \a errorLoc is specified, it will be set to the geometry of the error location.
      */
-    virtual bool isValid( QString *errorMsg = nullptr, bool allowSelfTouchingHoles = false ) const = 0;
+    virtual bool isValid( QString *errorMsg = nullptr, bool allowSelfTouchingHoles = false, QgsGeometry *errorLoc = nullptr ) const = 0;
 
     /**
      * Checks if this is equal to \a geom.

diff --git a/src/core/geometry/qgsgeos.cpp b/src/core/geometry/qgsgeos.cpp
@@ -1658,7 +1658,7 @@ QgsAbstractGeometry *QgsGeos::convexHull( QString *errorMsg ) const
   CATCH_GEOS_WITH_ERRMSG( nullptr );
 }
 
-bool QgsGeos::isValid( QString *errorMsg, const bool allowSelfTouchingHoles ) const
+bool QgsGeos::isValid( QString *errorMsg, const bool allowSelfTouchingHoles, QgsGeometry *errorLoc ) const
 {
   if ( !mGeos )
   {
@@ -1673,7 +1673,38 @@ bool QgsGeos::isValid( QString *errorMsg, const bool allowSelfTouchingHoles ) co
     const bool invalid = res != 1;
 
     if ( invalid && errorMsg )
-      *errorMsg = QString( r );
+    {
+      static QgsStringMap translatedErrors;
+
+      if ( translatedErrors.empty() )
+      {
+        // Copied from https://git.osgeo.org/gitea/geos/geos/src/branch/master/src/operation/valid/TopologyValidationError.cpp
+        translatedErrors.insert( QStringLiteral( "topology validation error" ), QObject::tr( "Topology validation error", "GEOS Error" ) );
+        translatedErrors.insert( QStringLiteral( "repeated point" ), QObject::tr( "Repeated point", "GEOS Error" ) );
+        translatedErrors.insert( QStringLiteral( "hole lies outside shell" ), QObject::tr( "Hole lies outside shell", "GEOS Error" ) );
+        translatedErrors.insert( QStringLiteral( "holes are nested" ), QObject::tr( "Holes are nested", "GEOS Error" ) );
+        translatedErrors.insert( QStringLiteral( "interior is disconnected" ), QObject::tr( "Interior is disconnected", "GEOS Error" ) );
+        translatedErrors.insert( QStringLiteral( "self-intersection" ), QObject::tr( "Self-intersection", "GEOS Error" ) );
+        translatedErrors.insert( QStringLiteral( "ring self-intersection" ), QObject::tr( "Ring self-intersection", "GEOS Error" ) );
+        translatedErrors.insert( QStringLiteral( "nested shells" ), QObject::tr( "Nested shells", "GEOS Error" ) );
+        translatedErrors.insert( QStringLiteral( "duplicate rings" ), QObject::tr( "Duplicate rings", "GEOS Error" ) );
+        translatedErrors.insert( QStringLiteral( "too few points in geometry component" ), QObject::tr( "Too few points in geometry component", "GEOS Error" ) );
+        translatedErrors.insert( QStringLiteral( "invalid coordinate" ), QObject::tr( "Invalid coordinate", "GEOS Error" ) );
+        translatedErrors.insert( QStringLiteral( "ring is not closed" ), QObject::tr( "Ring is not closed", "GEOS Error" ) );
+      }
+
+      const QString error( r );
+      *errorMsg = translatedErrors.value( error.toLower(), error );
+
+      if ( g1 && errorLoc )
+      {
+        *errorLoc = geometryFromGeos( g1 );
+      }
+      else if ( g1 )
+      {
+        GEOSGeom_destroy_r( geosinit.ctxt, g1 );
+      }
+    }
     return !invalid;
   }
   CATCH_GEOS_WITH_ERRMSG( false );

diff --git a/src/core/geometry/qgsgeos.h b/src/core/geometry/qgsgeos.h
@@ -218,7 +218,7 @@ class CORE_EXPORT QgsGeos: public QgsGeometryEngine
     bool relatePattern( const QgsAbstractGeometry *geom, const QString &pattern, QString *errorMsg = nullptr ) const override;
     double area( QString *errorMsg = nullptr ) const override;
     double length( QString *errorMsg = nullptr ) const override;
-    bool isValid( QString *errorMsg = nullptr, bool allowSelfTouchingHoles = false ) const override;
+    bool isValid( QString *errorMsg = nullptr, bool allowSelfTouchingHoles = false, QgsGeometry *errorLoc = nullptr ) const override;
     bool isEqual( const QgsAbstractGeometry *geom, QString *errorMsg = nullptr ) const override;
     bool isEmpty( QString *errorMsg = nullptr ) const override;
     bool isSimple( QString *errorMsg = nullptr ) const override;

diff --git a/src/core/qgsgeometryvalidator.cpp b/src/core/qgsgeometryvalidator.cpp
@@ -217,65 +217,32 @@ void QgsGeometryValidator::run()
   {
     case QgsGeometry::ValidatorGeos:
     {
-      char *r = nullptr;
-      geos::unique_ptr g0 = QgsGeos::asGeos( mGeometry );
-      GEOSContextHandle_t handle = QgsGeos::getGEOSHandler();
-      if ( !g0 )
+      if ( mGeometry.isNull() )
       {
-        emit errorFound( QgsGeometry::Error( QObject::tr( "GEOS error: could not produce geometry for GEOS (check log window)" ) ) );
+        return;
       }
-      else
-      {
-        GEOSGeometry *g1 = nullptr;
-        char res = GEOSisValidDetail_r( handle, g0.get(), GEOSVALID_ALLOW_SELFTOUCHING_RING_FORMING_HOLE, &r, &g1 );
-        if ( res != 1 )
-        {
-          static QgsStringMap translatedErrors;
-
-          if ( translatedErrors.empty() )
-          {
-            // Copied from https://git.osgeo.org/gitea/geos/geos/src/branch/master/src/operation/valid/TopologyValidationError.cpp
-            translatedErrors.insert( QStringLiteral( "topology validation error" ), QObject::tr( "Topology validation error", "GEOS Error" ) );
-            translatedErrors.insert( QStringLiteral( "repeated point" ), QObject::tr( "Repeated point", "GEOS Error" ) );
-            translatedErrors.insert( QStringLiteral( "hole lies outside shell" ), QObject::tr( "Hole lies outside shell", "GEOS Error" ) );
-            translatedErrors.insert( QStringLiteral( "holes are nested" ), QObject::tr( "Holes are nested", "GEOS Error" ) );
-            translatedErrors.insert( QStringLiteral( "interior is disconnected" ), QObject::tr( "Interior is disconnected", "GEOS Error" ) );
-            translatedErrors.insert( QStringLiteral( "self-intersection" ), QObject::tr( "Self-intersection", "GEOS Error" ) );
-            translatedErrors.insert( QStringLiteral( "ring self-intersection" ), QObject::tr( "Ring self-intersection", "GEOS Error" ) );
-            translatedErrors.insert( QStringLiteral( "nested shells" ), QObject::tr( "Nested shells", "GEOS Error" ) );
-            translatedErrors.insert( QStringLiteral( "duplicate rings" ), QObject::tr( "Duplicate rings", "GEOS Error" ) );
-            translatedErrors.insert( QStringLiteral( "too few points in geometry component" ), QObject::tr( "Too few points in geometry component", "GEOS Error" ) );
-            translatedErrors.insert( QStringLiteral( "invalid coordinate" ), QObject::tr( "Invalid coordinate", "GEOS Error" ) );
-            translatedErrors.insert( QStringLiteral( "ring is not closed" ), QObject::tr( "Ring is not closed", "GEOS Error" ) );
-          }
-
-          const QString errorMsg( r );
-          const QString translatedErrorMsg = translatedErrors.value( errorMsg.toLower(), errorMsg );
-
-          if ( g1 )
-          {
-            const GEOSCoordSequence *cs = GEOSGeom_getCoordSeq_r( handle, g1 );
-
-            unsigned int n;
-            if ( GEOSCoordSeq_getSize_r( handle, cs, &n ) && n == 1 )
-            {
-              double x, y;
-              GEOSCoordSeq_getX_r( handle, cs, 0, &x );
-              GEOSCoordSeq_getY_r( handle, cs, 0, &y );
-
-              emit errorFound( QgsGeometry::Error( translatedErrorMsg, QgsPointXY( x, y ) ) );
-              mErrorCount++;
-            }
 
-            GEOSGeom_destroy_r( handle, g1 );
-          }
-          else
-          {
-            emit errorFound( QgsGeometry::Error( translatedErrorMsg ) );
-            mErrorCount++;
-          }
+      // avoid calling geos for trivial point geometries
+      if ( QgsWkbTypes::geometryType( mGeometry.wkbType() ) == QgsWkbTypes::PointGeometry )
+      {
+        return;
+      }
 
-          GEOSFree_r( handle, r );
+      QgsGeos geos( mGeometry.constGet() );
+      QString error;
+      QgsGeometry errorLoc;
+      if ( !geos.isValid( &error, true, &errorLoc ) )
+      {
+        if ( errorLoc.isNull() )
+        {
+          emit errorFound( QgsGeometry::Error( error ) );
+          mErrorCount++;
+        }
+        else
+        {
+          const QgsPointXY point = errorLoc.asPoint();
+          emit errorFound( QgsGeometry::Error( error, point ) );
+          mErrorCount++;
         }
       }
 

diff --git a/tests/src/python/test_qgsgeometry.py b/tests/src/python/test_qgsgeometry.py
@@ -5048,7 +5048,8 @@ def testIsGeosValid(self):
             ["LINESTRING (0 0, 0 100, 100 100)", True, True, ''],
             ["POLYGON((-1 -1, 4 0, 4 2, 0 2, -1 -1))", True, True, ''],
             ["MULTIPOLYGON(Polygon((-1 -1, 4 0, 4 2, 0 2, -1 -1)),Polygon((100 100, 200 100, 200 200, 100 200, 100 100)))", True, True, ''],
-            ['MultiPolygon (((159865.14786298031685874 6768656.31838363595306873, 159858.97975336571107619 6769211.44824895076453686, 160486.07089751763851382 6769211.44824895076453686, 160481.95882444124436006 6768658.37442017439752817, 160163.27316101978067309 6768658.37442017439752817, 160222.89822062765597366 6769116.87056819349527359, 160132.43261294672265649 6769120.98264127038419247, 160163.27316101978067309 6768658.37442017439752817, 159865.14786298031685874 6768656.31838363595306873)))', False, True, 'Ring Self-intersection']
+            ['MultiPolygon (((159865.14786298031685874 6768656.31838363595306873, 159858.97975336571107619 6769211.44824895076453686, 160486.07089751763851382 6769211.44824895076453686, 160481.95882444124436006 6768658.37442017439752817, 160163.27316101978067309 6768658.37442017439752817, 160222.89822062765597366 6769116.87056819349527359, 160132.43261294672265649 6769120.98264127038419247, 160163.27316101978067309 6768658.37442017439752817, 159865.14786298031685874 6768656.31838363595306873)))', False, True, 'Ring self-intersection'],
+            ['Polygon((0 3, 3 0, 3 3, 0 0, 0 3))', False, False, 'Self-intersection'],
         ]
         for t in tests:
             g1 = QgsGeometry.fromWkt(t[0])
@@ -5061,6 +5062,26 @@ def testIsGeosValid(self):
             self.assertEqual(res, t[2],
                              "mismatch for {}, expected:\n{}\nGot:\n{}\n".format(t[0], t[2], res))
 
+    def testValidateGeometry(self):
+        tests = [
+            ["", []],
+            ["Point (100 100)", [], []],
+            ["MultiPoint (100 100, 100 200)", [], []],
+            ["LINESTRING (0 0, 0 100, 100 100)", [], []],
+            ["POLYGON((-1 -1, 4 0, 4 2, 0 2, -1 -1))", [], []],
+            ["MULTIPOLYGON(Polygon((-1 -1, 4 0, 4 2, 0 2, -1 -1)),Polygon((100 100, 200 100, 200 200, 100 200, 100 100)))", [], []],
+            ['MultiPolygon (((159865.14786298031685874 6768656.31838363595306873, 159858.97975336571107619 6769211.44824895076453686, 160486.07089751763851382 6769211.44824895076453686, 160481.95882444124436006 6768658.37442017439752817, 160163.27316101978067309 6768658.37442017439752817, 160222.89822062765597366 6769116.87056819349527359, 160132.43261294672265649 6769120.98264127038419247, 160163.27316101978067309 6768658.37442017439752817, 159865.14786298031685874 6768656.31838363595306873)))', [], []],
+            ['Polygon((0 3, 3 0, 3 3, 0 0, 0 3))', [QgsGeometry.Error('Self-intersection', QgsPointXY(1.5, 1.5))], [QgsGeometry.Error('segments 0 and 2 of line 0 intersect at 1.5, 1.5', QgsPointXY(1.5, 1.5))]],
+        ]
+        for t in tests:
+            g1 = QgsGeometry.fromWkt(t[0])
+            res = g1.validateGeometry(QgsGeometry.ValidatorGeos)
+            self.assertEqual(res, t[1],
+                             "mismatch for {}, expected:\n{}\nGot:\n{}\n".format(t[0], t[1], res[0].where() if res else ''))
+            res = g1.validateGeometry(QgsGeometry.ValidatorQgisInternal)
+            self.assertEqual(res, t[2],
+                             "mismatch for {}, expected:\n{}\nGot:\n{}\n".format(t[0], t[2], res[0].where() if res else ''))
+
     def renderGeometry(self, geom, use_pen, as_polygon=False, as_painter_path=False):
         image = QImage(200, 200, QImage.Format_RGB32)
         image.fill(QColor(0, 0, 0))