search

qgis / QGIS-Enhancement-Proposals

QEP's (QGIS Enhancement Proposals) are used in the process of creating and discussing new enhancements for QGIS
117 stars 37 forks source link

Unification of all geometric and topological verification methods #236

Open Koyaani opened 2 years ago

Koyaani commented 2 years ago

QGIS Enhancement: Unification of all geometric and topological verification methods

Date 2021/10/22

Authors Antoine Facchini (@Koyaani), Loïc Bartoletti (@lbartoletti)

Contact antoine dot facchini at oslandia dot com loic dot bartoletti at oslandia dot com

Version QGIS 3.24+

Summary

Through the years, QGIS has been enhanced with many features to solve the vast problem that is the geometric and topological verifications and its correction. However, they are dispersed in many places in the code of Qgis. For example:

Some features are developed several times. An example is the verification / correction "point in polygon":

The verification methods return errors for each one encountered. Since, there are several implementations, there are several classes to keep the error information. Each of these classes has its own logic.

A detailed list of classes can be given.

In addition, all these methods can not be reached from the API, making their integration in processings, models and plugins hard even impossible. Also, implementing new methods is heavy because it potentially requires to add it in several places.

Finally, users get lost between the different verification and correction tools and they can't create a processing chain either.

Proposed Solution

This QEP proposes to work iteratively to provide an unification of these classes and corrections.

The geometrical verifications concern only one geometry, so they will be in the corresponding geometric classes.

The topological verifications analyze a collection of geometries in one or more layers. They will be in dedicated classes as with the 2 core plugins. We would like to take this occasion to improve the management of the Z/M dimensions (and curves) when it is possible.

There will be a dedicated class for the topological and geometric errors. It will be possible to push a formatted message for the user, also to easily provide error codes and information for the API. We will ensure not to break the existing compatibility, but we will suggest to deprecate some classes and methods.

Finally, for each verification / correction group, there will be a dedicated processing in a new processing group section. Another solution would be to create a processing that groups all detection / correction methods. But the advantage of the proposed solution is to keep a simple interface and to have model that are visually explicit. For geometric verifications, there will be expressions that return a boolean depending on the validity.

Example(s)

Each verification processing will have this similar interface:

A processing will take 1 layer as input and 2 layers as output (including one optional). 4 options will be possibles:

It will be possible to combine options. For example, by choosing "fix", "copy" and "remove", the processing will create 2 layers: one fully valid and another one with the unfixable geometries.

When importing data into QGIS, the data may contain errors. Thanks to the models and the new processings, it will be possible to make a reproducible and scalable processing workflow. It is important to understand how the data was cleaned a posteriori and to be able to reuse the same workflow with similar data. We want to propose an experience closed to other ETLs on these topics.

Affected Files

All files in plugins/topology, in analysis/vector/geoemtry_checker and the most of the files in plugins/geometry_checker.

There are also the core/qgsgeometryvalidator.cpp and core/qgsgeometryvalidator.h files.

All the files implementing verification / correction processings will be largely modified.

Globally, all files using the classes described in summary will probably be modified.

Further Considerations/Improvements

There is a need to have a new option "fix errors" in “Invalid Feature Filtering” (advanced parameter in processing). Another need is to have a more complete set of verifications / corrections, that will be easier with this QEP.

But this QEP have not the goal to modify the usage of Geos in QGIS nor to improve Geos. But this could be the subject of future additions.

Backwards Compatibility

The plugins will not be modified, the behaviors will be the same. But the code of these plugins will simply become an interface to the new classes located in the code.

About the processings, it will be hard not to break them. It is problematic for the current plugins and models. But it will be necessary to rename them as well as to modify their inputs and outputs. And in the case, the verification and the correction are in 2 different processings, it will be necessary to merge the 2 processings.

Issue Tracking ID(s)

qgis/QGIS#26491 qgis/QGIS#35183 qgis/QGIS#41963 qgis/QGIS#41380

Votes

gioman commented 2 years ago

@Koyaani @lbartoletti yes! Whatever it can be done to join together the simplicity of use of the Topology checker with the power of the Geometry checker will be a huge win.

vpicavet commented 2 years ago

That's an ambitious refactoring task ! :spider_web: Congrats for willing to get your hands dirty in all these dusty places of QGIS :-)

roya0045 commented 2 years ago

I'm not sure if my minimal PR on this can be of use or not but it was intended to do something similar but more minimalistically.

nyalldawson commented 2 years ago

I'm so glad to see someone look into this! It's been needed for a long time.

A couple of quick comments:

The geometrical verifications concern only one geometry, so they will be in the corresponding geometric classes.

The QgsAbstractGeometry classes are already quite a heavy API. Would you consider some form of visitor API for verification/repair instead?

The topological verifications analyze a collection of geometries in one or more layers. They will be in dedicated classes as with the 2 core plugins. We would like to take this occasion to improve the management of the Z/M dimensions (and curves) when it is possible.

There will be a dedicated class for the topological and geometric errors. It will be possible to push a formatted message for the user, also to easily provide error codes and information for the API. We will ensure not to break the existing compatibility, but we will suggest to deprecate some classes and methods.

Without seeing greater detail, this makes me a little nervous. It sounds a lot like we'll just end up with another set of error checkers in the code. I'd highly encourage you to explore the classes in analysis/vector/geometry_checker and extend/adapt those instead of adding new classes.

Otherwise we'll end up with just more fragmentation, and methods which work via processing but not via the real-time layer edit constraints. Ouch!

Can you add extra detail about the new classes/changes to existing classes to alleviate my fear?

Koyaani commented 2 years ago

@nyalldawson, I don't forget your comment. With @lbartoletti, we had exchanged on this way of doing. I am currently studying in more detail the different implementations to get a better view of the possibilities.

lbartoletti commented 2 years ago

The geometrical verifications concern only one geometry, so they will be in the corresponding geometric classes.

The QgsAbstractGeometry classes are already quite a heavy API. Would you consider some form of visitor API for verification/repair instead?

@nyalldawson :scream: I completely forgot to send you the answer. :scream:

We talked about visitor API, but not proposed it because there are few (or not) in QGIS and I thought it was not desired. But, we think it could be useful indeed. The SFCGAL API, we are developing, makes extensive use of them. So, yes, it's OK for us.

pathmapper commented 10 months ago

Great that there is now funding available https://blog.qgis.org/2023/09/20/qgis-grant-programme-2023-update/ 🎉

Question regarding the geometrical verifications:

How to deal with cases if there are conflicting validation rules for QGIS/GEOS?

This seems to be the case with the example "Island on an Island" (WKT from https://github.com/qgis/QGIS/issues/44424) :

grafik

If this geometry is constructed as a Polygon, it's valid with ValidatorQgisInternal but invalid with ValidatorGeos ->
Holes are nested

vlayer = QgsVectorLayer("polygon?crs=EPSG:3857", "test_polygon", "memory")
geom = QgsGeometry().fromWkt('Polygon ((0 0, 0 5, 5 5, 5 0, 0 0),(1 1, 4 1, 4 4, 1 4, 1 1),(2 2, 2 3, 3 3, 3 2, 2 2))')
print('QGIS:')
print(geom.validateGeometry(QgsGeometry.ValidatorQgisInternal))
print('GEOS:')
print(geom.validateGeometry(QgsGeometry.ValidatorGeos))
fet = QgsFeature()
fet.setGeometry(geom)
vlayer.dataProvider().addFeature(fet)
QgsProject.instance().addMapLayer(vlayer)

If this geometry is constructed as a MultiPolygon, it's valid with ValidatorGeos but invalid with ValidatorQgisInternal ->
Polygon 1 lies inside polygon 0:

vlayer = QgsVectorLayer("multipolygon?crs=EPSG:3857", "test_multipolygon", "memory")
geom = QgsGeometry().fromWkt('MultiPolygon (((0 0, 0 5, 5 5, 5 0, 0 0),(1 1, 4 1, 4 4, 1 4, 1 1)),((2 2, 2 3, 3 3, 3 2, 2 2)))')
print('QGIS:')
print(geom.validateGeometry(QgsGeometry.ValidatorQgisInternal))
print('GEOS:')
print(geom.validateGeometry(QgsGeometry.ValidatorGeos))
fet = QgsFeature()
fet.setGeometry(geom)
vlayer.dataProvider().addFeature(fet)
QgsProject.instance().addMapLayer(vlayer)

It would be good if there would be one way to construct the geometry so that it is valid according to ValidatorQgisInternal and ValidatorGeos.

strk commented 10 months ago

I think validity only ever makes sense in relation to a specification, on which algorithms depend. GEOS itself has different validity kind which can be requested using a flag. And PostGIS for example has an SFCGALValid (can't remember the exact name) which verifies validity according to the SFCGAL specs. They are not necessarely the same.

I don't think validity should be unified, but rather the user should be given a choice of what kind of validity to check.

pathmapper commented 10 months ago

validity only ever makes sense in relation to a specification, on which algorithms depend.

:+1:

I don't think validity should be unified, but rather the user should be given a choice of what kind of validity to check.

This is currently the case with the "Check Validity" processing tool.

Accordingly it might be good to be able to choose kind of validity for the "Fix geometries" tool.

But it's hard for a user to determine which kind of geometry validity is needed for a certain algorithm (processing tool) or use case.

jfbourdon commented 10 months ago

Something that would be great is if the help of a unified "Check validity" processing tool for example could provide a link to a page where is listed what exactly is checked for each method/standard. Currently, we can select "QGIS" or "GEOS" method with this tool, but it's not clear what are the differences. Having a reference page like this for OGC validity with PostGIS or this for OGC validity with FME would be of great help.

Accordingly it might be good to be able to choose kind of validity for the "Fix geometries" tool.

Knowing what exactly "Fix geometries" is able to fix would be great! It seems to be like GEOS makeValid() but being explicit would be a plus.

pathmapper commented 10 months ago

The above MultiPolygon is valid according to OGC Simple Feature -> https://github.com/qgis/QGIS/issues/54846

lbartoletti commented 8 months ago

I'd highly encourage you to explore the classes in analysis/vector/geometry_checker and extend/adapt those instead of adding new classes.

@nyalldawson indeed, geometry_checker is the good place to merge algorithms from topology checker and missing ones. I created some processing with geometry_checker and it's ok for us.

anitagraser commented 7 months ago

Hi @Koyaani and @lbartoletti . I'm working on wrapping up the 2023 grant programme. How is the progress on this project? Can you give me an estimate for the completion and final report? As you probably know, I'm collecting all final reports and summarizing them so everyone can read up on the results of the programme.

lbartoletti commented 7 months ago

Hi @Koyaani and @lbartoletti . I'm working on wrapping up the 2023 grant programme. How is the progress on this project? Can you give me an estimate for the completion and final report? As you probably know, I'm collecting all final reports and summarizing them so everyone can read up on the results of the programme.

Hi @anitagraser, with @Djedouas will send a mail on PSC list very soon (next weekt) to write our report. Sorry, for this late response.

anitagraser commented 6 months ago

@lbartoletti Thank you for responding. I've checked the mailing list (https://lists.osgeo.org/pipermail/qgis-psc/) but couldn't locate the mail. Could you point me to where I can find the report?

lbartoletti commented 6 months ago

@lbartoletti Thank you for responding. I've checked the mailing list (https://lists.osgeo.org/pipermail/qgis-psc/) but couldn't locate the mail. Could you point me to where I can find the report?

Done, and against, sorry for the late. 🙏

gioman commented 6 months ago

Done, and against, sorry for the late. 🙏

@lbartoletti was this https://github.com/qgis/QGIS/issues/35183 implemented in this work?