Cache validity check results

For non-point geometry subclasses (points are always valid!) we now cache the results of a geometry validity check. Subsequent checks utilise the cached result wherever possible. Because QgsGeometry/QgsFeature objects are implicitly shared, this means that we avoid a *lot* of duplicate validity checks as features and geometries are thrown around during processing model execution.
qgis · Feb 28, 2019 · 75697d7 · 75697d7
1 parent 16c114b
commit 75697d7
Show file tree

Hide file tree

Showing 19 changed files with 149 additions and 25 deletions.
diff --git a/python/core/auto_generated/geometry/qgsabstractgeometry.sip.in b/python/core/auto_generated/geometry/qgsabstractgeometry.sip.in
@@ -607,6 +607,20 @@ Converts the geometry to a specified type.
 .. versionadded:: 2.14
 %End
 
+    virtual bool isValid( QString &error /Out/, int flags = 0 ) const = 0;
+%Docstring
+Checks validity of the geometry, and returns ``True`` if the geometry is valid.
+
+:param flags: indicates optional flags which control the type of validity checking performed
+              (corresponding to QgsGeometry.ValidityFlags).
+
+:return: - ``True`` if geometry is valid
+         - error: will be set to the validity error message
+
+
+.. versionadded:: 3.8
+%End
+
 
     QgsGeometryPartIterator parts();
 %Docstring

diff --git a/python/core/auto_generated/geometry/qgscurve.sip.in b/python/core/auto_generated/geometry/qgscurve.sip.in
@@ -172,6 +172,8 @@ Returns a geometry without curves. Caller takes ownership
 
     virtual QgsRectangle boundingBox() const;
 
+    virtual bool isValid( QString &error /Out/, int flags = 0 ) const;
+
 
     virtual double xAt( int index ) const = 0;
 %Docstring

diff --git a/python/core/auto_generated/geometry/qgsgeometrycollection.sip.in b/python/core/auto_generated/geometry/qgsgeometrycollection.sip.in
@@ -206,6 +206,8 @@ Returns a geometry without curves. Caller takes ownership
 
     virtual QgsPoint vertexAt( QgsVertexId id ) const;
 
+    virtual bool isValid( QString &error /Out/, int flags = 0 ) const;
+
 
     virtual bool addZValue( double zValue = 0 );
 

diff --git a/python/core/auto_generated/geometry/qgsmultipoint.sip.in b/python/core/auto_generated/geometry/qgsmultipoint.sip.in
@@ -50,6 +50,8 @@ Multi point geometry collection.
 
     virtual double segmentLength( QgsVertexId startVertex ) const;
 
+    virtual bool isValid( QString &error /Out/, int flags = 0 ) const;
+
 
 
     virtual QgsMultiPoint *createEmptyWithSameType() const /Factory/;

diff --git a/python/core/auto_generated/geometry/qgspoint.sip.in b/python/core/auto_generated/geometry/qgspoint.sip.in
@@ -371,6 +371,8 @@ M value is preserved.
 
     virtual QgsAbstractGeometry *boundary() const /Factory/;
 
+    virtual bool isValid( QString &error /Out/, int flags = 0 ) const;
+
 
     virtual bool insertVertex( QgsVertexId position, const QgsPoint &vertex );
 

diff --git a/python/core/auto_generated/geometry/qgssurface.sip.in b/python/core/auto_generated/geometry/qgssurface.sip.in
@@ -25,14 +25,16 @@ Ownership is transferred to the caller.
 %End
 
     virtual QgsRectangle boundingBox() const;
-%Docstring
-Returns the minimal bounding box for the geometry
-%End
+
+    virtual bool isValid( QString &error /Out/, int flags = 0 ) const;
+
+
 
   protected:
 
     virtual void clearCache() const;
 
+
 };
 
 /************************************************************************

diff --git a/src/core/geometry/qgsabstractgeometry.h b/src/core/geometry/qgsabstractgeometry.h
@@ -577,6 +577,19 @@ class CORE_EXPORT QgsAbstractGeometry
      */
     virtual bool convertTo( QgsWkbTypes::Type type );
 
+    /**
+     * Checks validity of the geometry, and returns TRUE if the geometry is valid.
+     *
+     * \param error will be set to the validity error message
+     * \param flags indicates optional flags which control the type of validity checking performed
+     * (corresponding to QgsGeometry::ValidityFlags).
+     *
+     * \returns TRUE if geometry is valid
+     *
+     * \since QGIS 3.8
+     */
+    virtual bool isValid( QString &error SIP_OUT, int flags = 0 ) const = 0;
+
 #ifndef SIP_RUN
 
     /**

diff --git a/src/core/geometry/qgscurve.cpp b/src/core/geometry/qgscurve.cpp
@@ -21,6 +21,7 @@
 #include "qgslinestring.h"
 #include "qgspoint.h"
 #include "qgsmultipoint.h"
+#include "qgsgeos.h"
 
 bool QgsCurve::operator==( const QgsAbstractGeometry &other ) const
 {
@@ -188,6 +189,25 @@ QgsRectangle QgsCurve::boundingBox() const
   return mBoundingBox;
 }
 
+bool QgsCurve::isValid( QString &error, int flags ) const
+{
+  if ( flags == 0 && mHasCachedValidity )
+  {
+    // use cached validity results
+    error = mValidityFailureReason;
+    return error.isEmpty();
+  }
+
+  QgsGeos geos( this );
+  bool res = geos.isValid( &error, flags & QgsGeometry::FlagAllowSelfTouchingHoles, nullptr );
+  if ( flags == 0 )
+  {
+    mValidityFailureReason = !res ? error : QString();
+    mHasCachedValidity = true;
+  }
+  return res;
+}
+
 QPolygonF QgsCurve::asQPolygonF() const
 {
   const int nb = numPoints();
@@ -224,6 +244,8 @@ QgsCurve::Orientation QgsCurve::orientation() const
 void QgsCurve::clearCache() const
 {
   mBoundingBox = QgsRectangle();
+  mHasCachedValidity = false;
+  mValidityFailureReason.clear();
   QgsAbstractGeometry::clearCache();
 }
 

diff --git a/src/core/geometry/qgscurve.h b/src/core/geometry/qgscurve.h
@@ -162,6 +162,7 @@ class CORE_EXPORT QgsCurve: public QgsAbstractGeometry
     QgsCurve *toCurveType() const override SIP_FACTORY;
 
     QgsRectangle boundingBox() const override;
+    bool isValid( QString &error SIP_OUT, int flags = 0 ) const override;
 
     /**
      * Returns the x-coordinate of the specified node in the line string.
@@ -290,6 +291,9 @@ class CORE_EXPORT QgsCurve: public QgsAbstractGeometry
   private:
 
     mutable QgsRectangle mBoundingBox;
+
+    mutable bool mHasCachedValidity = false;
+    mutable QString mValidityFailureReason;
 };
 
 #endif // QGSCURVE_H
diff --git a/src/core/geometry/qgsgeometry.cpp b/src/core/geometry/qgsgeometry.cpp
@@ -2504,15 +2504,7 @@ bool QgsGeometry::isGeosValid( const QgsGeometry::ValidityFlags flags ) const
     return false;
   }
 
-  // avoid calling geos for trivial point geometries
-  if ( QgsWkbTypes::geometryType( d->geometry->wkbType() ) == QgsWkbTypes::PointGeometry )
-  {
-    return true;
-  }
-
-  QgsGeos geos( d->geometry.get() );
-  mLastError.clear();
-  return geos.isValid( &mLastError, flags & FlagAllowSelfTouchingHoles, nullptr );
+  return d->geometry->isValid( mLastError, static_cast< int >( flags ) );
 }
 
 bool QgsGeometry::isSimple() const

diff --git a/src/core/geometry/qgsgeometrycollection.cpp b/src/core/geometry/qgsgeometrycollection.cpp
@@ -26,6 +26,7 @@ email                : marco.hugentobler at sourcepole dot com
 #include "qgspolygon.h"
 #include "qgsmultipolygon.h"
 #include "qgswkbptr.h"
+#include "qgsgeos.h"
 #include <memory>
 
 QgsGeometryCollection::QgsGeometryCollection()
@@ -458,6 +459,8 @@ QgsRectangle QgsGeometryCollection::calculateBoundingBox() const
 void QgsGeometryCollection::clearCache() const
 {
   mBoundingBox = QgsRectangle();
+  mHasCachedValidity = false;
+  mValidityFailureReason.clear();
   QgsAbstractGeometry::clearCache();
 }
 
@@ -772,6 +775,25 @@ QgsPoint QgsGeometryCollection::vertexAt( QgsVertexId id ) const
   return mGeometries[id.part]->vertexAt( id );
 }
 
+bool QgsGeometryCollection::isValid( QString &error, int flags ) const
+{
+  if ( flags == 0 && mHasCachedValidity )
+  {
+    // use cached validity results
+    error = mValidityFailureReason;
+    return error.isEmpty();
+  }
+
+  QgsGeos geos( this );
+  bool res = geos.isValid( &error, flags & QgsGeometry::FlagAllowSelfTouchingHoles, nullptr );
+  if ( flags == 0 )
+  {
+    mValidityFailureReason = !res ? error : QString();
+    mHasCachedValidity = true;
+  }
+  return res;
+}
+
 bool QgsGeometryCollection::addZValue( double zValue )
 {
   if ( QgsWkbTypes::hasZ( mWkbType ) )

diff --git a/src/core/geometry/qgsgeometrycollection.h b/src/core/geometry/qgsgeometrycollection.h
@@ -206,6 +206,7 @@ class CORE_EXPORT QgsGeometryCollection: public QgsAbstractGeometry
     int ringCount( int part = 0 ) const override;
     int partCount() const override;
     QgsPoint vertexAt( QgsVertexId id ) const override;
+    bool isValid( QString &error SIP_OUT, int flags = 0 ) const override;
 
     bool addZValue( double zValue = 0 ) override;
     bool addMValue( double mValue = 0 ) override;
@@ -321,6 +322,8 @@ class CORE_EXPORT QgsGeometryCollection: public QgsAbstractGeometry
   private:
 
     mutable QgsRectangle mBoundingBox;
+    mutable bool mHasCachedValidity = false;
+    mutable QString mValidityFailureReason;
 };
 
 // clazy:excludeall=qstring-allocations

diff --git a/src/core/geometry/qgsmultipoint.cpp b/src/core/geometry/qgsmultipoint.cpp
@@ -182,6 +182,11 @@ double QgsMultiPoint::segmentLength( QgsVertexId ) const
   return 0.0;
 }
 
+bool QgsMultiPoint::isValid( QString &, int ) const
+{
+  return true;
+}
+
 void QgsMultiPoint::filterVertices( const std::function<bool ( const QgsPoint & )> &filter )
 {
   mGeometries.erase( std::remove_if( mGeometries.begin(), mGeometries.end(), // clazy:exclude=detaching-member

diff --git a/src/core/geometry/qgsmultipoint.h b/src/core/geometry/qgsmultipoint.h
@@ -45,6 +45,7 @@ class CORE_EXPORT QgsMultiPoint: public QgsGeometryCollection
     QgsAbstractGeometry *boundary() const override SIP_FACTORY;
     int vertexNumberFromVertexId( QgsVertexId id ) const override;
     double segmentLength( QgsVertexId startVertex ) const override;
+    bool isValid( QString &error SIP_OUT, int flags = 0 ) const override;
 
 #ifndef SIP_RUN
     void filterVertices( const std::function< bool( const QgsPoint & ) > &filter ) override;

diff --git a/src/core/geometry/qgspoint.cpp b/src/core/geometry/qgspoint.cpp
@@ -349,6 +349,11 @@ QgsAbstractGeometry *QgsPoint::boundary() const
   return nullptr;
 }
 
+bool QgsPoint::isValid( QString &, int ) const
+{
+  return true;
+}
+
 bool QgsPoint::insertVertex( QgsVertexId position, const QgsPoint &vertex )
 {
   Q_UNUSED( position );

diff --git a/src/core/geometry/qgspoint.h b/src/core/geometry/qgspoint.h
@@ -444,6 +444,7 @@ class CORE_EXPORT QgsPoint: public QgsAbstractGeometry
     int nCoordinates() const override;
     int vertexNumberFromVertexId( QgsVertexId id ) const override;
     QgsAbstractGeometry *boundary() const override SIP_FACTORY;
+    bool isValid( QString &error SIP_OUT, int flags = 0 ) const override;
 
     //low-level editing
     bool insertVertex( QgsVertexId position, const QgsPoint &vertex ) override;

diff --git a/src/core/geometry/qgssurface.cpp b/src/core/geometry/qgssurface.cpp
@@ -18,5 +18,32 @@
 #include "qgssurface.h"
 #include "qgspoint.h"
 #include "qgspolygon.h"
-
+#include "qgsgeos.h"
 #include <memory>
+
+bool QgsSurface::isValid( QString &error, int flags ) const
+{
+  if ( flags == 0 && mHasCachedValidity )
+  {
+    // use cached validity results
+    error = mValidityFailureReason;
+    return error.isEmpty();
+  }
+
+  QgsGeos geos( this );
+  bool res = geos.isValid( &error, flags & QgsGeometry::FlagAllowSelfTouchingHoles, nullptr );
+  if ( flags == 0 )
+  {
+    mValidityFailureReason = !res ? error : QString();
+    mHasCachedValidity = true;
+  }
+  return res;
+}
+
+void QgsSurface::clearCache() const
+{
+  mBoundingBox = QgsRectangle();
+  mHasCachedValidity = false;
+  mValidityFailureReason.clear();
+  QgsAbstractGeometry::clearCache();
+}
diff --git a/src/core/geometry/qgssurface.h b/src/core/geometry/qgssurface.h
@@ -39,9 +39,6 @@ class CORE_EXPORT QgsSurface: public QgsAbstractGeometry
      */
     virtual QgsPolygon *surfaceToPolygon() const = 0 SIP_FACTORY;
 
-    /**
-     * Returns the minimal bounding box for the geometry
-     */
     QgsRectangle boundingBox() const override
     {
       if ( mBoundingBox.isNull() )
@@ -51,6 +48,9 @@ class CORE_EXPORT QgsSurface: public QgsAbstractGeometry
       return mBoundingBox;
     }
 
+    bool isValid( QString &error SIP_OUT, int flags = 0 ) const override;
+
+
 #ifndef SIP_RUN
 
     /**
@@ -75,9 +75,11 @@ class CORE_EXPORT QgsSurface: public QgsAbstractGeometry
 #endif
   protected:
 
-    void clearCache() const override { mBoundingBox = QgsRectangle(); QgsAbstractGeometry::clearCache(); }
+    void clearCache() const override;
 
     mutable QgsRectangle mBoundingBox;
+    mutable bool mHasCachedValidity = false;
+    mutable QString mValidityFailureReason;
 };
 
 #endif // QGSSURFACE_H
diff --git a/tests/src/python/test_qgsgeometry.py b/tests/src/python/test_qgsgeometry.py
@@ -5052,15 +5052,18 @@ def testIsGeosValid(self):
             ['Polygon((0 3, 3 0, 3 3, 0 0, 0 3))', False, False, 'Self-intersection'],
         ]
         for t in tests:
+            # run each check 2 times to allow for testing of cached value
             g1 = QgsGeometry.fromWkt(t[0])
-            res = g1.isGeosValid()
-            self.assertEqual(res, t[1],
-                             "mismatch for {}, expected:\n{}\nGot:\n{}\n".format(t[0], t[1], res))
-            if not res:
-                self.assertEqual(g1.lastError(), t[3], t[0])
-            res = g1.isGeosValid(QgsGeometry.FlagAllowSelfTouchingHoles)
-            self.assertEqual(res, t[2],
-                             "mismatch for {}, expected:\n{}\nGot:\n{}\n".format(t[0], t[2], res))
+            for i in range(2):
+                res = g1.isGeosValid()
+                self.assertEqual(res, t[1],
+                                 "mismatch for {}, expected:\n{}\nGot:\n{}\n".format(t[0], t[1], res))
+                if not res:
+                    self.assertEqual(g1.lastError(), t[3], t[0])
+            for i in range(2):
+                res = g1.isGeosValid(QgsGeometry.FlagAllowSelfTouchingHoles)
+                self.assertEqual(res, t[2],
+                                 "mismatch for {}, expected:\n{}\nGot:\n{}\n".format(t[0], t[2], res))
 
     def testValidateGeometry(self):
         tests = [