Merge pull request #9299 from m-kuhn/geometry-validation-only-report-affected-features

m-kuhn · web-flow · commit 22b052da39a4 · 2019-03-02T09:48:37.000+01:00
[geometry validation] only report affected features
diff --git a/python/analysis/auto_generated/vector/geometry_checker/qgsgeometrycheckerror.sip.in b/python/analysis/auto_generated/vector/geometry_checker/qgsgeometrycheckerror.sip.in
@@ -162,6 +162,7 @@ Will be used to update existing errors whenever they are re-checked.
 %End
 
 
+
   protected:
 
     QgsGeometryCheckError( const QgsGeometryCheck *check,
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
@@ -179,6 +179,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:
 
     /**
diff --git a/src/analysis/vector/geometry_checker/qgsgeometrygapcheck.cpp b/src/analysis/vector/geometry_checker/qgsgeometrygapcheck.cpp
@@ -287,3 +287,39 @@ 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
@@ -55,36 +55,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
@@ -135,7 +135,7 @@ void QgsGeometryMissingVertexCheck::processPolygon( const QgsCurvePolygon *polyg
   const int numRings = polygon->numInteriorRings();
   for ( int i = 0; i < numRings; ++i )
   {
-    geomEngine = QgsGeometryCheckerUtils::createGeomEngine( polygon->exteriorRing(), mContext->tolerance );
+    geomEngine = QgsGeometryCheckerUtils::createGeomEngine( polygon->interiorRing( i ), mContext->tolerance );
     boundaries->addGeometry( geomEngine->buffer( mContext->tolerance, 5 ) );
   }
 
@@ -177,14 +177,40 @@ void QgsGeometryMissingVertexCheck::processPolygon( const QgsCurvePolygon *polyg
               }
             }
             if ( !alreadyReported )
-              errors.append( new QgsGeometryCheckError( this, layerFeature, QgsPointXY( pt ) ) );
+            {
+              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() );
+            }
           }
         }
       }
     }
   }
 }
 
+QgsRectangle QgsGeometryMissingVertexCheck::contextBoundingBox( const QgsCurvePolygon *polygon, const QgsVertexId &vertexId, const QgsPoint &point ) const
+{
+  QgsVertexId vertexBefore;
+  QgsVertexId vertexAfter;
+
+  polygon->adjacentVertices( vertexId, vertexBefore, vertexAfter );
+
+  QgsPoint ptBefore = polygon->vertexAt( vertexBefore );
+  QgsPoint ptAt = polygon->vertexAt( vertexId );
+  QgsPoint ptAfter = polygon->vertexAt( vertexAfter );
+
+  double length = std::abs( ptAt.distance( ptBefore ) ) + std::abs( ptAt.distance( ptAfter ) );
+
+  QgsRectangle rect( point.x() - length / 2, point.y() - length / 2, point.x() + length / 2, point.y() + length / 2 );
+  return rect;
+}
+
 QString QgsGeometryMissingVertexCheck::id() const
 {
   return factoryId();
@@ -236,3 +262,28 @@ QgsGeometryCheck::CheckType QgsGeometryMissingVertexCheck::factoryCheckType()
   return QgsGeometryCheck::LayerCheck;
 }
 ///@endcond private
+
+QgsGeometryMissingVertexCheckError::QgsGeometryMissingVertexCheckError( const QgsGeometryCheck *check, const QgsGeometryCheckerUtils::LayerFeature &layerFeature, const QgsPointXY &errorLocation, QgsVertexId vidx, const QVariant &value, QgsGeometryCheckError::ValueType valueType )
+  : QgsGeometryCheckError( check, layerFeature, errorLocation, vidx, value, valueType )
+{
+}
+
+QgsRectangle QgsGeometryMissingVertexCheckError::affectedAreaBBox() const
+{
+  return mAffectedAreaBBox;
+}
+
+void QgsGeometryMissingVertexCheckError::setAffectedAreaBBox( const QgsRectangle &affectedAreaBBox )
+{
+  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
@@ -23,6 +23,54 @@
 
 class QgsCurvePolygon;
 
+/**
+ * \ingroup analysis
+ *
+ * A geometry check error for a missing vertex.
+ * Includes additional details about the bounding box of the error,
+ * centered on the missing error location and scaled by taking neighbouring
+ * vertices into account.
+ *
+ * \since QGIS 3.8
+ */
+class ANALYSIS_EXPORT QgsGeometryMissingVertexCheckError : public QgsGeometryCheckError
+{
+  public:
+
+    /**
+     * Create a new missing vertex check error.
+     */
+    QgsGeometryMissingVertexCheckError( const QgsGeometryCheck *check,
+                                        const QgsGeometryCheckerUtils::LayerFeature &layerFeature,
+                                        const QgsPointXY &errorLocation,
+                                        QgsVertexId vidx = QgsVertexId(),
+                                        const QVariant &value = QVariant(),
+                                        ValueType valueType = ValueOther );
+
+    QgsRectangle affectedAreaBBox() const override;
+
+    /**
+     * Set the bounding box of the affected area.
+     *
+     * \since QGIS 3.8
+     */
+    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;
+};
+
 /**
  * \ingroup analysis
  *
@@ -73,6 +121,8 @@ class ANALYSIS_EXPORT QgsGeometryMissingVertexCheck : public QgsGeometryCheck
 
   private:
     void processPolygon( const QgsCurvePolygon *polygon, QgsFeaturePool *featurePool, QList<QgsGeometryCheckError *> &errors, const QgsGeometryCheckerUtils::LayerFeature &layerFeature, QgsFeedback *feedback ) const;
+
+    QgsRectangle contextBoundingBox( const QgsCurvePolygon *polygon, const QgsVertexId &vertexId, const QgsPoint &point ) const;
 };
 
 
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 ) )
       {
@@ -261,7 +262,45 @@ 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() ) );
 }
+
+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
@@ -69,38 +69,16 @@ class ANALYSIS_EXPORT QgsGeometryOverlapCheckError : public QgsGeometryCheckErro
      */
     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
@@ -244,17 +244,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

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 change
     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;
   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;
   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.
     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 );