Skip to content
Permalink
Browse files

Merge pull request #9551 from m-kuhn/geom-validator-fixes

[geometry validation] Various bugfixes and improvements
  • Loading branch information
m-kuhn committed Mar 19, 2019
2 parents d1910f5 + 6bcba42 commit 9383b00a4b5ea2f8e33fd4a809824ad15cc7ec70
Showing with 549 additions and 304 deletions.
  1. +13 −7 python/analysis/auto_generated/vector/geometry_checker/qgsfeaturepool.sip.in
  2. +1 −0 python/analysis/auto_generated/vector/geometry_checker/qgsgeometrycheckerror.sip.in
  3. +6 −2 python/analysis/auto_generated/vector/geometry_checker/qgsgeometrycheckerutils.sip.in
  4. +8 −0 resources/qgis_global_settings.ini
  5. +28 −13 src/analysis/vector/geometry_checker/qgsfeaturepool.cpp
  6. +19 −11 src/analysis/vector/geometry_checker/qgsfeaturepool.h
  7. +7 −1 src/analysis/vector/geometry_checker/qgsgeometrycheckerror.cpp
  8. +9 −0 src/analysis/vector/geometry_checker/qgsgeometrycheckerror.h
  9. +6 −6 src/analysis/vector/geometry_checker/qgsgeometrycheckerutils.cpp
  10. +6 −2 src/analysis/vector/geometry_checker/qgsgeometrycheckerutils.h
  11. +46 −4 src/analysis/vector/geometry_checker/qgsgeometrygapcheck.cpp
  12. +7 −26 src/analysis/vector/geometry_checker/qgsgeometrygapcheck.h
  13. +1 −1 src/analysis/vector/geometry_checker/qgsgeometrylinelayerintersectioncheck.cpp
  14. +60 −6 src/analysis/vector/geometry_checker/qgsgeometrymissingvertexcheck.cpp
  15. +50 −0 src/analysis/vector/geometry_checker/qgsgeometrymissingvertexcheck.h
  16. +56 −9 src/analysis/vector/geometry_checker/qgsgeometryoverlapcheck.cpp
  17. +6 −28 src/analysis/vector/geometry_checker/qgsgeometryoverlapcheck.h
  18. +19 −42 src/app/qgsgeometryvalidationdock.cpp
  19. +1 −2 src/app/qgsgeometryvalidationdock.h
  20. +29 −0 src/app/qgsgeometryvalidationmodel.cpp
  21. +10 −0 src/app/qgsgeometryvalidationmodel.h
  22. +34 −6 src/app/qgsgeometryvalidationservice.cpp
  23. +7 −0 src/app/qgsgeometryvalidationservice.h
  24. +8 −1 src/core/qgsgeometryoptions.cpp
  25. +1 −1 src/core/qgsgeometryoptions.h
  26. +14 −7 src/core/qgsvectorlayereditutils.cpp
  27. +80 −124 src/ui/qgsgeometryvalidationdockbase.ui
  28. +17 −5 tests/src/geometry_checker/testqgsvectorlayerfeaturepool.cpp
@@ -10,7 +10,6 @@




class QgsFeaturePool : QgsFeatureSink /Abstract/
{
%Docstring
@@ -30,12 +29,11 @@ A feature pool is based on a vector layer and caches features.
QgsFeaturePool( QgsVectorLayer *layer );
virtual ~QgsFeaturePool();

bool getFeature( QgsFeatureId id, QgsFeature &feature, QgsFeedback *feedback = 0 );
bool getFeature( QgsFeatureId id, QgsFeature &feature );
%Docstring
Retrieve the feature with the specified ``id`` into ``feature``.
It will be retrieved from the cache or from the underlying layer if unavailable.
If the feature is neither available from the cache nor from the layer it will return false.
If ``feedback`` is specified, the call may return if the feedback is canceled.
Retrieves the feature with the specified ``id`` into ``feature``.
It will be retrieved from the cache or from the underlying feature source if unavailable.
If the feature is neither available from the cache nor from the source it will return FALSE.
%End


@@ -74,11 +72,19 @@ The geometry type of this layer.
QgsCoordinateReferenceSystem crs() const;
%Docstring
The coordinate reference system of this layer.
%End

QString layerName() const;
%Docstring
Returns the name of the layer.

Should be preferred over layer().name() because it can directly be run on
the background thread.
%End

protected:

void insertFeature( const QgsFeature &feature );
void insertFeature( const QgsFeature &feature, bool skipLock = false );
%Docstring
Inserts a feature into the cache and the spatial index.
To be used by implementations of ``addFeature``.
@@ -76,6 +76,7 @@ Will be used to update existing errors whenever they are re-checked.
%End



protected:
QgsGeometryCheckError( const QgsGeometryCheck *check,
const QString &layerId,
@@ -44,7 +44,7 @@ If ``useMapCrs`` is True, geometries will be reprojected to the mapCrs defined
in ``context``.
%End

const QgsFeature &feature() const;
QgsFeature feature() const;
%Docstring
Returns the feature.
The geometry will not be reprojected regardless of useMapCrs.
@@ -56,13 +56,17 @@ The geometry will not be reprojected regardless of useMapCrs.
The layer id.
%End

const QgsGeometry &geometry() const;
QgsGeometry geometry() const;
%Docstring
Returns the geometry of this feature.
If useMapCrs was specified, it will already be reprojected into the
CRS specified in the context specified in the constructor.
%End

QString id() const;
%Docstring
Returns a combination of the layerId and the feature id.
%End
bool operator==( const QgsGeometryCheckerUtils::LayerFeature &other ) const;
bool operator!=( const QgsGeometryCheckerUtils::LayerFeature &other ) const;

@@ -83,3 +83,11 @@ PostgreSQL\default_timeout=30
# cause performance issues, as the records must all be loaded from the related table on display.
maxEntriesRelationWidget=100

[geometry_validation]
# A comma separated list of geometry validations to enable by default for newly added layers
# Available checks: QgsIsValidCheck,QgsGeometryGapCheck,QgsGeometryOverlapCheck,QgsGeometryMissingVertexCheck
default_checks=

# Enable problem resolution for geometry errors
# This feature is experimental and has known issues.
enable_problem_resolution=false
@@ -29,14 +29,14 @@
QgsFeaturePool::QgsFeaturePool( QgsVectorLayer *layer )
: mFeatureCache( CACHE_SIZE )
, mLayer( layer )
, mLayerId( layer->id() )
, mGeometryType( layer->geometryType() )
, mCrs( layer->crs() )
, mFeatureSource( qgis::make_unique<QgsVectorLayerFeatureSource>( layer ) )
, mLayerName( layer->name() )
{

}

bool QgsFeaturePool::getFeature( QgsFeatureId id, QgsFeature &feature, QgsFeedback *feedback )
bool QgsFeaturePool::getFeature( QgsFeatureId id, QgsFeature &feature )
{
// Why is there a write lock acquired here? Weird, we only want to read a feature from the cache, right?
// A method like `QCache::object(const Key &key) const` certainly would not modify its internals.
@@ -55,11 +55,9 @@ bool QgsFeaturePool::getFeature( QgsFeatureId id, QgsFeature &feature, QgsFeedba
}
else
{
std::unique_ptr<QgsVectorLayerFeatureSource> source = QgsVectorLayerUtils::getFeatureSource( mLayer, feedback );

// Feature not in cache, retrieve from layer
// TODO: avoid always querying all attributes (attribute values are needed when merging by attribute)
if ( !source || !source->getFeatures( QgsFeatureRequest( id ) ).nextFeature( feature ) )
if ( !mFeatureSource->getFeatures( QgsFeatureRequest( id ) ).nextFeature( feature ) )
{
return false;
}
@@ -72,15 +70,22 @@ bool QgsFeaturePool::getFeature( QgsFeatureId id, QgsFeature &feature, QgsFeedba

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;

std::unique_ptr<QgsVectorLayerFeatureSource> source = QgsVectorLayerUtils::getFeatureSource( mLayer, feedback );
mFeatureSource = qgis::make_unique<QgsVectorLayerFeatureSource>( mLayer );

QgsFeatureIterator it = source->getFeatures( request );
QgsFeatureIterator it = mFeatureSource->getFeatures( request );
QgsFeature feature;
while ( it.nextFeature( feature ) )
{
insertFeature( feature );
insertFeature( feature, true );
fids << feature.id();
}

@@ -111,9 +116,11 @@ QPointer<QgsVectorLayer> QgsFeaturePool::layerPtr() const
return mLayer;
}

void QgsFeaturePool::insertFeature( const QgsFeature &feature )
void QgsFeaturePool::insertFeature( const QgsFeature &feature, bool skipLock )
{
QgsReadWriteLocker locker( mCacheLock, QgsReadWriteLocker::Write );
QgsReadWriteLocker locker( mCacheLock, QgsReadWriteLocker::Unlocked );
if ( !skipLock )
locker.changeMode( QgsReadWriteLocker::Write );
mFeatureCache.insert( feature.id(), new QgsFeature( feature ) );
QgsFeature indexFeature( feature );
mIndex.addFeature( indexFeature );
@@ -150,12 +157,19 @@ void QgsFeaturePool::setFeatureIds( const QgsFeatureIds &ids )

bool QgsFeaturePool::isFeatureCached( QgsFeatureId fid )
{
QgsReadWriteLocker locker( mCacheLock, QgsReadWriteLocker::Read );
return mFeatureCache.contains( fid );
}

QString QgsFeaturePool::layerName() const
{
return mLayerName;
}

QgsCoordinateReferenceSystem QgsFeaturePool::crs() const
{
return mCrs;
QgsReadWriteLocker( mCacheLock, QgsReadWriteLocker::Read );
return mFeatureSource->crs();
}

QgsWkbTypes::GeometryType QgsFeaturePool::geometryType() const
@@ -165,5 +179,6 @@ QgsWkbTypes::GeometryType QgsFeaturePool::geometryType() const

QString QgsFeaturePool::layerId() const
{
return mLayerId;
QgsReadWriteLocker( mCacheLock, QgsReadWriteLocker::Read );
return mFeatureSource->id();
}
@@ -25,8 +25,7 @@
#include "qgsfeature.h"
#include "qgsspatialindex.h"
#include "qgsfeaturesink.h"

class QgsVectorLayer;
#include "qgsvectorlayerfeatureiterator.h"

/**
* \ingroup analysis
@@ -43,19 +42,20 @@ class ANALYSIS_EXPORT QgsFeaturePool : public QgsFeatureSink SIP_ABSTRACT
virtual ~QgsFeaturePool() = default;

/**
* Retrieve the feature with the specified \a id into \a feature.
* It will be retrieved from the cache or from the underlying layer if unavailable.
* If the feature is neither available from the cache nor from the layer it will return false.
* If \a feedback is specified, the call may return if the feedback is canceled.
* Retrieves the feature with the specified \a id into \a feature.
* It will be retrieved from the cache or from the underlying feature source if unavailable.
* If the feature is neither available from the cache nor from the source it will return FALSE.
*/
bool getFeature( QgsFeatureId id, QgsFeature &feature, QgsFeedback *feedback = nullptr );
bool getFeature( QgsFeatureId id, QgsFeature &feature );

/**
* Get features for the provided \a request. No features will be fetched
* from the cache and the request is sent directly to the underlying feature source.
* Results of the request are cached in the pool and the ids of all the features
* are returned. This can be used to warm the cache for a particular area of interest
* are returned. This is used to warm the cache for a particular area of interest
* (bounding box) or other set of features.
* This will get a new feature source from the source vector layer.
* This needs to be called from the main thread.
* If \a feedback is specified, the call may return if the feedback is canceled.
*/
QgsFeatureIds getFeatures( const QgsFeatureRequest &request, QgsFeedback *feedback = nullptr ) SIP_SKIP;
@@ -121,13 +121,21 @@ class ANALYSIS_EXPORT QgsFeaturePool : public QgsFeatureSink SIP_ABSTRACT
*/
QgsCoordinateReferenceSystem crs() const;

/**
* Returns the name of the layer.
*
* Should be preferred over layer().name() because it can directly be run on
* the background thread.
*/
QString layerName() const;

protected:

/**
* Inserts a feature into the cache and the spatial index.
* To be used by implementations of ``addFeature``.
*/
void insertFeature( const QgsFeature &feature );
void insertFeature( const QgsFeature &feature, bool skipLock = false );

/**
* Changes a feature in the cache and the spatial index.
@@ -170,9 +178,9 @@ class ANALYSIS_EXPORT QgsFeaturePool : public QgsFeatureSink SIP_ABSTRACT
mutable QReadWriteLock mCacheLock;
QgsFeatureIds mFeatureIds;
QgsSpatialIndex mIndex;
QString mLayerId;
QgsWkbTypes::GeometryType mGeometryType;
QgsCoordinateReferenceSystem mCrs;
std::unique_ptr<QgsVectorLayerFeatureSource> mFeatureSource;
QString mLayerName;
};

#endif // QGS_FEATUREPOOL_H
@@ -53,7 +53,8 @@ QgsGeometryCheckError::QgsGeometryCheckError( const QgsGeometryCheck *check,
{
if ( vidx.part != -1 )
{
mGeometry = QgsGeometry( QgsGeometryCheckerUtils::getGeomPart( layerFeature.geometry().constGet(), vidx.part )->clone() );
const QgsGeometry geom = layerFeature.geometry();
mGeometry = QgsGeometry( QgsGeometryCheckerUtils::getGeomPart( geom.constGet(), vidx.part )->clone() );
}
else
{
@@ -180,6 +181,11 @@ bool QgsGeometryCheckError::handleChanges( const QgsGeometryCheck::Changes &chan
return true;
}

QMap<QString, QgsFeatureIds> QgsGeometryCheckError::involvedFeatures() const
{
return QMap<QString, QSet<QgsFeatureId> >();
}

void QgsGeometryCheckError::update( const QgsGeometryCheckError *other )
{
Q_ASSERT( mCheck == other->mCheck );
@@ -93,6 +93,15 @@ class ANALYSIS_EXPORT QgsGeometryCheckError
*/
virtual bool handleChanges( const QgsGeometryCheck::Changes &changes ) SIP_SKIP;

/**
* Returns a list of involved features.
* By default returns an empty map.
* The map keys are layer ids, the map value is a set of feature ids.
*
* \since QGIS 3.8
*/
virtual QMap<QString, QgsFeatureIds > involvedFeatures() const SIP_SKIP;

protected:
// Users of this constructor must ensure geometry and errorLocation are in map coordinates
QgsGeometryCheckError( const QgsGeometryCheck *check,
@@ -52,7 +52,7 @@ QgsGeometryCheckerUtils::LayerFeature::LayerFeature( const QgsFeaturePool *pool,
}
}

const QgsFeature &QgsGeometryCheckerUtils::LayerFeature::feature() const
QgsFeature QgsGeometryCheckerUtils::LayerFeature::feature() const
{
return mFeature;
}
@@ -67,24 +67,24 @@ QString QgsGeometryCheckerUtils::LayerFeature::layerId() const
return mFeaturePool->layerId();
}

const QgsGeometry &QgsGeometryCheckerUtils::LayerFeature::geometry() const
QgsGeometry QgsGeometryCheckerUtils::LayerFeature::geometry() const
{
return mGeometry;
}

QString QgsGeometryCheckerUtils::LayerFeature::id() const
{
return QStringLiteral( "%1:%2" ).arg( layer()->name() ).arg( mFeature.id() );
return QStringLiteral( "%1:%2" ).arg( mFeaturePool->layerName() ).arg( mFeature.id() );
}

bool QgsGeometryCheckerUtils::LayerFeature::operator==( const LayerFeature &other ) const
{
return layer()->id() == other.layer()->id() && feature().id() == other.feature().id();
return layerId() == other.layerId() && mFeature.id() == other.mFeature.id();
}

bool QgsGeometryCheckerUtils::LayerFeature::operator!=( const LayerFeature &other ) const
{
return layer()->id() != other.layer()->id() || feature().id() != other.feature().id();
return layerId() != other.layerId() || mFeature.id() != other.mFeature.id();
}

/////////////////////////////////////////////////////////////////////////////
@@ -197,7 +197,7 @@ bool QgsGeometryCheckerUtils::LayerFeatures::iterator::nextFeature( bool begin )
QgsFeature feature;
if ( featurePool->getFeature( *mFeatureIt, feature ) && !feature.geometry().isNull() )
{
mCurrentFeature.reset( new LayerFeature( mParent->mFeaturePools[*mLayerIt], feature, mParent->mContext, mParent->mUseMapCrs ) );
mCurrentFeature = qgis::make_unique<LayerFeature>( featurePool, feature, mParent->mContext, mParent->mUseMapCrs );
return true;
}
++mFeatureIt;
@@ -56,7 +56,7 @@ class ANALYSIS_EXPORT QgsGeometryCheckerUtils
* Returns the feature.
* The geometry will not be reprojected regardless of useMapCrs.
*/
const QgsFeature &feature() const;
QgsFeature feature() const;

/**
* The layer.
@@ -73,7 +73,11 @@ class ANALYSIS_EXPORT QgsGeometryCheckerUtils
* If useMapCrs was specified, it will already be reprojected into the
* CRS specified in the context specified in the constructor.
*/
const QgsGeometry &geometry() const;
QgsGeometry geometry() const;

/**
* Returns a combination of the layerId and the feature id.
*/
QString id() const;
bool operator==( const QgsGeometryCheckerUtils::LayerFeature &other ) const;
bool operator!=( const QgsGeometryCheckerUtils::LayerFeature &other ) const;

0 comments on commit 9383b00

Please sign in to comment.
You can’t perform that action at this time.