Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix inconsistency between layer providers in geometry checker #58132

Closed
wants to merge 4 commits into from
Closed

Fix inconsistency between layer providers in geometry checker #58132

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3393264
fix(qgsfeaturepool): cache management - fixes #58113
Djedouas Aug 1, 2024
57fce5a
fix(qgsfeaturepool): correctly remove features from spatial index
Djedouas Aug 6, 2024
cf0da99
fix(qgsfeaturepool): better synchronization between cache and spatial…
Djedouas Aug 27, 2024
542c10a
fix(qgsfeaturepool): fix locker mode
Djedouas Aug 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion python/PyQt6/analysis/auto_generated/vector/geometry_checker/qgsfeaturepool.sip.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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``.
Expand Down
2 changes: 1 addition & 1 deletion python/analysis/auto_generated/vector/geometry_checker/qgsfeaturepool.sip.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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``.
Expand Down
8 changes: 3 additions & 5 deletions src/analysis/vector/geometry_checker/qgsfeaturepool.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 );
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at this, I'm actually unsure how this ever worked without crashes 🙃

QgsSpatialIndex requires that the ORIGINAL feature added to the index is passed. It's not operating on the feature ID alone, but rather the combination of the feature's bounding box + feature ID. So here we ask the spatial index to remove a feature which has a different bounding box from the spatial index, which will corrupt the index! 😱

::removeFeature works OK, because it's retrieving the original feature from the feature ID and then removing that from the index. We need to do similar here -- first retrieve the original feature, remove from the index, and THEN update the cache/spatial index.

But that's still fraught with potential traps -- eg the subclasses are updating the original feature first before calling refreshCache, so if the feature has been evicted from mFeatureCache already then we'll end up retrieving the UPDATED feature from the call to getFeature and the spatial index will still be corrupted.

Maybe we'll need to add a QHash< QgsFeatureId, QgsRectangle > mOriginalFeatureBounds here and use that with QgsSpatialIndex::deleteFeature( QgsFeatureId id, const QgsRectangle &bounds ) instead...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All right... I think we are getting closer to the solution... 😆

We need to do similar here -- first retrieve the original feature, remove from the index, and THEN update the cache/spatial index.

I see 😄

Here is a new commit, I think we can do without an additional class member, as we can manage to get the original feature geometry each time we need to refresh the index and give it to ::refreshCache

Also adapted QgsVectorLayerFeaturePool::onGeometryChanged and its test, because the intent of was clearly to keep the cache up to date when an external geometry change occurs.

mIndex.addFeature( feature );
locker.unlock();

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

void QgsFeaturePool::removeFeature( const QgsFeatureId featureId )
Expand Down
2 changes: 1 addition & 1 deletion src/analysis/vector/geometry_checker/qgsfeaturepool.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 );
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this change?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

QgsSpatialIndex::addFeature doesn't have const qualifier on its input feature, so I made this change to pass by copy.


/**
* Removes a feature from the cache and the spatial index.
Expand Down
102 changes: 100 additions & 2 deletions tests/src/geometry_checker/testqgsgeometrychecks.cpp
View file
Open in desktop
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