Skip to content
Permalink
Browse files

Fix #11489 (crash with force point inside polygon in centroid fill st…

…yle)

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 >= 4 points)

Along the way I have added some comments that may help others decode why the code does things it does.
  • Loading branch information
wonder-sk committed Oct 27, 2014
1 parent bb85761 commit 83d070e8dd5f91dc949809a92478df33e217644a
Showing with 46 additions and 24 deletions.
  1. +12 −1 src/core/qgsgeometry.cpp
  2. +34 −23 src/core/qgsmaptopixelgeometrysimplifier.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 )

This comment has been minimized.

Copy link
@ahuarte47

ahuarte47 Oct 27, 2014

Contributor

Hi @wonder-sk, IMHO I think this code is dangerous: when a geometry has very small holes, a ring can be invalid, and now the main polygon will not be labeled (or other GEOS operation) because the GEOSGeometry is not created.

This comment has been minimized.

Copy link
@wonder-sk

wonder-sk Oct 27, 2014

Author Member

But the geometry classes are not used just for rendering - users can call QgsGeometry.fromPolygon( ... ) when they want to do some analyses. Then I think it is better to fail early rather than silently returning a different geometry.

I believe the right approach is to fix the source of the problem (invalid polygons from simplification) rather than consequences.

{
// 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 );
@@ -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 )

5 comments on commit 83d070e

@haubourg

This comment has been minimized.

Copy link
Contributor

@haubourg haubourg replied Nov 16, 2015

Hi, for the record, here in 2.12, we still face crashes with centroid fill with Point on Surface option. Deactivating simplification helps with less crashes, but still some remain.
I see a Geos Exception raising Self Intersection near crazy coordinates (exemple 133.2351 - 78.12654 when data are projected around 2000000 6000000).

@ahuarte47

This comment has been minimized.

Copy link
Contributor

@ahuarte47 ahuarte47 replied Nov 17, 2015

Hi @haubourg, related to this I have proposed an idea to avoid these errors.

http://hub.qgis.org/issues/3517#note-17

Best regards
Alvaro

@ahuarte47

This comment has been minimized.

Copy link
Contributor

@ahuarte47 ahuarte47 replied Nov 23, 2015

Hi @haubourg, please, could you test this patch?

@haubourg

This comment has been minimized.

Copy link
Contributor

@haubourg haubourg replied Nov 23, 2015

Hi @ahuarte47 , Unfortunatly not. I have no compile environment ready and a lot of other priorities this week.
I can share a dataset + qlr file that currently triggers the crash every time if needed.

@ahuarte47

This comment has been minimized.

Copy link
Contributor

@ahuarte47 ahuarte47 replied Nov 23, 2015

ok, do not worry, it can be postponed

Please sign in to comment.
You can’t perform that action at this time.