Touching inner rings #177

msbarry · 2024-12-13T13:48:07Z

I've been noticing incorrect triangulations from earcut on real world data recently, even when the input polygons are simple and valid. It seems to happen when polygons contain inner rings that touch at a single vertex. Here is a screenshot of the issue in the wild:

I reduced the left one to this minimal reproduction case:

[
  [[0,0],[20,0],[20,25],[0,25],[0,0]],
  [[3,3],[2,12],[9,15],[3,3]],
  [[9,21],[2,12],[7,22],[9,21]]
]

Since eliminateHoles sorts all holes by their leftmost point, when multiple holes meet at that point if they are processed in the wrong order it makes the border of the new outer polygon shape criss-cross so triangulation can't do the right thing.

This change makes eliminateHoles process holes that meet at the same point clockwise so that each ring can be inserted after the end of the one above it.

It fixes these cases:

before	after

msbarry · 2024-12-13T13:59:08Z

This fixes cases where holes meet at a left-most point, but there still appear to be issues for valid polygons that meet at a right-most point:

before	after
touching-holes4.json	touching-holes5.json

It appears in these cases that eliminateHoles produces a correct outer shell, but then triangulation doesn't treat any of those inner shells as ears because pointInTriangle considers the vertices of different triangles that share an endpoint as inside the triangle and returns true. I tried changing pointInTriangle to return false when the point is a vertex. That makes these test cases pass, but causes others to fail:

Any ideas what might be needed to fix those cases?

msbarry · 2024-12-14T16:18:08Z

OK I think I figured out those remaining issues and the test pass now. Let me know what you think.

touching-holes4.json		touching-holes5.json

msbarry · 2024-12-14T16:21:50Z

bench/bench.js

-import {earcut, flatten} from '../src/earcut.js';
+import earcut, {flatten} from '../src/earcut.js';


I had to make this tweak to the benchmarks to get them to run

main:

typical OSM building (15 vertices): x 2,751,261 ops/sec ±0.50% (96 runs sampled) dude shape (94 vertices): x 76,587 ops/sec ±0.42% (100 runs sampled) dude shape with holes (104 vertices): x 61,820 ops/sec ±0.32% (100 runs sampled) complex OSM water (2523 vertices): x 1,024 ops/sec ±0.49% (97 runs sampled)

this branch:

typical OSM building (15 vertices): x 2,644,854 ops/sec ±0.46% (92 runs sampled) dude shape (94 vertices): x 76,872 ops/sec ±0.31% (100 runs sampled) dude shape with holes (104 vertices): x 61,708 ops/sec ±0.28% (100 runs sampled) complex OSM water (2523 vertices): x 1,040 ops/sec ±0.22% (98 runs sampled)

msbarry · 2024-12-14T16:25:33Z

test/test.js

-            assert.ok(err <= expectedDeviation, `deviation ${err} <= ${expectedDeviation}`);
-        }
-    });
+    for (const rotation of [0, 90, 180, 270]) {


I added this while debugging to test some adjacent cases, I can roll it it back if you don't want it.

msbarry · 2024-12-14T16:26:59Z

viz/viz.js

@@ -2,125 +2,133 @@

 import earcut, {flatten, deviation} from '../src/earcut.js';

-const testPoints = [


I added while debugging to make it easy to switch between test fixtures, for example http://localhost:8000/viz/?fixture=touching-holes5&rotation=180, but can roll it back if you don't want it.

mourner

This is a great contribution, really happy to see improvements to make Earcut more resilient! I know how hard it is to debug issue like this (sometimes I'd just give up).

I'm wondering about that regression for the simple use case — I guess this is because of the additional equals check? Since it is added almost everywhere where pointInTriangle is called, should the check possibly be inside that function instead, which could theoretically help with the regression and slightly simplify the change?

Rotations for test variations are a great idea, and easier test loading for viz looks good to me too.

msbarry · 2024-12-16T22:49:12Z

Thanks! By "that regression for the simple use case" do you mean the "typical OSM building (15 vertices)" benchmark? Or is it behavior on one of the test cases?

mourner · 2024-12-16T23:01:32Z

do you mean the "typical OSM building (15 vertices)" benchmark?

Yep, that one, assuming it's not a fluke result.

msbarry · 2024-12-17T02:07:51Z

OK got it. I pushed the check inside pointInTriangle but it broke the tests because of the other usages of that function, so I split it into a second function that excludes the first point. Now they are closer...

this branch:

typical OSM building (15 vertices): x 2,785,989 ops/sec ±0.27% (97 runs sampled)
dude shape (94 vertices): x 76,892 ops/sec ±0.26% (101 runs sampled)
dude shape with holes (104 vertices): x 62,760 ops/sec ±0.23% (101 runs sampled)
complex OSM water (2523 vertices): x 1,040 ops/sec ±0.15% (98 runs sampled)

and main right afterwards:

typical OSM building (15 vertices): x 2,803,361 ops/sec ±0.29% (97 runs sampled)
dude shape (94 vertices): x 77,019 ops/sec ±0.26% (96 runs sampled)
dude shape with holes (104 vertices): x 62,016 ops/sec ±0.99% (97 runs sampled)
complex OSM water (2523 vertices): x 1,024 ops/sec ±0.43% (97 runs sampled)

mourner

Nice! Just for the sake of experiment, what happens if you call pointInTriangle inside pointInTriangleExceptFirst? In theory V8 inlines things like that under the hood so I'm curious if it will affect benchmarks. Looks great otherwise.

msbarry · 2024-12-18T01:26:35Z

Ah good call, performance is the same calling pointInTriangle inside pointInTriangleExceptFirst, but less repetetive.

Also this isn't related to my change at all but I noticed there might be some improvement to be had by changing the bbox calculations to use Math.min(ax, bx, cx).

this branch:

typical OSM building (15 vertices): x 2,780,054 ops/sec ±0.24% (97 runs sampled)
dude shape (94 vertices): x 78,125 ops/sec ±0.46% (101 runs sampled)
dude shape with holes (104 vertices): x 62,822 ops/sec ±1.06% (100 runs sampled)
complex OSM water (2523 vertices): x 1,081 ops/sec ±0.16% (99 runs sampled)

main:

typical OSM building (15 vertices): x 2,784,181 ops/sec ±0.44% (94 runs sampled)
dude shape (94 vertices): x 76,417 ops/sec ±1.08% (101 runs sampled)
dude shape with holes (104 vertices): x 62,351 ops/sec ±0.27% (99 runs sampled)
complex OSM water (2523 vertices): x 1,020 ops/sec ±0.34% (96 runs sampled)

mourner · 2024-12-18T09:11:53Z

@msbarry excellent! There were times when Math.min / Math.max were slower, but v8 is evolving so it's no surprise it's faster now.

msbarry · 2024-12-18T10:51:59Z

Great! Thanks for the help on this! For reference those benchmarks were all node 21 on a 2021 M1 Macbook Pro.

mourner · 2024-12-25T20:27:51Z

@msbarry Merry Christmas! Looks like this change produces some weird triangulation failures in GL JS tests (seeing results from mapbox/mapbox-gl-js#13371, e.g. here https://output.circle-artifacts.com/output/job/d76e512a-1057-4c34-9a84-c32415369555/artifacts/0/test/integration/render-tests/index.html) — will need investigation. If it's not an easy thing to fix, we'll probably have to hold off on upgrading and potentially revert...

msbarry · 2024-12-25T21:29:57Z

Are you able to reproduce those failures as test cases in earcut? If so I can take a look in a few days.

msbarry added 3 commits December 12, 2024 07:16

fix all except touching-holes4

f5743e1

other

ad7dd76

fix benchmarks

8630230

viz tweaks, test rotations, and fix other bugs

7fa2edc

msbarry commented Dec 14, 2024

View reviewed changes

msbarry added 2 commits December 14, 2024 11:45

rm diffs

2bbc725

rm extra diff

90b1907

mourner approved these changes Dec 16, 2024

View reviewed changes

perf tweak

d7be843

comment

bcb40ee

mourner reviewed Dec 17, 2024

View reviewed changes

perf tweaks

320f431

mourner merged commit 38bccee into mapbox:main Dec 18, 2024
1 check passed

mourner mentioned this pull request Dec 18, 2024

v3.0.1 #179

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Touching inner rings #177

Touching inner rings #177

msbarry commented Dec 13, 2024 •

edited

Loading

msbarry commented Dec 13, 2024 •

edited

Loading

msbarry commented Dec 14, 2024

msbarry Dec 14, 2024

msbarry Dec 14, 2024

msbarry Dec 14, 2024

mourner left a comment

msbarry commented Dec 16, 2024

mourner commented Dec 16, 2024

msbarry commented Dec 17, 2024

mourner left a comment

msbarry commented Dec 18, 2024 •

edited

Loading

mourner commented Dec 18, 2024

msbarry commented Dec 18, 2024

mourner commented Dec 25, 2024

msbarry commented Dec 25, 2024

		import {earcut, flatten} from '../src/earcut.js';
		import earcut, {flatten} from '../src/earcut.js';

		@@ -2,125 +2,133 @@

		import earcut, {flatten, deviation} from '../src/earcut.js';

		const testPoints = [

Touching inner rings #177

Touching inner rings #177

Conversation

msbarry commented Dec 13, 2024 • edited Loading

msbarry commented Dec 13, 2024 • edited Loading

msbarry commented Dec 14, 2024

msbarry Dec 14, 2024

Choose a reason for hiding this comment

msbarry Dec 14, 2024

Choose a reason for hiding this comment

msbarry Dec 14, 2024

Choose a reason for hiding this comment

mourner left a comment

Choose a reason for hiding this comment

msbarry commented Dec 16, 2024

mourner commented Dec 16, 2024

msbarry commented Dec 17, 2024

mourner left a comment

Choose a reason for hiding this comment

msbarry commented Dec 18, 2024 • edited Loading

mourner commented Dec 18, 2024

msbarry commented Dec 18, 2024

mourner commented Dec 25, 2024

msbarry commented Dec 25, 2024

msbarry commented Dec 13, 2024 •

edited

Loading

msbarry commented Dec 13, 2024 •

edited

Loading

msbarry commented Dec 18, 2024 •

edited

Loading