Skip to content

Commit

Permalink
Allow converting polygons with unclosed rings to GEOS
Browse files Browse the repository at this point in the history 
Forces ring close on conversion, fixing a regression
from 2.8 (and 2.14). See qgis#13635

Adds test for identifying invalid polygons, currently only
testing for the unclosed-ring invalidity.

The test was verified to fail without the fixes included
in this same commit, and to pass in 2.14.
  • Loading branch information
@strk
strk committed Jun 22, 2016
1 parent 3fb87de commit e92e7fe
Show file tree
Hide file tree
Showing 3 changed files with 77 additions and 9 deletions.
23 changes: 15 additions & 8 deletions src/core/geometry/qgsgeos.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1478,7 +1478,7 @@ bool QgsGeos::isEmpty( QString* errorMsg ) const
CATCH_GEOS_WITH_ERRMSG( false );
}

GEOSCoordSequence* QgsGeos::createCoordinateSequence( const QgsCurveV2* curve, double precision )
GEOSCoordSequence* QgsGeos::createCoordinateSequence( const QgsCurveV2* curve, double precision, bool forceClose )
{
bool segmentize = false;
const QgsLineStringV2* line = dynamic_cast<const QgsLineStringV2*>( curve );
Expand Down Expand Up @@ -1507,20 +1507,27 @@ GEOSCoordSequence* QgsGeos::createCoordinateSequence( const QgsCurveV2* curve, d
}

int numPoints = line->numPoints();

int numOutPoints = numPoints;
if ( forceClose && ( line->pointN( 0 ) != line->pointN( numPoints - 1 ) ) )
{
++numOutPoints;
}

GEOSCoordSequence* coordSeq = nullptr;
try
{
coordSeq = GEOSCoordSeq_create_r( geosinit.ctxt, numPoints, coordDims );
coordSeq = GEOSCoordSeq_create_r( geosinit.ctxt, numOutPoints, coordDims );
if ( !coordSeq )
{
QgsMessageLog::logMessage( QObject::tr( "Could not create coordinate sequence for %1 points in %2 dimensions" ).arg( numPoints ).arg( coordDims ), QObject::tr( "GEOS" ) );
return nullptr;
}
if ( precision > 0. )
{
for ( int i = 0; i < numPoints; ++i )
for ( int i = 0; i < numOutPoints; ++i )
{
QgsPointV2 pt = line->pointN( i ); //todo: create method to get const point reference
const QgsPointV2 &pt = line->pointN( i % numPoints ); //todo: create method to get const point reference
GEOSCoordSeq_setX_r( geosinit.ctxt, coordSeq, i, qgsRound( pt.x() / precision ) * precision );
GEOSCoordSeq_setY_r( geosinit.ctxt, coordSeq, i, qgsRound( pt.y() / precision ) * precision );
if ( hasZ )
Expand All @@ -1535,9 +1542,9 @@ GEOSCoordSequence* QgsGeos::createCoordinateSequence( const QgsCurveV2* curve, d
}
else
{
for ( int i = 0; i < numPoints; ++i )
for ( int i = 0; i < numOutPoints; ++i )
{
QgsPointV2 pt = line->pointN( i ); //todo: create method to get const point reference
const QgsPointV2 &pt = line->pointN( i % numPoints ); //todo: create method to get const point reference
GEOSCoordSeq_setX_r( geosinit.ctxt, coordSeq, i, pt.x() );
GEOSCoordSeq_setY_r( geosinit.ctxt, coordSeq, i, pt.y() );
if ( hasZ )
Expand Down Expand Up @@ -1640,7 +1647,7 @@ GEOSGeometry* QgsGeos::createGeosPolygon( const QgsAbstractGeometryV2* poly , do
GEOSGeometry* geosPolygon = nullptr;
try
{
GEOSGeometry* exteriorRingGeos = GEOSGeom_createLinearRing_r( geosinit.ctxt, createCoordinateSequence( exteriorRing, precision ) );
GEOSGeometry* exteriorRingGeos = GEOSGeom_createLinearRing_r( geosinit.ctxt, createCoordinateSequence( exteriorRing, precision, true ) );


int nHoles = polygon->numInteriorRings();
Expand All @@ -1653,7 +1660,7 @@ GEOSGeometry* QgsGeos::createGeosPolygon( const QgsAbstractGeometryV2* poly , do
for ( int i = 0; i < nHoles; ++i )
{
const QgsCurveV2* interiorRing = polygon->interiorRing( i );
holes[i] = GEOSGeom_createLinearRing_r( geosinit.ctxt, createCoordinateSequence( interiorRing, precision ) );
holes[i] = GEOSGeom_createLinearRing_r( geosinit.ctxt, createCoordinateSequence( interiorRing, precision, true ) );
}
geosPolygon = GEOSGeom_createPolygon_r( geosinit.ctxt, exteriorRingGeos, holes, nHoles );
delete[] holes;
Expand Down
2 changes: 1 addition & 1 deletion src/core/geometry/qgsgeos.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ class CORE_EXPORT QgsGeos: public QgsGeometryEngine
void cacheGeos() const;
QgsAbstractGeometryV2* overlay( const QgsAbstractGeometryV2& geom, Overlay op, QString* errorMsg = nullptr ) const;
bool relation( const QgsAbstractGeometryV2& geom, Relation r, QString* errorMsg = nullptr ) const;
static GEOSCoordSequence* createCoordinateSequence( const QgsCurveV2* curve , double precision );
static GEOSCoordSequence* createCoordinateSequence( const QgsCurveV2* curve , double precision, bool forceClose = false );
static QgsLineStringV2* sequenceToLinestring( const GEOSGeometry* geos, bool hasZ, bool hasM );
static int numberOfGeometries( GEOSGeometry* g );
static GEOSGeometry* nodeGeometries( const GEOSGeometry *splitLine, const GEOSGeometry *geom );
Expand Down
61 changes: 61 additions & 0 deletions tests/src/app/testqgsmaptoolidentifyaction.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,11 +45,36 @@ class TestQgsMapToolIdentifyAction : public QObject
void areaCalculation(); //test calculation of derived area attribute
void identifyRasterFloat32(); // test pixel identification and decimal precision
void identifyRasterFloat64(); // test pixel identification and decimal precision
void identifyInvalidPolygons(); // test selecting invalid polygons

private:
QgsMapCanvas* canvas;

QString testIdentifyRaster( QgsRasterLayer* layer, double xGeoref, double yGeoref );
QList<QgsMapToolIdentify::IdentifyResult> testIdentifyVector( QgsVectorLayer* layer, double xGeoref, double yGeoref );

// Release return with delete []
unsigned char *
hex2bytes( const char *hex, int *size )
{
QByteArray ba = QByteArray::fromHex( hex );
unsigned char *out = new unsigned char[ba.size()];
memcpy( out, ba.data(), ba.size() );
*size = ba.size();
return out;
}

// TODO: make this a QgsGeometry member...
QgsGeometry geomFromHexWKB( const char *hexwkb )
{
int wkbsize;
unsigned char *wkb = hex2bytes( hexwkb, &wkbsize );
QgsGeometry geom;
// NOTE: QgsGeometry takes ownership of wkb
geom.fromWkb( wkb, wkbsize );
return geom;
}

};

void TestQgsMapToolIdentifyAction::initTestCase()
Expand Down Expand Up @@ -248,6 +273,7 @@ void TestQgsMapToolIdentifyAction::areaCalculation()
QVERIFY( qgsDoubleNear( area, 389.6117, 0.001 ) );
}

// private
QString TestQgsMapToolIdentifyAction::testIdentifyRaster( QgsRasterLayer* layer, double xGeoref, double yGeoref )
{
QScopedPointer< QgsMapToolIdentifyAction > action( new QgsMapToolIdentifyAction( canvas ) );
Expand All @@ -258,6 +284,16 @@ QString TestQgsMapToolIdentifyAction::testIdentifyRaster( QgsRasterLayer* layer,
return result[0].mAttributes["Band 1"];
}

// private
QList<QgsMapToolIdentify::IdentifyResult>
TestQgsMapToolIdentifyAction::testIdentifyVector( QgsVectorLayer* layer, double xGeoref, double yGeoref )
{
QScopedPointer< QgsMapToolIdentifyAction > action( new QgsMapToolIdentifyAction( canvas ) );
QgsPoint mapPoint = canvas->getCoordinateTransform()->transform( xGeoref, yGeoref );
QList<QgsMapToolIdentify::IdentifyResult> result = action->identify( mapPoint.x(), mapPoint.y(), QList<QgsMapLayer*>() << layer );
return result;
}

void TestQgsMapToolIdentifyAction::identifyRasterFloat32()
{
//create a temporary layer
Expand Down Expand Up @@ -315,6 +351,31 @@ void TestQgsMapToolIdentifyAction::identifyRasterFloat64()
QCOMPARE( testIdentifyRaster( tempLayer.data(), 6.5, 0.5 ), QString( "1.2345678901234" ) );
}

void TestQgsMapToolIdentifyAction::identifyInvalidPolygons()
{
//create a temporary layer
QScopedPointer< QgsVectorLayer > memoryLayer( new QgsVectorLayer( "Polygon?field=pk:int", "vl", "memory" ) );
QVERIFY( memoryLayer->isValid() );
QgsFeature f1( memoryLayer->dataProvider()->fields(), 1 );
f1.setAttribute( "pk", 1 );
f1.setGeometry( geomFromHexWKB(
"010300000001000000030000000000000000000000000000000000000000000000000024400000000000000000000000000000244000000000000024400000000000000000"
) );
// TODO: check why we need the ->dataProvider() part, since
// there's a QgsVectorLayer::addFeatures method too
//memoryLayer->addFeatures( QgsFeatureList() << f1 );
memoryLayer->dataProvider()->addFeatures( QgsFeatureList() << f1 );

canvas->setExtent( QgsRectangle( 0, 0, 10, 10 ) );
QList<QgsMapToolIdentify::IdentifyResult> identified;
identified = testIdentifyVector( memoryLayer.data(), 4, 6 );
QCOMPARE( identified.length(), 0 );
identified = testIdentifyVector( memoryLayer.data(), 6, 4 );
QCOMPARE( identified.length(), 1 );
QCOMPARE( identified[0].mFeature.attribute( "pk" ), QVariant( 1 ) );

}


QTEST_MAIN( TestQgsMapToolIdentifyAction )
#include "testqgsmaptoolidentifyaction.moc"
Expand Down

0 comments on commit e92e7fe

Please sign in to comment.