Added support for wagyu on polygon overzooming and clipping by flippmoke · Pull Request #100 · mapbox/vtcomposite

flippmoke · 2019-08-07T21:35:44Z

Currently we can produce rings with the incorrect winding order due to the lack of wagyu being used in our clipping step.

src/feature_builder.hpp

springmeyer

My thoughts are:

If invalid polygons are being passed to vtcomposite, it is not vtcomposite's job to clean them up. Rather they should be fixed upstream.
If vtcomposite is creating invalid polygons from valid polygons, this is a bug that should be solvable without needing to depend on wagyu to do cleanup

So, without evidence that wagyu is actually needed here I think this is not the right approach.

e-n-f · 2019-08-07T22:42:06Z

src/feature_builder.hpp

+        }
+        mapbox::geometry::multi_polygon<coordinate_type> results;
+        mapbox::geometry::wagyu::wagyu<std::int64_t> wagyu;
+        bool data_added = false;


It seems like it might be clearer to combine data_added and !skip_inner as a single added_outer_ring condition.

flippmoke · 2019-08-07T22:42:21Z

@springmeyer in the process of clipping you can create an invalid polygon, even if they were valid prior. I honestly do not trust the boost implementation of intersection to get the results of clipping correct and if I remember this is partially why we started to use angus and then wagyu.

e-n-f

This looks good to me. My questions about why each outer ring and its children are cleaned separately rather than doing the entire geometry together were answered in Slack.

e-n-f · 2019-08-07T23:04:16Z

@flippmoke Could you add one of the known-bad tiles here as a test case to demonstrate the problem and prevent future regressions?

Any easy one to reproduce:

Natural Earth bathymetry
Tile with maxzoom of 4 with tippecanoe -f -o bathymetry.mbtiles -zg --drop-densest-as-needed Downloads/ne_10m_bathymetry_all/*json
Fails in z8 at 37.564/130.923

springmeyer · 2019-08-08T00:16:38Z

@springmeyer in the process of clipping you can create an invalid polygon, even if they were valid prior.

The fundamental design of vtcomposite is based on the premise that boost::geometry::intersection will produce valid geometries if the input geometries are valid. If this is not the case then you need to provide evidence in the form of a reduced testcase. Then we need to figure out if the problem is a boost geometry bug (like boostorg/geometry#566 perhaps?) or not.

I honestly do not trust the boost implementation of intersection to get the results of clipping correct

Don't trust it because you see obvious cases where it will produce valid geometries or don't trust it due to the potential for software bugs?

and if I remember this is partially why we started to use angus and then wagyu.

We needed wagyu at the tile generation stage for the clipping arbitrary input geometries that were potentially invalid because boost::geometry::intersection algorithm would throw on invalid geometries (as it is designed to expecting valid input). In vtcomposite we can and want to assume all input geometries are valid and therefore wagyu should be unneeded, right?

springmeyer · 2019-08-08T00:18:14Z

and was being broken by the previous clipping implementation here during overzooming.

@ericfischer how exactly is the output tile being broken by the current implementation?

e-n-f · 2019-08-08T01:07:15Z

@springmeyer The best example I have for this is tile 4/13/6 in enf.4djr0amh (Natural Earth bathymetry), tiled to z4, overzoomed to tile 8/221/99.

As you can see here, two of the clipped inner rings have a collinear edge with the right or top side of the outer ring, an invalid geometry that causes triangulation errors and causes the entire tile to be filled instead of just the area excluding the holes.

Wagyu correctly cuts both of these out of the outer ring instead.

e-n-f · 2019-08-08T16:50:30Z

@springmeyer To be even clearer: the fundamental flaw of the current implementation is that it clips each polygon ring independently instead of considering the feature as a whole, so it will always be wrong when the edge of the clip region intersects a hole.

artemp · 2019-08-08T17:14:36Z

@ericfischer Clipping individual rings is the issue, I'll look into fixing this. Thanks for providing background here!
/cc @springmeyer

springmeyer · 2019-08-08T17:34:59Z

Per chat with @ericfischer and @artemp, @artemp is going to create a second branch for comparison and testing purposes that attempts to fix the problem @ericfischer describes clearly at #100 (comment) by changing the usage of boost::geometry. I think it makes sense to continue to try to get this branch (wagyu method) production ready and the two approaches can be compared before releasing.

For historical reference, wagyu was integrated in an experimental way last year in #45 but abandoned since it was only considered at that time for handling known invalid v1 vector tiles and other workarounds were successful.

flippmoke · 2019-08-08T17:41:27Z

node bench/bench.js --iterations 1000 --concurrency 5 --package vtcomposite

Performance in master:

1: single tile in/out ... 43478 runs/s (23ms)
2: two different tiles at the same zoom level, zero buffer ... 6250 runs/s (160ms)
3: two different tiles from different zoom levels (separated by one zoom), zero buffer ... 1082 runs/s (924ms)
4: two different tiles from different zoom levels (separated by more than one zoom), zero buffer ... 1838 runs/s (544ms)
5: tiles completely made of points, overzooming, no properties ... 9346 runs/s (107ms)
6: tiles completely made of points, same zoom, no properties ... 40000 runs/s (25ms)
7: tiles completely made of points, overzoooming, lots of properties ... 6452 runs/s (155ms)
8: tiles completely made of points, same zoom, lots of properties ... 34483 runs/s (29ms)
9: buffer_size 128 - tiles completely made of points, same zoom, lots of properties ... 25641 runs/s (39ms)
10: tiles completely made of linestrings, overzooming and lots of properties ... 2294 runs/s (436ms)
11: tiles completely made of polygons, overzooming and lots of properties ... 493 runs/s (2027ms)
12: tiles completely made of points and linestrings, overzooming and lots of properties ... 20000 runs/s (50ms)
13: buffer_size 128 - tiles completely made of points and linestrings, overzooming and lots of properties ... 20408 runs/s (49ms)
14: tiles completely made of points and linestrings, overzooming (2x) and lots of properties ... 24390 runs/s (41ms)
15: tiles completely made of polygons, overzooming and lots of properties ... 1859 runs/s (538ms)
16: tiles completely made of polygons, overzooming (2x) and lots of properties ... 3610 runs/s (277ms)
17: buffer_size 4096 - tiles completely made of polygons, overzooming (2x) and lots of properties ... 1587 runs/s (630ms)
Note: pass --mem to track memory usage
Benchmark iterations: 1000 concurrency: 5

Performance with wagyu:

1: single tile in/out ... 47619 runs/s (21ms)
2: two different tiles at the same zoom level, zero buffer ... 6098 runs/s (164ms)
3: two different tiles from different zoom levels (separated by one zoom), zero buffer ... 732 runs/s (1366ms)
4: two different tiles from different zoom levels (separated by more than one zoom), zero buffer ... 1078 runs/s (928ms)
5: tiles completely made of points, overzooming, no properties ... 13889 runs/s (72ms)
6: tiles completely made of points, same zoom, no properties ... 9259 runs/s (108ms)
7: tiles completely made of points, overzoooming, lots of properties ... 6897 runs/s (145ms)
8: tiles completely made of points, same zoom, lots of properties ... 17241 runs/s (58ms)
9: buffer_size 128 - tiles completely made of points, same zoom, lots of properties ... 22727 runs/s (44ms)
10: tiles completely made of linestrings, overzooming and lots of properties ... 2222 runs/s (450ms)
11: tiles completely made of polygons, overzooming and lots of properties ... 337 runs/s (2970ms)
12: tiles completely made of points and linestrings, overzooming and lots of properties ... 20000 runs/s (50ms)
13: buffer_size 128 - tiles completely made of points and linestrings, overzooming and lots of properties ... 19608 runs/s (51ms)
14: tiles completely made of points and linestrings, overzooming (2x) and lots of properties ... 29412 runs/s (34ms)
15: tiles completely made of polygons, overzooming and lots of properties ... 1024 runs/s (977ms)
16: tiles completely made of polygons, overzooming (2x) and lots of properties ... 2294 runs/s (436ms)
17: buffer_size 4096 - tiles completely made of polygons, overzooming (2x) and lots of properties ... 1041 runs/s (961ms)
Note: pass --mem to track memory usage
Benchmark iterations: 1000 concurrency: 5

artemp · 2019-08-09T12:49:56Z

@ericfischer @flippmoke @springmeyer - Fixed in b404cf2 , working on a test..

flippmoke · 2019-08-14T18:47:22Z

closed for now in favor of #101

Added support for wagyu on polygon overzooming and clipping

eee27a4

flippmoke requested a review from a team as a code owner August 7, 2019 21:35

flippmoke added 3 commits August 7, 2019 16:51

test with no v6 of node

e5af75b

Update boost and aws sdk for lodash security issues

c958e5e

Fix travis

e4fd268

flippmoke requested a review from springmeyer August 7, 2019 22:15

e-n-f reviewed Aug 7, 2019

View reviewed changes

src/feature_builder.hpp Show resolved Hide resolved

springmeyer suggested changes Aug 7, 2019

View reviewed changes

e-n-f reviewed Aug 7, 2019

View reviewed changes

e-n-f approved these changes Aug 7, 2019

View reviewed changes

Changes as requested by eric

446869e

artemp added a commit that referenced this pull request Aug 9, 2019

assemble polygons before intersecting with bounding box (ref #100)

b404cf2

artemp added a commit that referenced this pull request Aug 9, 2019

add test (ref #100)

fce6a9c

flippmoke mentioned this pull request Aug 9, 2019

Polygon clipping fix #101

Merged

flippmoke closed this Aug 14, 2019

Conversation

flippmoke commented Aug 7, 2019

Uh oh!

Uh oh!

springmeyer left a comment

Choose a reason for hiding this comment

Uh oh!

e-n-f Aug 7, 2019

Choose a reason for hiding this comment

Uh oh!

flippmoke commented Aug 7, 2019

Uh oh!

e-n-f left a comment

Choose a reason for hiding this comment

Uh oh!

e-n-f commented Aug 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

springmeyer commented Aug 8, 2019

Uh oh!

springmeyer commented Aug 8, 2019

Uh oh!

e-n-f commented Aug 8, 2019

Uh oh!

e-n-f commented Aug 8, 2019

Uh oh!

artemp commented Aug 8, 2019

Uh oh!

springmeyer commented Aug 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

flippmoke commented Aug 8, 2019

Uh oh!

artemp commented Aug 9, 2019

Uh oh!

flippmoke commented Aug 14, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

e-n-f commented Aug 7, 2019 •

edited

Loading

springmeyer commented Aug 8, 2019 •

edited

Loading