Fix #11489 (crash with force point inside polygon in centroid fill style)

wonder-sk · wonder-sk · commit 83d070e8dd5f · 2014-10-27T21:46:34.000+07:00
This isn't actually a very good fix. The issue in the "maptopixel" simplification
is still there, it is just less obvious, while sacrificing a bit of QgsGeometry correctness
(like the fact that linear ring should have &gt;= 4 points)

Along the way I have added some comments that may help others decode why the code does things it does.
diff --git a/src/core/qgsgeometry.cpp b/src/core/qgsgeometry.cpp
@@ -284,6 +284,8 @@ static GEOSGeometry *createGeosLinearRing( const QgsPolyline& polyline )
     }
     else
     {
+      // XXX [MD] this exception should not be silenced!
+      // this is here just because maptopixel simplification can return invalid linear rings
       if ( polyline.count() == 3 ) //-> Avoid 'GEOS::IllegalArgumentException: Invalid number of points in LinearRing found 3 - must be 0 or >= 4'
         return 0;
 
@@ -359,7 +361,16 @@ static GEOSGeometry *createGeosPolygon( const QgsPolygon& polygon )
     for ( int i = 0; i < polygon.count(); i++ )
     {
       GEOSGeometry *ring = createGeosLinearRing( polygon[i] );
-      if ( ring ) geoms << ring;
+      if ( !ring )
+      {
+        // something went really wrong - exit
+        for ( int j = 0; j < geoms.count(); j++ )
+          GEOSGeom_destroy_r( geosinit.ctxt, geoms[j] );
+        // XXX [MD] we just silently return here - but we shouldn't
+        // this is just because maptopixel simplification can return invalid linear rings
+        return 0;
+      }
+      geoms << ring;
     }
 
     return createGeosPolygon( geoms );
diff --git a/src/core/qgsmaptopixelgeometrysimplifier.cpp b/src/core/qgsmaptopixelgeometrysimplifier.cpp
@@ -61,7 +61,7 @@ inline static QgsRectangle calculateBoundingBox( QGis::WkbType wkbType, unsigned
 }
 
 //! Generalize the WKB-geometry using the BBOX of the original geometry
-inline static bool generalizeWkbGeometry(
+static bool generalizeWkbGeometryByBoundingBox(
   QGis::WkbType wkbType,
   unsigned char* sourceWkb, size_t sourceWkbSize,
   unsigned char* targetWkb, size_t& targetWkbSize,
@@ -169,7 +169,7 @@ bool QgsMapToPixelSimplifier::simplifyWkbGeometry(
   if (( simplifyFlags & QgsMapToPixelSimplifier::SimplifyEnvelope ) &&
       isGeneralizableByMapBoundingBox( envelope, map2pixelTol ) )
   {
-    isGeneralizable = generalizeWkbGeometry( wkbType, sourceWkb, sourceWkbSize, targetWkb, targetWkbSize, envelope, writeHeader );
+    isGeneralizable = generalizeWkbGeometryByBoundingBox( wkbType, sourceWkb, sourceWkbSize, targetWkb, targetWkbSize, envelope, writeHeader );
     if ( isGeneralizable )
       return true;
   }
@@ -222,8 +222,8 @@ bool QgsMapToPixelSimplifier::simplifyWkbGeometry(
     double* ptr = ( double* )targetWkb;
     map2pixelTol *= map2pixelTol; //-> Use mappixelTol for 'LengthSquare' calculations.
 
-    bool isaUngenerizableSegment;
-    bool hasUngenerizableSegments = false; //-> To avoid replace the simplified geometry by its BBOX when there are 'long' segments.
+    bool isLongSegment;
+    bool hasLongSegments = false; //-> To avoid replace the simplified geometry by its BBOX when there are 'long' segments.
 
     // Check whether the LinearRing is really closed.
     if ( isaLinearRing )
@@ -249,44 +249,55 @@ bool QgsMapToPixelSimplifier::simplifyWkbGeometry(
       memcpy( &x, sourceWkb, sizeof( double ) ); sourceWkb += sizeOfDoubleX;
       memcpy( &y, sourceWkb, sizeof( double ) ); sourceWkb += sizeOfDoubleY;
 
-      isaUngenerizableSegment = false;
+      isLongSegment = false;
 
       if ( i == 0 ||
            !isGeneralizable ||
-           ( isaUngenerizableSegment = ( calculateLengthSquared2D( x, y, lastX, lastY ) > map2pixelTol ) ) ||
+           ( isLongSegment = ( calculateLengthSquared2D( x, y, lastX, lastY ) > map2pixelTol ) ) ||
            ( !isaLinearRing && ( i == 1 || i >= numPoints - 2 ) ) )
       {
         memcpy( ptr, &x, sizeof( double ) ); lastX = x; ptr++;
         memcpy( ptr, &y, sizeof( double ) ); lastY = y; ptr++;
         numTargetPoints++;
 
-        if ( isaUngenerizableSegment && !hasUngenerizableSegments )
-        {
-          hasUngenerizableSegments = true;
-        }
+        hasLongSegments |= isLongSegment;
       }
 
       r.combineExtentWith( x, y );
     }
     targetWkb = wkb2 + 4;
 
-    // Fix the topology of the geometry
-    if ( numTargetPoints <= ( isaLinearRing ? 2 : 1 ) && !hasUngenerizableSegments )
+    if ( numTargetPoints < ( isaLinearRing ? 4 : 2 ) )
     {
-      unsigned char* targetTempWkb = targetWkb;
-      int targetWkbTempSize = targetWkbSize;
-
-      sourceWkb = sourcePrevWkb;
-      targetWkb = targetPrevWkb;
-      targetWkbSize = targetWkbPrevSize;
-      if ( generalizeWkbGeometry( wkbType, sourceWkb, sourceWkbSize, targetWkb, targetWkbSize, r, writeHeader ) )
-        return true;
-
-      targetWkb = targetTempWkb;
-      targetWkbSize = targetWkbTempSize;
+      // we simplified the geometry too much!
+      if ( !hasLongSegments )
+      {
+        // approximate the geometry's shape by its bounding box
+        // (rect for linear ring / one segment for line string)
+        unsigned char* targetTempWkb = targetWkb;
+        int targetWkbTempSize = targetWkbSize;
+
+        sourceWkb = sourcePrevWkb;
+        targetWkb = targetPrevWkb;
+        targetWkbSize = targetWkbPrevSize;
+        if ( generalizeWkbGeometryByBoundingBox( wkbType, sourceWkb, sourceWkbSize, targetWkb, targetWkbSize, r, writeHeader ) )
+          return true;
+
+        targetWkb = targetTempWkb;
+        targetWkbSize = targetWkbTempSize;
+      }
+      else
+      {
+        // Bad luck! The simplified geometry is invalid and approximation by bounding box
+        // would create artifacts due to long segments. Worst of all, we may have overwritten
+        // the original coordinates by the simplified ones (source and target WKB ptr can be the same)
+        // so we cannot even undo the changes here. We will return invalid geometry and hope that
+        // other pieces of QGIS will survive that :-/
+      }
     }
     if ( isaLinearRing )
     {
+      // make sure we keep the linear ring closed
       memcpy( &x, targetWkb + 0, sizeof( double ) );
       memcpy( &y, targetWkb + sizeof( double ), sizeof( double ) );
       if ( lastX != x || lastY != y )

Original file line number	Diff line number	Diff line change
`@@ -284,6 +284,8 @@ static GEOSGeometry *createGeosLinearRing( const QgsPolyline& polyline )`
`284`	`284`	`}`
`285`	`285`	`else`
`286`	`286`	`{`
	`287`	`+ // XXX [MD] this exception should not be silenced!`
	`288`	`+ // this is here just because maptopixel simplification can return invalid linear rings`
`287`	`289`	`if ( polyline.count() == 3 ) //-> Avoid 'GEOS::IllegalArgumentException: Invalid number of points in LinearRing found 3 - must be 0 or >= 4'`
`288`	`290`	`return 0;`
`289`	`291`
`@@ -359,7 +361,16 @@ static GEOSGeometry *createGeosPolygon( const QgsPolygon& polygon )`
`359`	`361`	`for ( int i = 0; i < polygon.count(); i++ )`
`360`	`362`	`{`
`361`	`363`	`GEOSGeometry *ring = createGeosLinearRing( polygon[i] );`
`362`		`- if ( ring ) geoms << ring;`
	`364`	`+ if ( !ring )`
	`365`	`+ {`
	`366`	`+ // something went really wrong - exit`
	`367`	`+ for ( int j = 0; j < geoms.count(); j++ )`
	`368`	`+ GEOSGeom_destroy_r( geosinit.ctxt, geoms[j] );`
	`369`	`+ // XXX [MD] we just silently return here - but we shouldn't`
	`370`	`+ // this is just because maptopixel simplification can return invalid linear rings`
	`371`	`+ return 0;`
	`372`	`+ }`
	`373`	`+ geoms << ring;`
`363`	`374`	`}`
`364`	`375`
`365`	`376`	`return createGeosPolygon( geoms );`