Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix bug #9777: Simplification of polygons may return invalid polygons -> ST_MakeValid #1241

Closed
wants to merge 7 commits into from

Conversation

ahuarte47
Copy link
Contributor

This pull fixes the bug 9777 (http://hub.qgis.org/issues/9777) or others errors when GEOS uses an invalid geometry (labeling, centroid calculations, spatial operations, ...):

It implements the -ST_MakeValid- function for QgsGeometry class. The code is a fork from the original code in postgis implementation with slight changes:

https://github.com/postgis/postgis/blob/svn-trunk/liblwgeom/lwgeom_geos_clean.c

Also, the user can configure automatic validation of the fetched geometries of a layer and convert them to valid when needed.

The validation code is only executed when needed, drawing labels of a simplified geometry, or any code requires the GEOS geometry and this flag is enabled. If a simplified geometry is drawed without internally call to 'exportWkbToGeos()', as usual, then the validation is not executed and there is no penalty in the performance.

@ahuarte47 ahuarte47 changed the title Fux bug #9777: Simplification of polygons may return invalid polygons Fix bug #9777: Simplification of polygons may return invalid polygons Mar 18, 2014
@ahuarte47
Copy link
Contributor Author

Hi, now I'm working in this pull, it is not finished. I'm implementing ST_MakeValid function in QgsGeometry to solve this issue better and then convert to valid the self-crossing geometries.

@ahuarte47 ahuarte47 changed the title Fix bug #9777: Simplification of polygons may return invalid polygons Fix bug #9777: Simplification of polygons may return invalid polygons -> ST_MakeValid Apr 2, 2014
@ahuarte47
Copy link
Contributor Author

There are two new options to configure the automatic validation of geometries. One new CheckBox to define the behavior for newly added layers, and other CheckBox for each layer.

st_makevalid_global

st_makevalid_layer

@dakcarto
Copy link
Member

dakcarto commented Apr 2, 2014

@ahuarte47 Nice work on this! I have not tested it, but it seems to me the wording for the option might be better if it was something like:

Enable automatic repair of invalid geometries for newly added layers
and
Enable automatic repair of invalid geometries for this layer

because 'validation' refers to just checking the validity of the geometry, whereas your routine is attempting to repair geometries found to be invalid.

@ahuarte47
Copy link
Contributor Author

Done, thank you very much @dakcarto !

@mhugo
Copy link

mhugo commented Apr 4, 2014

@ahuarte47 It seems nice !
I am just wondering if the liblwgeom code should be externally linked or copied here. I am usually not really for code duplication, but liblwgeom may be not very well packaged ... not sure ...

@pcav
Copy link
Member

pcav commented Apr 4, 2014

liblwgeom is well packaged now, and included in the standalone installer for win; in fact, I suggest to make more use of it, e.g. adding the relevant plugin to the standard qgis installation.
in short, +1 from me to an external link

@ahuarte47
Copy link
Contributor Author

Hi, I am adding this code to GEOS lib in its github repo, but while it is not published and QGIS not use the new version, we already have that functionality. Do you agree ?

@mhugo
Copy link

mhugo commented Apr 4, 2014

Not sure to understand. You plan to add this to GEOS ? Isn't it already extracted from liblwgeom ?

@ahuarte47
Copy link
Contributor Author

Sorry, I did not express myself well, I want append this "ST_makeValid" method in the class GEOSGeometry to be directly used in QGIS in class QgsGeometry when possible.

@mhugo
Copy link

mhugo commented Apr 4, 2014

Ok. So yes if you managed to get this directly into GEOS, that would be perfect. With a test for GEOS version in the QGIS source I guess.

@jef-n
Copy link
Member

jef-n commented Apr 4, 2014

@ahuarte47 don't forget to add a C API for the GEOSGeometry method.

@ahuarte47
Copy link
Contributor Author

ok, I had thought to add the code to GEOS when it is found in QGIS that this is correct.

@ahuarte47
Copy link
Contributor Author

Hi @mhugo, about ...

'... You plan to add this to GEOS ? Isn't it already extracted from liblwgeom ? '

The functionality is in PostGIS. It just uses GEOS calls, but the actual fixing is done in PostGIS:

https://github.com/postgis/postgis/blob/svn-trunk/liblwgeom/lwgeom_geos_clean.c

I have done a simple refactoring for inclusion in QgsGeometry. this code I want to include in GEOS (and C API).

https://github.com/ahuarte47/QGIS/blob/12133d5480c7e9a69050cb67bd66b54880cfcab8/src/core/qgsgeometryvalidator.cpp#L356

Best Regards
Alvaro

@mhugent
Copy link
Contributor

mhugent commented May 12, 2014

assigned to @jef-n

@nirvn
Copy link
Contributor

nirvn commented Oct 8, 2014

@jef-n @ahuarte47 could we please commit this before 2.6 is released? I regularly get contacted by students who wonder why polygons aren't being labelled. I just ran into this issue myself today:

missing_labels

@gioman
Copy link
Contributor

gioman commented Oct 8, 2014

agree, this would be very important to merge now so we can test before the release.

@ahuarte47
Copy link
Contributor Author

Hi @nirvn @gioman, this pull request fix the issue, QGIS now shows all labels.

#1617

This new PR is independent but complementary to the ST_MakeValid implementation of this PR.

Best Regards
Alvaro

@strk
Copy link
Contributor

strk commented Sep 10, 2015

What's left to do here @ahuarte47 ? Can you rebase to master and ensure travis passes ?

@ahuarte47
Copy link
Contributor Author

Hi sandro, this pull request is obsolete from the changes in the QgsGeometry class using now the QgsGeometryPrivate struct. The automatic validation and repair of the geometry can not be needed now.

But it would possible append the method QgsGeometry::makeValid for QGIS developers (cloned from method ST_MakeValid in postgis source code) if you think it as interesting. Otherwise, you can close this PR freely.

Alvaro

@strk
Copy link
Contributor

strk commented Sep 10, 2015

I'll close. Beside I think there was some interest in linking liblwgeom directly, that'd make the "makeValid" algorithm available to QGIS too.

@strk strk closed this Sep 10, 2015
@ahuarte47
Copy link
Contributor Author

ok, thanks @strk

@nyalldawson
Copy link
Collaborator

@strk @ahuarte47 do you know what the current status regarding liblwgeom is? I gather that it's now its own library, independent from PostGIS. Should we revisit now making a makevalid function available in QGIS?

@ahuarte47
Copy link
Contributor Author

liblwgeom implements it in:
https://github.com/postgis/postgis/blob/svn-trunk/liblwgeom/lwgeom_geos_clean.c#L852

The code uses GEOS library.

@strk
Copy link
Contributor

strk commented Nov 16, 2015 via email

@ahuarte47
Copy link
Contributor Author

Hi, related to this issue (many times caused by on-the-fly-simplification), I would like propose a change to avoid them!

http://hub.qgis.org/issues/3517#note-17

Best regards
Alvaro

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants