Angle Object AngleBetweenTwoPoints is Wrong

Bugs
1

The method BABYLON.Angle.BetweenTwoPoints seems to have been written for two 2D points given by position vectors, hence the name rather than BABYLON.Angle.BetweenTwoVectors. Unfortunately however it was intended it does not work.

Let A and B be two points, in the 2D Cartesian plane, with position vectors a and b as in the diagram below

image

The angle between a and b is the red one.

The calculation for

BABYLON.Angle.BetweenTwoPoints(a, b);

uses

var delta = b.subtract(a);
var theta = Math.atan2(delta.y, delta.x);
return new Angle(theta);

and unless I am missing something this results in the green angle not the red angle.

I would suggest the correct calculation, giving the angle between 0 and Ļ€ is

var theta = Math.acos(Vector2.Dot(a.clone().normalize(), b.clone.normalize()));
return new Angle(theta)

This would then work as Angle.BetweenTwoVectors and could work for Vector3s

2

atan2 range is [-pi, pi] and acos range is [0,pi]
Moreover, Iā€™d suggest you to clamp the Dot result to [-1,1]. Back in the days, Iā€™ve found that sometime, dot returns 1.000001 for some colinear values resulting in acos returning NaN. Better be safe.

3

@Cedric I shouldnā€™t rush to post before dashing off to lunch. I missed a bracket so image didnā€™t show. Also forgot in my ā€˜solutionā€™ to consider colinear points.

I do know that ranges for atan2 and acos are different. However that is not the issue.

Now that the image is displayed would anyone agree that the calculation for Angle.BetweenTwoPoints is flawed.

Sticking to the case I think the Angle.BetweenTwoPoints was probably designed for and to give answers in the range [-Ļ€ Ļ€] I think the method should be more clearly defined as

/**
* Gets a new Angle object valued with the angle value in radians between the two 2D points given as position vectors
* @param a defines first position vector2
* @param b defines second position vector2
* @returns a new Angle from a to b
*/
public static BetweenTwoPoints(a: DeepImmutable<Vector2>, b: DeepImmutable<Vector2>): Angle {
    var thetaA = Math.atan2(a.y, a.x);
    var thetaB = Math.atan2(b.y, b.x);
    var theta = thetaB - thetaA;
    return new Angle(theta); 
}

You can see the error in this PG Babylon.js Playground

4

The current behavior is what I would personally expect, with it giving the angle to point b with a as origin

5

@Gijs that interpretation makes sense of the result. However that is not how it is described in the source and API Angle - Babylon.js Documentation

which is

Gets a new Angle object valued with the angle value in radians between the two given vectors

So suggest that if your interpretation is what is intended comments are changed from

/**
* Gets a new Angle object valued with the angle value in radians between the two given vectors
* @param a defines first vector
* @param b defines second vector
 * @returns a new Angle
     */
public static BetweenTwoPoints(a: DeepImmutable<Vector2>, b: DeepImmutable<Vector2>): Angle {
    var delta = b.subtract(a);
    var theta = Math.atan2(delta.y, delta.x);
    return new Angle(theta);
}

/**
* Given a point A with a position vector2 and another point B, as a position vector2, with the line AX parallel to the X axis gets a new Angle object giving the angle XAB in radians 
* @param a defines origin vector2
* @param b defines second vector2
 * @returns a new Angle
 */
public static BetweenTwoPoints(a: DeepImmutable<Vector2>, b: DeepImmutable<Vector2>): Angle {
    var delta = b.subtract(a);
    var theta = Math.atan2(delta.y, delta.x);
    return new Angle(theta);
}
6

Ok, I understand your confusion now, because of vectors not actually being vectors. Replacing it with ā€œpointā€ and making clear a is the origin should clear it up right? Or else it becomes a bit verbose IMO:

/**
* Gets a new Angle object valued with the angle value in radians between the two given points
* @param a defines first point as the origin
* @param b defines second point
* @returns a new Angle
*/
public static BetweenTwoPoints(a: DeepImmutable<Vector2>, b: DeepImmutable<Vector2>): Angle {
    var delta = b.subtract(a).normalize();
    var theta = Math.acos(delta.y, delta.x);
    return new Angle(theta);
}

And it would be great to add support for 3d points as well since they could both use the dot product instead to get the angle, but I donā€™t know of a way to pick the right class (Vector2|Vector3) without checking the type, which is bad for performanceā€¦

7

Unfortunately the term angle between two points has no meaning in maths so can be interpreted in too many different ways

I could settle for

/**
* Gets a new Angle object valued with the gradient angle in radians of the line joining two points
* @param a defines first point as the origin
* @param b defines second point
* @returns a new Angle
*/
public static BetweenTwoPoints(a: DeepImmutable<Vector2>, b: DeepImmutable<Vector2>): Angle {
    var delta = b.subtract(a).normalize();
    var theta = Math.acos(delta.y, delta.x);
    return new Angle(theta);
}

Already have

https://doc.babylonjs.com/api/classes/babylon.vector3#getanglebetweenvectors

though it returns an angle not an Angle object.

Could do same for vector2

3 Likes
8

@JohnK Did you mean to put Math.acos in the quoted code? Changing the math for BetweenTwoPoints has implications since there is existing code that use it.

9

You are correct I meant

/**
* Gets a new Angle object valued with the gradient angle in radians of the line joining two points
* @param a defines first point as the origin
* @param b defines second point
* @returns a new Angle
*/
public static BetweenTwoPoints(a: DeepImmutable<Vector2>, b: DeepImmutable<Vector2>): Angle {
    var delta = b.subtract(a);
    var theta = Math.atan2(delta.y, delta.x);
    return new Angle(theta);
}

It is only the comments I meant to change at this point having now understood the purpose. There were some copy and paste confusion.

2 Likes