d3 / d3-delaunay

Compute the Voronoi diagram of a set of two-dimensional points.
https://d3js.org/d3-delaunay
ISC License
611 stars 57 forks source link

use a Float32Array for points? #85

Closed Fil closed 5 years ago

Fil commented 5 years ago

to go around https://github.com/mapbox/delaunator/issues/43

it seems to fix all the situations we had in various tests — still not clear if it creates new trouble in some other configurations, but I wasn't able to find an example.

semver question: if this is merged, should it be considered a breaking change?

Fil commented 5 years ago

In action:

mbostock commented 5 years ago

It feels too surprising to me that Delaunay.from would choose Float32Array instead of Float64Array, but that new Delaunay would be agnostic about the type. If we don’t want to do this in all cases (and it sounds like we don’t), then the opt-in should be more explicit.

Fil commented 5 years ago

No change here: it's already the case that new Delaunay accepts any flat array while Delaunay.from creates a specific type (Float64Array currently, Float32Array in this PR).

(I agree it's not intellectually satisfying and I'd prefer to find the spot where the code needs to be changed rather than this gross rounding method.)

mbostock commented 5 years ago

This is a little verbose but one option would be a new Delaunay.fromTyped method, where you’d say

Delaunay.fromTyped(Float32Array, points)

instead of

Delaunay.from(points)

if you want to use Float32Array instead of Float64Array. And Delaunay.from(…) would be equivalent to Delaunay.fromTyped(Float64Array, …).

Or we think Float32Array and Float64Array are the only types people will need, we could say:

Delaunay.from32(points)
Fil commented 5 years ago

I've found a solution that works for all of our test cases. Interestingly the change in dist solves the 3rd and 4th test cases, while the change in orient solves the three first.

function dist(ax, ay, bx, by) {
    [ax, ay, bx, by] = [ax, ay, bx, by].map(Math.fround);
    const dx = ax - bx;
    const dy = ay - by;
    return dx * dx + dy * dy;
}

function orient(px, py, qx, qy, rx, ry) {
    [px, py, qx, qy, rx, ry] = [px, py, qx, qy, rx, ry].map(Math.fround);
    return (qy - py) * (rx - qx) - (qx - px) * (ry - qy) < 0;
}

(I know the syntax is dreadful, bear with me.)

Not sure what to do with this yet — but it shows this PR is not what we need.