Skip to content
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

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

Merged
merged 5 commits into from
Mar 26, 2021
Merged

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

merged 5 commits into from
Mar 26, 2021

Conversation

mourner
Copy link
Member

@mourner mourner commented Feb 19, 2021

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?

@mourner mourner requested a review from Fil February 19, 2021 11:19
@Fil
Copy link
Collaborator

Fil commented Feb 19, 2021

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
Copy link
Collaborator

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
Copy link
Member Author

mourner commented Feb 19, 2021

@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
Copy link
Collaborator

Fil commented Feb 19, 2021 via email

@mourner
Copy link
Member Author

mourner commented Feb 19, 2021

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

@Fil
Copy link
Collaborator

Fil commented Feb 19, 2021

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
Copy link
Member Author

mourner commented Feb 19, 2021

@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
Copy link
Member Author

mourner commented Feb 19, 2021

but the unit tests fail

Yes, this will need adjustments — in short:

@mourner
Copy link
Member Author

mourner commented Feb 24, 2021

@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 Fil mentioned this pull request Feb 25, 2021
@Fil
Copy link
Collaborator

Fil commented Mar 25, 2021

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
Copy link

jrus commented Mar 27, 2021

This is great!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants