From 3393264cee5799def7df63ace2e4b9fc0bf11850 Mon Sep 17 00:00:00 2001 From: Jacky Volpes Date: Thu, 1 Aug 2024 12:37:39 +0200 Subject: [PATCH 1/4] 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.sip.in | 2 +- .../geometry_checker/qgsfeaturepool.sip.in | 2 +- .../geometry_checker/qgsfeaturepool.cpp | 8 +- .../vector/geometry_checker/qgsfeaturepool.h | 2 +- .../testqgsgeometrychecks.cpp | 102 +++++++++++++++++- 5 files changed, 106 insertions(+), 10 deletions(-) diff --git a/python/PyQt6/analysis/auto_generated/vector/geometry_checker/qgsfeaturepool.sip.in b/python/PyQt6/analysis/auto_generated/vector/geometry_checker/qgsfeaturepool.sip.in index 2c31414de3cf..4bc203aeef4d 100644 --- a/python/PyQt6/analysis/auto_generated/vector/geometry_checker/qgsfeaturepool.sip.in +++ b/python/PyQt6/analysis/auto_generated/vector/geometry_checker/qgsfeaturepool.sip.in @@ -121,7 +121,7 @@ To be used by implementations of ``addFeature``. the original layer's thread. %End - void refreshCache( const QgsFeature &feature ); + void refreshCache( QgsFeature feature ); %Docstring Changes a feature in the cache and the spatial index. To be used by implementations of ``updateFeature``. diff --git a/python/analysis/auto_generated/vector/geometry_checker/qgsfeaturepool.sip.in b/python/analysis/auto_generated/vector/geometry_checker/qgsfeaturepool.sip.in index 2c31414de3cf..4bc203aeef4d 100644 --- a/python/analysis/auto_generated/vector/geometry_checker/qgsfeaturepool.sip.in +++ b/python/analysis/auto_generated/vector/geometry_checker/qgsfeaturepool.sip.in @@ -121,7 +121,7 @@ To be used by implementations of ``addFeature``. the original layer's thread. %End - void refreshCache( const QgsFeature &feature ); + void refreshCache( QgsFeature feature ); %Docstring Changes a feature in the cache and the spatial index. To be used by implementations of ``updateFeature``. 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() ); From 57fce5a79246d377daba607d78f16aaa2eec2e9f Mon Sep 17 00:00:00 2001 From: Jacky Volpes Date: Tue, 6 Aug 2024 12:05:29 +0200 Subject: [PATCH 2/4] fix(qgsfeaturepool): correctly remove features from spatial index --- .../geometry_checker/qgsfeaturepool.sip.in | 5 ++++- .../geometry_checker/qgsfeaturepool.sip.in | 5 ++++- .../geometry_checker/qgsfeaturepool.cpp | 4 ++-- .../vector/geometry_checker/qgsfeaturepool.h | 5 ++++- .../qgsvectordataproviderfeaturepool.cpp | 2 +- .../qgsvectorlayerfeaturepool.cpp | 18 ++++++++++----- .../testqgsvectorlayerfeaturepool.cpp | 22 ++++++------------- 7 files changed, 34 insertions(+), 27 deletions(-) diff --git a/python/PyQt6/analysis/auto_generated/vector/geometry_checker/qgsfeaturepool.sip.in b/python/PyQt6/analysis/auto_generated/vector/geometry_checker/qgsfeaturepool.sip.in index 4bc203aeef4d..a8e76b076759 100644 --- a/python/PyQt6/analysis/auto_generated/vector/geometry_checker/qgsfeaturepool.sip.in +++ b/python/PyQt6/analysis/auto_generated/vector/geometry_checker/qgsfeaturepool.sip.in @@ -121,11 +121,14 @@ To be used by implementations of ``addFeature``. the original layer's thread. %End - void refreshCache( QgsFeature feature ); + void refreshCache( QgsFeature feature, const QgsFeature origFeature ); %Docstring Changes a feature in the cache and the spatial index. To be used by implementations of ``updateFeature``. +:param feature: the new feature to put in the cache and index +:param origFeature: the original feature to remove from the index + .. note:: This method can safely be called from a different thread vs the object's creation thread or diff --git a/python/analysis/auto_generated/vector/geometry_checker/qgsfeaturepool.sip.in b/python/analysis/auto_generated/vector/geometry_checker/qgsfeaturepool.sip.in index 4bc203aeef4d..a8e76b076759 100644 --- a/python/analysis/auto_generated/vector/geometry_checker/qgsfeaturepool.sip.in +++ b/python/analysis/auto_generated/vector/geometry_checker/qgsfeaturepool.sip.in @@ -121,11 +121,14 @@ To be used by implementations of ``addFeature``. the original layer's thread. %End - void refreshCache( QgsFeature feature ); + void refreshCache( QgsFeature feature, const QgsFeature origFeature ); %Docstring Changes a feature in the cache and the spatial index. To be used by implementations of ``updateFeature``. +:param feature: the new feature to put in the cache and index +:param origFeature: the original feature to remove from the index + .. note:: This method can safely be called from a different thread vs the object's creation thread or diff --git a/src/analysis/vector/geometry_checker/qgsfeaturepool.cpp b/src/analysis/vector/geometry_checker/qgsfeaturepool.cpp index da62b5913e5c..5a53b3a6e5f6 100644 --- a/src/analysis/vector/geometry_checker/qgsfeaturepool.cpp +++ b/src/analysis/vector/geometry_checker/qgsfeaturepool.cpp @@ -131,11 +131,11 @@ void QgsFeaturePool::insertFeature( const QgsFeature &feature, bool skipLock ) mIndex.addFeature( indexFeature ); } -void QgsFeaturePool::refreshCache( QgsFeature feature ) +void QgsFeaturePool::refreshCache( QgsFeature feature, const QgsFeature origFeature ) { QgsReadWriteLocker locker( mCacheLock, QgsReadWriteLocker::Write ); mFeatureCache.insert( feature.id(), new QgsFeature( feature ) ); - mIndex.deleteFeature( feature ); + mIndex.deleteFeature( origFeature ); mIndex.addFeature( feature ); locker.unlock(); } diff --git a/src/analysis/vector/geometry_checker/qgsfeaturepool.h b/src/analysis/vector/geometry_checker/qgsfeaturepool.h index 803ce1c73010..d2fd8af17dec 100644 --- a/src/analysis/vector/geometry_checker/qgsfeaturepool.h +++ b/src/analysis/vector/geometry_checker/qgsfeaturepool.h @@ -171,10 +171,13 @@ class ANALYSIS_EXPORT QgsFeaturePool : public QgsFeatureSink SIP_ABSTRACT * Changes a feature in the cache and the spatial index. * To be used by implementations of ``updateFeature``. * + * \param feature the new feature to put in the cache and index + * \param origFeature the original feature to remove from the index + * * \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( QgsFeature feature ); + void refreshCache( QgsFeature feature, const QgsFeature origFeature ); /** * Removes a feature from the cache and the spatial index. diff --git a/src/analysis/vector/geometry_checker/qgsvectordataproviderfeaturepool.cpp b/src/analysis/vector/geometry_checker/qgsvectordataproviderfeaturepool.cpp index 7c0397a76407..403351df1209 100644 --- a/src/analysis/vector/geometry_checker/qgsvectordataproviderfeaturepool.cpp +++ b/src/analysis/vector/geometry_checker/qgsvectordataproviderfeaturepool.cpp @@ -153,7 +153,7 @@ void QgsVectorDataProviderFeaturePool::updateFeature( QgsFeature &feature ) } } ); - refreshCache( feature ); + refreshCache( feature, origFeature ); } void QgsVectorDataProviderFeaturePool::deleteFeature( QgsFeatureId fid ) diff --git a/src/analysis/vector/geometry_checker/qgsvectorlayerfeaturepool.cpp b/src/analysis/vector/geometry_checker/qgsvectorlayerfeaturepool.cpp index 88f4fde91059..3bb44aed914e 100644 --- a/src/analysis/vector/geometry_checker/qgsvectorlayerfeaturepool.cpp +++ b/src/analysis/vector/geometry_checker/qgsvectorlayerfeaturepool.cpp @@ -108,6 +108,9 @@ bool QgsVectorLayerFeaturePool::addFeatures( QgsFeatureList &features, QgsFeatur void QgsVectorLayerFeaturePool::updateFeature( QgsFeature &feature ) { + QgsFeature origFeature; + getFeature( feature.id(), origFeature ); + QgsThreadingUtils::runOnMainThread( [this, &feature]() { QgsVectorLayer *lyr = layer(); @@ -117,7 +120,7 @@ void QgsVectorLayerFeaturePool::updateFeature( QgsFeature &feature ) } } ); - refreshCache( feature ); + refreshCache( feature, origFeature ); } void QgsVectorLayerFeaturePool::deleteFeature( QgsFeatureId fid ) @@ -135,14 +138,17 @@ void QgsVectorLayerFeaturePool::deleteFeature( QgsFeatureId fid ) void QgsVectorLayerFeaturePool::onGeometryChanged( QgsFeatureId fid, const QgsGeometry &geometry ) { - Q_UNUSED( geometry ) - + QgsFeature feature; if ( isFeatureCached( fid ) ) { - QgsFeature feature; - getFeature( fid, feature ); - refreshCache( feature ); + QgsFeature origFeature; + getFeature( fid, origFeature ); // gets the cached feature + feature = origFeature; + feature.setGeometry( geometry ); + refreshCache( feature, origFeature ); } + else + getFeature( fid, feature ); // gets the feature and adds it to the cache and index } void QgsVectorLayerFeaturePool::onFeatureDeleted( QgsFeatureId fid ) diff --git a/tests/src/geometry_checker/testqgsvectorlayerfeaturepool.cpp b/tests/src/geometry_checker/testqgsvectorlayerfeaturepool.cpp index 62b8c6e742f8..f979be740200 100644 --- a/tests/src/geometry_checker/testqgsvectorlayerfeaturepool.cpp +++ b/tests/src/geometry_checker/testqgsvectorlayerfeaturepool.cpp @@ -142,29 +142,21 @@ void TestQgsVectorLayerFeaturePool::changeGeometry() feat.setGeometry( QgsGeometry::fromWkt( QStringLiteral( "Polygon((100 100, 110 100, 110 110, 100 110, 100 100))" ) ) ); vl->updateFeature( feat ); - // Still working on the cached data + // Cached data updated with geometryChanged vector layer signal const QgsFeatureIds ids3 = pool.getIntersects( QgsRectangle( 0, 0, 10, 10 ) ); - QCOMPARE( ids3.size(), 1 ); - - // Repopulate the cache - const QgsFeatureIds ids4 = pool.getFeatures( QgsFeatureRequest().setFilterRect( QgsRectangle( 0, 0, 10, 10 ) ) ); - QCOMPARE( ids4.size(), 0 ); + QCOMPARE( ids3.size(), 0 ); // Still working on the cached data - const QgsFeatureIds ids5 = pool.getIntersects( QgsRectangle( 0, 0, 10, 10 ) ); - QCOMPARE( ids5.size(), 0 ); + const QgsFeatureIds ids4 = pool.getIntersects( QgsRectangle( 0, 0, 10, 10 ) ); + QCOMPARE( ids4.size(), 0 ); // Update a feature to be inside the AOI feat.setGeometry( QgsGeometry::fromWkt( QStringLiteral( "Polygon((0 0, 10 0, 10 10, 0 10, 0 0))" ) ) ); vl->updateFeature( feat ); - // Still cached - const QgsFeatureIds ids6 = pool.getIntersects( QgsRectangle( 0, 0, 10, 10 ) ); - QCOMPARE( ids6.size(), 0 ); - - // One in there again - const QgsFeatureIds ids7 = pool.getFeatures( QgsFeatureRequest().setFilterRect( QgsRectangle( 0, 0, 10, 10 ) ) ); - QCOMPARE( ids7.size(), 1 ); + // Cached data updated with geometryChanged vector layer signal + const QgsFeatureIds ids5 = pool.getIntersects( QgsRectangle( 0, 0, 10, 10 ) ); + QCOMPARE( ids5.size(), 1 ); } std::unique_ptr TestQgsVectorLayerFeaturePool::createPopulatedLayer() From cf0da99b7780f28186060bb77dd89d4adbd124ce Mon Sep 17 00:00:00 2001 From: Jacky Volpes Date: Tue, 27 Aug 2024 11:05:20 +0200 Subject: [PATCH 3/4] fix(qgsfeaturepool): better synchronization between cache and spatial index --- .../vector/geometry_checker/qgsfeaturepool.cpp | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/src/analysis/vector/geometry_checker/qgsfeaturepool.cpp b/src/analysis/vector/geometry_checker/qgsfeaturepool.cpp index 5a53b3a6e5f6..3923251c8f82 100644 --- a/src/analysis/vector/geometry_checker/qgsfeaturepool.cpp +++ b/src/analysis/vector/geometry_checker/qgsfeaturepool.cpp @@ -66,7 +66,21 @@ bool QgsFeaturePool::getFeature( QgsFeatureId id, QgsFeature &feature ) } locker.changeMode( QgsReadWriteLocker::Write ); mFeatureCache.insert( id, new QgsFeature( feature ) ); - mIndex.addFeature( feature ); + + QgsGeometry indexGeom = mIndex.geometry( id ); + + // feature not in the index: add it + if ( indexGeom.isNull() ) + mIndex.addFeature( feature ); + + // feature already in the index but with a different geometry (it has been evicted from the cache and externally modified): + // remove the old geometry and add the new one + else if ( !mIndex.geometry( id ).equals( feature.geometry() ) ) + { + mIndex.deleteFeature( id, indexGeom.boundingBox() ); + mIndex.addFeature( feature ); + } + } return true; } From 542c10aeb51c3ff48fb91278944beb81603a0447 Mon Sep 17 00:00:00 2001 From: Jacky Volpes Date: Thu, 29 Aug 2024 10:57:17 +0200 Subject: [PATCH 4/4] fix(qgsfeaturepool): fix locker mode --- src/analysis/vector/geometry_checker/qgsfeaturepool.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/analysis/vector/geometry_checker/qgsfeaturepool.cpp b/src/analysis/vector/geometry_checker/qgsfeaturepool.cpp index 3923251c8f82..7a3f0d704884 100644 --- a/src/analysis/vector/geometry_checker/qgsfeaturepool.cpp +++ b/src/analysis/vector/geometry_checker/qgsfeaturepool.cpp @@ -49,7 +49,7 @@ bool QgsFeaturePool::getFeature( QgsFeatureId id, QgsFeature &feature ) // // https://bugreports.qt.io/browse/QTBUG-19794 - QgsReadWriteLocker locker( mCacheLock, QgsReadWriteLocker::Write ); + QgsReadWriteLocker locker( mCacheLock, QgsReadWriteLocker::Read ); QgsFeature *cachedFeature = mFeatureCache.object( id ); if ( cachedFeature ) {