Fix bug #9655: some polygons not labelled when layer's simplification is on

[ahuarte47]

This pull request fixes the bug http://hub.qgis.org/issues/9655

Changes:

• fix a bug in the simplification of multipolygon geometries
• fix invalid polygons using buffer_0

[nirvn]

@ahuarte47 have a look at this too: http://hub.qgis.org/issues/9673

[ahuarte47]

Hi @nirvn, thanks, these issues are related, PalLabeling fails with simplified geometries (with edges that intersect).

[dakcarto]

I don't think we want to continue on returned exception here, do we?
i.e., instead:

// ignore invalid geometries (e.g. polygons with self-intersecting rings)
int geomvalid = GEOSisValid( geom ); // 0=invalid, 1=valid, 2=exception
if ( geomvalid == 2 || ( ignoreInvalidGeometries && geomvalid == 0 ) )
{
  std::cerr << "ignoring invalid feature " << geom_id << std::endl;
  continue;
}

[ahuarte47]

Hi, I ignored fully the geometry validation because I have thought it best to at least a value is calculated, and not silently not return anything, but if you think your code better I use it

[dakcarto]

I think it would be best to be conservative here. The developers of PAL put the geometry validity check in there for a reason.

Even allowing further processing of invalid geometries (hard-coded in your request) may turn out to be problematic, which is why it probably needs a GUI checkbox. But IMO, allowing further processing when a simple validity check fails with an exception seems like it will lead to problems.

Regardless, further testing needs done.

[dakcarto]

This seems like a good approach, but I just have to wonder if there is ultimately a good reason for PAL to ignore invalid geometries. I couldn't get it to crash or throw an error with the sample data (from issue 9655).

Also, with Simplify geometry ON, I do not get good results, e.g. if any extent clipping happens to the noted problematic simplified feature, it is not labeled. This includes only clipping off an island, which should leave a valid geometry. I think there is something else going on here, stemming from simplification of multi-geometry features. I don't think this fixes issue 9655, though should probably be committed as is nonetheless.

We can add a checkbox to the Labels->Rendering GUI later for "Allow labeling of invalid features" if needed.

[ahuarte47]

You are right, when the invalid geometry is clipped with the extent, the intersection method fails...
https://github.com/qgis/QGIS/blob/master/src/core/qgspallabeling.cpp#L1851

The solution definitely seems implement the 'ST_makeValid()' method

---

De: Larry Shaffer notifications@github.com
Para: qgis/QGIS QGIS@noreply.github.com
CC: Alvaro Huarte ahuarte47@yahoo.es
Enviado: Lunes 3 de marzo de 2014 22:37
Asunto: Re: [QGIS] Fix bug #9655: some polygons not labelled when layer's simplification is on (#1219)

This seems like a good approach, but I just have to wonder if there is ultimately a good reason for PAL to ignore invalid geometries. I couldn't get it to crash or throw an error with the sample data (from issue 9655).
Also, with Simplify geometry ON, I do not get good results, e.g. if any extent clipping happens to the noted problematic simplified feature, it is not labeled. This includes only clipping off an island, which should leave a valid geometry. I think there is something else going on here, stemming from simplification of multi-geometry features. I don't think this fixes issue 9655, though should probably be committed as is nonetheless.
We can add a checkbox to the Labels->Rendering GUI later for "Allow labeling of invalid features" if needed.
—
Reply to this email directly or view it on GitHub.

[ahuarte47]

Hi @dakcarto, I have changed how resolve the issue. I use 'buffer' method to convert the geometry to valid, and it works better

[dakcarto]

Hmm. Seems I tried the 'GEOSBuffer - 0' method before as a quick fix. Didn't work so well for me, but maybe it was how/when I did it. Will test your changes ASAP. Thanks.

[ahuarte47]

Thanks for your tests @dakcarto

[dakcarto]

@ahuarte47 Sorry for the delay. I tested with your latest change, but also with the previous changes and my suggested edit (above) as a combined patch. Issue #9655 now seems to be fixed, as well as #9673.

Could you force push your changes again, so they include everything (including the compiler warning fix)?

I'd like to test on Mac 10.9 as well, but that shouldn't hold up the push of the request.

[dakcarto]

@ahuarte47 here is the patch (all changes, combined) that works for me: https://gist.github.com/dakcarto/9380317

[ahuarte47]

Hi @dakcarto, I think now it is not necessary change the "core/pal/layer.cpp" and "core/pal/layer.h", that is why I removed it from the pull.

With the current modification, the geometry is tested before execute any labeling code.
The key is ...
https://github.com/ahuarte47/QGIS/compare/Issue_9655-1#diff-a3ce81e20eab5de8d69fffffff63d41fR1844

[dakcarto]

Ok, looks good here on Mac OS X 10.9. Should be merged IMO.

[mhugent]

assigned to @dakcarto

[ahuarte47]

Thank you very much @dakcarto