systemed / tilemaker

Make OpenStreetMap vector tiles without the stack
https://tilemaker.org/
Other
1.49k stars 231 forks source link

Incorrect render for a large multipolygon lake #191

Open ttsirkia opened 3 years ago

ttsirkia commented 3 years ago

I don't know if this is related to some existing and known issues:

kuva

The multipolygon area overlaps with other water area and there is a blank area without any water. This should be Lake Saimaa, https://www.openstreetmap.org/relation/7379046#map=9/61.3026/28.4216

As you can see, it is very complex area. The picture here is based on Geofabrik's latest Finnish file and is drawn here without any simplification. Are there any tricks that I could try?

ttsirkia commented 3 years ago

This is made with the latest build from master branch. I'm wondering if this is related somehow to #181 although I understood that there should be a workaround for that in the latest commits.

systemed commented 3 years ago

Ugh, that's insanely complex. Unfortunately something that big is going to be near-impossible to reduce to a testcase.

One possible reason is that Tilemaker can't cope with non-closed inners, and there are several in here (e.g. https://www.openstreetmap.org/way/868890259). A quick hack to ignore them might be adding a check around line 247 of osm_store.h, viz:

Ring inner;
if (!ways.isClosed(*it)) { continue; } /* <--- new */
if (ways.count(*it)==1) { fillPoints(inner, ways.at(*it)); }
ttsirkia commented 3 years ago

Thanks! I'll try that later today.

ttsirkia commented 3 years ago

I found another position in which the lake polygon is not that complex:

kuva

https://www.openstreetmap.org/relation/409844#map=14/62.2353/28.8916

systemed commented 3 years ago

Yes, that does look like an issue with non-closed inners (the break at the top island is exactly where that would happen).

systemed commented 3 years ago

I've just pushed https://github.com/systemed/tilemaker/commit/12cb7422fa5431fbf52967daffcabf7c76d1fa21 which will piece together multiple ways to form an inner. It seems to fix both of the above examples on a quick test, though I've not looked in great detail.

There still appears to be an issue at z7/8 where Sääminginsalo disappears - unsure what's going on there but it's possibly a simplification issue.

ttsirkia commented 3 years ago

Oh, that's cool! I'll compile the new version, recreate the tiles and see how it behaves. Thank you so much!

ttsirkia commented 3 years ago

Hmm, here it looks like this:

kuva

and

kuva

I've used Osmium to pick only water areas osmium tags-filter finland-latest.osm.pbf a/natural=water --overwrite --verbose -o fin_water.osm.pbf but I guess it shouldn't otherwise affect the result.

systemed commented 3 years ago

Can you tell me where those are? And what zoom levels?

ttsirkia commented 3 years ago

The big area is 7.38/62.059/28.879 and the origin of spikes is 9.2/61.0754/28.0492. I use tileserver-gl-light to visualize the raw data of the mbtiles file. I processed these with the original Lua file in resources folder but removed the simplification for water totally and set min zoom level to be four for all.

ttsirkia commented 3 years ago

Strange, the spikes will disappear in zoom level 9 after MapBox has redrawn the map. They come back when zooming to 8.

ttsirkia commented 3 years ago

This can also be a problem with MapBox, I'll try to re-enable the simplification and setting a proper min zoom level. Probably the tile contains too much information.

systemed commented 3 years ago

I don't see any issues at z10, but at z9 various islands start getting dropped. I certainly don't see any renderings like the above, but different clients may of course treat invalid geometries in different ways.

I think this is going to need a bit of time to track down, creating a minimal test case (i.e. one that doesn't take hours to process!) and that's not time I have at the moment. But I'll keep this open for future reference.

Places to look (for my reference):

ttsirkia commented 3 years ago

Yes, this is unfortunately very time consuming. My tiles are still being processed.

I noticed yesterday that some lakes disappeared too quickly and realized that there might be a problem with setting the minimum zoom. The values (meters per pixel) apply for the equator but not for north: https://docs.mapbox.com/help/glossary/zoom-level/ And I'm not sure if they are one level off or not. That was the reason I disabled that part of the Lua code but didn't have more time yet to continue that research.

systemed commented 3 years ago

Out of interest, do you know what Boost version you have?

ttsirkia commented 3 years ago

It is 1.71.0-6ubuntu6. I'm running Ubuntu 20.04 LTS under WSL.

ttsirkia commented 3 years ago

Finally got the new tiles with simplification and min zoom setting enabled. Those issues in the pictures above disappeared. So it was a MapBox GL JS issue. But I noticed that when zooming out, when the simplification will start, some land areas become water and vice versa. And there seems to be two overlays of water in some places. That is probably a simplification issue.

Another drawing issue is here: kuva

https://www.openstreetmap.org/relation/445551#map=13/62.7629/28.5447

And somehow it feels that the latest commit is slower in generating the tiles, is the algorithm more time consuming?

systemed commented 3 years ago

It will be a bit slower, yes, because it's piecing together ways to make the multipolygon inner rings and that takes time. I think there's probably scope for a bit of optimisation though.

The land/water issue is the same as I'm seeing.

ttsirkia commented 3 years ago

I can also see that some polygons get duplicated, such as this lake: https://www.openstreetmap.org/way/31185773 However, I'm not sure in which part of the process this happens. The raw viewer of tileserver-gl-light uses an opacity for the lakes and I can easily see that there are lots of this kind of duplicated lakes.

ttsirkia commented 3 years ago

To validate the input data, I used osmium to produce a geojson file for the same water data and then I used tippecanoe to produce the tiles. I can see the duplicated lakes there as well so that is not related to tilemaker. The rendering issues were not visible in the result of tippecanoe.

systemed commented 3 years ago

The issue with overlapping lakes appears to be at least partly an OSM mapping issue.

Puruvesi (https://www.openstreetmap.org/relation/899111) and Saimaa (https://www.openstreetmap.org/relation/7379046) cover the same area, using the same member ways. This looks like pretty bad practice to me and I've just posted to the OSM lists about it.

I'll continue to look into the rendering issues.

systemed commented 3 years ago

Ok, this is a bit of a hack, but I think it's the only sensible way forward right now:

The Saimaa multipolygon is insane (7223 members) and entirely duplicates the geometry of other relations (Pihlajavesi, Puruvesi, Haukivesi, etc.). There is no need for tilemaker to render it.

I'm not sure exactly what it is that's causing tilemaker to flake out with it: it might be geometry breakage (tilemaker is less resilient at recovering from this than a PostGIS setup), it might simply be so big that it's hitting some internal limit, it might be an issue with Boost.geometry, or it might be that tilemaker has a design issue which is causing the processing the geometry properly. Because the multipolygon is so big and complex it's very, very hard to debug, even using a .pbf which just contains this multipolygon and nothing else.

I've started a conversation on the OSM lists about removing this multipolygon, but who knows where that will go. Until then, we can filter it out in the Lua script. At line 353 of process-openmaptiles.lua, adding:

if class=="lake" and way:Find("wikidata")=="Q192770" then return end

will skip over Saimaa. The individual lakes that make it up are still rendered, and as a bonus, processing speed is several times faster as it doesn't have to keep clipping this ridiculously large polygon!

ttsirkia commented 3 years ago

Thank you very much for your effort!

mboeringa commented 3 years ago

@ttsirkia

I technically fixed the Saimaa multipolygon using JOSM today. There were a few issues needing resolving, including a self intersection. Would be interesting to know if this fixes your issues.

I did not resolve all warnings generated by JOSM, as they were irrelevant to the geometrical issues, nor did I resolve potential overlaps between different water tagged (multi)polygons (not sure if the current database contains any).

ttsirkia commented 3 years ago

Thanks for pinging, wow! I took the latest version of tilemaker and downloaded the latest Finnish .osm.pbf. I couldn't find any problems with the lakes now.

I'll close this issue, thanks once more!

ttsirkia commented 3 years ago

Actually, the only problem seems to be that the latest version of tilemaker doesn't show Saimaa at all, so this https://www.openstreetmap.org/relation/7379046 is missing.

kuva

9/61.4/27.8934

But maybe this is another story.

ttsirkia commented 3 years ago

Oops, I had still the hack from February in the Lua file to skip that particular relation. Let me try again.

ttsirkia commented 3 years ago

It took several hours to build the tiles now. Unfortunately, the problem still exists.

image

7/62.259/28.249

ttsirkia commented 3 years ago

But for example. the location I mentioned yesterday now looks fine:

image

9/61.4/27.8934

It looks like those spikes are visible if the zoom level is 8 or less.

mboeringa commented 3 years ago

@ttsirkia

If the high zoom display of the relation is OK, than that seems to indicate that my fix of the multipolygon is fine, and that the issue is possibly related to the generalization routines likely used for low zoom display.

ttsirkia commented 3 years ago

I can try again with different simplification settings to see does it change anything.

arturbil commented 2 years ago

Hi @ttsirkia, did you have any luck with the simplification settings? I'm stuck with a similar problem. These Finnish lakes are driving me insane.

ttsirkia commented 2 years ago

I tried a few settings without any change but I can still try some more variations.

mboeringa commented 2 years ago

Hi @ttsirkia, did you have any luck with the simplification settings? I'm stuck with a similar problem. These Finnish lakes are driving me insane.

Apart from possible issues with the simplification processing, please do post links to the actual features / multipolygon relations you are having problems with. I can have a look if I can technically fix them (if needed and JOSM shows issues), so that the source data in OSM is at least right. I have dealt with other complex issues like this before.

arturbil commented 2 years ago

I have a problem with the Saimaa lake (https://www.openstreetmap.org/relation/7379046) mentioned above. If I don't simplify it, Maplibre (based on Mapbox GL JS) doesn't render it properly, If I do simplify, then the peninsula south-east of Puumala dissapears at zoom 7 (but seems ok at other zooms, like 6 or 8): puumala

mboeringa commented 2 years ago

@arturbil

I have loaded the Saimaa MP in JOSM, and JOSM reported one minor geometric error about an "inner" falsely tagged as "outer" in the MP relation.

I have now fixed this. I doubt if it is related to your issue, but you might try and attempt to download the latest version of the Saimaa MP.

I noticed there were a couple of people having made edits to this MP since the last time I fixed it, some of them using iD... I shudder to think how you would handle this monster in iD...

arturbil commented 2 years ago

Thank you for your effort, @mboeringa. As you expected, it didn't help in this case. It feels the issue is more on the tilemaker side, as the lake seems ok on all zoom levels if I generate tiles with another tool (tippecanoe).

ttsirkia commented 2 years ago

I've also noticed that tippecanoe is able to generate the tiles. Maybe it has some internal error fixing mechanism or the algorithm is somehow different.

systemed commented 2 years ago

Multipolygons can be insanely complex and there's not much that can cope with everything flawlessly apart from the JTS/GEOS/GDAL/PostGIS family.

If you generate tiles from OSM data with tippecanoe, you are probably taking an intermediate step via ogr2ogr (essentially a wrapper around GEOS) to convert the .osm.pbf into the .geojson required by tippecanoe. GEOS is doing the hard work there of rationalising OSM's nutso multipolygons into the more orderly GeoJSON spec.

tilemaker doesn't use the JTS/GEOS stack, it uses Boost.Geometry with our own code for some operations. tilemaker's multipolygon support is now pretty good, not least due to the superb dissolve code introduced by @kleunen in #249, and it can cope with almost everything. But there may still be issues on the really complex ones such as Saimaa and US National Forests. If we could isolate these down to a simple test case that would be ideal, rather than needing to crunch through the Finland extract each time, but I suspect this won't be an easy one to fix.

kleunen commented 2 years ago

If you have a test polygon, i can add it to: https://github.com/kleunen/boost_geometry_correct

ttsirkia commented 2 years ago

Yes, it would be very useful to be able to extract the simplest not-working lake and check the data for that. I've used osmium to provide the JSON data for tippecanoe.

mboeringa commented 2 years ago

Multipolygons can be insanely complex and there's not much that can cope with everything flawlessly apart from the JTS/GEOS/GDAL/PostGIS family.

But there may still be issues on the really complex ones such as Saimaa and US National Forests.

@systemed, although I certainly haven't inspected all of the 7k+ parts and 400k+ nodes of the Saimaa MP, I do have the feeling the Saimaa MP - from a pure technical point of view in relation to how MultiPolygons are supposed to be assembled in OSM and in relation to the Simple Features specification - is not in such a bad state at all. In fact, as I showed, it validates fine in JOSM after my last fix.

I think the Saimaa MP in that respect is quite different from the US National Forests polygons you mention, that suffer from many "one-point" touching parts and all kinds of other stuff that probably won't validate in JOSM, nor fit the SF specifications.

That said, I fully appreciate this is an insanely big one. On the other hand, I also think this is quite an exceptional situation. There aren't that many geological and climatic areas on the globe, where this type of "lake" can realistically form, so I don't think we will see many more of the complexity of Saimaa.

I also wouldn't hesitate to break it up in smaller portions, if there was realistically a logical way to do it (or at least one that makes sense to the local Finish community because it is ultimately their call to decide). E.g. there appears to be an overlapping MP with its own name covering part of the same areas.

Lastly, of course I also fully appreciate the fact that simplifying polygons this complex, is bound to introduce a whole range of new issues not present in the original Saimaa MP. So the original MP being valid or not, is ultimately of little comfort or consequence when developing a generalization processing workflow. Any generalization processing flow, needs its own "validity / MakeValid" procedures, and of course there is https://github.com/kleunen/boost_geometry_correct in this case.

pnorman commented 2 years ago

If we could isolate these down to a simple test case that would be ideal, rather than needing to crunch through the Finland extract each time, but I suspect this won't be an easy one to fix.

Would downloading the relation and converting it to PBF help with a smaller test case?

kleunen commented 2 years ago

It might be related to the simplification. I know I fixed some corner cases related to simplification, but I am not sure they are all in the tilemaker simplifier. And another thing is, the mapping of the coordinates from float -> int, for the pbf format, might also create self-intersection in some cases.

arturbil commented 2 years ago

A similar issue occurs for another insanely complex lake Inari (https://www.openstreetmap.org/relation/402543) at zoom 6. In this case even without any extra simplification parameters (no "simplify_level", no "simplify_ratio") for the water layer in config.json.

arturbil commented 2 years ago

Found another problematic lake (https://www.openstreetmap.org/relation/900367), still complex, but 40x smaller in size than Saimaa. Attaching geojson if that can be of any use: suvasvesi.geojson.zip

mboeringa commented 2 years ago

Would downloading the relation and converting it to PBF help with a smaller test case?

The Saimaa MP as both OSM XML and GeoJSON format exports from JOSM: Saimaa_GeoJSON.zip Saimaa_OSM_XML.zip

mboeringa commented 2 years ago

@arturbil

A similar issue occurs for another insanely complex lake Inari (https://www.openstreetmap.org/relation/402543) at zoom 6. In this case even without any extra simplification parameters (no "simplify_level", no "simplify_ratio") for the water layer in config.json.

I've checked this MP in JOSM using the validation options, and it comes out fine. So the original MP appears technically sound.

mboeringa commented 2 years ago

Found another problematic lake (https://www.openstreetmap.org/relation/900367), still complex,

That one also checks out fine in JOSM

kleunen commented 2 years ago

Can you guys run tilemaker with --verbose? if tilemaker thinks the input relation is invalid, it will give an output message. If it gives no output message on this relation, in that case, the input geometry is valid, but tilemaker makes the geometry invalid during processing.