Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
Avoid firing up a thread for blocking GEOS based validity checks
It's an unnecessary expense. Also fix duplicate code.
  • Loading branch information
nyalldawson committed Feb 26, 2019
1 parent 6dbe4ee commit 35f613d
Show file tree
Hide file tree
Showing 9 changed files with 128 additions and 63 deletions.
2 changes: 2 additions & 0 deletions python/core/auto_generated/geometry/qgsgeometry.sip.in
Expand Up @@ -1793,6 +1793,8 @@ True if the location available from :py:func:`where` is valid.
sipRes = PyUnicode_FromString( str.toUtf8().data() );
%End

bool operator==( const QgsGeometry::Error &other ) const;

};

enum ValidationMethod
Expand Down
4 changes: 3 additions & 1 deletion python/core/auto_generated/geometry/qgsgeometryengine.sip.in
Expand Up @@ -208,7 +208,7 @@ pattern.
virtual double area( QString *errorMsg = 0 ) const = 0;
virtual double length( QString *errorMsg = 0 ) const = 0;

virtual bool isValid( QString *errorMsg = 0, bool allowSelfTouchingHoles = false ) const = 0;
virtual bool isValid( QString *errorMsg = 0, bool allowSelfTouchingHoles = false, QgsGeometry *errorLoc = 0 ) const = 0;
%Docstring
Returns true if the geometry is valid.

Expand All @@ -217,6 +217,8 @@ If the geometry is invalid, ``errorMsg`` will be filled with the reported geomet
The ``allowSelfTouchingHoles`` argument specifies whether self-touching holes are permitted.
OGC validity states that self-touching holes are NOT permitted, whilst other vendor
validity checks (e.g. ESRI) permit self-touching holes.

If ``errorLoc`` is specified, it will be set to the geometry of the error location.
%End

virtual bool isEqual( const QgsAbstractGeometry *geom, QString *errorMsg = 0 ) const = 0;
Expand Down
39 changes: 37 additions & 2 deletions src/core/geometry/qgsgeometry.cpp
Expand Up @@ -2459,7 +2459,42 @@ QgsGeometry QgsGeometry::forceRHR() const

void QgsGeometry::validateGeometry( QVector<QgsGeometry::Error> &errors, ValidationMethod method ) const
{
QgsGeometryValidator::validateGeometry( *this, errors, method );
errors.clear();
if ( !d->geometry )
return;

// avoid expensive calcs for trivial point geometries
if ( QgsWkbTypes::geometryType( d->geometry->wkbType() ) == QgsWkbTypes::PointGeometry )
{
return;
}

switch ( method )
{
case ValidatorQgisInternal:
QgsGeometryValidator::validateGeometry( *this, errors, method );
return;

case ValidatorGeos:
{
QgsGeos geos( d->geometry.get() );
QString error;
QgsGeometry errorLoc;
if ( !geos.isValid( &error, true, &errorLoc ) )
{
if ( errorLoc.isNull() )
{
errors.append( QgsGeometry::Error( error ) );
}
else
{
const QgsPointXY point = errorLoc.asPoint();
errors.append( QgsGeometry::Error( error, point ) );
}
return;
}
}
}
}

bool QgsGeometry::isGeosValid( const QgsGeometry::ValidityFlags flags ) const
Expand All @@ -2477,7 +2512,7 @@ bool QgsGeometry::isGeosValid( const QgsGeometry::ValidityFlags flags ) const

QgsGeos geos( d->geometry.get() );
mLastError.clear();
return geos.isValid( &mLastError, flags & FlagAllowSelfTouchingHoles );
return geos.isValid( &mLastError, flags & FlagAllowSelfTouchingHoles, nullptr );
}

bool QgsGeometry::isSimple() const
Expand Down
5 changes: 5 additions & 0 deletions src/core/geometry/qgsgeometry.h
Expand Up @@ -1851,6 +1851,11 @@ class CORE_EXPORT QgsGeometry
% End
#endif

bool operator==( const QgsGeometry::Error &other ) const
{
return other.mMessage == mMessage && other.mHasLocation == mHasLocation && other.mLocation == mLocation;
}

private:
QString mMessage;
QgsPointXY mLocation;
Expand Down
4 changes: 3 additions & 1 deletion src/core/geometry/qgsgeometryengine.h
Expand Up @@ -226,8 +226,10 @@ class CORE_EXPORT QgsGeometryEngine
* The \a allowSelfTouchingHoles argument specifies whether self-touching holes are permitted.
* OGC validity states that self-touching holes are NOT permitted, whilst other vendor
* validity checks (e.g. ESRI) permit self-touching holes.
*
* If \a errorLoc is specified, it will be set to the geometry of the error location.
*/
virtual bool isValid( QString *errorMsg = nullptr, bool allowSelfTouchingHoles = false ) const = 0;
virtual bool isValid( QString *errorMsg = nullptr, bool allowSelfTouchingHoles = false, QgsGeometry *errorLoc = nullptr ) const = 0;

/**
* Checks if this is equal to \a geom.
Expand Down
35 changes: 33 additions & 2 deletions src/core/geometry/qgsgeos.cpp
Expand Up @@ -1658,7 +1658,7 @@ QgsAbstractGeometry *QgsGeos::convexHull( QString *errorMsg ) const
CATCH_GEOS_WITH_ERRMSG( nullptr );
}

bool QgsGeos::isValid( QString *errorMsg, const bool allowSelfTouchingHoles ) const
bool QgsGeos::isValid( QString *errorMsg, const bool allowSelfTouchingHoles, QgsGeometry *errorLoc ) const
{
if ( !mGeos )
{
Expand All @@ -1673,7 +1673,38 @@ bool QgsGeos::isValid( QString *errorMsg, const bool allowSelfTouchingHoles ) co
const bool invalid = res != 1;

if ( invalid && errorMsg )
*errorMsg = QString( r );
{
static QgsStringMap translatedErrors;

if ( translatedErrors.empty() )
{
// Copied from https://git.osgeo.org/gitea/geos/geos/src/branch/master/src/operation/valid/TopologyValidationError.cpp
translatedErrors.insert( QStringLiteral( "topology validation error" ), QObject::tr( "Topology validation error", "GEOS Error" ) );
translatedErrors.insert( QStringLiteral( "repeated point" ), QObject::tr( "Repeated point", "GEOS Error" ) );
translatedErrors.insert( QStringLiteral( "hole lies outside shell" ), QObject::tr( "Hole lies outside shell", "GEOS Error" ) );
translatedErrors.insert( QStringLiteral( "holes are nested" ), QObject::tr( "Holes are nested", "GEOS Error" ) );
translatedErrors.insert( QStringLiteral( "interior is disconnected" ), QObject::tr( "Interior is disconnected", "GEOS Error" ) );
translatedErrors.insert( QStringLiteral( "self-intersection" ), QObject::tr( "Self-intersection", "GEOS Error" ) );
translatedErrors.insert( QStringLiteral( "ring self-intersection" ), QObject::tr( "Ring self-intersection", "GEOS Error" ) );
translatedErrors.insert( QStringLiteral( "nested shells" ), QObject::tr( "Nested shells", "GEOS Error" ) );
translatedErrors.insert( QStringLiteral( "duplicate rings" ), QObject::tr( "Duplicate rings", "GEOS Error" ) );
translatedErrors.insert( QStringLiteral( "too few points in geometry component" ), QObject::tr( "Too few points in geometry component", "GEOS Error" ) );
translatedErrors.insert( QStringLiteral( "invalid coordinate" ), QObject::tr( "Invalid coordinate", "GEOS Error" ) );
translatedErrors.insert( QStringLiteral( "ring is not closed" ), QObject::tr( "Ring is not closed", "GEOS Error" ) );
}

const QString error( r );
*errorMsg = translatedErrors.value( error.toLower(), error );

if ( g1 && errorLoc )
{
*errorLoc = geometryFromGeos( g1 );
}
else if ( g1 )
{
GEOSGeom_destroy_r( geosinit.ctxt, g1 );
}
}
return !invalid;
}
CATCH_GEOS_WITH_ERRMSG( false );
Expand Down
2 changes: 1 addition & 1 deletion src/core/geometry/qgsgeos.h
Expand Up @@ -218,7 +218,7 @@ class CORE_EXPORT QgsGeos: public QgsGeometryEngine
bool relatePattern( const QgsAbstractGeometry *geom, const QString &pattern, QString *errorMsg = nullptr ) const override;
double area( QString *errorMsg = nullptr ) const override;
double length( QString *errorMsg = nullptr ) const override;
bool isValid( QString *errorMsg = nullptr, bool allowSelfTouchingHoles = false ) const override;
bool isValid( QString *errorMsg = nullptr, bool allowSelfTouchingHoles = false, QgsGeometry *errorLoc = nullptr ) const override;
bool isEqual( const QgsAbstractGeometry *geom, QString *errorMsg = nullptr ) const override;
bool isEmpty( QString *errorMsg = nullptr ) const override;
bool isSimple( QString *errorMsg = nullptr ) const override;
Expand Down
77 changes: 22 additions & 55 deletions src/core/qgsgeometryvalidator.cpp
Expand Up @@ -217,65 +217,32 @@ void QgsGeometryValidator::run()
{
case QgsGeometry::ValidatorGeos:
{
char *r = nullptr;
geos::unique_ptr g0 = QgsGeos::asGeos( mGeometry );
GEOSContextHandle_t handle = QgsGeos::getGEOSHandler();
if ( !g0 )
if ( mGeometry.isNull() )
{
emit errorFound( QgsGeometry::Error( QObject::tr( "GEOS error: could not produce geometry for GEOS (check log window)" ) ) );
return;
}
else
{
GEOSGeometry *g1 = nullptr;
char res = GEOSisValidDetail_r( handle, g0.get(), GEOSVALID_ALLOW_SELFTOUCHING_RING_FORMING_HOLE, &r, &g1 );
if ( res != 1 )
{
static QgsStringMap translatedErrors;

if ( translatedErrors.empty() )
{
// Copied from https://git.osgeo.org/gitea/geos/geos/src/branch/master/src/operation/valid/TopologyValidationError.cpp
translatedErrors.insert( QStringLiteral( "topology validation error" ), QObject::tr( "Topology validation error", "GEOS Error" ) );
translatedErrors.insert( QStringLiteral( "repeated point" ), QObject::tr( "Repeated point", "GEOS Error" ) );
translatedErrors.insert( QStringLiteral( "hole lies outside shell" ), QObject::tr( "Hole lies outside shell", "GEOS Error" ) );
translatedErrors.insert( QStringLiteral( "holes are nested" ), QObject::tr( "Holes are nested", "GEOS Error" ) );
translatedErrors.insert( QStringLiteral( "interior is disconnected" ), QObject::tr( "Interior is disconnected", "GEOS Error" ) );
translatedErrors.insert( QStringLiteral( "self-intersection" ), QObject::tr( "Self-intersection", "GEOS Error" ) );
translatedErrors.insert( QStringLiteral( "ring self-intersection" ), QObject::tr( "Ring self-intersection", "GEOS Error" ) );
translatedErrors.insert( QStringLiteral( "nested shells" ), QObject::tr( "Nested shells", "GEOS Error" ) );
translatedErrors.insert( QStringLiteral( "duplicate rings" ), QObject::tr( "Duplicate rings", "GEOS Error" ) );
translatedErrors.insert( QStringLiteral( "too few points in geometry component" ), QObject::tr( "Too few points in geometry component", "GEOS Error" ) );
translatedErrors.insert( QStringLiteral( "invalid coordinate" ), QObject::tr( "Invalid coordinate", "GEOS Error" ) );
translatedErrors.insert( QStringLiteral( "ring is not closed" ), QObject::tr( "Ring is not closed", "GEOS Error" ) );
}

const QString errorMsg( r );
const QString translatedErrorMsg = translatedErrors.value( errorMsg.toLower(), errorMsg );

if ( g1 )
{
const GEOSCoordSequence *cs = GEOSGeom_getCoordSeq_r( handle, g1 );

unsigned int n;
if ( GEOSCoordSeq_getSize_r( handle, cs, &n ) && n == 1 )
{
double x, y;
GEOSCoordSeq_getX_r( handle, cs, 0, &x );
GEOSCoordSeq_getY_r( handle, cs, 0, &y );

emit errorFound( QgsGeometry::Error( translatedErrorMsg, QgsPointXY( x, y ) ) );
mErrorCount++;
}

GEOSGeom_destroy_r( handle, g1 );
}
else
{
emit errorFound( QgsGeometry::Error( translatedErrorMsg ) );
mErrorCount++;
}
// avoid calling geos for trivial point geometries
if ( QgsWkbTypes::geometryType( mGeometry.wkbType() ) == QgsWkbTypes::PointGeometry )
{
return;
}

GEOSFree_r( handle, r );
QgsGeos geos( mGeometry.constGet() );
QString error;
QgsGeometry errorLoc;
if ( !geos.isValid( &error, true, &errorLoc ) )
{
if ( errorLoc.isNull() )
{
emit errorFound( QgsGeometry::Error( error ) );
mErrorCount++;
}
else
{
const QgsPointXY point = errorLoc.asPoint();
emit errorFound( QgsGeometry::Error( error, point ) );
mErrorCount++;
}
}

Expand Down
23 changes: 22 additions & 1 deletion tests/src/python/test_qgsgeometry.py
Expand Up @@ -5048,7 +5048,8 @@ def testIsGeosValid(self):
["LINESTRING (0 0, 0 100, 100 100)", True, True, ''],
["POLYGON((-1 -1, 4 0, 4 2, 0 2, -1 -1))", True, True, ''],
["MULTIPOLYGON(Polygon((-1 -1, 4 0, 4 2, 0 2, -1 -1)),Polygon((100 100, 200 100, 200 200, 100 200, 100 100)))", True, True, ''],
['MultiPolygon (((159865.14786298031685874 6768656.31838363595306873, 159858.97975336571107619 6769211.44824895076453686, 160486.07089751763851382 6769211.44824895076453686, 160481.95882444124436006 6768658.37442017439752817, 160163.27316101978067309 6768658.37442017439752817, 160222.89822062765597366 6769116.87056819349527359, 160132.43261294672265649 6769120.98264127038419247, 160163.27316101978067309 6768658.37442017439752817, 159865.14786298031685874 6768656.31838363595306873)))', False, True, 'Ring Self-intersection']
['MultiPolygon (((159865.14786298031685874 6768656.31838363595306873, 159858.97975336571107619 6769211.44824895076453686, 160486.07089751763851382 6769211.44824895076453686, 160481.95882444124436006 6768658.37442017439752817, 160163.27316101978067309 6768658.37442017439752817, 160222.89822062765597366 6769116.87056819349527359, 160132.43261294672265649 6769120.98264127038419247, 160163.27316101978067309 6768658.37442017439752817, 159865.14786298031685874 6768656.31838363595306873)))', False, True, 'Ring self-intersection'],
['Polygon((0 3, 3 0, 3 3, 0 0, 0 3))', False, False, 'Self-intersection'],
]
for t in tests:
g1 = QgsGeometry.fromWkt(t[0])
Expand All @@ -5061,6 +5062,26 @@ def testIsGeosValid(self):
self.assertEqual(res, t[2],
"mismatch for {}, expected:\n{}\nGot:\n{}\n".format(t[0], t[2], res))

def testValidateGeometry(self):
tests = [
["", []],
["Point (100 100)", [], []],
["MultiPoint (100 100, 100 200)", [], []],
["LINESTRING (0 0, 0 100, 100 100)", [], []],
["POLYGON((-1 -1, 4 0, 4 2, 0 2, -1 -1))", [], []],
["MULTIPOLYGON(Polygon((-1 -1, 4 0, 4 2, 0 2, -1 -1)),Polygon((100 100, 200 100, 200 200, 100 200, 100 100)))", [], []],
['MultiPolygon (((159865.14786298031685874 6768656.31838363595306873, 159858.97975336571107619 6769211.44824895076453686, 160486.07089751763851382 6769211.44824895076453686, 160481.95882444124436006 6768658.37442017439752817, 160163.27316101978067309 6768658.37442017439752817, 160222.89822062765597366 6769116.87056819349527359, 160132.43261294672265649 6769120.98264127038419247, 160163.27316101978067309 6768658.37442017439752817, 159865.14786298031685874 6768656.31838363595306873)))', [], []],
['Polygon((0 3, 3 0, 3 3, 0 0, 0 3))', [QgsGeometry.Error('Self-intersection', QgsPointXY(1.5, 1.5))], [QgsGeometry.Error('segments 0 and 2 of line 0 intersect at 1.5, 1.5', QgsPointXY(1.5, 1.5))]],
]
for t in tests:
g1 = QgsGeometry.fromWkt(t[0])
res = g1.validateGeometry(QgsGeometry.ValidatorGeos)
self.assertEqual(res, t[1],
"mismatch for {}, expected:\n{}\nGot:\n{}\n".format(t[0], t[1], res[0].where() if res else ''))
res = g1.validateGeometry(QgsGeometry.ValidatorQgisInternal)
self.assertEqual(res, t[2],
"mismatch for {}, expected:\n{}\nGot:\n{}\n".format(t[0], t[2], res[0].where() if res else ''))

def renderGeometry(self, geom, use_pen, as_polygon=False, as_painter_path=False):
image = QImage(200, 200, QImage.Format_RGB32)
image.fill(QColor(0, 0, 0))
Expand Down

0 comments on commit 35f613d

Please sign in to comment.