d3 / d3-shape

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

d3.arc startAngle and endAngle are switched when startAngle > endAngle #187

Closed Lasering closed 2 years ago

Lasering commented 2 years ago
const arc = d3.arc();

arc({
  innerRadius: 0,
  outerRadius: 100,
  startAngle: Math.PI,
  endAngle: Math.PI / 2
});

This draws an arc starting at Math.PI / 2 and ending at Math.PI, aka, the smallest sector between those two angles. However it should draw the biggest sector since the startAngle is Math.PI (180º) and not the Math.PI / 2 (90º).

The fix is really easy:

const adjustedStartAngle = startAngle > endAngle ? startAngle - 2 * Math.PI : startAngle
Fil commented 2 years ago

I don't think this case is documented or tested.

Lasering commented 2 years ago

In the documentation of arc.startAngle and arc.endAngle its stated:

The angle is specified in radians, with 0 at -y (12 o’clock) and positive angles proceeding clockwise.

So if you start at Math.PI and end at Math.PI / 2 one would expect the arc to take the long way (an arc with 270 degrees). The short way would be if you start at Math.PI / 2 and end at Math.PI (an arc with 90 degrees).

TL;DR: its already documented. The implementation is producing incorrect results.

mbostock commented 2 years ago

I’m pretty sure it’s intended to be this way (so that it doesn’t matter which value is the startAngle or the endAngle; the arc is not directional). If you want the larger sector, I expect you need to do this:

arc({
  innerRadius: 0,
  outerRadius: 100,
  startAngle: Math.PI,
  endAngle: 5 * Math.PI / 2
}); 
Lasering commented 2 years ago

If the intended way if for the start and end angles to be interchangeable than it should be documented, the words start and end suggest direction.

Fil commented 2 years ago

Speaking of "suggesting direction", I read "positive angles proceeding clockwise" as implying "negative angles proceeding anticlockwise" (which is conform to what the implementation does); otherwise, the sentence would have been "all angles proceeding clockwise". But I understand how you can read that sentence as meaning only the position and not the span.

If there was a debate whether the implementation should be changed, I'd be strongly against the change, if only because it would make the arc generator unstable when a computation returns a very small span—going from 1e-17 to -1e-17 would suddenly make the arc go from a sliver to a full circle.

Lasering commented 2 years ago

But I understand how you can read that sentence as meaning only the position and not the span.

Not sure If I understand. Starting at π and ending at π/2 will have a positive angle span of 3π/2. The reverse will have a positive angle span of π/2.

when a computation returns a very small span—going from 1e-17 to -1e-17 would suddenly make the arc go from a sliver to a full circle.

Starting at 1e-17 and ending at -1e-17 would produce a full circle. However starting at -1e-17 and ending at 1e-17 would still produce a sliver. Both are still consistent with the "suggesting direction" idea.

Fil commented 2 years ago

Starting at π and ending at π/2 will have a positive angle span of 3π/2. The reverse will have a positive angle span of π/2.

To clarify, that's the convention you're proposing, which is to go from point(startAngle) to point(endAngle) in the clockwise direction. It is opposite to the current implementation, which uses Math.abs(startAngle - endAngle) as the length of the arc.

Starting at 1e-17 and ending at -1e-17 would produce a full circle.

Exactly: my comment is that this would create potentially disastrous visual instability if the numbers originate from an instable formula—like, for example going from startAngle = 0 to endAngle = .6 + .3 - .9 (which is a negative number -1.1102230246251565e-16).

Lasering commented 2 years ago

It is opposite to the current implementation, which uses Math.abs(startAngle - endAngle) as the length of the arc.

That makes sense being the current implementation hence the incorrect result (from my point of view). Following the idea of "suggesting direction" plus the adjustedStartAngle the length of the arc would be computed using endAngle - startAngle and it would always be positive (in the clockwise direction).

this would create potentially disastrous visual instability if the numbers originate from an instable formula—like, for example going from startAngle = 0 to endAngle = .6 + .3 - .9 (which is a negative number -1.1102230246251565e-16).

The core problem in that scenario is the unstable formula which is outside of the d3-shape, hence the correction should be applied outside. The developer will have a hard time figuring it out since the current implementation is correcting/masking it.