Switch to earcut

[jfirebaugh]

We're currently using libtess2, but the caveat is that it crashes hard on geometries that aren't completely valid. Therefore, we also use ClipperLib to cleanup geometries before passing them on. This adds quite a bit of code, as well as a huge performance hit for processing tiles.

Instead, we should switch to earcut.hpp. Earcut also requires valid geometries, but doesn't crash hard when they aren't. It's also a 2⨉ to 15⨉ speed improvement in tessellation alone, in particular for polygons with low vertex counts, such as buildings. It also allows us to drop ClipperLib.

• Update earcut.hpp with recent fixes from JS version
• Make sure that all data sources return valid geometries
• Integrate earcut into GL native

Alternatively, we could drop libtess2 in a first step, but still clean up geometries with ClipperLib.

[mourner]

You could remove clipper and integrate earcut without waiting for validness fixes in the data sources (which can take a while), and test whether it's really a problem.

[kkaefer]

It's not a problem in that the renderer crashes, but some of the polygons will be invalid. It'll probably be hard to spot that because you have no direct comparison, though.

[mourner]

@kkaefer yeah, I mean, if the problems are hard to spot, we might get by without fully valid polygons for some time.

[jfirebaugh]

@kkaefer Is the total_vertex_count > 65536 check only to avoid a libtess limit, or is it something we need to enforce with earcut too?

[kkaefer]

That check is because we're using vertex buffers with elements buffers referencing those elements. Since elements buffers are 2 bytes per item, we can address a max of 65536 unique vertices. This check is a bit overly aggressive since there may be duplicate points.

[jfirebaugh]

First renders:

@kkaefer @mourner @ansis Any tips on how to debug stuff like this?

[mourner]

@jfirebaugh do you try to infer ring structure somehow? When I tested on GL JS, artifacts like this appeared because multipolygons are flattened into a set of rings (e.g. outer, hole, hole, outer, hole), and earcut requires polygons as outer ring + its holes.

[jfirebaugh]

Much progress in the last 4 hours:

I ported the ring classification code from JS, and then discovered that earcut.hpp has a bug somewhere if you use short as the coordinate type. Above screenshot was generated using int instead.

Still some rendering errors in the test suite, possibly from an outdated vector tile? Not seeing this issue in the test app at zoom 0:

[springmeyer]

Great progress @jfirebaugh. Outdated tile (rendered prior to the "simplification" push which also landed winding order fix - https://github.com/mapbox/mapnik-internal/issues/10) is likely. But lowzoom tiles may still have self-intersections in the latest tiles and that is the case @flippmoke is focused on in mapnik/node-mapnik#533.

[mourner]

@jfirebaugh Sorry John, was sleepy yesterday and pointed you to the wrong branch. The ring classification routine you ported depends on naive point in polygon tests (all rings tested against each other) which can sometimes be unreliable.

There's a different, much simpler ring clasification routine in mapbox/mapbox-gl-js@77d2061 (earcut-alt branch) that infers ring structure from their winding order (opposite winding order means different types, outer vs hole) and order they appear in VT (holes always follow their outer ring), which should perform much better and be more reliable provided there are no more winding order bugs left in the current VT.

Still great progress though! Did you remove the clipper processing or is it clipper + earcut? Would be cool to measure real world performance of earcut vs clipper + libtess.

[jfirebaugh]

@mourner Okay, I'll port the winding order-based classification routine. Can we clean up the various earcut* gl-js branches into one canonical branch?

[mourner]

@jfirebaugh yes, will do tomorrow.

[jfirebaugh]

After updating the 0 tile test fixture pbf, down to the following tesselation error:

This is visible in real-world styles too:

[mourner]

@jfirebaugh is this the old ring classification or the new one?

[jfirebaugh]

New one, winding order based.

[mourner]

Can you find out if it's a VT winding order bug or Earcut failure?

[mourner]

If it's not easy to determine the exact cause, just send me the gist with the exact input to earcut (isolated single polygon that causes the problem) and I'll figure it out.

[jfirebaugh]

Conclusion from testing is that we will need to roll out improved polygon validity in our datasources before this can land.

[jfirebaugh]

Instruments Time Profiler results from two scenes, rough average of 3 loads for each, all times are sums over all tiles loaded for the scene. I had to look at slightly different functions due to oddities in how the profiler nested functions (likely due to inlining).

Mapbox SF @ z15, 6 tiles

• With libtess, FillBucket::tesselate takes 70-80 ms, of which 35-45 ms is libtess and 25 ms is clipper
• With earcut, FillBucket::addGeometry takes 12-20 ms, of which 6-7 ms is earcut, 1 ms is ring classification, and the rest varies between runs

SF @ z7, 9 tiles

• With libtess, FillBucket::tesselate takes 310 ms, of which 190 ms is libtess and 80 ms is clipper
• With earcut, FillBucket::addGeometry takes 135 ms, of which 115 is earcut

So as a rough guideline, earcut is at least 2x faster and in the best case (lots of simple polygons like building footprints), 6-7x faster.

[mourner]

[lucaswoj]

improved polygon validity in our datasources

Is this ticketed somewhere?

[mourner]

@jfirebaugh @yhahn a middle option to make the transition easier and get at least half of the gain while we try to tackle the challenge of VT compatibility is to run Clipper + Earcut. In theory this should work for all legacy VT while still shaving off some good ms.

[jfirebaugh]

That's an option, although from the numbers above, it would reduce the gain on fill tessellation to about 1.5-2x. Let's see how the source validity work pans out first.

[mourner]

@jfirebaugh yeah, I just thought it would be easy enough to break down into multiple steps:

1. libtess -> earcut
2. VT validity
3. remove clipper

Another advantage of trying clipper+earcut is that for shape annotations to be 100% stable, we'll probably have to run clipper on it since GeoJSON-VT simplification routine can make valid polygons invalid. Although artifacts would be extremely rare for user annotations (unlike crazy water polys in Streets), they're still a possibility we need to take care of.

[jfirebaugh]

I'm seeing some performance hot spots on the same complex European landcover polygons that cause issues with libtess2 (#4777):

[mourner]

@jfirebaugh try to isolate a particularly nasty polygon, and I'll see whether there's something in particular that makes the hole bridging routine get stuck for so long.

[jfirebaugh]

The problematic polygons are almost certainly the same ones that are causing the problem for libtess in #4777.

I added some debug output for tiles taking longer than 1 second to process with earcut (in debug mode, so worst case):

2272ms layer=wood sourceLayer=landuse tileID=6/32/22
3399ms layer=wood sourceLayer=landuse tileID=6/32/23
8310ms layer=wood sourceLayer=landuse tileID=6/34/22
10681ms layer=wood sourceLayer=landuse tileID=6/33/22
5038ms layer=wood sourceLayer=landuse tileID=6/33/21
12985ms layer=wood sourceLayer=landuse tileID=6/34/21

https://gist.github.com/jfirebaugh/14c6d1b1aceb1c6f880baf1610e06817 contains the data for the fourth polygon above. It looks like this when tesselated with earcut:

[jfirebaugh]

gl-js branch that logs slow earcut times: https://github.com/mapbox/mapbox-gl-js/compare/earcut-timing

[jfirebaugh]

@mourner 👀 on the latest change?

I think this is good to go -- with the combined Mapbox Streets VT update and ring limiting code, earcut time is down to a reasonable fraction of overall ticks in profiles. (In Europe at z6-8, it's still the dominant processing cost on worker threads, so any further optimizations are welcome.)

[mourner]

Looks good! The only minor thing is that maxRings name implies a limit to a total number of rings, including the outer ring, so you would need to remove a few +1s. Or rename to maxHoles.