Geometry validation of editing session #8103

m-kuhn · 2018-10-03T13:38:21Z

Introduces geometry validation of the current edit session.

The approach is like this:

There are single geometry checks, currently a single on which is "is valid". These checks are executed whenever a geometry on a layer is modified / added / deleted. So if a user introduces a self-intersection, he will be notified about this immediately and can fix it right away. As soon as it's fixed, the message disappears.
There are topology checks. Currently gaps, overlaps and missing vertices of neighbouring geometries can be enabled. These checks are triggered whenever a user tries to save a layer (or triggers them manually). They are then reported and will also need to be fixed, before the user is able to save the layer. These errors also propose fixes (thanks to the geometry check implementation) or can be manually resolved.
If no checks are enabled, no change in behaviour is noticeable in QGIS, so the whole functionality is opt-in.

This definitely is a lot of code to land at a late stage and I am fully aware of that. Much of the work in here can actually be considered "pre-freeze-bugfixing-and-cleanup" like porting APIs to QGIS 3 style and making things threadsafe. In order of work it just made more sense to do them before the actual implementation (or just while stumbling upon things while working).

nyalldawson · 2018-10-03T23:01:31Z

An initial glance shows that this includes #8034 -- can we resolve the convo there separately? I don't want to split that discussion

m-kuhn · 2018-10-04T06:12:12Z

of course, I'll just adapt here whenever there is a conclusion

nyalldawson

Code review only -- I haven't tested this in action ;)

src/analysis/vector/geometry_checker/qgsfeaturepool.h

nyalldawson · 2018-10-08T23:08:06Z

src/analysis/vector/geometry_checker/qgsgeometrygapcheck.h

@@ -76,10 +77,18 @@ class ANALYSIS_EXPORT QgsGeometryGapCheckError : public QgsGeometryCheckError
 class ANALYSIS_EXPORT QgsGeometryGapCheck : public QgsGeometryCheck
 {
  public:
+    enum ResolutionMethod { MergeLongestEdge, NoChange };


nyalldawson · 2018-10-08T23:09:49Z

src/analysis/vector/geometry_checker/qgsgeometrymissingvertexcheck.cpp

@@ -73,11 +76,32 @@ void QgsGeometryMissingVertexCheck::fixError( const QMap<QString, QgsFeaturePool
  {
    error->setFixed( method );
  }
+  if ( method == AddMissingVertex )


Would a switch be more appropriate here? (for future build warnings)

it's an integer that's passed in because the API is from the base class where the enum is unknown.
Would you cast to the proper enum for the switch?

@m-kuhn done here 3nids@dc6a87c#diff-5893c5403c3be6af7ddfbf56586c08f6R139

nyalldawson · 2018-10-08T23:13:19Z

src/analysis/vector/geometry_checker/qgsgeometryoverlapcheck.cpp

@@ -57,15 +63,15 @@ void QgsGeometryOverlapCheck::collectErrors( const QMap<QString, QgsFeaturePool
      QString errMsg;
      if ( geomEngineA->overlaps( layerFeatureB.geometry().constGet(), &errMsg ) )
      {
-        QgsAbstractGeometry *interGeom = geomEngineA->intersection( layerFeatureB.geometry().constGet() );
+        std::unique_ptr<QgsAbstractGeometry> interGeom( geomEngineA->intersection( layerFeatureB.geometry().constGet() ) );


There's a lot of places (like this) where QgsGeometryCheckerUtils::createGeomEngine is called and then the resulting engine is used to perform intersects/overlaps checks/etc. But the engine is never prepared, negating most of the benefits of using it. I think QgsGeometryCheckerUtils::createGeomEngine should probably always prepare the created engine before returning it to simplify this -- it'll greatly speed up the whole checking process.

Do we always want to prepare an engine or are there cases where it's slower? And in the second case, how will a developer using the API know that he has got a case where preparing is worse than not preparing?

nyalldawson · 2018-10-08T23:13:55Z

src/analysis/vector/geometry_checker/qgsgeometryoverlapcheck.h

@@ -25,13 +25,34 @@
 class ANALYSIS_EXPORT QgsGeometryOverlapCheckError : public QgsGeometryCheckError
 {
  public:
+
+    struct OverLappedFeature


OverLapped -> Overlapped

nyalldawson · 2018-10-08T23:30:25Z

src/app/qgsgeometryvalidationservice.cpp

+  // We keep all connections around in a list, so if in the future all checks get disabled
+  // we can kill those connections to be sure the layer does not even get a tiny bit of overhead.
+  checkInformation.connections
+      << connect( layer, &QgsVectorLayer::featureAdded, this, [this, layer]( QgsFeatureId fid )


Unique connections do not work for lambdas -- check Qt docs.

Makes sense even without reading the docs.
These are here just for safety, it shouldn't normally happen. However I just replaced it with another safety measure to be on the safe side if this is suddenly called in other circumstances in the future.

nyalldawson · 2018-10-08T23:30:57Z

src/app/qgsgeometryvalidationservice.cpp

+{
+  emit geometryCheckStarted( layer, fid );
+
+  QgsFeature feature = layer->getFeature( fid );


attributes not required

nyalldawson · 2018-10-08T23:31:57Z

src/app/qgsgeometryvalidationservice.cpp

+  QgsRectangle area;
+  while ( it.nextFeature( feature ) )
+  {
+    area.combineExtentWith( feature.geometry().boundingBox() );


Random thought -- maybe we need a feature request flag for "bounding boxes only". It'd certainly speed up a lot of code like this.

Random thought response: I'd like to have "derived values" in requests, like "request specific virtual fields".

request.setFlags(QgsFeatureRequest::NoGeometry) request.addDerivedValue("bbox", "bounds($geometry)")

This would be very flexible, although I admit for this particular use case a custom flag could be justified

nyalldawson · 2018-10-08T23:32:32Z

src/app/qgsgeometryvalidationservice.h

+  public:
+    struct FeatureError
+    {
+      FeatureError()


nyalldawson · 2018-10-08T23:32:51Z

src/app/qgsgeometryvalidationservice.h

+    typedef QList<FeatureError> FeatureErrors;
+
+    QgsGeometryValidationService( QgsProject *project );
+    ~QgsGeometryValidationService() = default;


not required

…rited classes yet)

for Gap, Sliver and Overlap checks

3nids · 2018-10-15T12:59:36Z

src/analysis/vector/geometry_checker/qgsgeometrymissingvertexcheck.h

  public:
    enum ResolutionMethod
    {
      NoChange,
      AddMissingVertex
    };
+    Q_ENUM( ResolutionMethod )


just thinking we could use a template for collect/fix errors that would take the resolution method enums?

I guess the code for Q_GADGET, Q_ENUM etc. cannot be put into a template, and a template with only half the code is not soooo nice.

3nids · 2018-10-15T16:46:35Z

src/app/qgsgeometryvalidationdock.cpp

+
+  if ( mCurrentLayer )
+  {
+    disconnect( mCurrentLayer, &QgsVectorLayer::editingStarted, this, &QgsGeometryValidationDock::onLayerEditingStatusChanged );


I think this is causing a crash when reloading project.
I guess we need to remove the connection when the layer is removed from the layer tree?
Or can we use some smart pointer here?

Do you have a stack trace?

Latest commit should have changed that anyway

it was on a current layer changed from the layer tree, don't have it anymore.
just out of curiosity, why can't we use a scoped pointer here?

A QPointer would probably work as well

m-kuhn · 2018-10-16T08:13:04Z

Thanks a lot for the reviews @nyalldawson and @3nids !

m-kuhn · 2018-10-16T09:35:20Z

So, let's give this a test drive in the wild. Any feedback much welcome!!

agiudiceandrea · 2018-10-24T20:41:10Z

@m-kuhn I'm experiencing (on W7 64 bit) a strange behaviour upon startup: a "Geometry Validation" ghost window appears and suddenly disappears (see in the red box)

!

nirvn · 2018-10-25T01:42:57Z

Ah, I was wondering what caused this regression, I see it on my machine too.

agiudiceandrea · 2018-10-25T14:16:39Z

@nirvn in qgis-dev 3.3.0-97 (osgeo4w 64) the ghost window didn't appear, in 3.3.0-100, 101 and 102 it appears

m-kuhn · 2018-10-25T14:22:09Z

I'll have a look

m-kuhn · 2018-10-25T16:38:16Z

Does it happen on every start of QGIS?

What I can reproduce here is, that when I set the dock visible once, close QGIS and reopen it, it will appear at startup and disappear a bit later (in the dock area, but I guess I've put it there once manually, that's why it's not floating here). However, this is a one-time issue and the next time it will not show up until I enable it again manually.

I'm still looking how to avoid that, but I'm not completely sure it's the same issue.

agiudiceandrea · 2018-10-25T18:24:52Z

Does it happen on every start of QGIS?

Yes. Also with a new profile or deleting the QGIS folder in %appdata%.

m-kuhn · 2018-10-25T18:34:45Z

And also without a new profile?

agiudiceandrea · 2018-10-28T12:19:10Z

I can confirm the bug in 3.4 too.

m-kuhn · 2018-10-29T07:47:28Z

@agiudiceandrea

And also without a new profile?

agiudiceandrea · 2018-10-29T08:54:35Z

I don't understand exactly your question. Anyway the ghost window appears upon every startup with old profiles or new profiles.
I've filed a bug report https://issues.qgis.org/issues/20245

m-kuhn · 2018-10-29T08:58:44Z

the ghost window appears upon every startup with old profiles

that was the question, thanks

borysiasty · 2018-10-29T19:14:13Z

@m-kuhn there is also another problem. I can't find the dock on the panels list (neither in Lin nor Win).
Didn't you forget to addDockWidget( Qt::RightDockWidgetArea, mGeometryValidationDock ); ?

EDIT: Sorry, the problem is different. The dock appears at feature insertion, but only if the project is saved. If I just start a fresh project, add a memory layer, setup its validation properties, insert invalid feature and commit, it is quietly saved. If I then save the project, reopen and continue editing, the dock works well.
It's why I thought the dock is somehow not added to the GUI...

EDIT 2: Even if the dock is not necessary to be placed in the panels menu, I can see it in QGIS 3.4 and not in master.

m-kuhn · 2018-10-29T21:02:49Z

Yeah, activating the tests without reloading the project would be really nice.

But you think there's still something to change with adding the dock? Do you see the issue @agiudiceandrea is experiencing too?

borysiasty · 2018-10-30T18:45:59Z

@m-kuhn the ghost dock seems to be dependant on the window state saved in QSettings. I can see it also on Debian in some user profiles:

In a fresh profile, there is no ghost, and the dock is not listed in the menu (what is expected as it's not added yet). But it is listed in the customization menu.
In another profile, there is no ghost, and the dock IS listed. Apparently it comes from the settings. So if the reason for not adding it to the dock areas was to not clutter the docks menu, it seems to be pointless: once the dock gets saved into settings, it will be listed anyway. Moreover, I can see advantages of having it listed: first, users are not confused by a disappearing dock. Second, you can always open it manually and e.g test topological issues without saving the layer.
In yet another profile, the dock is not listed, but there is the floating ghost for a while (probably until mGeometryValidationDock->close(); is called). Now I added it explicitly to Qt::RightDockWidgetArea, and this profile behaves exactly like the previous one.

So it seems adding the dock solves the problem.

borysiasty · 2018-10-30T19:02:05Z

@m-kuhn - please note all the observations above were made without your today's commits (especially the early hide instead of late close). Unfortunately I can't test these changes, as I accidentally fixed my only ghost-affected profile in the meantime.

m-kuhn · 2018-10-31T07:08:55Z

@borysiasty please tell me if you have new insights!

Second, you can always open it manually and e.g test topological issues without saving the layer.

That's not possible (yet?), the layer needs to be configured for this.

borysiasty · 2018-11-01T12:40:47Z

@borysiasty please tell me if you have new insights!

I didn't manage to call the ghost anymore. Neither before nor after your changes. I'm sorry I've overwritten that affected project. I'm an idiot! ;-)

That's not possible (yet?), the layer needs to be configured for this.

Yes of course. I just meant: without adding the dock, you need to save the layer in order to trigger the topology test and eventually open the dock. Once it's opened, you can trigger the test just by the button. It's just a minor inconsistency.

jef-n · 2018-11-01T12:44:58Z

I'm an idiot! ;-)

Hey, careful, don't insult fellow developers.

m-kuhn changed the title ~~Geometry validator code 1~~ Geometry validation of editing session Oct 3, 2018

m-kuhn force-pushed the geometryValidatorCode_1 branch 2 times, most recently from e537fbd to cedea4f Compare October 8, 2018 06:03

nyalldawson reviewed Oct 8, 2018

View reviewed changes

nyalldawson added Feature Freeze Exempt Feature Freeze exemption granted labels Oct 9, 2018

nyalldawson added this to the 3.4.0 milestone Oct 9, 2018

m-kuhn force-pushed the geometryValidatorCode_1 branch from cedea4f to 33cf9bc Compare October 15, 2018 11:44

m-kuhn and others added 20 commits October 15, 2018 14:27

Add single geometry check

f5486ee

YAY, it is working

56d299e

docstrings

71dbd5a

Save geometry configuration to layer

5e27163

Registry

1b0bd51

UI improvements

cde294e

Move to unified single geometry check

5d7c33e

New configuration page for geometry validitiy

2f5c7a3

single geometry xyz

d7597f9

Add python bindings for geometry checker

ceec910

Fix build warnings

e0697fb

More python bindings

dd608bb

Flags

6f261f7

Fix build

e55bb7a

Registry updates

e8a7ab4

activate/deactivate geometry checks in vector layer properties

0b20ae6

add flags and isCompatible to QgsGeometryCheck

1df0a3e

add feature pool as argument to collect/fix error methods

7593d51

QgsGeometryCheckError class requires context in constructor (not inhe…

b0810b1

…rited classes yet)

Add additional check factories

7df2c63

for Gap, Sliver and Overlap checks

3nids reviewed Oct 15, 2018

View reviewed changes

m-kuhn added 4 commits October 15, 2018 15:06

Resolve some TODOs

3f42395

Standardize geometry checker tests

0bb6a16

Fix python bindings

3d31d72

Add some tests for QgsVectorLayerFeaturePool

8da2910

3nids reviewed Oct 15, 2018

View reviewed changes

Avoid crash when disconnecting layer

6c97fcf

m-kuhn merged commit 190f938 into qgis:master Oct 16, 2018

m-kuhn deleted the geometryValidatorCode_1 branch October 16, 2018 09:34

qgib mentioned this pull request May 25, 2019

A "Geometry Validation" ghost window appears and suddenly disappears upon every startup #28066

Closed

Geometry validation of editing session #8103

Geometry validation of editing session #8103

Conversation

m-kuhn commented Oct 3, 2018

nyalldawson commented Oct 3, 2018

m-kuhn commented Oct 4, 2018

nyalldawson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m-kuhn commented Oct 16, 2018

m-kuhn commented Oct 16, 2018

agiudiceandrea commented Oct 24, 2018 • edited Loading

nirvn commented Oct 25, 2018

agiudiceandrea commented Oct 25, 2018

m-kuhn commented Oct 25, 2018

m-kuhn commented Oct 25, 2018

agiudiceandrea commented Oct 25, 2018

m-kuhn commented Oct 25, 2018

agiudiceandrea commented Oct 28, 2018

m-kuhn commented Oct 29, 2018

agiudiceandrea commented Oct 29, 2018 • edited Loading

m-kuhn commented Oct 29, 2018

borysiasty commented Oct 29, 2018 • edited Loading

m-kuhn commented Oct 29, 2018

borysiasty commented Oct 30, 2018 • edited Loading

borysiasty commented Oct 30, 2018 • edited Loading

m-kuhn commented Oct 31, 2018

borysiasty commented Nov 1, 2018 • edited Loading

jef-n commented Nov 1, 2018

agiudiceandrea commented Oct 24, 2018 •

edited

Loading

agiudiceandrea commented Oct 29, 2018 •

edited

Loading

borysiasty commented Oct 29, 2018 •

edited

Loading

borysiasty commented Oct 30, 2018 •

edited

Loading

borysiasty commented Oct 30, 2018 •

edited

Loading

borysiasty commented Nov 1, 2018 •

edited

Loading