forwardRotationLambda does not normalize angles beyond ±3π

[jrus]

If you try to project angles outside the range [–3π, 3π], it can result in wonky artifacts from resampling:

Here is the current code for forwardRotationLambda https://github.com/d3/d3-geo/blob/v3.0.1/src/rotation.js#L17-L21

function forwardRotationLambda(deltaLambda) {
  return function(lambda, phi) {
    return lambda += deltaLambda, [lambda > pi ? lambda - tau : lambda < -pi ? lambda + tau : lambda, phi];
  };
}

My proposed alternative is:

function forwardRotationLambda(deltaLambda) {
  return function(lambda, phi) {
    lambda += deltaLambda;
    while (lambda > pi) lambda -= tau;
    while (lambda < -pi) lambda += tau;
    return [lambda, phi];
  };
}

This should run at the same speed, and in my opinion is also a bit more legible.

[jrus]

I suppose one potential issue with a loop would be dealing with cases where someone puts the angle as 1e30 or something.

[Fil]

Hmmm… maybe it's the Mercator projection that's taking too short a shortcut? I wasn't able to reproduce though with d3.geoMercator.

[jrus]

Example: https://observablehq.com/d/32af8d71be6ee180

{
  const
    width = 600,
    segment = {type: "LineString", coordinates: [[-800, 55], [-1000, -35]]},
    sphere = {type: "Sphere"},
    extent = [[5,5], [width-5, width-5]],
    projection = d3.geoMercator().fitExtent(extent, sphere).rotate([45]),
    ctx = DOM.context2d(width, width),
    path = d3.geoPath(projection, ctx);
  
  ctx.beginPath(), path(d3.geoGraticule10()),
    ctx.lineWidth = 1.5; ctx.strokeStyle = '#ccc', ctx.stroke();
  ctx.beginPath(), path(segment),
    ctx.lineWidth = 6; ctx.strokeStyle = '#26b', ctx.stroke();
  ctx.beginPath(), path(sphere),
    ctx.lineWidth = 2; ctx.strokeStyle = '#000', ctx.stroke();
  
  return ctx.canvas;
}

[jrus]

Oh, I guess the identity rotation removes the appropriate number of turns: https://github.com/d3/d3-geo/blob/v3.0.1/src/rotation.js#L4-L6

function rotationIdentity(lambda, phi) {
  return [abs(lambda) > pi ? lambda + Math.round(-lambda / tau) * tau : lambda, phi];
}

So we could also just use that method for forwardRotationLambda. e.g.

function forwardRotationLambda(deltaLambda) {
  return function(lambda, phi) {
    lambda += deltaLambda;
    if (abs(lambda) > pi) lambda -= Math.round(lambda / tau) * tau;
    return [lambda, phi];
  };
}

[Fil]

I was wondering if the alternative was to fix mercatorRaw instead (in https://github.com/d3/d3-geo/blob/v3.0.1/src/projection/mercator.js#L6), but the same issue impacts all the projections that use lambda without trig, for example geoNaturalEarth1 and geoEquirectangular.

I guess #147 should have covered forwardRotationLambda too!

[jrus]

Is there a preferred style between e.g.

function forwardRotationLambda(deltaLambda) {
  return function(lambda, phi) {
    lambda += deltaLambda;
    if (abs(lambda) > pi) lambda -= Math.round(lambda / tau) * tau;
    return [lambda, phi];
  };
}

function forwardRotationLambda(deltaLambda) {
  return function(lambda, phi) {
    return [abs(lambda += deltaLambda) > pi ? lambda - Math.round(lambda / tau) * tau : lambda, phi];
  };
}

function forwardRotationLambda(deltaLambda) {
  return function(lambda, phi) {
    return [(lambda += deltaLambda) - (abs(lambda) > pi ? Math.round(lambda / tau) * tau : 0), phi];
  };
}

I find the crammed-in-one-line versions less legible, but they seem closer to the prevailing style.

[Fil]

I'm happy with the first version.

[mbostock]

Fixed in #244.