[Geometry] Fix deleteVertex() for CircularString, CompoundCurve and C…

…urvePolygon In some situations, deleteVertex() could generate an invalid geometry, causing later crashes due to unexpected structure. Fix #15087
qgis · Jun 20, 2016 · dabc3b1 · dabc3b1
1 parent b015fb4
commit dabc3b1
Show file tree

Hide file tree

Showing 4 changed files with 251 additions and 9 deletions.
diff --git a/src/core/geometry/qgscircularstringv2.cpp b/src/core/geometry/qgscircularstringv2.cpp
@@ -779,9 +779,10 @@ bool QgsCircularStringV2::deleteVertex( QgsVertexId position )
   int nVertices = this->numPoints();
   if ( nVertices < 4 ) //circular string must have at least 3 vertices
   {
-    return false;
+    clear();
+    return true;
   }
-  if ( position.vertex < 1 || position.vertex > ( nVertices - 2 ) )
+  if ( position.vertex < 0 || position.vertex > ( nVertices - 1 ) )
   {
     return false;
   }

diff --git a/src/core/geometry/qgscompoundcurvev2.cpp b/src/core/geometry/qgscompoundcurvev2.cpp
@@ -539,10 +539,87 @@ bool QgsCompoundCurveV2::moveVertex( QgsVertexId position, const QgsPointV2& new
 bool QgsCompoundCurveV2::deleteVertex( QgsVertexId position )
 {
   QList< QPair<int, QgsVertexId> > curveIds = curveVertexId( position );
-  QList< QPair<int, QgsVertexId> >::const_iterator idIt = curveIds.constBegin();
-  for ( ; idIt != curveIds.constEnd(); ++idIt )
+  if ( curveIds.size() == 1 )
+  {
+    if ( !mCurves.at( curveIds.at( 0 ).first )->deleteVertex( curveIds.at( 0 ).second ) )
+    {
+      clearCache(); //bbox may have changed
+      return false;
+    }
+    if ( mCurves.at( curveIds.at( 0 ).first )->numPoints() == 0 )
+    {
+      removeCurve( curveIds.at( 0 ).first );
+    }
+  }
+  else if ( curveIds.size() == 2 )
   {
-    mCurves.at( idIt->first )->deleteVertex( idIt->second );
+    Q_ASSERT( curveIds.at( 1 ).first == curveIds.at( 0 ).first + 1 );
+    Q_ASSERT( curveIds.at( 0 ).second.vertex == mCurves.at( curveIds.at( 0 ).first )->numPoints() - 1 );
+    Q_ASSERT( curveIds.at( 1 ).second.vertex == 0 );
+    QgsPointV2 startPoint = mCurves.at( curveIds.at( 0 ).first ) ->startPoint();
+    QgsPointV2 endPoint = mCurves.at( curveIds.at( 1 ).first ) ->endPoint();
+    if ( QgsWKBTypes::flatType( mCurves.at( curveIds.at( 0 ).first )->wkbType() ) == QgsWKBTypes::LineString &&
+         QgsWKBTypes::flatType( mCurves.at( curveIds.at( 1 ).first )->wkbType() ) == QgsWKBTypes::CircularString &&
+         mCurves.at( curveIds.at( 1 ).first )->numPoints() > 3 )
+    {
+      QgsPointV2 intermediatePoint;
+      QgsVertexId::VertexType type;
+      mCurves.at( curveIds.at( 1 ).first ) ->pointAt( 2, intermediatePoint, type );
+      mCurves.at( curveIds.at( 0 ).first )->moveVertex(
+        QgsVertexId( 0, 0, mCurves.at( curveIds.at( 0 ).first )->numPoints() - 1 ), intermediatePoint );
+    }
+    else if ( !mCurves.at( curveIds.at( 0 ).first )->deleteVertex( curveIds.at( 0 ).second ) )
+    {
+      clearCache(); //bbox may have changed
+      return false;
+    }
+    if ( QgsWKBTypes::flatType( mCurves.at( curveIds.at( 0 ).first )->wkbType() ) == QgsWKBTypes::CircularString &&
+         mCurves.at( curveIds.at( 0 ).first )->numPoints() > 0 &&
+         QgsWKBTypes::flatType( mCurves.at( curveIds.at( 1 ).first )->wkbType() ) == QgsWKBTypes::LineString )
+    {
+      QgsPointV2 intermediatePoint = mCurves.at( curveIds.at( 0 ).first ) ->endPoint();
+      mCurves.at( curveIds.at( 1 ).first )->moveVertex( QgsVertexId( 0, 0, 0 ), intermediatePoint );
+    }
+    else if ( !mCurves.at( curveIds.at( 1 ).first )->deleteVertex( curveIds.at( 1 ).second ) )
+    {
+      clearCache(); //bbox may have changed
+      return false;
+    }
+    if ( mCurves.at( curveIds.at( 0 ).first )->numPoints() == 0 &&
+         mCurves.at( curveIds.at( 1 ).first )->numPoints() != 0 )
+    {
+      removeCurve( curveIds.at( 0 ).first );
+      mCurves.at( curveIds.at( 1 ).first )->moveVertex( QgsVertexId( 0, 0, 0 ), startPoint );
+    }
+    else if ( mCurves.at( curveIds.at( 0 ).first )->numPoints() != 0 &&
+              mCurves.at( curveIds.at( 1 ).first )->numPoints() == 0 )
+    {
+      removeCurve( curveIds.at( 1 ).first );
+      mCurves.at( curveIds.at( 0 ).first )->moveVertex(
+        QgsVertexId( 0, 0, mCurves.at( curveIds.at( 0 ).first )->numPoints() - 1 ), endPoint );
+    }
+    else if ( mCurves.at( curveIds.at( 0 ).first )->numPoints() == 0 &&
+              mCurves.at( curveIds.at( 1 ).first )->numPoints() == 0 )
+    {
+      removeCurve( curveIds.at( 1 ).first );
+      removeCurve( curveIds.at( 0 ).first );
+      QgsLineStringV2* line = new QgsLineStringV2();
+      line->insertVertex( QgsVertexId( 0, 0, 0 ), startPoint );
+      line->insertVertex( QgsVertexId( 0, 0, 1 ), endPoint );
+      mCurves.insert( curveIds.at( 0 ).first, line );
+    }
+    else
+    {
+      QgsPointV2 endPointOfFirst = mCurves.at( curveIds.at( 0 ).first ) ->endPoint();
+      QgsPointV2 startPointOfSecond = mCurves.at( curveIds.at( 1 ).first ) ->startPoint();
+      if ( endPointOfFirst != startPointOfSecond )
+      {
+        QgsLineStringV2* line = new QgsLineStringV2();
+        line->insertVertex( QgsVertexId( 0, 0, 0 ), endPointOfFirst );
+        line->insertVertex( QgsVertexId( 0, 0, 1 ), startPointOfSecond );
+        mCurves.insert( curveIds.at( 1 ).first, line );
+      }
+    }
   }
 
   bool success = !curveIds.isEmpty();
@@ -574,6 +651,7 @@ QList< QPair<int, QgsVertexId> > QgsCompoundCurveV2::curveVertexId( QgsVertexId
         vid.vertex = 0;
         curveIds.append( qMakePair( i + 1, vid ) );
       }
+      break;
     }
     currentVertexIndex += increment;
   }

diff --git a/src/core/geometry/qgscurvepolygonv2.cpp b/src/core/geometry/qgscurvepolygonv2.cpp
@@ -161,10 +161,13 @@ bool QgsCurvePolygonV2::fromWkt( const QString& wkt )
   {
     QPair<QgsWKBTypes::Type, QString> childParts = QgsGeometryUtils::wktReadBlock( childWkt );
 
-    if ( QgsWKBTypes::flatType( childParts.first ) == QgsWKBTypes::LineString )
+    QgsWKBTypes::Type flatCurveType = QgsWKBTypes::flatType( childParts.first );
+    if ( flatCurveType == QgsWKBTypes::LineString )
       mInteriorRings.append( new QgsLineStringV2() );
-    else if ( QgsWKBTypes::flatType( childParts.first ) == QgsWKBTypes::CircularString )
+    else if ( flatCurveType == QgsWKBTypes::CircularString )
       mInteriorRings.append( new QgsCircularStringV2() );
+    else if ( flatCurveType == QgsWKBTypes::CompoundCurve )
+      mInteriorRings.append( new QgsCompoundCurveV2() );
     else
     {
       clear();
@@ -739,10 +742,12 @@ bool QgsCurvePolygonV2::deleteVertex( QgsVertexId vId )
   if ( success )
   {
     // If first or last vertex is removed, re-sync the last/first vertex
+    // Do not use "n - 2", but "ring->numPoints() - 1" as more than one vertex
+    // may have been deleted (e.g. with CircularString)
     if ( vId.vertex == 0 )
-      ring->moveVertex( QgsVertexId( 0, 0, n - 2 ), ring->vertexAt( QgsVertexId( 0, 0, 0 ) ) );
+      ring->moveVertex( QgsVertexId( 0, 0, ring->numPoints() - 1 ), ring->vertexAt( QgsVertexId( 0, 0, 0 ) ) );
     else if ( vId.vertex == n - 1 )
-      ring->moveVertex( QgsVertexId( 0, 0, 0 ), ring->vertexAt( QgsVertexId( 0, 0, n - 2 ) ) );
+      ring->moveVertex( QgsVertexId( 0, 0, 0 ), ring->vertexAt( QgsVertexId( 0, 0, ring->numPoints() - 1 ) ) );
     clearCache();
   }
   return success;

diff --git a/tests/src/python/test_qgsgeometry.py b/tests/src/python/test_qgsgeometry.py
@@ -3146,6 +3146,164 @@ def testWkbTypes(self):
         self.assertEqual(QgsWKBTypes.zmType(QgsWKBTypes.MultiSurfaceZM, False, True), QgsWKBTypes.MultiSurfaceM)
         self.assertEqual(QgsWKBTypes.zmType(QgsWKBTypes.MultiSurfaceZM, True, True), QgsWKBTypes.MultiSurfaceZM)
 
+    def testDeleteVertexCircularString(self):
+
+        wkt = "CircularString ((0 0,1 1,2 0))"
+        geom = QgsGeometry.fromWkt(wkt)
+        assert geom.deleteVertex(0)
+        self.assertEqual(geom.exportToWkt(), QgsCircularStringV2().asWkt())
+
+        wkt = "CircularString ((0 0,1 1,2 0,3 -1,4 0))"
+        geom = QgsGeometry.fromWkt(wkt)
+        assert geom.deleteVertex(0)
+        expected_wkt = "CircularString (2 0, 3 -1, 4 0)"
+        self.assertEqual(geom.exportToWkt(), QgsGeometry.fromWkt(expected_wkt).exportToWkt())
+
+        wkt = "CircularString ((0 0,1 1,2 0,3 -1,4 0))"
+        geom = QgsGeometry.fromWkt(wkt)
+        assert geom.deleteVertex(1)
+        expected_wkt = "CircularString (0 0, 3 -1, 4 0)"
+        self.assertEqual(geom.exportToWkt(), QgsGeometry.fromWkt(expected_wkt).exportToWkt())
+
+        wkt = "CircularString ((0 0,1 1,2 0,3 -1,4 0))"
+        geom = QgsGeometry.fromWkt(wkt)
+        assert geom.deleteVertex(2)
+        expected_wkt = "CircularString (0 0, 1 1, 4 0)"
+        self.assertEqual(geom.exportToWkt(), QgsGeometry.fromWkt(expected_wkt).exportToWkt())
+
+        wkt = "CircularString ((0 0,1 1,2 0,3 -1,4 0))"
+        geom = QgsGeometry.fromWkt(wkt)
+        assert geom.deleteVertex(3)
+        expected_wkt = "CircularString (0 0, 1 1, 4 0)"
+        self.assertEqual(geom.exportToWkt(), QgsGeometry.fromWkt(expected_wkt).exportToWkt())
+
+        wkt = "CircularString ((0 0,1 1,2 0,3 -1,4 0))"
+        geom = QgsGeometry.fromWkt(wkt)
+        assert geom.deleteVertex(4)
+        expected_wkt = "CircularString (0 0,1 1,2 0)"
+        self.assertEqual(geom.exportToWkt(), QgsGeometry.fromWkt(expected_wkt).exportToWkt())
+
+        wkt = "CircularString ((0 0,1 1,2 0,3 -1,4 0))"
+        geom = QgsGeometry.fromWkt(wkt)
+        assert not geom.deleteVertex(-1)
+        assert not geom.deleteVertex(5)
+
+    def testDeleteVertexCompoundCurve(self):
+
+        wkt = "CompoundCurve ((0 0,1 1))"
+        geom = QgsGeometry.fromWkt(wkt)
+        assert not geom.deleteVertex(-1)
+        assert not geom.deleteVertex(2)
+        assert geom.deleteVertex(0)
+        self.assertEqual(geom.exportToWkt(), QgsCompoundCurveV2().asWkt())
+
+        wkt = "CompoundCurve ((0 0,1 1))"
+        geom = QgsGeometry.fromWkt(wkt)
+        assert geom.deleteVertex(1)
+        self.assertEqual(geom.exportToWkt(), QgsCompoundCurveV2().asWkt())
+
+        wkt = "CompoundCurve ((0 0,1 1),(1 1,2 2))"
+        geom = QgsGeometry.fromWkt(wkt)
+        assert geom.deleteVertex(0)
+        expected_wkt = "CompoundCurve ((1 1,2 2))"
+        self.assertEqual(geom.exportToWkt(), QgsGeometry.fromWkt(expected_wkt).exportToWkt())
+
+        wkt = "CompoundCurve ((0 0,1 1),(1 1,2 2))"
+        geom = QgsGeometry.fromWkt(wkt)
+        assert geom.deleteVertex(1)
+        expected_wkt = "CompoundCurve ((0 0,2 2))"
+        self.assertEqual(geom.exportToWkt(), QgsGeometry.fromWkt(expected_wkt).exportToWkt())
+
+        wkt = "CompoundCurve ((0 0,1 1),(1 1,2 2))"
+        geom = QgsGeometry.fromWkt(wkt)
+        assert geom.deleteVertex(2)
+        expected_wkt = "CompoundCurve ((0 0,1 1))"
+        self.assertEqual(geom.exportToWkt(), QgsGeometry.fromWkt(expected_wkt).exportToWkt())
+
+        wkt = "CompoundCurve ((0 0,1 1),CircularString(1 1,2 0,1 -1))"
+        geom = QgsGeometry.fromWkt(wkt)
+        assert geom.deleteVertex(1)
+        expected_wkt = "CompoundCurve ((0 0,1 -1))"
+        self.assertEqual(geom.exportToWkt(), QgsGeometry.fromWkt(expected_wkt).exportToWkt())
+
+        wkt = "CompoundCurve (CircularString(0 0,1 1,2 0),CircularString(2 0,3 -1,4 0))"
+        geom = QgsGeometry.fromWkt(wkt)
+        assert geom.deleteVertex(2)
+        expected_wkt = "CompoundCurve ((0 0, 4 0))"
+        self.assertEqual(geom.exportToWkt(), QgsGeometry.fromWkt(expected_wkt).exportToWkt())
+
+        wkt = "CompoundCurve (CircularString(0 0,1 1,2 0,1.5 -0.5,1 -1),(1 -1,0 0))"
+        geom = QgsGeometry.fromWkt(wkt)
+        assert geom.deleteVertex(4)
+        expected_wkt = "CompoundCurve (CircularString (0 0, 1 1, 2 0),(2 0, 0 0))"
+        self.assertEqual(geom.exportToWkt(), QgsGeometry.fromWkt(expected_wkt).exportToWkt())
+
+        wkt = "CompoundCurve ((-1 0,0 0),CircularString(0 0,1 1,2 0,1.5 -0.5,1 -1))"
+        geom = QgsGeometry.fromWkt(wkt)
+        assert geom.deleteVertex(1)
+        expected_wkt = "CompoundCurve ((-1 0, 2 0),CircularString (2 0, 1.5 -0.5, 1 -1))"
+        self.assertEqual(geom.exportToWkt(), QgsGeometry.fromWkt(expected_wkt).exportToWkt())
+
+        wkt = "CompoundCurve (CircularString(-1 -1,-1.5 -0.5,-2 0,-1 1,0 0),CircularString(0 0,1 1,2 0,1.5 -0.5,1 -1))"
+        geom = QgsGeometry.fromWkt(wkt)
+        assert geom.deleteVertex(4)
+        expected_wkt = "CompoundCurve (CircularString (-1 -1, -1.5 -0.5, -2 0),(-2 0, 2 0),CircularString (2 0, 1.5 -0.5, 1 -1))"
+        self.assertEqual(geom.exportToWkt(), QgsGeometry.fromWkt(expected_wkt).exportToWkt())
+
+    def testDeleteVertexCurvePolygon(self):
+
+        wkt = "CurvePolygon (CompoundCurve (CircularString(0 0,1 1,2 0),(2 0,0 0)))"
+        geom = QgsGeometry.fromWkt(wkt)
+        assert not geom.deleteVertex(-1)
+        assert not geom.deleteVertex(4)
+        assert geom.deleteVertex(0)
+        self.assertEqual(geom.exportToWkt(), QgsCurvePolygonV2().asWkt())
+
+        wkt = "CurvePolygon (CompoundCurve (CircularString(0 0,1 1,2 0),(2 0,0 0)))"
+        geom = QgsGeometry.fromWkt(wkt)
+        assert geom.deleteVertex(1)
+        self.assertEqual(geom.exportToWkt(), QgsCurvePolygonV2().asWkt())
+
+        wkt = "CurvePolygon (CompoundCurve (CircularString(0 0,1 1,2 0),(2 0,0 0)))"
+        geom = QgsGeometry.fromWkt(wkt)
+        assert geom.deleteVertex(2)
+        self.assertEqual(geom.exportToWkt(), QgsCurvePolygonV2().asWkt())
+
+        wkt = "CurvePolygon (CompoundCurve (CircularString(0 0,1 1,2 0),(2 0,0 0)))"
+        geom = QgsGeometry.fromWkt(wkt)
+        assert geom.deleteVertex(3)
+        self.assertEqual(geom.exportToWkt(), QgsCurvePolygonV2().asWkt())
+
+        wkt = "CurvePolygon (CompoundCurve (CircularString(0 0,1 1,2 0,1.5 -0.5,1 -1),(1 -1,0 0)))"
+        geom = QgsGeometry.fromWkt(wkt)
+        assert geom.deleteVertex(0)
+        expected_wkt = "CurvePolygon (CompoundCurve (CircularString (2 0, 1.5 -0.5, 1 -1),(1 -1, 2 0)))"
+        self.assertEqual(geom.exportToWkt(), QgsGeometry.fromWkt(expected_wkt).exportToWkt())
+
+        wkt = "CurvePolygon (CompoundCurve (CircularString(0 0,1 1,2 0,1.5 -0.5,1 -1),(1 -1,0 0)))"
+        geom = QgsGeometry.fromWkt(wkt)
+        assert geom.deleteVertex(1)
+        expected_wkt = "CurvePolygon (CompoundCurve (CircularString (0 0, 1.5 -0.5, 1 -1),(1 -1, 0 0)))"
+        self.assertEqual(geom.exportToWkt(), QgsGeometry.fromWkt(expected_wkt).exportToWkt())
+
+        wkt = "CurvePolygon (CompoundCurve (CircularString(0 0,1 1,2 0,1.5 -0.5,1 -1),(1 -1,0 0)))"
+        geom = QgsGeometry.fromWkt(wkt)
+        assert geom.deleteVertex(2)
+        expected_wkt = "CurvePolygon (CompoundCurve (CircularString (0 0, 1 1, 1 -1),(1 -1, 0 0)))"
+        self.assertEqual(geom.exportToWkt(), QgsGeometry.fromWkt(expected_wkt).exportToWkt())
+
+        wkt = "CurvePolygon (CompoundCurve (CircularString(0 0,1 1,2 0,1.5 -0.5,1 -1),(1 -1,0 0)))"
+        geom = QgsGeometry.fromWkt(wkt)
+        assert geom.deleteVertex(3)
+        expected_wkt = "CurvePolygon (CompoundCurve (CircularString (0 0, 1 1, 1 -1),(1 -1, 0 0)))"
+        self.assertEqual(geom.exportToWkt(), QgsGeometry.fromWkt(expected_wkt).exportToWkt())
+
+        wkt = "CurvePolygon (CompoundCurve (CircularString(0 0,1 1,2 0,1.5 -0.5,1 -1),(1 -1,0 0)))"
+        geom = QgsGeometry.fromWkt(wkt)
+        assert geom.deleteVertex(4)
+        expected_wkt = "CurvePolygon (CompoundCurve (CircularString (0 0, 1 1, 2 0),(2 0, 0 0)))"
+        self.assertEqual(geom.exportToWkt(), QgsGeometry.fromWkt(expected_wkt).exportToWkt())
+
 
 if __name__ == '__main__':
     unittest.main()