Only report topology errors in affected features

m-kuhn · m-kuhn · commit 29a62736cef7 · 2019-03-18T17:37:24.000+01:00
The geometry validation only works on the current edit session (added / edited geometries). To detect topology
errors it is required to also get more features within the context, therefore, the bounding box of the edited
geometries is taken to populate the list of features to check.

This commit filters the found problems so only the ones which actually affect one of the edited geometries
will be reported.
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,15 @@ Will be used to update existing errors whenever they are re-checked.
 %End
 
 
+    virtual QMap<QString, QgsFeatureIds> involvedFeatures() const;
+%Docstring
+Return 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.
+
+.. versionadded:: 3.8
+%End
+
   protected:
     QgsGeometryCheckError( const QgsGeometryCheck *check,
                            const QString &layerId,
diff --git a/src/analysis/vector/geometry_checker/qgsgeometrycheckerror.cpp b/src/analysis/vector/geometry_checker/qgsgeometrycheckerror.cpp
@@ -180,6 +180,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;
 
+    /**
+     * Return 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;
+
   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/qgsgeometrygapcheck.cpp b/src/analysis/vector/geometry_checker/qgsgeometrygapcheck.cpp
@@ -287,3 +287,40 @@ QgsGeometryCheck::CheckType QgsGeometryGapCheck::factoryCheckType()
 {
   return QgsGeometryCheck::LayerCheck;
 }
+///@endcond private
+
+bool QgsGeometryGapCheckError::isEqual( QgsGeometryCheckError *other ) const
+{
+  QgsGeometryGapCheckError *err = dynamic_cast<QgsGeometryGapCheckError *>( other );
+  return err && QgsGeometryCheckerUtils::pointsFuzzyEqual( err->location(), location(), mCheck->context()->reducedTolerance ) && err->neighbors() == neighbors();
+}
+
+bool QgsGeometryGapCheckError::closeMatch( QgsGeometryCheckError *other ) const
+{
+  QgsGeometryGapCheckError *err = dynamic_cast<QgsGeometryGapCheckError *>( other );
+  return err && err->layerId() == layerId() && err->neighbors() == neighbors();
+}
+
+void QgsGeometryGapCheckError::update( const QgsGeometryCheckError *other )
+{
+  QgsGeometryCheckError::update( other );
+  // Static cast since this should only get called if isEqual == true
+  const QgsGeometryGapCheckError *err = static_cast<const QgsGeometryGapCheckError *>( other );
+  mNeighbors = err->mNeighbors;
+  mGapAreaBBox = err->mGapAreaBBox;
+}
+
+bool QgsGeometryGapCheckError::handleChanges( const QgsGeometryCheck::Changes & )
+{
+  return true;
+}
+
+QgsRectangle QgsGeometryGapCheckError::affectedAreaBBox() const
+{
+  return mGapAreaBBox;
+}
+
+QMap<QString, QgsFeatureIds> QgsGeometryGapCheckError::involvedFeatures() const
+{
+  return mNeighbors;
+}
diff --git a/src/analysis/vector/geometry_checker/qgsgeometrygapcheck.h b/src/analysis/vector/geometry_checker/qgsgeometrygapcheck.h
@@ -38,36 +38,17 @@ class ANALYSIS_EXPORT QgsGeometryGapCheckError : public QgsGeometryCheckError
     }
     const QMap<QString, QgsFeatureIds> &neighbors() const { return mNeighbors; }
 
-    bool isEqual( QgsGeometryCheckError *other ) const override
-    {
-      QgsGeometryGapCheckError *err = dynamic_cast<QgsGeometryGapCheckError *>( other );
-      return err && QgsGeometryCheckerUtils::pointsFuzzyEqual( err->location(), location(), mCheck->context()->reducedTolerance ) && err->neighbors() == neighbors();
-    }
+    bool isEqual( QgsGeometryCheckError *other ) const override;
 
-    bool closeMatch( QgsGeometryCheckError *other ) const override
-    {
-      QgsGeometryGapCheckError *err = dynamic_cast<QgsGeometryGapCheckError *>( other );
-      return err && err->layerId() == layerId() && err->neighbors() == neighbors();
-    }
+    bool closeMatch( QgsGeometryCheckError *other ) const override;
 
-    void update( const QgsGeometryCheckError *other ) override
-    {
-      QgsGeometryCheckError::update( other );
-      // Static cast since this should only get called if isEqual == true
-      const QgsGeometryGapCheckError *err = static_cast<const QgsGeometryGapCheckError *>( other );
-      mNeighbors = err->mNeighbors;
-      mGapAreaBBox = err->mGapAreaBBox;
-    }
+    void update( const QgsGeometryCheckError *other ) override;
 
-    bool handleChanges( const QgsGeometryCheck::Changes & /*changes*/ ) override
-    {
-      return true;
-    }
+    bool handleChanges( const QgsGeometryCheck::Changes & /*changes*/ ) override;
 
-    QgsRectangle affectedAreaBBox() const override
-    {
-      return mGapAreaBBox;
-    }
+    QgsRectangle affectedAreaBBox() const override;
+
+    QMap<QString, QgsFeatureIds > involvedFeatures() const override;
 
   private:
     QMap<QString, QgsFeatureIds> mNeighbors;
diff --git a/src/analysis/vector/geometry_checker/qgsgeometrymissingvertexcheck.cpp b/src/analysis/vector/geometry_checker/qgsgeometrymissingvertexcheck.cpp
@@ -180,6 +180,10 @@ void QgsGeometryMissingVertexCheck::processPolygon( const QgsCurvePolygon *polyg
             {
               std::unique_ptr<QgsGeometryMissingVertexCheckError> error = qgis::make_unique<QgsGeometryMissingVertexCheckError>( this, layerFeature, QgsPointXY( pt ) );
               error->setAffectedAreaBBox( contextBoundingBox( polygon, vertexId, pt ) );
+              QMap<QString, QgsFeatureIds> involvedFeatures;
+              involvedFeatures[layerFeature.layerId()].insert( layerFeature.feature().id() );
+              involvedFeatures[featurePool->layerId()].insert( fid );
+              error->setInvolvedFeatures( involvedFeatures );
 
               errors.append( error.release() );
             }
@@ -272,3 +276,13 @@ void QgsGeometryMissingVertexCheckError::setAffectedAreaBBox( const QgsRectangle
 {
   mAffectedAreaBBox = affectedAreaBBox;
 }
+
+QMap<QString, QgsFeatureIds> QgsGeometryMissingVertexCheckError::involvedFeatures() const
+{
+  return mInvolvedFeatures;
+}
+
+void QgsGeometryMissingVertexCheckError::setInvolvedFeatures( const QMap<QString, QgsFeatureIds> &involvedFeatures )
+{
+  mInvolvedFeatures = involvedFeatures;
+}
diff --git a/src/analysis/vector/geometry_checker/qgsgeometrymissingvertexcheck.h b/src/analysis/vector/geometry_checker/qgsgeometrymissingvertexcheck.h
@@ -56,8 +56,19 @@ class ANALYSIS_EXPORT QgsGeometryMissingVertexCheckError : public QgsGeometryChe
      */
     void setAffectedAreaBBox( const QgsRectangle &affectedAreaBBox );
 
+    QMap<QString, QgsFeatureIds> involvedFeatures() const override;
+
+    /**
+     * The two involved features, that share a common boundary but not all common
+     * vertices on this boundary.
+     *
+     * \since QGIS 3.8
+     */
+    void setInvolvedFeatures( const QMap<QString, QgsFeatureIds> &involvedFeatures );
+
   private:
     QgsRectangle mAffectedAreaBBox;
+    QMap<QString, QgsFeatureIds> mInvolvedFeatures;
 };
 
 /**
diff --git a/src/analysis/vector/geometry_checker/qgsgeometryoverlapcheck.cpp b/src/analysis/vector/geometry_checker/qgsgeometryoverlapcheck.cpp
@@ -60,6 +60,7 @@ void QgsGeometryOverlapCheck::collectErrors( const QMap<QString, QgsFeaturePool
       {
         continue;
       }
+
       QString errMsg;
       if ( geomEngineA->overlaps( layerFeatureB.geometry().constGet(), &errMsg ) )
       {
@@ -254,6 +255,36 @@ QgsGeometryOverlapCheckError::QgsGeometryOverlapCheckError( const QgsGeometryChe
 
 }
 
+bool QgsGeometryOverlapCheckError::isEqual( QgsGeometryCheckError *other ) const
+{
+  QgsGeometryOverlapCheckError *err = dynamic_cast<QgsGeometryOverlapCheckError *>( other );
+  return err &&
+         other->layerId() == layerId() &&
+         other->featureId() == featureId() &&
+         err->overlappedFeature() == overlappedFeature() &&
+         QgsGeometryCheckerUtils::pointsFuzzyEqual( location(), other->location(), mCheck->context()->reducedTolerance ) &&
+         std::fabs( value().toDouble() - other->value().toDouble() ) < mCheck->context()->reducedTolerance;
+}
+
+bool QgsGeometryOverlapCheckError::closeMatch( QgsGeometryCheckError *other ) const
+{
+  QgsGeometryOverlapCheckError *err = dynamic_cast<QgsGeometryOverlapCheckError *>( other );
+  return err && other->layerId() == layerId() && other->featureId() == featureId() && err->overlappedFeature() == overlappedFeature();
+}
+
+bool QgsGeometryOverlapCheckError::handleChanges( const QgsGeometryCheck::Changes &changes )
+{
+  if ( !QgsGeometryCheckError::handleChanges( changes ) )
+  {
+    return false;
+  }
+  if ( changes.value( mOverlappedFeature.layerId() ).keys().contains( mOverlappedFeature.featureId() ) )
+  {
+    return false;
+  }
+  return true;
+}
+
 QString QgsGeometryOverlapCheckError::description() const
 {
   return QCoreApplication::translate( "QgsGeometryTypeCheckError", "Overlap with %1 at feature %2" ).arg( mOverlappedFeature.layerName(), QString::number( mOverlappedFeature.featureId() ) );
@@ -263,3 +294,11 @@ QgsGeometryCheck::CheckType QgsGeometryOverlapCheck::factoryCheckType()
 {
   return QgsGeometryCheck::LayerCheck;
 }
+
+QMap<QString, QgsFeatureIds> QgsGeometryOverlapCheckError::involvedFeatures() const
+{
+  QMap<QString, QgsFeatureIds> features;
+  features[layerId()].insert( featureId() );
+  features[mOverlappedFeature.layerId()].insert( mOverlappedFeature.featureId() );
+  return features;
+}
diff --git a/src/analysis/vector/geometry_checker/qgsgeometryoverlapcheck.h b/src/analysis/vector/geometry_checker/qgsgeometryoverlapcheck.h
@@ -54,38 +54,16 @@ class ANALYSIS_EXPORT QgsGeometryOverlapCheckError : public QgsGeometryCheckErro
                                   const QgsGeometryCheckerUtils::LayerFeature &overlappedFeature );
     const OverlappedFeature &overlappedFeature() const { return mOverlappedFeature; }
 
-    bool isEqual( QgsGeometryCheckError *other ) const override
-    {
-      QgsGeometryOverlapCheckError *err = dynamic_cast<QgsGeometryOverlapCheckError *>( other );
-      return err &&
-             other->layerId() == layerId() &&
-             other->featureId() == featureId() &&
-             err->overlappedFeature() == overlappedFeature() &&
-             QgsGeometryCheckerUtils::pointsFuzzyEqual( location(), other->location(), mCheck->context()->reducedTolerance ) &&
-             std::fabs( value().toDouble() - other->value().toDouble() ) < mCheck->context()->reducedTolerance;
-    }
-
-    bool closeMatch( QgsGeometryCheckError *other ) const override
-    {
-      QgsGeometryOverlapCheckError *err = dynamic_cast<QgsGeometryOverlapCheckError *>( other );
-      return err && other->layerId() == layerId() && other->featureId() == featureId() && err->overlappedFeature() == overlappedFeature();
-    }
+    bool isEqual( QgsGeometryCheckError *other ) const override;
 
-    bool handleChanges( const QgsGeometryCheck::Changes &changes ) override
-    {
-      if ( !QgsGeometryCheckError::handleChanges( changes ) )
-      {
-        return false;
-      }
-      if ( changes.value( mOverlappedFeature.layerId() ).keys().contains( mOverlappedFeature.featureId() ) )
-      {
-        return false;
-      }
-      return true;
-    }
+    bool closeMatch( QgsGeometryCheckError *other ) const override;
+
+    bool handleChanges( const QgsGeometryCheck::Changes &changes ) override;
 
     QString description() const override;
 
+    QMap<QString, QgsFeatureIds > involvedFeatures() const override;
+
   private:
     OverlappedFeature mOverlappedFeature;
 };
diff --git a/src/app/qgsgeometryvalidationdock.cpp b/src/app/qgsgeometryvalidationdock.cpp
@@ -243,17 +243,6 @@ void QgsGeometryValidationDock::onCurrentErrorChanged( const QModelIndex &curren
 
   bool hasFeature = !FID_IS_NULL( current.data( QgsGeometryValidationModel::ErrorFeatureIdRole ) );
   mZoomToFeatureButton->setEnabled( hasFeature );
-
-  switch ( mLastZoomToAction )
-  {
-    case  ZoomToProblem:
-      zoomToProblem();
-      break;
-
-    case ZoomToFeature:
-      zoomToFeature();
-      break;
-  }
 }
 
 void QgsGeometryValidationDock::onCurrentLayerChanged( QgsMapLayer *layer )
diff --git a/src/app/qgsgeometryvalidationservice.cpp b/src/app/qgsgeometryvalidationservice.cpp
@@ -393,11 +393,12 @@ void QgsGeometryValidationService::triggerTopologyChecks( QgsVectorLayer *layer
     affectedFeatureIds.unite( layer->editBuffer()->addedFeatures().keys().toSet() );
   }
 
-  QgsFeaturePool *featurePool = mFeaturePools.value( layer->id() );
+  const QString layerId = layer->id();
+  QgsFeaturePool *featurePool = mFeaturePools.value( layerId );
   if ( !featurePool )
   {
     featurePool = new QgsVectorLayerFeaturePool( layer );
-    mFeaturePools.insert( layer->id(), featurePool );
+    mFeaturePools.insert( layerId, featurePool );
   }
 
   QList<std::shared_ptr<QgsGeometryCheckError>> &allErrors = mLayerChecks[layer].topologyCheckErrors;
@@ -416,7 +417,7 @@ void QgsGeometryValidationService::triggerTopologyChecks( QgsVectorLayer *layer
   QgsFeatureRequest areaRequest = QgsFeatureRequest().setFilterRect( area );
   QgsFeatureIds checkFeatureIds = featurePool->getFeatures( areaRequest );
 
-  layerIds.insert( layer->id(), checkFeatureIds );
+  layerIds.insert( layerId, checkFeatureIds );
   QgsGeometryCheck::LayerFeatureIds layerFeatureIds( layerIds );
 
   const QList<QgsGeometryCheck *> checks = mLayerChecks[layer].topologyChecks;
@@ -427,7 +428,7 @@ void QgsGeometryValidationService::triggerTopologyChecks( QgsVectorLayer *layer
 
   mLayerChecks[layer].topologyCheckFeedbacks = feedbacks.values();
 
-  QFuture<void> future = QtConcurrent::map( checks, [&allErrors, layerFeatureIds, layer, feedbacks, this]( const QgsGeometryCheck * check )
+  QFuture<void> future = QtConcurrent::map( checks, [&allErrors, layerFeatureIds, layer, layerId, feedbacks, affectedFeatureIds, this]( const QgsGeometryCheck * check )
   {
     // Watch out with the layer pointer in here. We are running in a thread, so we do not want to actually use it
     // except for using its address to report the error.
@@ -439,9 +440,30 @@ void QgsGeometryValidationService::triggerTopologyChecks( QgsVectorLayer *layer
     QgsReadWriteLocker errorLocker( mTopologyCheckLock, QgsReadWriteLocker::Write );
 
     QList<std::shared_ptr<QgsGeometryCheckError> > sharedErrors;
-    for ( QgsGeometryCheckError *error : errors )
+    for ( QgsGeometryCheckError *err : errors )
     {
-      sharedErrors.append( std::shared_ptr<QgsGeometryCheckError>( error ) );
+      std::shared_ptr<QgsGeometryCheckError> error( err );
+      // Check if the error happened in one of the edited / checked features
+      // Errors which are happen to be in the same area "by chance" are ignored.
+      const auto involvedFeatures = error->involvedFeatures();
+
+      bool errorAffectsEditedFeature = true;
+      if ( !involvedFeatures.isEmpty() )
+      {
+        errorAffectsEditedFeature = false;
+        const auto involvedFids = involvedFeatures.value( layerId );
+        for ( const QgsFeatureId id : involvedFids )
+        {
+          if ( affectedFeatureIds.contains( id ) )
+          {
+            errorAffectsEditedFeature = true;
+            break;
+          }
+        }
+      }
+
+      if ( errorAffectsEditedFeature )
+        sharedErrors.append( error );
     }
 
     allErrors.append( sharedErrors );

Original file line number	Diff line number	Diff line change
`@@ -180,6 +180,11 @@ bool QgsGeometryCheckError::handleChanges( const QgsGeometryCheck::Changes &chan`
`180`	`180`	`return true;`
`181`	`181`	`}`
`182`	`182`
	`183`	`+QMap<QString, QgsFeatureIds> QgsGeometryCheckError::involvedFeatures() const`
	`184`	`+{`
	`185`	`+ return QMap<QString, QSet<QgsFeatureId> >();`
	`186`	`+}`
	`187`	`+`
`183`	`188`	`void QgsGeometryCheckError::update( const QgsGeometryCheckError *other )`
`184`	`189`	`{`
`185`	`190`	`Q_ASSERT( mCheck == other->mCheck );`
Original file line number	Diff line number	Diff line change
`@@ -180,6 +180,10 @@ void QgsGeometryMissingVertexCheck::processPolygon( const QgsCurvePolygon *polyg`
`180`	`180`	`{`
`181`	`181`	`std::unique_ptr<QgsGeometryMissingVertexCheckError> error = qgis::make_unique<QgsGeometryMissingVertexCheckError>( this, layerFeature, QgsPointXY( pt ) );`
`182`	`182`	`error->setAffectedAreaBBox( contextBoundingBox( polygon, vertexId, pt ) );`
	`183`	`+ QMap<QString, QgsFeatureIds> involvedFeatures;`
	`184`	`+ involvedFeatures[layerFeature.layerId()].insert( layerFeature.feature().id() );`
	`185`	`+ involvedFeatures[featurePool->layerId()].insert( fid );`
	`186`	`+ error->setInvolvedFeatures( involvedFeatures );`
`183`	`187`
`184`	`188`	`errors.append( error.release() );`
`185`	`189`	`}`
`@@ -272,3 +276,13 @@ void QgsGeometryMissingVertexCheckError::setAffectedAreaBBox( const QgsRectangle`
`272`	`276`	`{`
`273`	`277`	`mAffectedAreaBBox = affectedAreaBBox;`
`274`	`278`	`}`
	`279`	`+`
	`280`	`+QMap<QString, QgsFeatureIds> QgsGeometryMissingVertexCheckError::involvedFeatures() const`
	`281`	`+{`
	`282`	`+ return mInvolvedFeatures;`
	`283`	`+}`
	`284`	`+`
	`285`	`+void QgsGeometryMissingVertexCheckError::setInvolvedFeatures( const QMap<QString, QgsFeatureIds> &involvedFeatures )`
	`286`	`+{`
	`287`	`+ mInvolvedFeatures = involvedFeatures;`
	`288`	`+}`