Bug report #3517

new labeling engine fails to label self intersecting polygons

Added by Mathieu Pellerin - nIRV over 8 years ago. Updated over 3 years ago.

Status:Closed
Priority:Normal
Assignee:-
Category:Labelling
Affected QGIS version:master Regression?:No
Operating System: Easy fix?:No
Pull Request or Patch supplied:No Resolution:fixed/implemented
Crashes QGIS or corrupts data:No Copied to github as #:13577

Description

The summary says it all. The new labeling engine fails to label self intersecting polygons.

The old labeling engine is tolerant to these types of polygon and provides labeling.

It actually took me some time to understand why qgis's new labeling engine was refusing to label some polygons from a shapefile I was given.

qgis-labeling-missing.jpg (74.4 KB) Mathieu Pellerin - nIRV, 2011-02-23 01:34 AM

qgis-labeling-ok.jpg (75.1 KB) Mathieu Pellerin - nIRV, 2011-02-23 01:34 AM

Capture.JPG (30.3 KB) Alex Leith, 2013-11-06 09:47 PM

evidence.png (26.4 KB) Mathieu Pellerin - nIRV, 2016-06-16 02:29 AM

History

#1 Updated by Paolo Cavallini almost 8 years ago

  • Category changed from Symbology to Labelling
  • Assignee deleted (nobody -)
  • Pull Request or Patch supplied set to No

#2 Updated by Giovanni Manghi almost 8 years ago

  • Target version changed from Version 1.7.0 to Version 1.7.4

#3 Updated by Paolo Cavallini over 7 years ago

  • Crashes QGIS or corrupts data set to No
  • Target version changed from Version 1.7.4 to Version 1.8.0
  • Affected QGIS version set to master

#4 Updated by Paolo Cavallini about 7 years ago

  • Target version changed from Version 1.8.0 to Version 2.0.0

#5 Updated by Giovanni Manghi about 7 years ago

  • Operating System deleted (Debian)
  • Status info deleted (0)

at least I would consider it a good way to know that I have a self intersecting geometry :)

#6 Updated by Mathieu Pellerin - nIRV about 7 years ago

Giovanni, a silent unexplained failure is not a good UX in any kind of way :)

I think there could be great benefit in QGIS indicating, either in the labelling dialogue itself or elsewhere, that a layer contains intersecting geometry that will prevent some labels from showing up. That way, your not leaving the user with a "huh, label now showing, black voodoo" but instead a useful hint that leads said user into fixing issue.

Btw Giovanni, thanks for the efforts your putting into keeping the hub's issues refreshed.

#7 Updated by Giovanni Manghi about 7 years ago

nirvn - wrote:

Giovanni, a silent unexplained failure is not a good UX in any kind of way :)

of course, I was just joking :) cheers!

#8 Updated by Gerhard Spieles about 7 years ago

Hi,

see also #2949
It seems to be fixed?

Gerhard

#9 Updated by Giovanni Manghi about 7 years ago

gespiel - wrote:

Hi,

see also #2949
It seems to be fixed?

Gerhard

no, it works for lines, but it fails for polygons.

#10 Updated by Larry Shaffer about 7 years ago

  • Priority changed from Low to Normal

See also #4079-23

and

http://osgeo-org.1560.n6.nabble.com/Add-ST-MakeValid-functionality-to-QgsGeometry-td5002449.html

nirvn - wrote:

The old labeling engine is tolerant to these types of polygon and provides labeling.

The old labeling engine essentially just labeled points. How a point was generated for a prospective label was determined by the user options for the given vector layer type. As noted in the above 'see also', creating a centroid now has the same apparent tolerance for some invalid geometries. So, I think the old and new labeling engines are now on par with each other in this regard.

However, any label where an invalid polygon (not as a point) has to go through PAL to produce some of the newer labeling effects (Free, Perimeter, etc ) will currently silently fail. Adding some means of fixing those geometries, before sending through PAL might alleviate the issue, and allow the feature to be labeled.

I think there could be great benefit in QGIS indicating, either in the labelling dialogue itself or elsewhere, that a layer contains intersecting geometry that will prevent some labels from showing up. That way, your not leaving the user with a "huh, label now showing, black voodoo" but instead a useful hint that leads said user into fixing issue.

This is a good idea. The simplest thing would be to add a button in the labeling dialog to run fTools 'Check geometry validity'. Having such a function run automatically on dialog open may cause a long delay (for no apparent reason to the user) if there is a lot of geometry to check, especially if the function is to check all features in a layer, not just those currently visible.

Another option might be to add a 'Notify about unlabeled invalid geometries' checkbox to the labeling dialog (unchecked by default). Then, when features for that layer are encountered that are invalid (at the current zoom level, within the extent and not as a result of extent clipping) the fTools 'Check geometry validity' dialog would pop up listing invalid features found during the current labeling operation. That fTools dialog already has the ability to pan to those features and display the red X. This way, if a user does not see an expected label show up, they have an option to turn on to help find the culprit. The notify dialog would be invalid geometries of the current subset of features being labeled, with possibly a shortcut button to run 'Check geometry validity' for the whole layer.

#11 Updated by Alex Leith about 6 years ago

I just noticed this bug is still about in version 2.0 64 bit on Windows.

It was difficult to discover that it was invalid geometry causing it, and I was just now able to reproduce the missing labels on invalid geometry by drawing a trivial polygon with on self-intersection.

I think that falling back on a 'dodgy' centroid is much preferred over silently failing.

#12 Updated by Paolo Cavallini almost 6 years ago

  • Target version changed from Version 2.0.0 to Future Release - High Priority

#13 Updated by Alvaro Huarte over 5 years ago

I have implemented the ST_MakeValid function for QgsGeometry class to fix the related issues #9861 and #9777. 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.

https://github.com/qgis/QGIS/pull/1241

Best regards
Alvaro

#14 Updated by Giovanni Manghi about 4 years ago

  • Status changed from Open to Closed
  • Resolution set to fixed/implemented

#15 Updated by Nyall Dawson about 4 years ago

  • Status changed from Closed to Reopened

Giovanni - that PR was never merged, and this is still an issue in master

#16 Updated by Giovanni Manghi about 4 years ago

  • Resolution deleted (fixed/implemented)

Nyall Dawson wrote:

Giovanni - that PR was never merged, and this is still an issue in master

Hi Nyall, yes I have seen that the PR was not committed, but labelling self intersecting polygons works for me. Maybe there is a better solution (the label do not seems to be over the centroid of the whole geometry) but is better than before (no label) :)

#17 Updated by Alvaro Huarte about 4 years ago

Hi, I would like to propose an idea in order to avoid these problems derivated from on-the-fly-simplification.

Now, the geometries are simplified just when the data provider fetchs the geometry (https://github.com/qgis/QGIS/blob/master/src/providers/ogr/qgsogrfeatureiterator.cpp#L322). These simplified geometries can be invalid and there are some problems with it in labeling engine, or expression evaluations that use its geometric attributes, and so on.

For all that, as I said, I would like to propose move the simplification to just where the geometry is converted to be drawed (https://github.com/qgis/QGIS/blob/master/src/core/symbology-ng/qgsrendererv2.cpp#L317). I think this is slower but safer than current status.

What do you think?

Best regards
Alvaro

#18 Updated by Alvaro Huarte about 4 years ago

Sorry, I forgot to say that the simplification executed in provider side remains unchanged. For postgis it executes "ST_snapToGrid" or "st_removerepeatedpoints" functions.

#19 Updated by Regis Haubourg about 4 years ago

Alvaro Huarte wrote:

Hi, I would like to propose an idea in order to avoid these problems derivated from on-the-fly-simplification.

Now, the geometries are simplified just when the data provider fetchs the geometry (https://github.com/qgis/QGIS/blob/master/src/providers/ogr/qgsogrfeatureiterator.cpp#L286). These simplified geometries can be invalid and there are some problems with it in labeling engine, or expression evaluations that use its geometric attributes, and so on.

For all that, as I said, I would like to propose move the simplification to just where the geometry is converted to be drawed (https://github.com/qgis/QGIS/blob/master/src/core/symbology-ng/qgsrendererv2.cpp#L317). I think this is slower but safer than current status.

What do you think?

Best regards
Alvaro

Hi, that sounds reasonable. Nyall, Martin, opinions?

#20 Updated by Nyall Dawson about 4 years ago

For all that, as I said, I would like to propose move the simplification to just where the geometry is converted to be drawed

I'm not a fan of this approach. I think it's great how simplification also aids labelling at the moment. For instance, without simplification the "free" and "horizontal" label placements are so slow as to be nearly unusable. With simplification, they are fast enough to be used interactively.

I'd rather see the approach where liblwgeom is used to repair broken geometries explored.

#21 Updated by Alvaro Huarte about 4 years ago

Nyall Dawson wrote:

I'm not a fan of this approach. I think it's great how simplification also aids labelling at the moment. For instance, without simplification the "free" and "horizontal" label placements are so slow as to be nearly unusable. With simplification, they are fast enough to be used interactively.

Hi, another alternative could be to add the "SnapToGrid" algorithm (See liblwgeom: https://github.com/postgis/postgis/blob/svn-trunk/liblwgeom/ptarray.c#L1798) and check results. It is pretty simple and feasible integrate it in https://github.com/qgis/QGIS/blob/master/src/core/qgsmaptopixelgeometrysimplifier.cpp#L254 defining a new "algorithm" enum (possibly configurable).

#22 Updated by Nyall Dawson about 4 years ago

That sounds like a better approach. I also think snap to grid simplification could be useful for composer exports... Eg, geometries could be simplied to a grid which is twice dpi, to remove redunant points from the resultant svg/PDF files.

#23 Updated by Alvaro Huarte about 4 years ago

I think I might try adding this new algorithm to test results (I will try to get time). It is complementary to the ST_makeValid implementation commented in other dedates.

#24 Updated by Alvaro Huarte almost 4 years ago

Hi, I have created a new pull https://github.com/qgis/QGIS/pull/2483 to test results using the "SnapToGrid" algorithm by default.

It is a try to resolve the issue.

Alvaro

#25 Updated by Mathieu Pellerin - nIRV over 3 years ago

  • File evidence.png added
  • Status changed from Reopened to Closed
  • Resolution set to fixed/implemented

I've tried labelling an simple intersecting polygon today, it worked (to my surprise actually). Closing, fixed.

Also available in: Atom PDF