From e92e7fe472bc0b6e040461ee4f2152a5369776ee Mon Sep 17 00:00:00 2001 From: Sandro Santilli Date: Fri, 17 Jun 2016 13:28:31 +0200 Subject: [PATCH] Allow converting polygons with unclosed rings to GEOS Forces ring close on conversion, fixing a regression from 2.8 (and 2.14). See #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. --- src/core/geometry/qgsgeos.cpp | 23 ++++--- src/core/geometry/qgsgeos.h | 2 +- .../src/app/testqgsmaptoolidentifyaction.cpp | 61 +++++++++++++++++++ 3 files changed, 77 insertions(+), 9 deletions(-) diff --git a/src/core/geometry/qgsgeos.cpp b/src/core/geometry/qgsgeos.cpp index a5f66aa42fc..113fa47f82f 100644 --- a/src/core/geometry/qgsgeos.cpp +++ b/src/core/geometry/qgsgeos.cpp @@ -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( curve ); @@ -1507,10 +1507,17 @@ 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" ) ); @@ -1518,9 +1525,9 @@ GEOSCoordSequence* QgsGeos::createCoordinateSequence( const QgsCurveV2* curve, d } 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 ) @@ -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 ) @@ -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(); @@ -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; diff --git a/src/core/geometry/qgsgeos.h b/src/core/geometry/qgsgeos.h index b1adcedcd44..e0046b66ed7 100644 --- a/src/core/geometry/qgsgeos.h +++ b/src/core/geometry/qgsgeos.h @@ -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 ); diff --git a/tests/src/app/testqgsmaptoolidentifyaction.cpp b/tests/src/app/testqgsmaptoolidentifyaction.cpp index 101e430ca81..477839ec6ca 100644 --- a/tests/src/app/testqgsmaptoolidentifyaction.cpp +++ b/tests/src/app/testqgsmaptoolidentifyaction.cpp @@ -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 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() @@ -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 ) ); @@ -258,6 +284,16 @@ QString TestQgsMapToolIdentifyAction::testIdentifyRaster( QgsRasterLayer* layer, return result[0].mAttributes["Band 1"]; } +// private +QList +TestQgsMapToolIdentifyAction::testIdentifyVector( QgsVectorLayer* layer, double xGeoref, double yGeoref ) +{ + QScopedPointer< QgsMapToolIdentifyAction > action( new QgsMapToolIdentifyAction( canvas ) ); + QgsPoint mapPoint = canvas->getCoordinateTransform()->transform( xGeoref, yGeoref ); + QList result = action->identify( mapPoint.x(), mapPoint.y(), QList() << layer ); + return result; +} + void TestQgsMapToolIdentifyAction::identifyRasterFloat32() { //create a temporary layer @@ -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 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"