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

Perform dissolve on self-intersecting polygons #249

Merged
merged 11 commits into from
Jun 22, 2021

Conversation

kleunen
Copy link
Contributor

@kleunen kleunen commented Jun 9, 2021

Have a try to see how it works, on my 'Deventer' export, it actually helps. This polygon which was causing rendering issues on the tile, it is repaired and no rending issue.

@kleunen kleunen mentioned this pull request Jun 9, 2021
@systemed
Copy link
Owner

I tried this last night and it seems to work really well!

If you patch it into https://github.com/systemed/tilemaker/blob/master/src/read_shp.cpp#L248 then it can fix geometries from shapefiles too - there's a faulty one in the coast around Britain which this successfully fixes.

@kleunen
Copy link
Contributor Author

kleunen commented Jun 10, 2021

I will do that hopefully tonight.

@kleunen
Copy link
Contributor Author

kleunen commented Jun 10, 2021

Have a try

@systemed
Copy link
Owner

systemed commented Jun 10, 2021

Working well on the UK and France too - I tried it on the France extract (which has eight gazillion buildings) and it worked well. In particular, the lost polygon in Brest from #242 seems to be fixed!

Screenshot 2021-06-10 at 21 39 25

Only issue I had was that it terminated badly with france-latest.osm.pbf:

richard@peyresourde:~/tilemaker$ time tilemaker --input /media/data1/planet/france-latest.osm.pbf \
--output /media/data3/france.mbtiles \
--process resources/process-openmaptiles.lua --config resources/config-openmaptiles.json \
--store /media/ssd/

[...]

Shape points: 0, lines: 0, polygons: 941
Generated points: 9311283, lines: 17823864, polygons: 51471225
Zoom level 8, writing tile 574799 of 574799                
Filled the tileset with good things at /media/data3/france.mbtiles
terminate called without an active exception
Aborted (core dumped)

real	33m52.533s
user	319m30.878s
sys	7m1.917s

This was after the .mbtiles had been written so no real effect, but still I wonder why it's doing that.

@kleunen
Copy link
Contributor Author

kleunen commented Jun 11, 2021

Probably in the deallocation of memory with this mmap allocator. At exit all shared pointers and data structures are released

@kleunen
Copy link
Contributor Author

kleunen commented Jun 11, 2021

Maybe it attempts to delete the directory before all deallocation is done. You can try disabling deallocation in the memory manager.

@kleunen
Copy link
Contributor Author

kleunen commented Jun 11, 2021

I think the polygon from #242 is not visible with the default style. It is landuse military. This is not exported with openmaptiles process?

@kleunen
Copy link
Contributor Author

kleunen commented Jun 11, 2021

I also pushed a fix for closing the mmap file in this PR, and now perform all corrections using the dissolve.

@QuentinC
Copy link
Contributor

I think the polygon from #242 is not visible with the default style. It is landuse military. This is not exported with openmaptiles process?

Hello,

You are right, here is my adapted process file that includes military:

        -- Set 'landuse'
        else
                if l=="" then l=amenity end
                if l=="" then l=tourism end
                if landuseKeys[l] then
                        way:Layer("landuse", true)
                        way:Attribute("class", l)
                        if l=="residential" then
                                if way:Area()<ZRES8^2 then way:MinZoom(8)
                                else SetMinZoomByArea(way) end
                        else way:MinZoom(11) end
                        write_name = true
                end
                if l=="military" then
                        SetMinZoomByArea(way)
                        write_name = false

                        way:LayerAsCentroid("landuse_detail")
                        SetNameAttributes(way)
                        way:Attribute("class", l)
                        SetMinZoomByArea(way)
                end
        end

I am on my way to run this PR to check if it helps !

@QuentinC
Copy link
Contributor

I will have to do some more investigation, right now it seems that my way is still having problems.

I've seen something strange however, my run time went from 8 to 47min ! It took a lot of time reading a shapefile (ne_10m_ocean.shp).
It might be related to 7f2a12d ?

I will do some more tests on this on the next days and I will comment here !

@kleunen
Copy link
Contributor Author

kleunen commented Jun 12, 2021

Yes, the dissolve can take a lot of time. Also, boost geometry correct takes a lot of time, i think most of the time of the input process. I profiled this once. But that is a big difference you mention.

I think the dissolve should use an rtree, some complex polygons are in these shape files.

@kleunen
Copy link
Contributor Author

kleunen commented Jun 12, 2021

Boost geometry is actually used by mysql:
https://mysqlserverteam.com/why-boost-geometry-in-mysql/

I found a dataset invalid polygons:
https://stackoverflow.com/questions/49902090/dataset-of-invalid-geometries-in-boostgeometry

And added this to my boost/geometry/correct:
https://github.com/kleunen/boost_geometry_correct

Would be nice if it someday would become a mysql makevalid

@kleunen
Copy link
Contributor Author

kleunen commented Jun 13, 2021

I am using an rtree for calculating the intersections now, this should be faster

@systemed
Copy link
Owner

Latest code run successfully with both UK and France:

real	24m39.318s
user	306m45.536s
sys	2m36.786s

real	33m44.434s
user	317m26.253s
sys	6m56.542s

With France I was using the mmap store and got an exception (Boost 1.71) at the end of processing:

terminate called after throwing an instance of 'boost::filesystem::filesystem_error'
  what():  boost::filesystem::remove: Permission denied: "/media/ssd/"
Aborted (core dumped)

This doesn't affect anything because the tiles were already generated; there seems to be a bit of commentary on StackOverflow etc. from other people who've encountered this, so maybe just catching that error would work.

It might be good to apply the dissolve fix to calculateCentroid in osm_lua_processing.cpp too - failing to get centroids of invalid polygons is quite a common error, e.g.:

Problem geometry relation 10316772: Boost.Geometry Centroid calculation exception

But that's not urgent and we could do it in a later PR.

@kleunen
Copy link
Contributor Author

kleunen commented Jun 17, 2021

With France I was using the mmap store and got an exception (Boost 1.71) at the end of processing:

terminate called after throwing an instance of 'boost::filesystem::filesystem_error'
  what():  boost::filesystem::remove: Permission denied: "/media/ssd/"
Aborted (core dumped)

What was the path that you used for the osm_store ?

@systemed
Copy link
Owner

Just /media/ssd/.

@kleunen
Copy link
Contributor Author

kleunen commented Jun 17, 2021

ah ok, yes, remove_all tries to remove all files/directories below /media/ssd and /media/ssd itself. Maybe it would be good not to use remove_all, but rather track the files that are actually created. I am bit scared tilemaker is going to delete any files of the user on disk...

@QuentinC
Copy link
Contributor

That might be because /media/ssd is a mount point ?

@systemed
Copy link
Owner

I think this is good to merge - shall we go for it?

@kleunen
Copy link
Contributor Author

kleunen commented Jun 21, 2021

There is interest from boost geometry to add a feature like this to boost, have a look at the discussion here:
boostorg/geometry#868

I have made some extra improvements to the correct, based on this. I can merge them into this PR. I don't think you will need all improvements that will come out of the boost/geometry discussion. They are probably very specific details.

There is still one change I would like to merge, so please wait with merging :)

@kleunen
Copy link
Contributor Author

kleunen commented Jun 21, 2021

Have a try, i pushed this last small change :)

@kleunen
Copy link
Contributor Author

kleunen commented Jun 22, 2021

Moving towards 2.0 release, i would say ?

@systemed
Copy link
Owner

Yes, definitely. I've set a deadline of State of the Map 2021 (9-11 July) for 2.0 :)

Just running a final test and will merge this afterwards, all things being equal!

@kleunen
Copy link
Contributor Author

kleunen commented Jun 22, 2021

can you still host a workshop there ? or is too late to enter ?

@systemed
Copy link
Owner

Too late for a workshop, but I've signed up for a lightning talk: https://wiki.openstreetmap.org/wiki/State_of_the_Map_2021/registration_lightning_talks

@systemed systemed merged commit 3b11dff into systemed:master Jun 22, 2021
@systemed
Copy link
Owner

We're good to go! Merged. Thank you for an absolutely outstanding contribution.

Really good to see the interest from Boost.Geometry too!

@kleunen
Copy link
Contributor Author

kleunen commented Jun 22, 2021

Yes, it will probably be a long process with boost geometry.
But hopefully someday it will be available for many other projects as well.

@kleunen
Copy link
Contributor Author

kleunen commented Jun 23, 2021

I think commit db992ce limits the performance quite severly, you might want to revert this

@kleunen
Copy link
Contributor Author

kleunen commented Jun 23, 2021

I've seen something strange however, my run time went from 8 to 47min ! It took a lot of time reading a shapefile (ne_10m_ocean.shp).
It might be related to 7f2a12d ?

I will do some more tests on this on the next days and I will comment here !

@QuentinC can you try if the performance is still acceptable ? If the performance is not back to very slow again ?

@QuentinC
Copy link
Contributor

I've seen something strange however, my run time went from 8 to 47min ! It took a lot of time reading a shapefile (ne_10m_ocean.shp).
It might be related to 7f2a12d ?
I will do some more tests on this on the next days and I will comment here !

@QuentinC can you try if the performance is still acceptable ? If the performance is not back to very slow again ?

It seems to be running fine here, last try has been done in 9min so no issue for me !
I will have a look at #258 !

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.

3 participants