Don't transform points two times in presimplify

[lukasappelhans]

I was working with a topojson, where d3.geo.bounds got abnormal results. I think the two coordinates loose precision when you transform them to absolute and then back to relative. This PR fixes the issue for me.

[mbostock]

This doesn’t look right. There are two cases to consider here:

If the TopoJSON is not quantized (as in, there’s no topology.transform) then each pair of coordinates contains floating-point values and the absolute and relative transforms are no-ops. Given you’re reporting abnormal results, I’m guessing the data you are using is quantized, so this case doesn’t apply here.

If the TopoJSON is quantized (as in, there is a topology.transform), then each pair of coordinates contains integer values with a delta-encoding, meaning that each pair of coordinates is relative to the previous pair, if any. The absolute transform both converts back to floating-point space and removes the delta encoding; the relative transform does the reverse. Thus, if you remove points, you can’t simply restore the original relative coordinates.

Ignore the scale and translate and just consider the delta-encoding in this example of a unit square, assuming Canvas-style coordinates with the origin [0, 0] in the top-left:

+--+
|  |
+--+

[[0, 0], [1, 0], [0, 1], [-1, 0], [0, -1]] // relative
[[0, 0], [1, 0], [1, 1], [0, 1], [0, 0]] // absolute

So what happens if I remove the third point [1, 1] in absolute coordinates?

+--+
| /
+

[[0, 0], [1, 0], [0, 1], [0, 0]] // absolute (simplified)

And if you remove the third point [0, 1] in relative coordinates?

+
|
+--+

[[0, 0], [1, 0], [-1, 0], [0, -1]] // relative (simplified, incorrect)

Removing the third point in the relative coordinates affects the following points in absolute coordinates, so you need to adjust the point following the removed point:

+--+
| /
+

[[0, 0], [1, 0], [-1, 1], [0, -1]] // relative (simplified, correct)

In addition, assuming that the TopoJSON is quantized, it’s unlikely that the level of quantization approaches the resolution of a IEEE 754 floating-point number (which can represent integers with up to 53 bits). That means it’s unlikely that converting from integer coordinates to floating-point and back again shouldn’t be lossy… but perhaps we need to use Math.round rather than bitwise floor (| 0)?

I really need a test case to investigate this further.

[mbostock]

Edit: Even though this fix doesn’t “look right” I still appreciate you bringing it to my attention and hope we can resolve this issue. 😄

[lukasappelhans]

Thank you for the long explanation! That was very helpful! Plus your guess about rounding vs flooring seems to be right.

You can check out the following blocks with the error and the (new) fix. The map is getting automatically projected with mercator. I use d3.geo.bounds for that, but the old code gives me a BB of [[-180, -90], [180, 90]](it gets logged to the console).

Error: http://bl.ocks.org/lukasappelhans/967f744f80ff2209ad946570063210f4
Fixed: http://bl.ocks.org/lukasappelhans/6e393c9ee954955a8799514386bd68bb

If this sounds right to you, I will update this pull request tomorrow.

[mbostock]

Sounds good!

[lukasappelhans]

Done. :)

[mbostock]

Looks good, especially given 1f19cc3!

[mbostock]

Merged and released as 1.6.25.

[lukasappelhans]

Awesome, thank you so much! :)