From e398577a2353816c7421bdc5d09a4a463f726c90 Mon Sep 17 00:00:00 2001 From: Jacky Volpes Date: Thu, 1 Aug 2024 12:37:39 +0200 Subject: [PATCH] fix(qgsfeaturepool): cache management - fixes #58113 Geometry checker cache does not work properly with memory layers. refreshCache now directly adds the geometry in the feature pool cache. --- .../geometry_checker/qgsfeaturepool.cpp | 8 +- .../vector/geometry_checker/qgsfeaturepool.h | 2 +- .../testqgsgeometrychecks.cpp | 102 +++++++++++++++++- 3 files changed, 104 insertions(+), 8 deletions(-) diff --git a/src/analysis/vector/geometry_checker/qgsfeaturepool.cpp b/src/analysis/vector/geometry_checker/qgsfeaturepool.cpp index ada95fdddaea..da62b5913e5c 100644 --- a/src/analysis/vector/geometry_checker/qgsfeaturepool.cpp +++ b/src/analysis/vector/geometry_checker/qgsfeaturepool.cpp @@ -131,15 +131,13 @@ void QgsFeaturePool::insertFeature( const QgsFeature &feature, bool skipLock ) mIndex.addFeature( indexFeature ); } -void QgsFeaturePool::refreshCache( const QgsFeature &feature ) +void QgsFeaturePool::refreshCache( QgsFeature feature ) { QgsReadWriteLocker locker( mCacheLock, QgsReadWriteLocker::Write ); - mFeatureCache.remove( feature.id() ); + mFeatureCache.insert( feature.id(), new QgsFeature( feature ) ); mIndex.deleteFeature( feature ); + mIndex.addFeature( feature ); locker.unlock(); - - QgsFeature tempFeature; - getFeature( feature.id(), tempFeature ); } void QgsFeaturePool::removeFeature( const QgsFeatureId featureId ) diff --git a/src/analysis/vector/geometry_checker/qgsfeaturepool.h b/src/analysis/vector/geometry_checker/qgsfeaturepool.h index e2dff164be43..803ce1c73010 100644 --- a/src/analysis/vector/geometry_checker/qgsfeaturepool.h +++ b/src/analysis/vector/geometry_checker/qgsfeaturepool.h @@ -174,7 +174,7 @@ class ANALYSIS_EXPORT QgsFeaturePool : public QgsFeatureSink SIP_ABSTRACT * \note This method can safely be called from a different thread vs the object's creation thread or * the original layer's thread. */ - void refreshCache( const QgsFeature &feature ); + void refreshCache( QgsFeature feature ); /** * Removes a feature from the cache and the spatial index. 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() );