Skip to content

Commit

Permalink
fix(featurepool): cache management - fixes #58113
Browse files Browse the repository at this point in the history 
Geometry checker cache does not work properly with memory layers.
refreshCache now handles a list of updated features to be thread-safe.

Also, fixes a locker mode, and correctly remove features from spatial
index.
  • Loading branch information
@Djedouas @lbartoletti
Djedouas authored and lbartoletti committed Nov 28, 2024
1 parent da86bc1 commit bd49d6c
Show file tree
Hide file tree
Showing 8 changed files with 208 additions and 37 deletions.
7 changes: 6 additions & 1 deletion python/PyQt6/analysis/auto_generated/vector/geometry_checker/qgsfeaturepool.sip.in
View file
Original file line number Diff line number Diff line change
Expand Up @@ -121,15 +121,20 @@ To be used by implementations of ``addFeature``.
the original layer's thread.
%End

void refreshCache( const 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
the original layer's thread.

.. versionadded:: 3.42
%End

void removeFeature( const QgsFeatureId featureId );
Expand Down
7 changes: 6 additions & 1 deletion python/analysis/auto_generated/vector/geometry_checker/qgsfeaturepool.sip.in
View file
Original file line number Diff line number Diff line change
Expand Up @@ -121,15 +121,20 @@ To be used by implementations of ``addFeature``.
the original layer's thread.
%End

void refreshCache( const 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
the original layer's thread.

.. versionadded:: 3.42
%End

void removeFeature( const QgsFeatureId featureId );
Expand Down
28 changes: 21 additions & 7 deletions src/analysis/vector/geometry_checker/qgsfeaturepool.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,18 @@ bool QgsFeaturePool::getFeature( QgsFeatureId id, QgsFeature &feature )
//
// https://bugreports.qt.io/browse/QTBUG-19794

QgsReadWriteLocker locker( mCacheLock, QgsReadWriteLocker::Write );
// If the feature we want is amongst the features that have been updated,
// then get it from the dedicated hash.
// It would not be thread-safe to get it directly from the layer,
// and it could be outdated in the feature source (in case of a memory layer),
// and it could have been cleared from the cache due to a cache overflow.
if ( mUpdatedFeatures.contains( id ) )
{
feature = mUpdatedFeatures[id];
return true;
}

QgsReadWriteLocker locker( mCacheLock, QgsReadWriteLocker::Read );
QgsFeature *cachedFeature = mFeatureCache.object( id );
if ( cachedFeature )
{
Expand Down Expand Up @@ -78,6 +89,7 @@ QgsFeatureIds QgsFeaturePool::getFeatures( const QgsFeatureRequest &request, Qgs
Q_ASSERT( mLayer );
Q_ASSERT( QThread::currentThread() == mLayer->thread() );

mUpdatedFeatures.clear();
mFeatureCache.clear();
mIndex = QgsSpatialIndex();

Expand Down Expand Up @@ -131,19 +143,21 @@ void QgsFeaturePool::insertFeature( const QgsFeature &feature, bool skipLock )
mIndex.addFeature( indexFeature );
}

void QgsFeaturePool::refreshCache( const QgsFeature &feature )
void QgsFeaturePool::refreshCache( QgsFeature feature, const QgsFeature &origFeature )
{
// insert/refresh the updated features as well
mUpdatedFeatures.insert( feature.id(), feature );

QgsReadWriteLocker locker( mCacheLock, QgsReadWriteLocker::Write );
mFeatureCache.remove( feature.id() );
mIndex.deleteFeature( feature );
mFeatureCache.insert( feature.id(), new QgsFeature( feature ) );
mIndex.deleteFeature( origFeature );
mIndex.addFeature( feature );
locker.unlock();

QgsFeature tempFeature;
getFeature( feature.id(), tempFeature );
}

void QgsFeaturePool::removeFeature( const QgsFeatureId featureId )
{
mUpdatedFeatures.remove( featureId );
QgsFeature origFeature;
QgsReadWriteLocker locker( mCacheLock, QgsReadWriteLocker::Unlocked );
if ( getFeature( featureId, origFeature ) )
Expand Down
20 changes: 19 additions & 1 deletion src/analysis/vector/geometry_checker/qgsfeaturepool.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -171,10 +171,14 @@ 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.
* \since QGIS 3.42
*/
void refreshCache( const QgsFeature &feature );
void refreshCache( QgsFeature feature, const QgsFeature &origFeature );

/**
* Removes a feature from the cache and the spatial index.
Expand Down Expand Up @@ -219,6 +223,20 @@ class ANALYSIS_EXPORT QgsFeaturePool : public QgsFeatureSink SIP_ABSTRACT
QString mLayerId;
QString mLayerName;
QgsCoordinateReferenceSystem mCrs;

/**
* Hash containing features whose geometry has been updated
* but which are not accessible if we get them
* from the layer (not thread-safe).
*
* The philosophy is that as soon as a feature geometry has
* changed, it will be in this hash, and the cache will not
* be used any more.
* \since QGIS 3.42
*/
QHash<QgsFeatureId, QgsFeature> mUpdatedFeatures;

friend class TestQgsVectorLayerFeaturePool;
};

#endif // QGS_FEATUREPOOL_H
2 changes: 1 addition & 1 deletion src/analysis/vector/geometry_checker/qgsvectordataproviderfeaturepool.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,7 @@ void QgsVectorDataProviderFeaturePool::updateFeature( QgsFeature &feature )
}
} );

refreshCache( feature );
refreshCache( feature, origFeature );
}

void QgsVectorDataProviderFeaturePool::deleteFeature( QgsFeatureId fid )
Expand Down
19 changes: 10 additions & 9 deletions src/analysis/vector/geometry_checker/qgsvectorlayerfeaturepool.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,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();
Expand All @@ -118,7 +121,7 @@ void QgsVectorLayerFeaturePool::updateFeature( QgsFeature &feature )
}
} );

refreshCache( feature );
refreshCache( feature, origFeature );
}

void QgsVectorLayerFeaturePool::deleteFeature( QgsFeatureId fid )
Expand All @@ -136,14 +139,12 @@ void QgsVectorLayerFeaturePool::deleteFeature( QgsFeatureId fid )

void QgsVectorLayerFeaturePool::onGeometryChanged( QgsFeatureId fid, const QgsGeometry &geometry )
{
Q_UNUSED( geometry )

if ( isFeatureCached( fid ) )
{
QgsFeature feature;
getFeature( fid, feature );
refreshCache( feature );
}
QgsFeature feature;
QgsFeature origFeature;
getFeature( fid, origFeature );
feature = origFeature;
feature.setGeometry( geometry );
refreshCache( feature, origFeature );
}

void QgsVectorLayerFeaturePool::onFeatureDeleted( QgsFeatureId fid )
Expand Down
102 changes: 100 additions & 2 deletions tests/src/geometry_checker/testqgsgeometrychecks.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<QgsGeometryCheckContext *, QMap<QString, QgsFeaturePool *> > createTestContext( QTemporaryDir &tempDir, QMap<QString, QString> &layers, const QgsCoordinateReferenceSystem &mapCrs = QgsCoordinateReferenceSystem( "EPSG:4326" ), double prec = 8 ) const;
QPair<QgsGeometryCheckContext *, QMap<QString, QgsFeaturePool *> > createTestContext( QTemporaryDir &tempDir, QMap<QString, QString> &layers, const QgsCoordinateReferenceSystem &mapCrs = QgsCoordinateReferenceSystem( "EPSG:4326" ), int prec = 8 ) const;
QPair<QgsGeometryCheckContext *, QMap<QString, QgsFeaturePool *> > createMemoryTestContext( QMap<QString, QString> &layers, const QgsCoordinateReferenceSystem &mapCrs = QgsCoordinateReferenceSystem( "EPSG:4326" ), int prec = 8 ) const;
void cleanupTestContext( QPair<QgsGeometryCheckContext *, QMap<QString, QgsFeaturePool *> > ctx ) const;
void listErrors( const QList<QgsGeometryCheckError *> &checkErrors, const QStringList &messages ) const;
QList<QgsGeometryCheckError *> searchCheckErrors( const QList<QgsGeometryCheckError *> &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;
Expand All @@ -78,6 +79,7 @@ class TestQgsGeometryChecks: public QObject

private slots:
void testAngleCheck();
void testAngleCheckMemoryLayers();
void testAreaCheck();
void testContainedCheck();
void testDangleCheck();
Expand Down Expand Up @@ -117,6 +119,85 @@ void TestQgsGeometryChecks::cleanupTestCase()

///////////////////////////////////////////////////////////////////////////////

void TestQgsGeometryChecks::testAngleCheckMemoryLayers()
{
QTemporaryDir dir;
QMap<QString, QString> layers;
layers.insert( "point_layer.shp", "" );
layers.insert( "line_layer.shp", "" );
layers.insert( "polygon_layer.shp", "" );
auto testContext = createMemoryTestContext( layers );

// Test detection
QList<QgsGeometryCheckError *> 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<QgsGeometryCheckError *> errs1;
QList<QgsGeometryCheckError *> 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;
Expand Down Expand Up @@ -1328,7 +1409,24 @@ QgsFeaturePool *TestQgsGeometryChecks::createFeaturePool( QgsVectorLayer *layer,
return new QgsVectorDataProviderFeaturePool( layer, selectedOnly );
}

QPair<QgsGeometryCheckContext *, QMap<QString, QgsFeaturePool *> > TestQgsGeometryChecks::createTestContext( QTemporaryDir &tempDir, QMap<QString, QString> &layers, const QgsCoordinateReferenceSystem &mapCrs, double prec ) const
QPair<QgsGeometryCheckContext *, QMap<QString, QgsFeaturePool *> > TestQgsGeometryChecks::createMemoryTestContext( QMap<QString, QString> &layers, const QgsCoordinateReferenceSystem &mapCrs, int prec ) const
{
const QDir testDataDir( QDir( TEST_DATA_DIR ).absoluteFilePath( "geometry_checker" ) );

QMap<QString, QgsFeaturePool *> 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<QgsGeometryCheckContext *, QMap<QString, QgsFeaturePool *> > TestQgsGeometryChecks::createTestContext( QTemporaryDir &tempDir, QMap<QString, QString> &layers, const QgsCoordinateReferenceSystem &mapCrs, int prec ) const
{
const QDir testDataDir( QDir( TEST_DATA_DIR ).absoluteFilePath( "geometry_checker" ) );
const QDir tmpDir( tempDir.path() );
Expand Down
Loading

0 comments on commit bd49d6c

Please sign in to comment.