Inverted polygons fixes (crashes, geometry checking, memory consuption, CPU efficiency)

wonder-sk · wonder-sk · commit 033bf6f6cc34 · 2014-06-16T17:40:51.000+07:00
diff --git a/src/core/symbology-ng/qgsinvertedpolygonrenderer.cpp b/src/core/symbology-ng/qgsinvertedpolygonrenderer.cpp
@@ -202,6 +202,10 @@ bool QgsInvertedPolygonRenderer::renderFeature( QgsFeature& feature, QgsRenderCo
       geom.reset( geom->buffer( 0, 0 ) );
     }
   }
+
+  if ( !geom )
+    return false; // do not let invalid geometries sneak in!
+
   // add the geometry to the list of geometries for this feature
   cFeat.geometries.append( geom.take() );
 
@@ -221,18 +225,15 @@ void QgsInvertedPolygonRenderer::stopRender( QgsRenderContext& context )
 
   for ( FeatureCategoryVector::iterator cit = mFeaturesCategories.begin(); cit != mFeaturesCategories.end(); ++cit )
   {
-    QgsFeature& feat = cit->feature;
+    QgsFeature feat = cit->feature; // just a copy, so that we do not accumulate geometries again
     if ( mPreprocessingEnabled )
     {
       // compute the unary union on the polygons
       QScopedPointer<QgsGeometry> unioned( QgsGeometry::unaryUnion( cit->geometries ) );
       // compute the difference with the extent
       QScopedPointer<QgsGeometry> rect( QgsGeometry::fromPolygon( mExtentPolygon ) );
-      QgsGeometry *final = rect->difference( const_cast<QgsGeometry*>(unioned.data()) );
-      if ( final )
-      {
-        feat.setGeometry( final );
-      }
+      QgsGeometry *final = rect->difference( const_cast<QgsGeometry*>( unioned.data() ) );
+      feat.setGeometry( final );
     }
     else
     {
@@ -248,7 +249,7 @@ void QgsInvertedPolygonRenderer::stopRender( QgsRenderContext& context )
       // operations do not need geometries to be valid
       QgsMultiPolygon finalMulti;
       finalMulti.append( mExtentPolygon );
-      foreach( QgsGeometry* geom, cit->geometries )
+      foreach ( QgsGeometry* geom, cit->geometries )
       {
         QgsMultiPolygon multi;
         if (( geom->wkbType() == QGis::WKBPolygon ) ||
@@ -264,8 +265,13 @@ void QgsInvertedPolygonRenderer::stopRender( QgsRenderContext& context )
 
         for ( int i = 0; i < multi.size(); i++ )
         {
+          const QgsPolyline& exterior = multi[i][0];
           // add the exterior ring as interior ring to the first polygon
-          finalMulti[0].append( multi[i][0] );
+          // make sure it satisfies at least very basic requirements of GEOS
+          // (otherwise the creation of GEOS geometry will fail)
+          if ( exterior.count() < 4 || exterior[0] != exterior[exterior.count() - 1] )
+            continue;
+          finalMulti[0].append( exterior );
 
           // add interior rings as new polygons
           for ( int j = 1; j < multi[i].size(); j++ )
@@ -275,14 +281,15 @@ void QgsInvertedPolygonRenderer::stopRender( QgsRenderContext& context )
             finalMulti.append( new_poly );
           }
         }
-        feat.setGeometry( QgsGeometry::fromMultiPolygon( finalMulti ) );
       }
+      feat.setGeometry( QgsGeometry::fromMultiPolygon( finalMulti ) );
     }
-    mSubRenderer->renderFeature( feat, mContext );
+    if ( feat.geometry() )
+      mSubRenderer->renderFeature( feat, mContext );
   }
   for ( FeatureCategoryVector::iterator cit = mFeaturesCategories.begin(); cit != mFeaturesCategories.end(); ++cit )
   {
-    foreach( QgsGeometry* g, cit->geometries )
+    foreach ( QgsGeometry* g, cit->geometries )
     {
       delete g;
     }

Original file line number	Diff line number	Diff line change
`@@ -202,6 +202,10 @@ bool QgsInvertedPolygonRenderer::renderFeature( QgsFeature& feature, QgsRenderCo`
`202`	`202`	`geom.reset( geom->buffer( 0, 0 ) );`
`203`	`203`	`}`
`204`	`204`	`}`
	`205`	`+`
	`206`	`+ if ( !geom )`
	`207`	`+ return false; // do not let invalid geometries sneak in!`
	`208`	`+`
`205`	`209`	`// add the geometry to the list of geometries for this feature`
`206`	`210`	`cFeat.geometries.append( geom.take() );`
`207`	`211`
`@@ -221,18 +225,15 @@ void QgsInvertedPolygonRenderer::stopRender( QgsRenderContext& context )`
`221`	`225`
`222`	`226`	`for ( FeatureCategoryVector::iterator cit = mFeaturesCategories.begin(); cit != mFeaturesCategories.end(); ++cit )`
`223`	`227`	`{`
`224`		`- QgsFeature& feat = cit->feature;`
	`228`	`+ QgsFeature feat = cit->feature; // just a copy, so that we do not accumulate geometries again`
`225`	`229`	`if ( mPreprocessingEnabled )`
`226`	`230`	`{`
`227`	`231`	`// compute the unary union on the polygons`
`228`	`232`	`QScopedPointer<QgsGeometry> unioned( QgsGeometry::unaryUnion( cit->geometries ) );`
`229`	`233`	`// compute the difference with the extent`
`230`	`234`	`QScopedPointer<QgsGeometry> rect( QgsGeometry::fromPolygon( mExtentPolygon ) );`
`231`		`- QgsGeometry final = rect->difference( const_cast<QgsGeometry>(unioned.data()) );`
`232`		`- if ( final )`
`233`		`- {`
`234`		`- feat.setGeometry( final );`
`235`		`- }`
	`235`	`+ QgsGeometry final = rect->difference( const_cast<QgsGeometry>( unioned.data() ) );`
	`236`	`+ feat.setGeometry( final );`
`236`	`237`	`}`
`237`	`238`	`else`
`238`	`239`	`{`
`@@ -248,7 +249,7 @@ void QgsInvertedPolygonRenderer::stopRender( QgsRenderContext& context )`
`248`	`249`	`// operations do not need geometries to be valid`
`249`	`250`	`QgsMultiPolygon finalMulti;`
`250`	`251`	`finalMulti.append( mExtentPolygon );`
`251`		`- foreach( QgsGeometry* geom, cit->geometries )`
	`252`	`+ foreach ( QgsGeometry* geom, cit->geometries )`
`252`	`253`	`{`
`253`	`254`	`QgsMultiPolygon multi;`
`254`	`255`	`if (( geom->wkbType() == QGis::WKBPolygon ) \|\|`
`@@ -264,8 +265,13 @@ void QgsInvertedPolygonRenderer::stopRender( QgsRenderContext& context )`
`264`	`265`
`265`	`266`	`for ( int i = 0; i < multi.size(); i++ )`
`266`	`267`	`{`
	`268`	`+ const QgsPolyline& exterior = multi[i][0];`
`267`	`269`	`// add the exterior ring as interior ring to the first polygon`
`268`		`- finalMulti[0].append( multi[i][0] );`
	`270`	`+ // make sure it satisfies at least very basic requirements of GEOS`
	`271`	`+ // (otherwise the creation of GEOS geometry will fail)`
	`272`	`+ if ( exterior.count() < 4 \|\| exterior[0] != exterior[exterior.count() - 1] )`
	`273`	`+ continue;`
	`274`	`+ finalMulti[0].append( exterior );`
`269`	`275`
`270`	`276`	`// add interior rings as new polygons`
`271`	`277`	`for ( int j = 1; j < multi[i].size(); j++ )`
`@@ -275,14 +281,15 @@ void QgsInvertedPolygonRenderer::stopRender( QgsRenderContext& context )`
`275`	`281`	`finalMulti.append( new_poly );`
`276`	`282`	`}`
`277`	`283`	`}`
`278`		`- feat.setGeometry( QgsGeometry::fromMultiPolygon( finalMulti ) );`
`279`	`284`	`}`
	`285`	`+ feat.setGeometry( QgsGeometry::fromMultiPolygon( finalMulti ) );`
`280`	`286`	`}`
`281`		`- mSubRenderer->renderFeature( feat, mContext );`
	`287`	`+ if ( feat.geometry() )`
	`288`	`+ mSubRenderer->renderFeature( feat, mContext );`
`282`	`289`	`}`
`283`	`290`	`for ( FeatureCategoryVector::iterator cit = mFeaturesCategories.begin(); cit != mFeaturesCategories.end(); ++cit )`
`284`	`291`	`{`
`285`		`- foreach( QgsGeometry* g, cit->geometries )`
	`292`	`+ foreach ( QgsGeometry* g, cit->geometries )`
`286`	`293`	`{`
`287`	`294`	`delete g;`
`288`	`295`	`}`