Remove unreachable code from vec2.angle

[arccoza]

As I understand it, because you're always calculating the inner angle (cosine θ),
the cosine var will never be greater than 1 or less than -1.

So

if(cosine > 1.0) {
  return 0;
}
else if(cosine < -1.0) {
  return Math.PI;
} else {
  return Math.acos(cosine);
}

will only ever reach the else clause.

The if statements could be updated with >= and <= to be reachable,
and that makes sense, but performance is actually better without the conditional branches.

[arccoza]

https://jsperf.com/angle-between-two-vectors/1
https://repl.it/@arccoza/glMatrix-vec2angle

[stefnotch]

Sadly floating point arithmetic isn't 100% precise. So, it can probably happen that the values end up being outside of the -1 to 1 range.

I tried it out experimentally and found a case where this happens.

function angle(a, b) {
  let x1 = a[0],
    y1 = a[1],
    x2 = b[0],
    y2 = b[1];

  let len1 = x1*x1 + y1*y1;
  if (len1 > 0) {
    //TODO: evaluate use of glm_invsqrt here?
    len1 = 1 / Math.sqrt(len1);
  }

  let len2 = x2*x2 + y2*y2;
  if (len2 > 0) {
    //TODO: evaluate use of glm_invsqrt here?
    len2 = 1 / Math.sqrt(len2);
  }

  let cosine = (x1 * x2 + y1 * y2) * len1 * len2;
  console.log(cosine);
  if(cosine > 1.0) {
    console.log(`${x1},${y1}, ${x2},${y2}`);
  }
  else if(cosine < -1.0) {
    console.log(`${x1},${y1}, ${x2},${y2}`);
  }
}

angle([-44688.06632211879,0], [36533.68996199467,0]);
// > -1.0000000000000002

[arccoza]

I suspected that might be the reason, but couldn't find a case.
Clamping with Math.min(Math.max(cosine, -1), 1) is quicker than the if statements in my tests on Chrome and Firefox (I've updated the bench, repl and my fork), and trims some lines.

FYI this version of the function is the fastest impl and doesn't trip on your example:

function angle2(a, b) {
  let x1 = a[0], y1 = a[1], x2 = b[0], y2 = b[1],
  mag1 = Math.sqrt(x1*x1 + y1*y1),
  mag2 = Math.sqrt(x2*x2 + y2*y2),
  mag = mag1 * mag2,
  cosine = mag && ((x1 * x2 + y1 * y2) / mag)
  return Math.acos(cosine)
}

Though that doesn't mean there isn't an edge case that might catch it.
But adding return Math.acos(Math.min(Math.max(cosine, -1), 1)) to be safe doesn't hurt much.

[stefnotch]

I indeed cannot find a case where your improved function trips up. However, I'm not sure if browsers are allowed to make floating point arithmetic optimizations where they reorder the instructions and possibly make the function fail again.

So, just in case, let's add return Math.acos(Math.min(Math.max(cosine, -1), 1)); and everything should be golden.

function angle2(a, b) {
  let x1 = a[0], y1 = a[1], x2 = b[0], y2 = b[1],
  mag1 = Math.sqrt(x1*x1 + y1*y1),
  mag2 = Math.sqrt(x2*x2 + y2*y2),
  mag = mag1 * mag2,
  cosine = mag && ((x1 * x2 + y1 * y2) / mag)
  return Math.acos(cosine)
}

[arccoza]

Alright, I should point out that while both implementations are faster with the min max clamp, the fastest version angle2 comes up with very slightly different answers, if you check the repl output you'll see what I mean, issue?

[stefnotch]

I don't think glMatrix has any guarantees about the results being precise down to the last bit. So, either one is fine.

[arccoza]

Ok updated.

[stefnotch]

Thank you very much for the PR. I'll merge it as soon as possible.