mapbox / delaunator

An incredibly fast JavaScript library for Delaunay triangulation of 2D points
https://mapbox.github.io/delaunator/
ISC License
2.34k stars 141 forks source link

V5 — fully robust, no transpilation, native ESM #68

Closed mourner closed 3 years ago

mourner commented 3 years ago

The robust-predicates change makes the bundle ~32% bigger — from 2286 to 3029 bytes minified/gzipped. Previously I thought I should expose a separate "robust" bundle in addition to the smaller one, but now it seems that at a scale of few kilobytes, it's not worth the added complexity and the increase isn't a big deal.

Adding robust incircle would increase it more significantly (to 5322 bytes), but it doesn't seem necessary since this check only affects which triangles flip, and doesn't affect the integrity of the triangulation unlike orient2d.

@mbostock @Fil what do you think?

Fil commented 3 years ago

On principle I agree that we still too often have these precision issues, in particular in generative art code where points will often be aligned (vs "actual" data where Delaunator never fails). So I'm all for the extra cost if it can solve these.

I wanted to build d3-delaunay with this branch, however I am so bad with javascript plumbing, and failed.

redblobgames commented 3 years ago

I was wondering how much the robust check would affect performance. I ran the bench test but my machine shows quite a bit of variance from one run to the next, and the slowdown below 1,000,000 points is within the margin of error (+/- 10ms). At 1,000,000 points you can see a slight slowdown:

v4, 1,000,000 points, 4 runs

test run 1 run 2 run 3 run 4
uniform 658.504ms 653.551ms 649.729ms 654.352ms
gaussian 640.57ms 644.188ms 648.516ms 641.86ms
grid 531.747ms 538.196ms 538.529ms 536.82ms
degenerate 239.893ms 235.587ms 241.171ms 229.518ms

v5, 1,000,000 points, 4 runs

test run 1 run 2 run 3 run 4
uniform 679.483ms 674.571ms 666.818ms 660.141ms
gaussian 661.089ms 665.268ms 662.053ms 664.995ms
grid 546.916ms 555.071ms 555.103ms 553.64ms
degenerate 248.789ms 245.65ms 241.847ms 243.951ms

I'm surprisingly bad at benchmarking though so I wouldn't be surprised if I did something wrong.

mourner commented 3 years ago

@Fil didn't expect any problems with integrating this into d3-delaunay, can look at it next week. Of if you have any more details, I'd be happy to help.

@redblobgames that looks similar to what I've been getting and I think the results are pretty reasonable, even good considering the change.

Fil commented 3 years ago

Nah no real problem, it's just that I don't know how to do it. What I tried was to replace the delaunator folder in npm_modules! probably not the right method :-)

mourner commented 3 years ago

@Fil you can just do npm install mapbox/delaunator#v5 and it will install the branch there.

Fil commented 3 years ago

Thanks! With this it builds*, but alas it doesn't seem to solve this half-baked experiment https://observablehq.com/d/62b5c48194cba6b2 ; although the errors are different!

* build with yarn rollup -c — as far as I understand, the build is correct, but the unit tests fail:

❯ yarn test
yarn run v1.22.5
$ tape -r esm 'test/**/*-test.js' && eslint src test
/Users/fil/Sites/d3/d3-delaunay/node_modules/delaunator/index.js:1
Error [ERR_REQUIRE_ESM]: Must use import to load ES Module: /Users/fil/Sites/d3/d3-delaunay/node_modules/delaunator/index.js
    at new NodeError (node:internal/errors:278:15)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1125:13) {
  code: 'ERR_REQUIRE_ESM'
}
mourner commented 3 years ago

@Fil great test case! At least the Delaunay triangulation on it visually looks correct, so there might be an issue hidden in the Voronoi code. Although Delaunay might be broken in a non-visual way (bad connectivity) — will check this one next week.

mourner commented 3 years ago

but the unit tests fail

Yes, this will need adjustments — in short:

mourner commented 3 years ago

@Fil just to make sure, I added a sample test case from your notebook to the test suite and it passes robustness tests (triangulation is valid, with no broken halfedges etc.), so the issue must be on the d3-delaunay side.

Fil commented 3 years ago

Sorry for the delay. (And thank you for the "adjustments" in tests!). I can now confirm that the issue lies with d3-delaunay, in the special case it does for circumcenters of degenerate triangles in https://github.com/d3/d3-delaunay/blob/master/src/voronoi.js#L43

I can fix it for this example, but then it breaks a few other tests which are themselves a bit dubious. More later, but please don't hesitate to merge this.

jrus commented 3 years ago

This is great!