Fix crash in node tool after deleting the whole geometry (fixes #15659)

Made sure that both closestVertex() and closestSegment() return negative distance on error (e.g. with null or emtpy geometry). Also fixes snapping when dealing with layers with null/invalid geometries (cherry picked from commit c093d51)
qgis · Oct 31, 2016 · 24fbe1a · 24fbe1a
1 parent dd4b34e
commit 24fbe1a
Show file tree

Hide file tree

Showing 12 changed files with 72 additions and 22 deletions.
diff --git a/python/core/geometry/qgsgeometry.sip b/python/core/geometry/qgsgeometry.sip
@@ -181,7 +181,7 @@ class QgsGeometry
      * @param afterVertex will be set to the vertex index of the next vertex after the closest one. Will be set to -1 if
      * not present.
      * @param sqrDist will be set to the square distance between the closest vertex and the specified point
-     * @returns closest point in geometry
+     * @returns closest point in geometry. If not found (empty geometry), returns null point nad sqrDist is negative.
      */
     //TODO QGIS 3.0 - rename beforeVertex to previousVertex, afterVertex to nextVertex
     QgsPoint closestVertex( const QgsPoint& point, int& atVertex /Out/, int& beforeVertex /Out/, int& afterVertex /Out/, double& sqrDist /Out/ ) const;

diff --git a/src/app/nodetool/qgsmaptoolnodetool.cpp b/src/app/nodetool/qgsmaptoolnodetool.cpp
@@ -274,12 +274,11 @@ void QgsMapToolNodeTool::canvasPressEvent( QgsMapMouseEvent* e )
 
     // get geometry and find if snapping is near it
     int atVertex, beforeVertex, afterVertex;
-    double dist;
-    QgsPoint closestLayerVertex = mSelectedFeature->geometry()->closestVertex( layerCoordPoint, atVertex, beforeVertex, afterVertex, dist );
-    dist = sqrt( dist );
+    double sqrDist; // will be negative on error
+    QgsPoint closestLayerVertex = mSelectedFeature->geometry()->closestVertex( layerCoordPoint, atVertex, beforeVertex, afterVertex, sqrDist );
 
     mSnapper.snapToCurrentLayer( e->pos(), snapResults, QgsSnapper::SnapToVertex, tol );
-    if ( dist <= tol )
+    if ( sqrDist >= 0 && sqrt( sqrDist ) <= tol )
     {
       // some vertex selected
       mMoving = true;

diff --git a/src/core/geometry/qgscircularstring.cpp b/src/core/geometry/qgscircularstring.cpp
@@ -861,6 +861,9 @@ double QgsCircularString::closestSegment( const QgsPointV2& pt, QgsPointV2& segm
     }
   }
 
+  if ( minDist == std::numeric_limits<double>::max() )
+    return -1; // error: no segments
+
   segmentPt = minDistSegmentPoint;
   vertexAfter = minDistVertexAfter;
   vertexAfter.part = 0;

diff --git a/src/core/geometry/qgscurvepolygon.cpp b/src/core/geometry/qgscurvepolygon.cpp
@@ -657,7 +657,7 @@ double QgsCurvePolygon::closestSegment( const QgsPointV2& pt, QgsPointV2& segmen
 {
   if ( !mExteriorRing )
   {
-    return 0.0;
+    return -1;
   }
   QList<QgsCurve*> segmentList;
   segmentList.append( mExteriorRing );

diff --git a/src/core/geometry/qgsgeometry.cpp b/src/core/geometry/qgsgeometry.cpp
@@ -369,6 +369,7 @@ QgsPoint QgsGeometry::closestVertex( const QgsPoint& point, int& atVertex, int&
 {
   if ( !d->geometry )
   {
+    sqrDist = -1;
     return QgsPoint( 0, 0 );
   }
 
@@ -609,12 +610,14 @@ double QgsGeometry::closestVertexWithContext( const QgsPoint& point, int& atVert
 {
   if ( !d->geometry )
   {
-    return 0.0;
+    return -1;
   }
 
   QgsVertexId vId;
   QgsPointV2 pt( point.x(), point.y() );
   QgsPointV2 closestPoint = QgsGeometryUtils::closestVertex( *( d->geometry ), pt, vId );
+  if ( !vId.isValid() )
+    return -1;
   atVertex = vertexNrFromVertexId( vId );
   return QgsGeometryUtils::sqrDistance2D( closestPoint, pt );
 }
@@ -628,14 +631,16 @@ double QgsGeometry::closestSegmentWithContext(
 {
   if ( !d->geometry )
   {
-    return 0;
+    return -1;
   }
 
   QgsPointV2 segmentPt;
   QgsVertexId vertexAfter;
   bool leftOfBool;
 
   double sqrDist = d->geometry->closestSegment( QgsPointV2( point.x(), point.y() ), segmentPt,  vertexAfter, &leftOfBool, epsilon );
+  if ( sqrDist < 0 )
+    return -1;
 
   minDistPoint.setX( segmentPt.x() );
   minDistPoint.setY( segmentPt.y() );

diff --git a/src/core/geometry/qgsgeometry.h b/src/core/geometry/qgsgeometry.h
@@ -225,7 +225,7 @@ class CORE_EXPORT QgsGeometry
      * @param afterVertex will be set to the vertex index of the next vertex after the closest one. Will be set to -1 if
      * not present.
      * @param sqrDist will be set to the square distance between the closest vertex and the specified point
-     * @returns closest point in geometry
+     * @returns closest point in geometry. If not found (empty geometry), returns null point nad sqrDist is negative.
      */
     //TODO QGIS 3.0 - rename beforeVertex to previousVertex, afterVertex to nextVertex
     QgsPoint closestVertex( const QgsPoint& point, int& atVertex, int& beforeVertex, int& afterVertex, double& sqrDist ) const;

diff --git a/src/core/geometry/qgsgeometryutils.cpp b/src/core/geometry/qgsgeometryutils.cpp
@@ -65,6 +65,7 @@ QgsPointV2 QgsGeometryUtils::closestVertex( const QgsAbstractGeometry& geom, con
   double minDist = std::numeric_limits<double>::max();
   double currentDist = 0;
   QgsPointV2 minDistPoint;
+  id = QgsVertexId(); // set as invalid
 
   QgsVertexId vertexId;
   QgsPointV2 vertex;

diff --git a/src/core/geometry/qgsgeometryutils.h b/src/core/geometry/qgsgeometryutils.h
@@ -37,7 +37,8 @@ class CORE_EXPORT QgsGeometryUtils
      */
     static QList<QgsLineString*> extractLineStrings( const QgsAbstractGeometry* geom );
 
-    /** Returns the closest vertex to a geometry for a specified point
+    /** Returns the closest vertex to a geometry for a specified point.
+     * On error null point will be returned and "id" argument will be invalid.
      */
     static QgsPointV2 closestVertex( const QgsAbstractGeometry& geom, const QgsPointV2& pt, QgsVertexId& id );
 
@@ -263,7 +264,7 @@ class CORE_EXPORT QgsGeometryUtils
       for ( int i = 0; i < container.size(); ++i )
       {
         sqrDist = container.at( i )->closestSegment( pt, segmentPt, vertexAfter, leftOf, epsilon );
-        if ( sqrDist < minDist )
+        if ( sqrDist >= 0 && sqrDist < minDist )
         {
           minDist = sqrDist;
           minDistSegmentX = segmentPt.x();
@@ -293,6 +294,9 @@ class CORE_EXPORT QgsGeometryUtils
         }
       }
 
+      if ( minDist == std::numeric_limits<double>::max() )
+        return -1;  // error: no segments
+
       segmentPt.setX( minDistSegmentX );
       segmentPt.setY( minDistSegmentY );
       vertexAfter = minDistVertexAfter;

diff --git a/src/core/geometry/qgslinestring.cpp b/src/core/geometry/qgslinestring.cpp
@@ -763,16 +763,10 @@ double QgsLineString::closestSegment( const QgsPointV2& pt, QgsPointV2& segmentP
   double segmentPtX, segmentPtY;
 
   int size = mX.size();
-  if ( size == 0 )
+  if ( size == 0 || size == 1 )
   {
     vertexAfter = QgsVertexId( 0, 0, 0 );
-    return sqrDist;
-  }
-  else if ( size == 1 )
-  {
-    segmentPt = pointN( 0 );
-    vertexAfter = QgsVertexId( 0, 0, 1 );
-    return QgsGeometryUtils::sqrDistance2D( pt, segmentPt );
+    return -1;
   }
   for ( int i = 1; i < size; ++i )
   {

diff --git a/src/core/qgspointlocator.cpp b/src/core/qgspointlocator.cpp
@@ -106,6 +106,8 @@ class QgsPointLocator_VisitorNearestVertex : public IVisitor
       int vertexIndex, beforeVertex, afterVertex;
       double sqrDist;
       QgsPoint pt = geom->closestVertex( mSrcPoint, vertexIndex, beforeVertex, afterVertex, sqrDist );
+      if ( sqrDist < 0 )
+        return;  // probably empty geometry
 
       QgsPointLocator::Match m( QgsPointLocator::Vertex, mLocator->mLayer, id, sqrt( sqrDist ), pt, vertexIndex );
       // in range queries the filter may reject some matches

diff --git a/tests/src/core/testqgsgeometry.cpp b/tests/src/core/testqgsgeometry.cpp
@@ -2047,11 +2047,10 @@ void TestQgsGeometry::lineString()
   //closest segment
   QgsLineString l35;
   bool leftOf = false;
+  p = QgsPointV2(); // reset all coords to zero
   ( void )l35.closestSegment( QgsPointV2( 1, 2 ), p, v, 0, 0 ); //empty line, just want no crash
   l35.setPoints( QgsPointSequence() << QgsPointV2( 5, 10 ) );
-  QVERIFY( qgsDoubleNear( l35.closestSegment( QgsPointV2( 5, 10 ), p, v, 0, 0 ), 0 ) );
-  QCOMPARE( p, QgsPointV2( 5, 10 ) );
-  QCOMPARE( v, QgsVertexId( 0, 0, 1 ) );
+  QVERIFY( l35.closestSegment( QgsPointV2( 5, 10 ), p, v, 0, 0 ) < 0 );
   l35.setPoints( QgsPointSequence() << QgsPointV2( 5, 10 ) << QgsPointV2( 10, 10 ) );
   QVERIFY( qgsDoubleNear( l35.closestSegment( QgsPointV2( 4, 11 ), p, v, &leftOf, 0 ), 2.0 ) );
   QCOMPARE( p, QgsPointV2( 5, 10 ) );

diff --git a/tests/src/core/testqgspointlocator.cpp b/tests/src/core/testqgspointlocator.cpp
@@ -23,6 +23,7 @@
 #include "qgsgeometry.h"
 #include "qgsmaplayerregistry.h"
 #include "qgspointlocator.h"
+#include "qgspolygon.h"
 
 
 struct FilterExcludePoint : public QgsPointLocator::MatchFilter
@@ -261,6 +262,48 @@ class TestQgsPointLocator : public QObject
       QVERIFY( m2.isValid() );
       QCOMPARE( m2.point(), QgsPoint( 1, 1 ) );
     }
+
+    void testNullGeometries()
+    {
+      QgsVectorLayer* vlNullGeom = new QgsVectorLayer( "Polygon", "x", "memory" );
+      QgsFeature ff( 0 );
+      ff.setGeometry( QgsGeometry() );
+      QgsFeatureList flist;
+      flist << ff;
+      vlNullGeom->dataProvider()->addFeatures( flist );
+
+      QgsPointLocator loc( vlNullGeom, 0, nullptr );
+
+      QgsPointLocator::Match m1 = loc.nearestVertex( QgsPoint( 2, 2 ), std::numeric_limits<double>::max() );
+      QVERIFY( !m1.isValid() );
+
+      QgsPointLocator::Match m2 = loc.nearestEdge( QgsPoint( 2, 2 ), std::numeric_limits<double>::max() );
+      QVERIFY( !m2.isValid() );
+
+      delete vlNullGeom;
+    }
+
+    void testEmptyGeometries()
+    {
+      QgsVectorLayer* vlEmptyGeom = new QgsVectorLayer( "Polygon", "x", "memory" );
+      QgsFeature ff( 0 );
+      QgsGeometry g;
+      g.setGeometry( new QgsPolygonV2() );
+      ff.setGeometry( g );
+      QgsFeatureList flist;
+      flist << ff;
+      vlEmptyGeom->dataProvider()->addFeatures( flist );
+
+      QgsPointLocator loc( vlEmptyGeom, 0, nullptr );
+
+      QgsPointLocator::Match m1 = loc.nearestVertex( QgsPoint( 2, 2 ), std::numeric_limits<double>::max() );
+      QVERIFY( !m1.isValid() );
+
+      QgsPointLocator::Match m2 = loc.nearestEdge( QgsPoint( 2, 2 ), std::numeric_limits<double>::max() );
+      QVERIFY( !m2.isValid() );
+
+      delete vlEmptyGeom;
+    }
 };
 
 QTEST_MAIN( TestQgsPointLocator )