-
Notifications
You must be signed in to change notification settings - Fork 235
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
Seperate line sections instead of clipping linestring #234
Conversation
For simplification of a polygon, you can do the following. Say you have a polygon consisting of an outer ring and inner rings. You can "clip" any of the inner rings as follows: Go around the points of the inner ring (call them a,b,c,d,e,f,g ...). If you have 3 consecutive points on the ring which are all outside the bounding box of the tile, for example point a, b, c are all outside the bounding box, you can possibly remove the intermediate point (point b). You just have to check if the line a-c does not intersect the bounding box (is outside the tile). That way, you can remove points from the ring which are not relevant to the tile. First you simplify all the inner rings that way, removing points which are not relevant. Then, you do the same process to the outer ring . If you find three points, a, b, c outside the bounding box and outside the inner rings, you check if the line a-c does not intersect the bounding box and the inner rings, and remove point b. It is a greedy approach, so you may not find the optimal polygon. But I guess it can simplify the polygons, keeping only the relevant part. |
That fixes it! 🎉 We probably need to check that we eliminate any really long linestrings beyond the border of the tile - possible in the case of ferry routes, in particular. Mapbox GL JS has an EXTENT_MAX of 16384, i.e. anything beyond that will cause a warning. |
Excellent. Do you know some area which has polygons with inners, just around the edge of tiles ? To test the "simplification" of polygons ? |
I do not understand what you mean here, maybe i have to read through the bug report. |
ah ok, you mean limit the length of the linestring to 16384 points ? Otherwise chop into multilinestring ? |
The issue is this: Basically if a vertex, when scaled to an internal MBGL tile coordinate, exceeds EXTENT_MIN or EXTENT_MAX in its x or y coordinate, then MBGL will throw an error. That won't happen too often, but for a long polyline with few vertices (e.g. a ferry route), it's quite possible. EXTENT_MAX is hardcoded to 16383 (or, strictly, 2*EXTENT-1, and EXTENT is hardcoded to 8192). So what I think this means is that we need to do an extra
This should (if my maths is correct, and it might not be) clip to the bounding box which is eventually translated to EXTENT_MIN-EXTENT_MAX. I don't know if this would be more efficient run before or after our new linestring clipping algorithm. (It's slightly confusing because our extent for tile coordinates is usually 4096, rather than 8192, and that's what we write into the vector tiles. But MBGL normalises everything to 8192 internally.) |
I would recommend running it after the new linestring clipping algorithm. Because it might introduce a rounding error. Possibly, when running the new algorithm first, it already fits within this large bounding box and the clipping is not needed. |
035c1a7
to
eac435e
Compare
2a96b59
to
4ca85c8
Compare
I think there might actually be an easier way to clip the polygons. So i will try this later. |
Try the following, i think the polygon reduction is now correct. Additional optimization i think is still possible, to further reduce the size of the polygons. But have a look if you see any incorrect polygons. |
Basicly what the algorithm does is the following:
It now only does this for 3 consecutive points repeatedly, but ideally it would try all combinations and see which combination of points can be used and gives the greatest reduction in points. |
I tried kleunen@57c7695, looks good! That's really great! |
The approach 'Possibly reduce polygon more significantly' gives a greater reduction in the in the tile polygon size. But this comes at the cost of slower runtime. I am not sure if the slower runtime is worth the reduction in the polygon size. |
I'll run my usual benchmarks! |
@systemed You should also check the total number of points within the polygons, see what the reduction is of the different approaches. |
Current master, Great Britain extract, renumbered:
This branch at 57c7695:
I tried with the very latest code (e13e9d5) but it was running so slowly that I killed it - this was while showing z10/1000 tiles, and had been running for well over an hour. It seemed to me that the lower zoom levels had the biggest slow-down, and of course the clipping wasn't a problem there - it's only been a problem at z14 where the user "overzooms". So we could maybe use the original Boost |
Yes, this I was thinking about also. I think you do need to set it fixed to z14, because this is where the possibly this rounding becomes significant. I also performed the conversion on my Netherlands extract, but did not see this significant slow down. |
The issue is when you overzoom a lot - so if you're generating tiles up to z14, the error becomes obvious when you're viewing the map at (say) z18. But if you're generating tiles up to z15 (i.e. you've set |
I will add a fixed value for now, so we can do some more benchmarking. |
I added the zoom check, this is still with the last approach. On my small extract, i did not notice any performance difference actually. |
Another approach would be to just clip to this extended bbox, i wonder what the file size will be if you do it like this. |
Can you try again ? If i limit the linestring pruning to zoom level 14 also, i do see a significant difference |
Just running it now! |
oh, i think there is some wrong. The linestrings are missing at zoom level < 14 Yes, there is a bug in the code. |
31a5694
to
dcc4935
Compare
dcc4935/309a312 Is a completely different approach I was thinking about yesterday. Initially i thought it would not work, but it does :) It might still introduce faults, but I do not see them in the rendered output. |
The linestring approach + dynamic bounding box for the polygon from 39dd652 is really the best we can do I think. |
I've just ctrl-C'ed the previous (i.e. before dynamic bbox) attempt. It mostly ran pretty well, but got stuck on (presumably) some particularly complex geometries, and has been sitting there for an hour with just a couple of threads still running. I'll try the latest code now. |
Looks good!
Weirdly, in-browser render time seems much, much slower. I'm getting a couple of "Geometry exceeds allowed extent, reduce your vector tile buffer size" messages in the console, but not many. I'll investigate that some more. The line and polygon issues seem to be fixed! |
Maybe double check the extent code. I rendered my Netherlands extract. It feels fast, and i do not see the extent messages. Ah yes, i see extent message now also |
It might be my machine being weird. I'll do some more checking tomorrow. Had the Covid vaccine today so I'm probably not feeling at my best for full-on debugging right now. 😆 |
That's great - merged. Thank you! I'll have a play around with tweaking the ExtendBox size later but that can go in a separate PR. Hope the beer was good. |
This PR chops a linestring into the different sections that overlap a tile bbox, instead of performing clipping.
Have a try, TBH i do not see any noticable difference.
before this was (but this was also before rounding)