Merge pull request #9551 from m-kuhn/geom-validator-fixes

[geometry validation] Various bugfixes and improvements
qgis · Mar 19, 2019 · 9383b00 · 9383b00
2 parents d1910f5 + 6bcba42
commit 9383b00
Show file tree

Hide file tree

Showing 28 changed files with 549 additions and 304 deletions.
diff --git a/python/analysis/auto_generated/vector/geometry_checker/qgsfeaturepool.sip.in b/python/analysis/auto_generated/vector/geometry_checker/qgsfeaturepool.sip.in
@@ -10,7 +10,6 @@
 
 
 
-
 class QgsFeaturePool : QgsFeatureSink /Abstract/
 {
 %Docstring
@@ -30,12 +29,11 @@ A feature pool is based on a vector layer and caches features.
     QgsFeaturePool( QgsVectorLayer *layer );
     virtual ~QgsFeaturePool();
 
-    bool getFeature( QgsFeatureId id, QgsFeature &feature, QgsFeedback *feedback = 0 );
+    bool getFeature( QgsFeatureId id, QgsFeature &feature );
 %Docstring
-Retrieve the feature with the specified ``id`` into ``feature``.
-It will be retrieved from the cache or from the underlying layer if unavailable.
-If the feature is neither available from the cache nor from the layer it will return false.
-If ``feedback`` is specified, the call may return if the feedback is canceled.
+Retrieves the feature with the specified ``id`` into ``feature``.
+It will be retrieved from the cache or from the underlying feature source if unavailable.
+If the feature is neither available from the cache nor from the source it will return FALSE.
 %End
 
 
@@ -74,11 +72,19 @@ The geometry type of this layer.
     QgsCoordinateReferenceSystem crs() const;
 %Docstring
 The coordinate reference system of this layer.
+%End
+
+    QString layerName() const;
+%Docstring
+Returns the name of the layer.
+
+Should be preferred over layer().name() because it can directly be run on
+the background thread.
 %End
 
   protected:
 
-    void insertFeature( const QgsFeature &feature );
+    void insertFeature( const QgsFeature &feature, bool skipLock = false );
 %Docstring
 Inserts a feature into the cache and the spatial index.
 To be used by implementations of ``addFeature``.

diff --git a/python/analysis/auto_generated/vector/geometry_checker/qgsgeometrycheckerror.sip.in b/python/analysis/auto_generated/vector/geometry_checker/qgsgeometrycheckerror.sip.in
@@ -76,6 +76,7 @@ Will be used to update existing errors whenever they are re-checked.
 %End
 
 
+
   protected:
     QgsGeometryCheckError( const QgsGeometryCheck *check,
                            const QString &layerId,

diff --git a/python/analysis/auto_generated/vector/geometry_checker/qgsgeometrycheckerutils.sip.in b/python/analysis/auto_generated/vector/geometry_checker/qgsgeometrycheckerutils.sip.in
@@ -44,7 +44,7 @@ If ``useMapCrs`` is True, geometries will be reprojected to the mapCrs defined
 in ``context``.
 %End
 
-        const QgsFeature &feature() const;
+        QgsFeature feature() const;
 %Docstring
 Returns the feature.
 The geometry will not be reprojected regardless of useMapCrs.
@@ -56,13 +56,17 @@ The geometry will not be reprojected regardless of useMapCrs.
 The layer id.
 %End
 
-        const QgsGeometry &geometry() const;
+        QgsGeometry geometry() const;
 %Docstring
 Returns the geometry of this feature.
 If useMapCrs was specified, it will already be reprojected into the
 CRS specified in the context specified in the constructor.
 %End
+
         QString id() const;
+%Docstring
+Returns a combination of the layerId and the feature id.
+%End
         bool operator==( const QgsGeometryCheckerUtils::LayerFeature &other ) const;
         bool operator!=( const QgsGeometryCheckerUtils::LayerFeature &other ) const;
 

diff --git a/resources/qgis_global_settings.ini b/resources/qgis_global_settings.ini
@@ -83,3 +83,11 @@ PostgreSQL\default_timeout=30
 # cause performance issues, as the records must all be loaded from the related table on display.
 maxEntriesRelationWidget=100
 
+[geometry_validation]
+# A comma separated list of geometry validations to enable by default for newly added layers
+# Available checks: QgsIsValidCheck,QgsGeometryGapCheck,QgsGeometryOverlapCheck,QgsGeometryMissingVertexCheck
+default_checks=
+
+# Enable problem resolution for geometry errors
+# This feature is experimental and has known issues.
+enable_problem_resolution=false
diff --git a/src/analysis/vector/geometry_checker/qgsfeaturepool.cpp b/src/analysis/vector/geometry_checker/qgsfeaturepool.cpp
@@ -29,14 +29,14 @@
 QgsFeaturePool::QgsFeaturePool( QgsVectorLayer *layer )
   : mFeatureCache( CACHE_SIZE )
   , mLayer( layer )
-  , mLayerId( layer->id() )
   , mGeometryType( layer->geometryType() )
-  , mCrs( layer->crs() )
+  , mFeatureSource( qgis::make_unique<QgsVectorLayerFeatureSource>( layer ) )
+  , mLayerName( layer->name() )
 {
 
 }
 
-bool QgsFeaturePool::getFeature( QgsFeatureId id, QgsFeature &feature, QgsFeedback *feedback )
+bool QgsFeaturePool::getFeature( QgsFeatureId id, QgsFeature &feature )
 {
   // Why is there a write lock acquired here? Weird, we only want to read a feature from the cache, right?
   // A method like `QCache::object(const Key &key) const` certainly would not modify its internals.
@@ -55,11 +55,9 @@ bool QgsFeaturePool::getFeature( QgsFeatureId id, QgsFeature &feature, QgsFeedba
   }
   else
   {
-    std::unique_ptr<QgsVectorLayerFeatureSource> source = QgsVectorLayerUtils::getFeatureSource( mLayer, feedback );
-
     // Feature not in cache, retrieve from layer
     // TODO: avoid always querying all attributes (attribute values are needed when merging by attribute)
-    if ( !source || !source->getFeatures( QgsFeatureRequest( id ) ).nextFeature( feature ) )
+    if ( !mFeatureSource->getFeatures( QgsFeatureRequest( id ) ).nextFeature( feature ) )
     {
       return false;
     }
@@ -72,15 +70,22 @@ bool QgsFeaturePool::getFeature( QgsFeatureId id, QgsFeature &feature, QgsFeedba
 
 QgsFeatureIds QgsFeaturePool::getFeatures( const QgsFeatureRequest &request, QgsFeedback *feedback )
 {
+  QgsReadWriteLocker( mCacheLock, QgsReadWriteLocker::Write );
+  Q_UNUSED( feedback )
+  Q_ASSERT( QThread::currentThread() == qApp->thread() );
+
+  mFeatureCache.clear();
+  mIndex = QgsSpatialIndex();
+
   QgsFeatureIds fids;
 
-  std::unique_ptr<QgsVectorLayerFeatureSource> source = QgsVectorLayerUtils::getFeatureSource( mLayer, feedback );
+  mFeatureSource = qgis::make_unique<QgsVectorLayerFeatureSource>( mLayer );
 
-  QgsFeatureIterator it = source->getFeatures( request );
+  QgsFeatureIterator it = mFeatureSource->getFeatures( request );
   QgsFeature feature;
   while ( it.nextFeature( feature ) )
   {
-    insertFeature( feature );
+    insertFeature( feature, true );
     fids << feature.id();
   }
 
@@ -111,9 +116,11 @@ QPointer<QgsVectorLayer> QgsFeaturePool::layerPtr() const
   return mLayer;
 }
 
-void QgsFeaturePool::insertFeature( const QgsFeature &feature )
+void QgsFeaturePool::insertFeature( const QgsFeature &feature, bool skipLock )
 {
-  QgsReadWriteLocker locker( mCacheLock, QgsReadWriteLocker::Write );
+  QgsReadWriteLocker locker( mCacheLock, QgsReadWriteLocker::Unlocked );
+  if ( !skipLock )
+    locker.changeMode( QgsReadWriteLocker::Write );
   mFeatureCache.insert( feature.id(), new QgsFeature( feature ) );
   QgsFeature indexFeature( feature );
   mIndex.addFeature( indexFeature );
@@ -150,12 +157,19 @@ void QgsFeaturePool::setFeatureIds( const QgsFeatureIds &ids )
 
 bool QgsFeaturePool::isFeatureCached( QgsFeatureId fid )
 {
+  QgsReadWriteLocker locker( mCacheLock, QgsReadWriteLocker::Read );
   return mFeatureCache.contains( fid );
 }
 
+QString QgsFeaturePool::layerName() const
+{
+  return mLayerName;
+}
+
 QgsCoordinateReferenceSystem QgsFeaturePool::crs() const
 {
-  return mCrs;
+  QgsReadWriteLocker( mCacheLock, QgsReadWriteLocker::Read );
+  return mFeatureSource->crs();
 }
 
 QgsWkbTypes::GeometryType QgsFeaturePool::geometryType() const
@@ -165,5 +179,6 @@ QgsWkbTypes::GeometryType QgsFeaturePool::geometryType() const
 
 QString QgsFeaturePool::layerId() const
 {
-  return mLayerId;
+  QgsReadWriteLocker( mCacheLock, QgsReadWriteLocker::Read );
+  return mFeatureSource->id();
 }
diff --git a/src/analysis/vector/geometry_checker/qgsfeaturepool.h b/src/analysis/vector/geometry_checker/qgsfeaturepool.h
@@ -25,8 +25,7 @@
 #include "qgsfeature.h"
 #include "qgsspatialindex.h"
 #include "qgsfeaturesink.h"
-
-class QgsVectorLayer;
+#include "qgsvectorlayerfeatureiterator.h"
 
 /**
  * \ingroup analysis
@@ -43,19 +42,20 @@ class ANALYSIS_EXPORT QgsFeaturePool : public QgsFeatureSink SIP_ABSTRACT
     virtual ~QgsFeaturePool() = default;
 
     /**
-     * Retrieve the feature with the specified \a id into \a feature.
-     * It will be retrieved from the cache or from the underlying layer if unavailable.
-     * If the feature is neither available from the cache nor from the layer it will return false.
-     * If \a feedback is specified, the call may return if the feedback is canceled.
+     * Retrieves the feature with the specified \a id into \a feature.
+     * It will be retrieved from the cache or from the underlying feature source if unavailable.
+     * If the feature is neither available from the cache nor from the source it will return FALSE.
      */
-    bool getFeature( QgsFeatureId id, QgsFeature &feature, QgsFeedback *feedback = nullptr );
+    bool getFeature( QgsFeatureId id, QgsFeature &feature );
 
     /**
      * Get features for the provided \a request. No features will be fetched
      * from the cache and the request is sent directly to the underlying feature source.
      * Results of the request are cached in the pool and the ids of all the features
-     * are returned. This can be used to warm the cache for a particular area of interest
+     * are returned. This is used to warm the cache for a particular area of interest
      * (bounding box) or other set of features.
+     * This will get a new feature source from the source vector layer.
+     * This needs to be called from the main thread.
      * If \a feedback is specified, the call may return if the feedback is canceled.
      */
     QgsFeatureIds getFeatures( const QgsFeatureRequest &request, QgsFeedback *feedback = nullptr ) SIP_SKIP;
@@ -121,13 +121,21 @@ class ANALYSIS_EXPORT QgsFeaturePool : public QgsFeatureSink SIP_ABSTRACT
      */
     QgsCoordinateReferenceSystem crs() const;
 
+    /**
+     * Returns the name of the layer.
+     *
+     * Should be preferred over layer().name() because it can directly be run on
+     * the background thread.
+     */
+    QString layerName() const;
+
   protected:
 
     /**
      * Inserts a feature into the cache and the spatial index.
      * To be used by implementations of ``addFeature``.
      */
-    void insertFeature( const QgsFeature &feature );
+    void insertFeature( const QgsFeature &feature, bool skipLock = false );
 
     /**
      * Changes a feature in the cache and the spatial index.
@@ -170,9 +178,9 @@ class ANALYSIS_EXPORT QgsFeaturePool : public QgsFeatureSink SIP_ABSTRACT
     mutable QReadWriteLock mCacheLock;
     QgsFeatureIds mFeatureIds;
     QgsSpatialIndex mIndex;
-    QString mLayerId;
     QgsWkbTypes::GeometryType mGeometryType;
-    QgsCoordinateReferenceSystem mCrs;
+    std::unique_ptr<QgsVectorLayerFeatureSource> mFeatureSource;
+    QString mLayerName;
 };
 
 #endif // QGS_FEATUREPOOL_H
diff --git a/src/analysis/vector/geometry_checker/qgsgeometrycheckerror.cpp b/src/analysis/vector/geometry_checker/qgsgeometrycheckerror.cpp
@@ -53,7 +53,8 @@ QgsGeometryCheckError::QgsGeometryCheckError( const QgsGeometryCheck *check,
 {
   if ( vidx.part != -1 )
   {
-    mGeometry = QgsGeometry( QgsGeometryCheckerUtils::getGeomPart( layerFeature.geometry().constGet(), vidx.part )->clone() );
+    const QgsGeometry geom = layerFeature.geometry();
+    mGeometry = QgsGeometry( QgsGeometryCheckerUtils::getGeomPart( geom.constGet(), vidx.part )->clone() );
   }
   else
   {
@@ -180,6 +181,11 @@ bool QgsGeometryCheckError::handleChanges( const QgsGeometryCheck::Changes &chan
   return true;
 }
 
+QMap<QString, QgsFeatureIds> QgsGeometryCheckError::involvedFeatures() const
+{
+  return QMap<QString, QSet<QgsFeatureId> >();
+}
+
 void QgsGeometryCheckError::update( const QgsGeometryCheckError *other )
 {
   Q_ASSERT( mCheck == other->mCheck );

diff --git a/src/analysis/vector/geometry_checker/qgsgeometrycheckerror.h b/src/analysis/vector/geometry_checker/qgsgeometrycheckerror.h
@@ -93,6 +93,15 @@ class ANALYSIS_EXPORT QgsGeometryCheckError
      */
     virtual bool handleChanges( const QgsGeometryCheck::Changes &changes ) SIP_SKIP;
 
+    /**
+     * Returns a list of involved features.
+     * By default returns an empty map.
+     * The map keys are layer ids, the map value is a set of feature ids.
+     *
+     * \since QGIS 3.8
+     */
+    virtual QMap<QString, QgsFeatureIds > involvedFeatures() const SIP_SKIP;
+
   protected:
     // Users of this constructor must ensure geometry and errorLocation are in map coordinates
     QgsGeometryCheckError( const QgsGeometryCheck *check,

diff --git a/src/analysis/vector/geometry_checker/qgsgeometrycheckerutils.cpp b/src/analysis/vector/geometry_checker/qgsgeometrycheckerutils.cpp
@@ -52,7 +52,7 @@ QgsGeometryCheckerUtils::LayerFeature::LayerFeature( const QgsFeaturePool *pool,
   }
 }
 
-const QgsFeature &QgsGeometryCheckerUtils::LayerFeature::feature() const
+QgsFeature QgsGeometryCheckerUtils::LayerFeature::feature() const
 {
   return mFeature;
 }
@@ -67,24 +67,24 @@ QString QgsGeometryCheckerUtils::LayerFeature::layerId() const
   return mFeaturePool->layerId();
 }
 
-const QgsGeometry &QgsGeometryCheckerUtils::LayerFeature::geometry() const
+QgsGeometry QgsGeometryCheckerUtils::LayerFeature::geometry() const
 {
   return mGeometry;
 }
 
 QString QgsGeometryCheckerUtils::LayerFeature::id() const
 {
-  return QStringLiteral( "%1:%2" ).arg( layer()->name() ).arg( mFeature.id() );
+  return QStringLiteral( "%1:%2" ).arg( mFeaturePool->layerName() ).arg( mFeature.id() );
 }
 
 bool QgsGeometryCheckerUtils::LayerFeature::operator==( const LayerFeature &other ) const
 {
-  return layer()->id() == other.layer()->id() && feature().id() == other.feature().id();
+  return layerId() == other.layerId() && mFeature.id() == other.mFeature.id();
 }
 
 bool QgsGeometryCheckerUtils::LayerFeature::operator!=( const LayerFeature &other ) const
 {
-  return layer()->id() != other.layer()->id() || feature().id() != other.feature().id();
+  return layerId() != other.layerId() || mFeature.id() != other.mFeature.id();
 }
 
 /////////////////////////////////////////////////////////////////////////////
@@ -197,7 +197,7 @@ bool QgsGeometryCheckerUtils::LayerFeatures::iterator::nextFeature( bool begin )
     QgsFeature feature;
     if ( featurePool->getFeature( *mFeatureIt, feature ) && !feature.geometry().isNull() )
     {
-      mCurrentFeature.reset( new LayerFeature( mParent->mFeaturePools[*mLayerIt], feature, mParent->mContext, mParent->mUseMapCrs ) );
+      mCurrentFeature = qgis::make_unique<LayerFeature>( featurePool, feature, mParent->mContext, mParent->mUseMapCrs );
       return true;
     }
     ++mFeatureIt;

diff --git a/src/analysis/vector/geometry_checker/qgsgeometrycheckerutils.h b/src/analysis/vector/geometry_checker/qgsgeometrycheckerutils.h
@@ -56,7 +56,7 @@ class ANALYSIS_EXPORT QgsGeometryCheckerUtils
          * Returns the feature.
          * The geometry will not be reprojected regardless of useMapCrs.
          */
-        const QgsFeature &feature() const;
+        QgsFeature feature() const;
 
         /**
          * The layer.
@@ -73,7 +73,11 @@ class ANALYSIS_EXPORT QgsGeometryCheckerUtils
          * If useMapCrs was specified, it will already be reprojected into the
          * CRS specified in the context specified in the constructor.
          */
-        const QgsGeometry &geometry() const;
+        QgsGeometry geometry() const;
+
+        /**
+         * Returns a combination of the layerId and the feature id.
+         */
         QString id() const;
         bool operator==( const QgsGeometryCheckerUtils::LayerFeature &other ) const;
         bool operator!=( const QgsGeometryCheckerUtils::LayerFeature &other ) const;