Skip to content

Commit dcc92de

Browse files
m-kuhnm-kuhn
authored
Merge pull request #9316 from m-kuhn/fix-geometry-validation-crashes
[geometry validation] Stability and performance improvements
2 parents 77f2b60 + 3efd4a8 commit dcc92de

8 files changed

+40
-24
lines changed

‎python/analysis/auto_generated/vector/geometry_checker/qgsgeometrycheckerutils.sip.in

+2-2
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,7 @@ If ``useMapCrs`` is ``True``, geometries will be reprojected to the mapCrs defin
5151
in ``context``.
5252
%End
5353

54-
const QgsFeature &feature() const;
54+
QgsFeature feature() const;
5555
%Docstring
5656
Returns the feature.
5757
The geometry will not be reprojected regardless of useMapCrs.
@@ -63,7 +63,7 @@ The geometry will not be reprojected regardless of useMapCrs.
6363
The layer id.
6464
%End
6565

66-
const QgsGeometry &geometry() const;
66+
QgsGeometry geometry() const;
6767
%Docstring
6868
Returns the geometry of this feature.
6969
If useMapCrs was specified, it will already be reprojected into the

‎src/analysis/vector/geometry_checker/qgsgeometrycheckerror.cpp

+2-1
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,8 @@ QgsGeometryCheckError::QgsGeometryCheckError( const QgsGeometryCheck *check,
5353
{
5454
if ( vidx.part != -1 )
5555
{
56-
mGeometry = QgsGeometry( QgsGeometryCheckerUtils::getGeomPart( layerFeature.geometry().constGet(), vidx.part )->clone() );
56+
const QgsGeometry geom = layerFeature.geometry();
57+
mGeometry = QgsGeometry( QgsGeometryCheckerUtils::getGeomPart( geom.constGet(), vidx.part )->clone() );
5758
}
5859
else
5960
{

‎src/analysis/vector/geometry_checker/qgsgeometrycheckerutils.cpp

+2-2
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,7 @@ QgsGeometryCheckerUtils::LayerFeature::LayerFeature( const QgsFeaturePool *pool,
5252
}
5353
}
5454

55-
const QgsFeature &QgsGeometryCheckerUtils::LayerFeature::feature() const
55+
QgsFeature QgsGeometryCheckerUtils::LayerFeature::feature() const
5656
{
5757
return mFeature;
5858
}
@@ -67,7 +67,7 @@ QString QgsGeometryCheckerUtils::LayerFeature::layerId() const
6767
return mFeaturePool->layerId();
6868
}
6969

70-
const QgsGeometry &QgsGeometryCheckerUtils::LayerFeature::geometry() const
70+
QgsGeometry QgsGeometryCheckerUtils::LayerFeature::geometry() const
7171
{
7272
return mGeometry;
7373
}

‎src/analysis/vector/geometry_checker/qgsgeometrycheckerutils.h

+2-2
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,7 @@ class ANALYSIS_EXPORT QgsGeometryCheckerUtils
6464
* Returns the feature.
6565
* The geometry will not be reprojected regardless of useMapCrs.
6666
*/
67-
const QgsFeature &feature() const;
67+
QgsFeature feature() const;
6868

6969
/**
7070
* The layer.
@@ -81,7 +81,7 @@ class ANALYSIS_EXPORT QgsGeometryCheckerUtils
8181
* If useMapCrs was specified, it will already be reprojected into the
8282
* CRS specified in the context specified in the constructor.
8383
*/
84-
const QgsGeometry &geometry() const;
84+
QgsGeometry geometry() const;
8585

8686
/**
8787
* Returns a combination of the layerId and the feature id.

‎src/analysis/vector/geometry_checker/qgsgeometrygapcheck.cpp

+9-4
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,7 @@ void QgsGeometryGapCheck::collectErrors( const QMap<QString, QgsFeaturePool *> &
5858
}
5959

6060
std::unique_ptr< QgsGeometryEngine > geomEngine = QgsGeometryCheckerUtils::createGeomEngine( nullptr, mContext->tolerance );
61+
geomEngine->prepareGeometry();
6162

6263
// Create union of geometry
6364
QString errMsg;
@@ -70,6 +71,7 @@ void QgsGeometryGapCheck::collectErrors( const QMap<QString, QgsFeaturePool *> &
7071

7172
// Get envelope of union
7273
geomEngine = QgsGeometryCheckerUtils::createGeomEngine( unionGeom.get(), mContext->tolerance );
74+
geomEngine->prepareGeometry();
7375
std::unique_ptr<QgsAbstractGeometry> envelope( geomEngine->envelope( &errMsg ) );
7476
if ( !envelope )
7577
{
@@ -79,11 +81,13 @@ void QgsGeometryGapCheck::collectErrors( const QMap<QString, QgsFeaturePool *> &
7981

8082
// Buffer envelope
8183
geomEngine = QgsGeometryCheckerUtils::createGeomEngine( envelope.get(), mContext->tolerance );
84+
geomEngine->prepareGeometry();
8285
QgsAbstractGeometry *bufEnvelope = geomEngine->buffer( 2, 0, GEOSBUF_CAP_SQUARE, GEOSBUF_JOIN_MITRE, 4. ); //#spellok //#spellok
8386
envelope.reset( bufEnvelope );
8487

8588
// Compute difference between envelope and union to obtain gap polygons
8689
geomEngine = QgsGeometryCheckerUtils::createGeomEngine( envelope.get(), mContext->tolerance );
90+
geomEngine->prepareGeometry();
8791
std::unique_ptr<QgsAbstractGeometry> diffGeom( geomEngine->difference( unionGeom.get(), &errMsg ) );
8892
if ( !diffGeom )
8993
{
@@ -115,10 +119,11 @@ void QgsGeometryGapCheck::collectErrors( const QMap<QString, QgsFeaturePool *> &
115119
const QgsGeometryCheckerUtils::LayerFeatures layerFeatures( featurePools, featureIds.keys(), gapAreaBBox, compatibleGeometryTypes(), mContext );
116120
for ( const QgsGeometryCheckerUtils::LayerFeature &layerFeature : layerFeatures )
117121
{
118-
if ( QgsGeometryCheckerUtils::sharedEdgeLength( gapGeom.get(), layerFeature.geometry().constGet(), mContext->reducedTolerance ) > 0 )
122+
const QgsGeometry geom = layerFeature.geometry();
123+
if ( QgsGeometryCheckerUtils::sharedEdgeLength( gapGeom.get(), geom.constGet(), mContext->reducedTolerance ) > 0 )
119124
{
120125
neighboringIds[layerFeature.layer()->id()].insert( layerFeature.feature().id() );
121-
gapAreaBBox.combineExtentWith( layerFeature.geometry().constGet()->boundingBox() );
126+
gapAreaBBox.combineExtentWith( layerFeature.geometry().boundingBox() );
122127
}
123128
}
124129

@@ -193,7 +198,7 @@ bool QgsGeometryGapCheck::mergeWithNeighbor( const QMap<QString, QgsFeaturePool
193198
{
194199
continue;
195200
}
196-
QgsGeometry featureGeom = testFeature.geometry();
201+
const QgsGeometry featureGeom = testFeature.geometry();
197202
const QgsAbstractGeometry *testGeom = featureGeom.constGet();
198203
for ( int iPart = 0, nParts = testGeom->partCount(); iPart < nParts; ++iPart )
199204
{
@@ -219,7 +224,7 @@ bool QgsGeometryGapCheck::mergeWithNeighbor( const QMap<QString, QgsFeaturePool
219224
std::unique_ptr<QgsAbstractGeometry> errLayerGeom( errGeometry->clone() );
220225
QgsCoordinateTransform ct( featurePool->crs(), mContext->mapCrs, mContext->transformContext );
221226
errLayerGeom->transform( ct, QgsCoordinateTransform::ReverseTransform );
222-
QgsGeometry mergeFeatureGeom = mergeFeature.geometry();
227+
const QgsGeometry mergeFeatureGeom = mergeFeature.geometry();
223228
const QgsAbstractGeometry *mergeGeom = mergeFeatureGeom.constGet();
224229
std::unique_ptr< QgsGeometryEngine > geomEngine = QgsGeometryCheckerUtils::createGeomEngine( errLayerGeom.get(), mContext->reducedTolerance );
225230
std::unique_ptr<QgsAbstractGeometry> combinedGeom( geomEngine->combine( QgsGeometryCheckerUtils::getGeomPart( mergeGeom, mergePartIdx ), &errMsg ) );

‎src/analysis/vector/geometry_checker/qgsgeometrylinelayerintersectioncheck.cpp

+1-1
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,7 @@ void QgsGeometryLineLayerIntersectionCheck::collectErrors( const QMap<QString, Q
5757
}
5858
else if ( const QgsPolygon *polygon = dynamic_cast<const QgsPolygon *>( part ) )
5959
{
60-
QList< const QgsLineString * > rings = QgsGeometryCheckerUtils::polygonRings( polygon );
60+
const QList< const QgsLineString * > rings = QgsGeometryCheckerUtils::polygonRings( polygon );
6161
for ( const QgsLineString *ring : rings )
6262
{
6363
const QList< QgsPoint > intersections = QgsGeometryCheckerUtils::lineIntersections( line, ring, mContext->tolerance );

‎src/analysis/vector/geometry_checker/qgsgeometrymissingvertexcheck.cpp

+5-3
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,8 @@ void QgsGeometryMissingVertexCheck::collectErrors( const QMap<QString, QgsFeatur
4848
break;
4949
}
5050

51-
const QgsAbstractGeometry *geom = layerFeature.geometry().constGet();
51+
const QgsGeometry geometry = layerFeature.geometry();
52+
const QgsAbstractGeometry *geom = geometry.constGet();
5253

5354
if ( QgsCurvePolygon *polygon = qgsgeometry_cast<QgsCurvePolygon *>( geom ) )
5455
{
@@ -129,7 +130,7 @@ void QgsGeometryMissingVertexCheck::processPolygon( const QgsCurvePolygon *polyg
129130
const QgsFeature &currentFeature = layerFeature.feature();
130131
std::unique_ptr<QgsMultiPolygon> boundaries = qgis::make_unique<QgsMultiPolygon>();
131132

132-
std::unique_ptr< QgsGeometryEngine > geomEngine = QgsGeometryCheckerUtils::createGeomEngine( polygon->exteriorRing(), mContext->tolerance );
133+
std::unique_ptr< QgsGeometryEngine > geomEngine = QgsGeometryCheckerUtils::createGeomEngine( polygon->exteriorRing()->clone(), mContext->tolerance );
133134
boundaries->addGeometry( geomEngine->buffer( mContext->tolerance, 5 ) );
134135

135136
const int numRings = polygon->numInteriorRings();
@@ -155,7 +156,8 @@ void QgsGeometryMissingVertexCheck::processPolygon( const QgsCurvePolygon *polyg
155156
if ( feedback && feedback->isCanceled() )
156157
break;
157158

158-
QgsVertexIterator vertexIterator = compareFeature.geometry().vertices();
159+
const QgsGeometry compareGeometry = compareFeature.geometry();
160+
QgsVertexIterator vertexIterator = compareGeometry.vertices();
159161
while ( vertexIterator.hasNext() )
160162
{
161163
const QgsPoint &pt = vertexIterator.next();

‎src/analysis/vector/geometry_checker/qgsgeometryoverlapcheck.cpp

+17-9
Original file line numberDiff line numberDiff line change
@@ -41,8 +41,10 @@ void QgsGeometryOverlapCheck::collectErrors( const QMap<QString, QgsFeaturePool
4141
// Ensure each pair of layers only gets compared once: remove the current layer from the layerIds, but add it to the layerList for layerFeaturesB
4242
layerIds.removeOne( layerFeatureA.layer()->id() );
4343

44-
QgsRectangle bboxA = layerFeatureA.geometry().constGet()->boundingBox();
45-
std::unique_ptr< QgsGeometryEngine > geomEngineA = QgsGeometryCheckerUtils::createGeomEngine( layerFeatureA.geometry().constGet(), mContext->tolerance );
44+
const QgsGeometry geomA = layerFeatureA.geometry();
45+
QgsRectangle bboxA = geomA.boundingBox();
46+
std::unique_ptr< QgsGeometryEngine > geomEngineA = QgsGeometryCheckerUtils::createGeomEngine( geomA.constGet(), mContext->tolerance );
47+
geomEngineA->prepareGeometry();
4648
if ( !geomEngineA->isValid() )
4749
{
4850
messages.append( tr( "Overlap check failed for (%1): the geometry is invalid" ).arg( layerFeatureA.id() ) );
@@ -56,15 +58,17 @@ void QgsGeometryOverlapCheck::collectErrors( const QMap<QString, QgsFeaturePool
5658
break;
5759

5860
// > : only report overlaps within same layer once
59-
if ( layerFeatureA.layer()->id() == layerFeatureB.layer()->id() && layerFeatureB.feature().id() >= layerFeatureA.feature().id() )
61+
if ( layerFeatureA.layerId() == layerFeatureB.layerId() && layerFeatureB.feature().id() >= layerFeatureA.feature().id() )
6062
{
6163
continue;
6264
}
6365

6466
QString errMsg;
65-
if ( geomEngineA->overlaps( layerFeatureB.geometry().constGet(), &errMsg ) )
67+
const QgsGeometry geometryB = layerFeatureB.geometry();
68+
const QgsAbstractGeometry *geomB = geometryB.constGet();
69+
if ( geomEngineA->overlaps( geomB, &errMsg ) )
6670
{
67-
std::unique_ptr<QgsAbstractGeometry> interGeom( geomEngineA->intersection( layerFeatureB.geometry().constGet() ) );
71+
std::unique_ptr<QgsAbstractGeometry> interGeom( geomEngineA->intersection( geomB ) );
6872
if ( interGeom && !interGeom->isEmpty() )
6973
{
7074
QgsGeometryCheckerUtils::filter1DTypes( interGeom.get() );
@@ -106,14 +110,17 @@ void QgsGeometryOverlapCheck::fixError( const QMap<QString, QgsFeaturePool *> &f
106110
// Check if error still applies
107111
QgsGeometryCheckerUtils::LayerFeature layerFeatureA( featurePoolA, featureA, mContext, true );
108112
QgsGeometryCheckerUtils::LayerFeature layerFeatureB( featurePoolB, featureB, mContext, true );
109-
std::unique_ptr< QgsGeometryEngine > geomEngineA = QgsGeometryCheckerUtils::createGeomEngine( layerFeatureA.geometry().constGet(), mContext->reducedTolerance );
113+
const QgsGeometry geometryA = layerFeatureA.geometry();
114+
std::unique_ptr< QgsGeometryEngine > geomEngineA = QgsGeometryCheckerUtils::createGeomEngine( geometryA.constGet(), mContext->reducedTolerance );
115+
geomEngineA->prepareGeometry();
110116

111-
if ( !geomEngineA->overlaps( layerFeatureB.geometry().constGet() ) )
117+
const QgsGeometry geometryB = layerFeatureB.geometry();
118+
if ( !geomEngineA->overlaps( geometryB.constGet() ) )
112119
{
113120
error->setObsolete();
114121
return;
115122
}
116-
std::unique_ptr< QgsAbstractGeometry > interGeom( geomEngineA->intersection( layerFeatureB.geometry().constGet(), &errMsg ) );
123+
std::unique_ptr< QgsAbstractGeometry > interGeom( geomEngineA->intersection( geometryB.constGet(), &errMsg ) );
117124
if ( !interGeom )
118125
{
119126
error->setFixFailed( tr( "Failed to compute intersection between overlapping features: %1" ).arg( errMsg ) );
@@ -154,7 +161,8 @@ void QgsGeometryOverlapCheck::fixError( const QMap<QString, QgsFeaturePool *> &f
154161
{
155162
QgsGeometryCheckerUtils::filter1DTypes( diff1.get() );
156163
}
157-
std::unique_ptr< QgsGeometryEngine > geomEngineB = QgsGeometryCheckerUtils::createGeomEngine( layerFeatureB.geometry().constGet(), mContext->reducedTolerance );
164+
std::unique_ptr< QgsGeometryEngine > geomEngineB = QgsGeometryCheckerUtils::createGeomEngine( geometryB.constGet(), mContext->reducedTolerance );
165+
geomEngineB->prepareGeometry();
158166
std::unique_ptr< QgsAbstractGeometry > diff2( geomEngineB->difference( interPart, &errMsg ) );
159167
if ( !diff2 || diff2->isEmpty() )
160168
{

0 commit comments

Comments
 (0)