diff --git a/src/analysis/vector/geometry_checker/qgsfeaturepool.cpp b/src/analysis/vector/geometry_checker/qgsfeaturepool.cpp index 9bb43091a6b0..f0e16be1f944 100644 --- a/src/analysis/vector/geometry_checker/qgsfeaturepool.cpp +++ b/src/analysis/vector/geometry_checker/qgsfeaturepool.cpp @@ -33,6 +33,7 @@ QgsFeaturePool::QgsFeaturePool( QgsVectorLayer *layer ) , mGeometryType( layer->geometryType() ) , mFeatureSource( std::make_unique( layer ) ) , mLayerName( layer->name() ) + , mIsMemoryLayer( layer->dataProvider()->name() == QStringLiteral( "memory" ) ) { } @@ -47,6 +48,17 @@ bool QgsFeaturePool::getFeature( QgsFeatureId id, QgsFeature &feature ) // // https://bugreports.qt.io/browse/QTBUG-19794 + // a memory layer is already a cache itself. retrieve feature from the feature source. + if ( mIsMemoryLayer ) + { + if ( !mLayer->getFeatures( QgsFeatureRequest( id ) ).nextFeature( feature ) ) + return false; + + mIndex.deleteFeature( feature ); // remove the feature from the index if any + mIndex.addFeature( feature ); // add the feature back to the index to have exactly one occurrence + return true; + } + QgsReadWriteLocker locker( mCacheLock, QgsReadWriteLocker::Write ); QgsFeature *cachedFeature = mFeatureCache.object( id ); if ( cachedFeature ) @@ -71,18 +83,21 @@ bool QgsFeaturePool::getFeature( QgsFeatureId id, QgsFeature &feature ) QgsFeatureIds QgsFeaturePool::getFeatures( const QgsFeatureRequest &request, QgsFeedback *feedback ) { - QgsReadWriteLocker( mCacheLock, QgsReadWriteLocker::Write ); Q_UNUSED( feedback ) Q_ASSERT( QThread::currentThread() == qApp->thread() ); - - mFeatureCache.clear(); mIndex = QgsSpatialIndex(); QgsFeatureIds fids; - - mFeatureSource = std::make_unique( mLayer ); - - QgsFeatureIterator it = mFeatureSource->getFeatures( request ); + QgsFeatureIterator it; + if ( mIsMemoryLayer ) + it = mLayer->getFeatures( request ); + else + { + const QgsReadWriteLocker locker( mCacheLock, QgsReadWriteLocker::Write ); + mFeatureCache.clear(); + mFeatureSource = std::make_unique( mLayer ); + it = mFeatureSource->getFeatures( request ); + } QgsFeature feature; while ( it.nextFeature( feature ) ) { @@ -119,16 +134,22 @@ QPointer QgsFeaturePool::layerPtr() const void QgsFeaturePool::insertFeature( const QgsFeature &feature, bool skipLock ) { - QgsReadWriteLocker locker( mCacheLock, QgsReadWriteLocker::Unlocked ); - if ( !skipLock ) - locker.changeMode( QgsReadWriteLocker::Write ); - mFeatureCache.insert( feature.id(), new QgsFeature( feature ) ); + if ( !mIsMemoryLayer ) + { + QgsReadWriteLocker locker( mCacheLock, QgsReadWriteLocker::Unlocked ); + if ( !skipLock ) + locker.changeMode( QgsReadWriteLocker::Write ); + mFeatureCache.insert( feature.id(), new QgsFeature( feature ) ); + } QgsFeature indexFeature( feature ); mIndex.addFeature( indexFeature ); } void QgsFeaturePool::refreshCache( const QgsFeature &feature ) { + if ( mIsMemoryLayer ) + return; + QgsReadWriteLocker locker( mCacheLock, QgsReadWriteLocker::Write ); mFeatureCache.remove( feature.id() ); mIndex.deleteFeature( feature ); @@ -147,8 +168,12 @@ void QgsFeaturePool::removeFeature( const QgsFeatureId featureId ) locker.changeMode( QgsReadWriteLocker::Write ); mIndex.deleteFeature( origFeature ); } - locker.changeMode( QgsReadWriteLocker::Write ); - mFeatureCache.remove( origFeature.id() ); + + if ( !mIsMemoryLayer ) + { + locker.changeMode( QgsReadWriteLocker::Write ); + mFeatureCache.remove( origFeature.id() ); + } } void QgsFeaturePool::setFeatureIds( const QgsFeatureIds &ids ) @@ -169,7 +194,7 @@ QString QgsFeaturePool::layerName() const QgsCoordinateReferenceSystem QgsFeaturePool::crs() const { - QgsReadWriteLocker( mCacheLock, QgsReadWriteLocker::Read ); + const QgsReadWriteLocker locker( mCacheLock, QgsReadWriteLocker::Read ); return mFeatureSource->crs(); } @@ -180,6 +205,6 @@ Qgis::GeometryType QgsFeaturePool::geometryType() const QString QgsFeaturePool::layerId() const { - QgsReadWriteLocker( mCacheLock, QgsReadWriteLocker::Read ); + const QgsReadWriteLocker locker( mCacheLock, QgsReadWriteLocker::Read ); return mFeatureSource->id(); } diff --git a/src/analysis/vector/geometry_checker/qgsfeaturepool.h b/src/analysis/vector/geometry_checker/qgsfeaturepool.h index 21fe0e969b96..496873995b2c 100644 --- a/src/analysis/vector/geometry_checker/qgsfeaturepool.h +++ b/src/analysis/vector/geometry_checker/qgsfeaturepool.h @@ -185,6 +185,11 @@ class ANALYSIS_EXPORT QgsFeaturePool : public QgsFeatureSink SIP_ABSTRACT Qgis::GeometryType mGeometryType; std::unique_ptr mFeatureSource; QString mLayerName; + + // A memory layer is a cache by construction. + // We need to handle this specific type of layer without + // using mFeatureCache and mFeatureSource for getting features. + const bool mIsMemoryLayer; }; #endif // QGS_FEATUREPOOL_H diff --git a/tests/src/geometry_checker/testqgsgeometrychecks.cpp b/tests/src/geometry_checker/testqgsgeometrychecks.cpp index 713bc18463ab..927684f01525 100644 --- a/tests/src/geometry_checker/testqgsgeometrychecks.cpp +++ b/tests/src/geometry_checker/testqgsgeometrychecks.cpp @@ -63,7 +63,8 @@ class TestQgsGeometryChecks: public QObject }; double layerToMapUnits( const QgsMapLayer *layer, const QgsCoordinateReferenceSystem &mapCrs ) const; QgsFeaturePool *createFeaturePool( QgsVectorLayer *layer, bool selectedOnly = false ) const; - QPair > createTestContext( QTemporaryDir &tempDir, QMap &layers, const QgsCoordinateReferenceSystem &mapCrs = QgsCoordinateReferenceSystem( "EPSG:4326" ), double prec = 8 ) const; + QPair > createTestContext( QTemporaryDir &tempDir, QMap &layers, const QgsCoordinateReferenceSystem &mapCrs = QgsCoordinateReferenceSystem( "EPSG:4326" ), int prec = 8 ) const; + QPair > createMemoryTestContext( QMap &layers, const QgsCoordinateReferenceSystem &mapCrs = QgsCoordinateReferenceSystem( "EPSG:4326" ), int prec = 8 ) const; void cleanupTestContext( QPair > ctx ) const; void listErrors( const QList &checkErrors, const QStringList &messages ) const; QList searchCheckErrors( const QList &checkErrors, const QString &layerId, const QgsFeatureId &featureId = -1, const QgsPointXY &pos = QgsPointXY(), const QgsVertexId &vid = QgsVertexId(), const QVariant &value = QVariant(), double tol = 1E-4 ) const; @@ -78,6 +79,7 @@ class TestQgsGeometryChecks: public QObject private slots: void testAngleCheck(); + void testAngleCheckMemoryLayers(); void testAreaCheck(); void testContainedCheck(); void testDangleCheck(); @@ -117,6 +119,85 @@ void TestQgsGeometryChecks::cleanupTestCase() /////////////////////////////////////////////////////////////////////////////// +void TestQgsGeometryChecks::testAngleCheckMemoryLayers() +{ + QTemporaryDir dir; + QMap layers; + layers.insert( "point_layer.shp", "" ); + layers.insert( "line_layer.shp", "" ); + layers.insert( "polygon_layer.shp", "" ); + auto testContext = createMemoryTestContext( layers ); + + // Test detection + QList checkErrors; + QStringList messages; + + QVariantMap configuration; + configuration.insert( "minAngle", 15 ); + + const QgsGeometryAngleCheck check( testContext.first, configuration ); + QgsFeedback feedback; + check.collectErrors( testContext.second, checkErrors, messages, &feedback ); + listErrors( checkErrors, messages ); + + QList errs1; + QList errs2; + + QCOMPARE( checkErrors.size(), 8 ); + QVERIFY( searchCheckErrors( checkErrors, layers["point_layer.shp"] ).isEmpty() ); + QVERIFY( ( errs1 = searchCheckErrors( checkErrors, layers["line_layer.shp"], 1, QgsPointXY( -0.2225, 0.5526 ), QgsVertexId( 0, 0, 3 ), 10.5865 ) ).size() == 1 ); + QVERIFY( searchCheckErrors( checkErrors, layers["line_layer.shp"], 1, QgsPointXY( -0.94996, 0.99967 ), QgsVertexId( 1, 0, 1 ), 8.3161 ).size() == 1 ); + QVERIFY( searchCheckErrors( checkErrors, layers["line_layer.shp"], 3, QgsPointXY( -0.4547, -0.3059 ), QgsVertexId( 0, 0, 1 ), 5.4165 ).size() == 1 ); + QVERIFY( searchCheckErrors( checkErrors, layers["line_layer.shp"], 3, QgsPointXY( -0.7594, -0.1971 ), QgsVertexId( 0, 0, 2 ), 12.5288 ).size() == 1 ); + QVERIFY( ( errs2 = searchCheckErrors( checkErrors, layers["polygon_layer.shp"], 1, QgsPointXY( 0.2402, 1.0786 ), QgsVertexId( 0, 0, 1 ), 13.5140 ) ).size() == 1 ); + QVERIFY( searchCheckErrors( checkErrors, layers["polygon_layer.shp"], 2, QgsPointXY( 0.6960, 0.5908 ), QgsVertexId( 0, 0, 0 ), 7.0556 ).size() == 1 ); + QVERIFY( searchCheckErrors( checkErrors, layers["polygon_layer.shp"], 2, QgsPointXY( 0.98690, 0.55699 ), QgsVertexId( 1, 0, 5 ), 7.7351 ).size() == 1 ); + QVERIFY( searchCheckErrors( checkErrors, layers["polygon_layer.shp"], 12, QgsPointXY( -0.3186, 1.6734 ), QgsVertexId( 0, 0, 1 ), 3.5092 ).size() == 1 ); + + // Test fixes + QgsFeature f; + int n1, n2; + + testContext.second[errs1[0]->layerId()]->getFeature( errs1[0]->featureId(), f ); + n1 = f.geometry().constGet()->vertexCount( errs1[0]->vidx().part, errs1[0]->vidx().ring ); + QVERIFY( fixCheckError( testContext.second, errs1[0], + QgsGeometryAngleCheck::DeleteNode, QgsGeometryCheckError::StatusFixed, + {{errs1[0]->layerId(), errs1[0]->featureId(), QgsGeometryCheck::ChangeNode, QgsGeometryCheck::ChangeRemoved, errs1[0]->vidx()}} ) ); + testContext.second[errs1[0]->layerId()]->getFeature( errs1[0]->featureId(), f ); + n2 = f.geometry().constGet()->vertexCount( errs1[0]->vidx().part, errs1[0]->vidx().ring ); + QCOMPARE( n1, n2 + 1 ); + + testContext.second[errs2[0]->layerId()]->getFeature( errs2[0]->featureId(), f ); + n1 = f.geometry().constGet()->vertexCount( errs2[0]->vidx().part, errs2[0]->vidx().ring ); + QVERIFY( fixCheckError( testContext.second, errs2[0], + QgsGeometryAngleCheck::DeleteNode, QgsGeometryCheckError::StatusFixed, + {{errs2[0]->layerId(), errs2[0]->featureId(), QgsGeometryCheck::ChangeNode, QgsGeometryCheck::ChangeRemoved, errs2[0]->vidx()}} ) ); + testContext.second[errs2[0]->layerId()]->getFeature( errs2[0]->featureId(), f ); + n2 = f.geometry().constGet()->vertexCount( errs2[0]->vidx().part, errs2[0]->vidx().ring ); + QCOMPARE( n1, n2 + 1 ); + + // Test change tracking + QVERIFY( !errs2[0]->handleChanges( change2changes( {errs2[0]->layerId(), errs2[0]->featureId(), QgsGeometryCheck::ChangeFeature, QgsGeometryCheck::ChangeRemoved, QgsVertexId()} ) ) ); + QVERIFY( !errs2[0]->handleChanges( change2changes( {errs2[0]->layerId(), errs2[0]->featureId(), QgsGeometryCheck::ChangeFeature, QgsGeometryCheck::ChangeChanged, QgsVertexId()} ) ) ); + QVERIFY( !errs2[0]->handleChanges( change2changes( {errs2[0]->layerId(), errs2[0]->featureId(), QgsGeometryCheck::ChangePart, QgsGeometryCheck::ChangeRemoved, QgsVertexId( errs2[0]->vidx().part )} ) ) ); + QVERIFY( !errs2[0]->handleChanges( change2changes( {errs2[0]->layerId(), errs2[0]->featureId(), QgsGeometryCheck::ChangePart, QgsGeometryCheck::ChangeChanged, QgsVertexId( errs2[0]->vidx().part )} ) ) ); + QVERIFY( errs2[0]->handleChanges( change2changes( {errs2[0]->layerId(), errs2[0]->featureId(), QgsGeometryCheck::ChangePart, QgsGeometryCheck::ChangeRemoved, QgsVertexId( errs2[0]->vidx().part + 1 )} ) ) ); + QVERIFY( errs2[0]->handleChanges( change2changes( {errs2[0]->layerId(), errs2[0]->featureId(), QgsGeometryCheck::ChangePart, QgsGeometryCheck::ChangeChanged, QgsVertexId( errs2[0]->vidx().part + 1 )} ) ) ); + QVERIFY( !errs2[0]->handleChanges( change2changes( {errs2[0]->layerId(), errs2[0]->featureId(), QgsGeometryCheck::ChangeRing, QgsGeometryCheck::ChangeRemoved, QgsVertexId( errs2[0]->vidx().part, errs2[0]->vidx().ring )} ) ) ); + QVERIFY( !errs2[0]->handleChanges( change2changes( {errs2[0]->layerId(), errs2[0]->featureId(), QgsGeometryCheck::ChangeRing, QgsGeometryCheck::ChangeChanged, QgsVertexId( errs2[0]->vidx().part, errs2[0]->vidx().ring )} ) ) ); + QVERIFY( errs2[0]->handleChanges( change2changes( {errs2[0]->layerId(), errs2[0]->featureId(), QgsGeometryCheck::ChangeRing, QgsGeometryCheck::ChangeRemoved, QgsVertexId( errs2[0]->vidx().part, errs2[0]->vidx().ring + 1 )} ) ) ); + QVERIFY( errs2[0]->handleChanges( change2changes( {errs2[0]->layerId(), errs2[0]->featureId(), QgsGeometryCheck::ChangeRing, QgsGeometryCheck::ChangeChanged, QgsVertexId( errs2[0]->vidx().part, errs2[0]->vidx().ring + 1 )} ) ) ); + QVERIFY( !errs2[0]->handleChanges( change2changes( {errs2[0]->layerId(), errs2[0]->featureId(), QgsGeometryCheck::ChangeNode, QgsGeometryCheck::ChangeRemoved, errs2[0]->vidx()} ) ) ); + QVERIFY( !errs2[0]->handleChanges( change2changes( {errs2[0]->layerId(), errs2[0]->featureId(), QgsGeometryCheck::ChangeNode, QgsGeometryCheck::ChangeChanged, errs2[0]->vidx()} ) ) ); + QVERIFY( errs2[0]->handleChanges( change2changes( {errs2[0]->layerId(), errs2[0]->featureId(), QgsGeometryCheck::ChangeNode, QgsGeometryCheck::ChangeRemoved, QgsVertexId( errs2[0]->vidx().part, errs2[0]->vidx().ring, errs2[0]->vidx().vertex + 1 )} ) ) ); + QVERIFY( errs2[0]->handleChanges( change2changes( {errs2[0]->layerId(), errs2[0]->featureId(), QgsGeometryCheck::ChangeNode, QgsGeometryCheck::ChangeChanged, QgsVertexId( errs2[0]->vidx().part, errs2[0]->vidx().ring, errs2[0]->vidx().vertex + 1 )} ) ) ); + const QgsVertexId oldVidx = errs2[0]->vidx(); + QVERIFY( errs2[0]->handleChanges( change2changes( {errs2[0]->layerId(), errs2[0]->featureId(), QgsGeometryCheck::ChangeNode, QgsGeometryCheck::ChangeRemoved, QgsVertexId( errs2[0]->vidx().part, errs2[0]->vidx().ring, errs2[0]->vidx().vertex - 1 )} ) ) ); + QVERIFY( errs2[0]->vidx().vertex == oldVidx.vertex - 1 ); + + cleanupTestContext( testContext ); +} + void TestQgsGeometryChecks::testAngleCheck() { QTemporaryDir dir; @@ -1328,7 +1409,24 @@ QgsFeaturePool *TestQgsGeometryChecks::createFeaturePool( QgsVectorLayer *layer, return new QgsVectorDataProviderFeaturePool( layer, selectedOnly ); } -QPair > TestQgsGeometryChecks::createTestContext( QTemporaryDir &tempDir, QMap &layers, const QgsCoordinateReferenceSystem &mapCrs, double prec ) const +QPair > TestQgsGeometryChecks::createMemoryTestContext( QMap &layers, const QgsCoordinateReferenceSystem &mapCrs, int prec ) const +{ + const QDir testDataDir( QDir( TEST_DATA_DIR ).absoluteFilePath( "geometry_checker" ) ); + + QMap featurePools; + for ( auto it = layers.begin(); it != layers.end(); it++ ) + { + const QString layerFile = it.key(); + QgsVectorLayer *layer = ( new QgsVectorLayer( testDataDir.absoluteFilePath( layerFile ), layerFile ) )->materialize( QgsFeatureRequest() ); + Q_ASSERT( layer && layer->isValid() ); + layers[layerFile] = layer->id(); + layer->dataProvider()->enterUpdateMode(); + featurePools.insert( layer->id(), createFeaturePool( layer ) ); + } + return qMakePair( new QgsGeometryCheckContext( prec, mapCrs, QgsProject::instance()->transformContext(), QgsProject::instance() ), featurePools ); +} + +QPair > TestQgsGeometryChecks::createTestContext( QTemporaryDir &tempDir, QMap &layers, const QgsCoordinateReferenceSystem &mapCrs, int prec ) const { const QDir testDataDir( QDir( TEST_DATA_DIR ).absoluteFilePath( "geometry_checker" ) ); const QDir tmpDir( tempDir.path() );