Vec2/3's `angle_between` function is unclear/misleading

[ua-kxie]

I ran off into a fruitless tangent just now wondering why Vec3's angle_between wouldn't give me a signed angle (was using Vec3 in a 2d context).

I think a quat_between method for Vec3 would be a good addition. It is a better analog to Vec2's angle_between than Vec3's current angle_between, which isn't the complete rotational information in 3D as it is for Vec2 in 2D.

I could probably work on a PR if the idea sounds good to the maintainers.

[bitshifter]

Does https://docs.rs/glam/latest/glam/f32/struct.Quat.html#method.from_rotation_arc do what you are after?

If it does, one thing that can be done is adding some metadata to docs so angle_between or between would find the quat method.

[ua-kxie]

Functionally from_rotation_arc does what I am looking for, thank you. I saw it in the sidebar but it wasn't evident from the name that it took two vectors as arg.

I'm not sure what you mean by metadata in the docs; I know one can reference other structs but I don't know if that's what you mean

[ua-kxie]

On the subject of docs it's also not apparent that "invalid" under matrix inverse comments should be checked with is_nan(). I only made this connection after reading

If a non-invertible matrix is inverted by glam or euclid the result will be invalid (it will contain NaNs).

in mathbench.

[bitshifter]

It is possible to add doc aliases so if someone searches for something it will match the doc alias. For example there is a doc alias on length methods for magnitude as this is a common alternative name for this method - https://docs.rs/glam/latest/src/glam/f32/sse2/vec4.rs.html#439. I don't know if you used the search function when you were looking but if you did potentially a doc alias would help here (e.g. if you searched for angle_between).

I'm happy to clarify docs, if you want to submit a PR go for it (it will require running codegen) otherwise I'll try get to it sometime.

[ua-kxie]

I was not searching for a function with this name, rather I was scanning the sidebar under Methods for Vec3 in the docs for something that sounds like it might be what I'm looking for.

From a documentation pov, I think the main problem is the gotcha suggested by docstring for angle_between under Vec3.

Returns the angle (in radians) between two vectors.
The inputs do not need to be unit vectors however they must be non-zero.

compare with the docstring for the same such named method under Vec2:

Returns the angle (in radians) between self and rhs in the range [-π, +π].
The inputs do not need to be unit vectors however they must be non-zero.

The similarities give users every reason to believe that they are eachothers' analog, while Vec2's documentation clearly states that it returns the complete rotational information to go from one vector to another by stating the output range. To realize that these two methods are fundamentally different, one must look at the method return signature and remember that a single float cannot complete describe a rotation vector in 3D. There's just a small hint where the range [-π, +π] is mentioned in one but not the other. But this may simply be dismissed (as I did) as a docstring improvement that missed the Vec3 version. The Vec3 documentation should be more explicit about what it does and how it is different than the same named function under Vec2, and reference from_rotation_arc as the 3d analog to Vec2's angle_between.

More generally, it makes sense that both Vec2 and Vec3 share a function named angle_between, but which both return floats in the range of [0, +π], with a method name that suggests invariance to direction/precedence/commutative. While both Vec2 and Vec3 should have another method named something along the lines of 'angle_from', which could return a signed float for Vec2 and a Quat for Vec3.

I hope this clarifies my issue, that it wasn't just that I didn't find the function I was looking for.

[bitshifter]

I had to do some digging to work out when these were added, it was very early in glam's life 2e5ec77.

I think they are fundamentally different because of the differences between 2D and 3D coordinate systems. In 3D we can have left or right handed coordinate systems. glam is coordinate system agnostic, it doesn't assume one or the other. So a 3D angle_between can't return a -ve or +ve rotation, because it doesn't know which way the rotation should go without further information about the coordinate system being used. I am a bit less clear on the situation in 2D but I don't think there's any ambiguity around the direction of rotation in the 2D case (some discussion https://gamedev.stackexchange.com/questions/168488/handedness-of-2d-coordinate-systems).

Given these methods have been around a long time, most likely they are in use, it's always hard to know with an open source project. I think my preference here would be to improve the docs over renaming at this stage. Describing the ranges and the different behaviour between the 2D and 3D cases might help avoid some confusion when moving between 2D and 3D coordinate systems in glam.

[ua-kxie]

a +/-ve rotation in 3D wouldn't capture the complete rotation. Even if assuming right or left handed coordinate system the axis of rotation is still left unspecified. If Quat::from_rotation_arc doesn't assume handedness then there's no reason a quat_from method for Vec3 would need to.

The table below summarizes what I think would make sense to name a set of related methods

Vec2	Vec3	Commutative?	desc
angle_between -> f32 [0, +π]	angle_between -> f32 [0, +π]	commutative	shortest angle in rad between two supplied vectors
rotation_from-> f32 [-π, +π]	quat_from -> Quat	non-commutative	returns data describing how to rotate other to self

Glam's angle_from methods for Vec2 and Vec3 respectively are rotation_from and angle_between from the table above.

[bitshifter]

I want to try and understand how these are currently used by glam users since they aren't methods that I use myself, if possible, before making any changes.

I can see the 2D version is used in bevy for example, but only once. They also have their own 2D rotation type which has an angle_between which returns a value in the range (-pi, pi], so if I change the meaning of angle_between in glam in the 2D case it will now have different semantics to this method in bevy.

The tricky part with changing the meaning of a function like you are proposing here is it can't be deprecated, it just changes the behaviour of existing code which silently breaks things. I can bump the version number in glam to indicated a breaking change but that can still catch people out if they don't pay attention to the release notes. It's not something I do lightly.

[ua-kxie]

yeah, I think just improving the docs at this point is a perfectly adequate way to address this. To be clear I only wanted to document/communicate clearly what I think is misleading about the way it is currently.

[bitshifter]

All good, I agree it's confusing.

[bitshifter]

One way I could introduce this change would be to rename the 2D version as you have suggested here and not add a 2D angle_between until a later glam version. Hopefully most users would get the breaking change and will have updated their code at the point that a 2D angle_between is added back.

[ua-kxie]

That sounds like a good way to introduce what would otherwise be silently breaking change.

I haven't put that much thought into the names. The ones I came up with was only meant to help communicate the issue. For one I'm not sure whether the function actually computes the angle_from or the angle_to. Another thing is that a name like radians_from might be better, if a newtype is undesireable.

I'm ok to have this issue sit awhile and see if theres anymore input, too.

[bitshifter]

Naming wise, there is currently a Vec2::rotate_towards(self, rhs, max_angle), so I think Vec2::angle_towards(self, rhs) could match up with that edit, changed to angle_to on the basis that rotate_towards doesn't necessarily reach the destination, angle_to is the full angle from self to rhs.

// the angle returned by angle_to can be used to rotate the input vector to the destination vector
assert_approx_eq!(
    Vec2::from_angle(Vec2::X.angle_to(Vec2::Y)).rotate(Vec2::X),
    Vec2::Y
);

Vec2::rotate_towards basically does this internally but clamps the angle used.

I actually have to make another breaking change so it's a good time to try and get this change in.

There currently isn't a Vec3::rotate_towards so I don't think that the 3D version necessarily needs to be added now, but I guess quat_to might be OK.

See draft PR #524

[ua-kxie]

sounds good!