search

d3 / d3-shape

Graphical primitives for visualization, such as lines and areas.
https://d3js.org/d3-shape
ISC License
2.47k stars 307 forks source link

Rounded corners on arc produces NaNs due to floating point imprecision #90

Closed sftrabbit closed 7 years ago

sftrabbit commented 7 years ago

This line in src/arc.js assumes that the argument to Math.acos is between -1 and 1, which is not always true due to floating point imprecision, resulting in NaNs in the arc's path and preventing the arc from rendering.

kc = 1 / Math.sin(Math.acos((ax * bx + ay * by) / (Math.sqrt(ax * ax + ay * ay) * Math.sqrt(bx * bx + by * by))) / 2),

I have a fix for this (#91), which introduces clamped version of acos, but I would also very much appreciate if this could also be fixed in d3 v3 (where there is already a d3_acos function to use). I'm not sure if pull requests are being accepted on v3.

mbostock commented 7 years ago

Fixed in #91; thank you.

If you have time, I would appreciate a test to verify this fix as I am unable to reproduce it.

sftrabbit commented 7 years ago

@mbostock Great!

Regarding the test case: unfortunately, because we're currently using v3 (and I'm now going to push moving to v4) and some of the maths above this line has changed and it's difficult to force floating point errors, I only have a case that produces 'NaN's in v3. However, the logic is of course the same - it's occurring because Math.acos isn't clamped. Here's the v3 test case:

d3.svg.arc()
  .innerRadius(230)
  .outerRadius(255)
  .cornerRadius(3)
  .startAngle(0.18479956785822316)
  .endAngle(3.3263922214480157)()

which results in 'MNaN,NaNANaN,NaN 0 1,1 NaN,NaNLNaN,NaNANaN,NaN 0 0,1 NaN,NaNZ'.

mbostock commented 7 years ago

It’s possible this could not have occurred in v4 anyway because it uses epsilon to test for a number of boundary conditions. (Here, the angle of the arc is 3.3263922214480157 - 0.18479956785822316 = 3.1415926535897927 which is approximately π.)

sftrabbit commented 7 years ago

Ah apologies - so either my PR was unnecessary, or the epsilon just happened to fix my case and there are other cases that would be a problem. I suppose it's better to be safe than sorry though, in case there's any possibility (ax * bx + ay * by) / (Math.sqrt(ax * ax + ay * ay) * Math.sqrt(bx * bx + by * by)) falls outside [-1, 1].

mbostock commented 7 years ago

Yep, happy to have this fix. Thank you!