Open ua-kxie opened 4 months ago
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.
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
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.
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.
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.
I had to do some digging to work out when these were added, it was very early in glam's life https://github.com/bitshifter/glam-rs/commit/2e5ec772994e33e257e7aa5f85ec76d12cdd5656.
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.
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.
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.
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.
All good, I agree it's confusing.
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.
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.
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
sounds good!
What about adding something like a signed_angle_between
which accepts a bool
or enum
for left/right-handedness or
is always right/left-handed and requires consumers to flip sign accordingly?
EDIT: Check out this stackoverflow post
I also have the use case that needs a Scalar angle, not a Quat. I have implemented that same Stackoverflow post previously as a work-around. no matter what happens to the documentation of the Quat function, I would appreciate if this Scalar version would be added too. +1
What happened was: Using IDE-completions for my prompt angle
returned the existing angle function, but no other function, but it is not signed, which I only later realized. That's why I think it is important to inculde "angle" in the function name.
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'sangle_between
than Vec3's currentangle_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.