Skip to content

Commit

Permalink
fix(qgsfeaturepool): cache management - fixes #58113
Browse files Browse the repository at this point in the history 
Geometry checker does not work with memory layers. We change
how QgsFeaturePool class works with memory layers to avoid
creating a "double-cached" system, as memory layers are already
a cache layer by construction.
  • Loading branch information
@Djedouas
Djedouas committed Jul 18, 2024
1 parent 3caa616 commit c75787d
Show file tree
Hide file tree
Showing 3 changed files with 145 additions and 17 deletions.
55 changes: 40 additions & 15 deletions src/analysis/vector/geometry_checker/qgsfeaturepool.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ QgsFeaturePool::QgsFeaturePool( QgsVectorLayer *layer )
, mGeometryType( layer->geometryType() )
, mFeatureSource( std::make_unique<QgsVectorLayerFeatureSource>( layer ) )
, mLayerName( layer->name() )
, mIsMemoryLayer( layer->dataProvider()->name() == QStringLiteral( "memory" ) )
{

}
Expand All @@ -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 )
Expand All @@ -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<QgsVectorLayerFeatureSource>( 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<QgsVectorLayerFeatureSource>( mLayer );
it = mFeatureSource->getFeatures( request );
}
QgsFeature feature;
while ( it.nextFeature( feature ) )
{
Expand Down Expand Up @@ -119,16 +134,22 @@ QPointer<QgsVectorLayer> 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 );
Expand All @@ -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 )
Expand All @@ -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();
}

Expand All @@ -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();
}
5 changes: 5 additions & 0 deletions src/analysis/vector/geometry_checker/qgsfeaturepool.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -185,6 +185,11 @@ class ANALYSIS_EXPORT QgsFeaturePool : public QgsFeatureSink SIP_ABSTRACT
Qgis::GeometryType mGeometryType;
std::unique_ptr<QgsVectorLayerFeatureSource> 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
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

0 comments on commit c75787d

Please sign in to comment.