Provide feature parity for Quaternion with Godot
#981
Conversation
|
API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-981 |
godot-core/src/builtin/quaternion.rs
Outdated
| /// Constructs a Quaternion representing the shortest arc between `arc_from` and `arc_to`. | ||
| /// | ||
| /// These can be imagined as two points intersecting a unit sphere's surface, with a radius of 1.0. | ||
| /// | ||
| // This is an undocumented assumption of Godot's as well. | ||
| /// The inputs must be unit vectors. | ||
| /// | ||
| /// For near-singular cases (`arc_from`≈`arc_to` or `arc_from`≈-`arc_to`) the current implementation is only accurate to about | ||
| /// 0.001, or better if `double-precision` is enabled. | ||
| /// | ||
| /// *Godot equivalent: `Quaternion(arc_from: Vector3, arc_to: Vector3)`* | ||
| pub fn from_rotation_arc(arc_to: Vector3, arc_from: Vector3) -> Self { | ||
| <Self as GlamConv>::Glam::from_rotation_arc(arc_to.to_glam(), arc_from.to_glam()).to_front() | ||
| } |
There was a problem hiding this comment.
I find the to, from order confusing; it's also inconsistent with Godot's own constructor:
glam is still an implementation detail in our library, so we shouldn't take its conventions at face value.
Should we check preconditions such as "unit vectors" via debug_assert?
There was a problem hiding this comment.
Oh sorry I missed that order. Emulating Godot was my intention, just missed that bit.
Debug asserts could be good; I didn't add anything like that since Godot doesn't. Glam also has a feature for this kind of debug assert, so it might be possible to enable glam_assert for debug builds and gain these kinds of assertions for free (though then would the error messages be any good?)
There was a problem hiding this comment.
I'd probably do our own asserts, then we're more in control -- and conditions for Godot are sometimes also different.
godot-core/src/builtin/quaternion.rs
Outdated
| pub fn get_euler(self, order: EulerOrder) -> Vector3 { | ||
| Basis::from_quat(self).to_euler(order) | ||
| } | ||
|
|
||
| #[deprecated = "Moved to `Quaternion::get_euler()`"] | ||
| pub fn to_euler(self, order: EulerOrder) -> Vector3 { | ||
| self.get_euler(order) | ||
| } | ||
|
|
There was a problem hiding this comment.
to_euler was a deliberate choice, see #124 (review). As such I would keep it.
It could however benefit from a doc-alias.
godot-core/src/builtin/quaternion.rs
Outdated
| impl From<Basis> for Quaternion { | ||
| fn from(basis: Basis) -> Self { | ||
| basis.to_quat() | ||
| } | ||
| } |
There was a problem hiding this comment.
I don't think this is necessary, given we already have Basis::to_quat.
There was a problem hiding this comment.
I think this does give the user more control, this provides both Quaternion::from or basis.into() too. Depending on the user's coding style and what's easier for them to understand, any of the above may be better for them.
There was a problem hiding this comment.
Multiple points here:
- New features need to have a justification to exist, satisfying all code styles isn't a good one. If we followed that principle, we'd have a cluttered mess in no time 😉
- Redundant features need an ever higher bar. Once there are multiple ways to achieve the same thing, the whole "which should I use" and "is there a difference" questions come up.
Fromhas the problem that it cannot do type inference. You can writelet a = basis.to_quat()but notlet a = basis.into()(at least not without further info after this line).- When you use
into(), your code is objectively harder to understand than a named method that shows what you convert into. The main benefit in my opinion is for generic programming, which isn't applicable here. - One argument is that
Fromdemonstrates equivalence between two types. This also doesn't apply here, because basis can store rotation + scale, while quaternion can only store rotation. This even goes against not one, but three official recommendations stating thatFromshould be lossless, value-preserving and obvious. - godot-rust as part of its API design doesn't use
Fromin many places. Traditionally we've had it for string types, and it helped keep arguments generic asimpl Into<GString>. Now there'sAsArgwhich takes over that role, but some conversions are still done viaFrom. Maybe named methods would be better there too, who knows.
This discussion comes up every now and then, I should probably add it to the book 🙂
There was a problem hiding this comment.
- About the two first points, yeah, you're right. I like having multiple ways of achieving the same (&str to owned, to string, into String and String from), but the code clutter and confusion it generates is non negligible.
- About type inference and understandability, when using into one can easily say into:: and it would make sense to say that as a sentence. to_quat is a specific function to know, it could be to_quaternion instead, it's not as intuitive imo. That said, if you are using Basis and Quaternions you know about them, so it makes sense one would found to_quat easily.
- Does Into have the same recommendations as From? It would make sense, but it would also make sense the opposite since From → Into but not the opposite. I agree under those guidelines this is a bad use of From.
- About the last part, I stand the same about named methods, imo they can be harder to find and use, rather than a To trait. But if the whole crate follows the convention to_name_struct that also fixes the above. I think it would be a nice addition to the book? A conversions between types section, maybe.
And yeah! It would be nice having it in the book. Thank you for illustrating me on this and sorry for having said something that dumb.
There was a problem hiding this comment.
- when using into one can easily say into::
Actually not, the way it's defined doesn't allow you to use turbofish .into::<Quaternion>(). It's another common pitfall -- because the into() function isn't generic itself, only the Into<T> trait is 🙂
to_quat is a specific function to know, it could be to_quaternion instead, it's not as intuitive imo.
Fair point 🤔 it was added in #124 alongside Basis::from_quat for the other direction. In the context it's probably quite clear, there are also other abbreviations in conversions, such as from_cols, into_dyn etc. But I'm also OK with changing it.
Thank you for illustrating me on this and sorry for having said something that dumb.
It's not dumb at all! I appreciate any input as long as it's constructive, and the fact that this has been brought up a few time shows that it's a frequent question, too. So no worries at all 😊
- I think it would be a nice addition to the book? A conversions between types section, maybe.
This particular instance is probably best kept in the API docs (as this is the place for looking up API of a concrete type). But what can make sense is to elaborate some general "conventions" or API designs that godot-rust uses. I'll make some notes about this.
There was a problem hiding this comment.
It's another common pitfall
Oh goddess Hylia I think I hate into now. I would then just advocate for a To trait with to() function, that makes the "to type" action a standard action in the repository, but I'm not against it not being a trait, I just think it could be easier to find the one function you need from a user standpoint.
But I'm also OK with changing it.
I'm not saying from_quat and to_quat are bad options though, I'm all on board of using abbreviations when it's clear. I just wish for times when there would be function aliases since what one people thinks obvious another would not. I'm not a library maintainer so I don't know much about these though.
So no worries at all 😊
I'm glad I don't annoy with my comments at the least.
But what can make sense is to elaborate some general "conventions" or API designs that godot-rust uses. I'll make some notes about this.
Yeah! This would be great, specially for contributors. I don't mind much so long as there's a convention I can follow safely and know users will understand.
d569d2f to
3a5e921
Compare
More notable change is the renaming with deprecations for a couple methods. The new names are what Godot uses.
3a5e921 to
3e1fbad
Compare
More notable change is the renaming with deprecations for a couple methods. The new names are what Godot uses.