Add doctests to Transform, reorder set_rotation_euler arguments.#1052
Add doctests to Transform, reorder set_rotation_euler arguments.#1052bors[bot] merged 1 commit intoamethyst:masterfrom
Conversation
torkleyy
added
type: improvement
AnneKitsune
left a comment
AnneKitsune
left a comment
There was a problem hiding this comment.
This is always so confusing to me, I'm probably wrong
| /// Vector3::new(0.0, 1.0, 0.0), | ||
| /// None); | ||
| /// assert_eq!(t.rotation, rotation); | ||
| /// assert_ulps_eq!(t.rotation, Quaternion::from( |
| /// assert_eq!(t.rotation, Quaternion::one()); | ||
| /// // look up with up pointing backwards | ||
| /// t.look_at(Point3::new(0.0, 1.0, 0.0), Vector3::new(0.0, 0.0, 1.0)); | ||
| /// t.look_at(Point3::new(0.0, 1.0, 0.0), Vector3::new(1.0, 0.0, 0.0)); |
There was a problem hiding this comment.
I have the feeling the first one my more sense in respect to the comment saying up is z+
There was a problem hiding this comment.
Maybe it's best we wait just a little to merge this since Ellie is so far along with nalgebra.. I'd really like to make visualizations for these things to help me and others but mostly me
There was a problem hiding this comment.
Because I feel as confused as you do here. I developed this current solution (as if this is a puzzle lol) with @Rhuagh
|
The nalgebra PR has been merged, do you think these examples can be rebased onto that? |
|
I'll be rebasing these tonight. |
|
Postponing until after 0.10. |
|
Sorry for the delay 😭 rebased! |
topokel
changed the title
|
Reordered the set_rotation_euler arguments to match nalgebra, documented in changelog. This should be ready to merge now. |
|
I think that change might have broken some of the examples so you should probably have a look through those or maybe just do a global search for the function. |
|
Ran tests, did a search and didn't find any usages except the ones I touched. We should be good afaict. |
torkleyy
left a comment
torkleyy
left a comment
There was a problem hiding this comment.
I like it, thanks!
Can we get this merged soon?
dotellie
left a comment
dotellie
left a comment
There was a problem hiding this comment.
Nice! If you just remove the newline, rebase and (maybe) squash this should be good to merge! 😄
|
Good catch. Rebased. |
|
👎 Rejected by label |
|
Oh wait, @torkleyy do you still want to wait? |
torkleyy
left a comment
torkleyy
left a comment
There was a problem hiding this comment.
PR was updated and release moved anyway, so let's merge this. Thanks for making sure though!
bors r=magnonellie,LucioFranco,torkleyy
Build succeeded |
7 participants
No description provided.