Add 2d and 3d rotations to euclid. #226

nical · 2017-09-14T18:14:32Z

The 2d and 3d rotation APIs are modeled using the same principles as the transform types: Use simple terminology but don't try to hide what's happening under the hood (for example that the 3d rotation is a quaternion).

This gives something that looks like this:

let r1 = Rotation3D::quaternion(x, y, z, w); // I speak quats fluently
let r2 = Rotation3D::euler(raw, pitch, yaw);
let r3 = r1.post_rotate(&r2); // just like Transform3D::post_rotate
let r4 = Rotation3D::around_y(foo);
let p = r3.transform_point(&point2(1.0, 2.0));

Some of the Transform3D APIs dealing with rotations should be modified to use the rotation type, but I'd like to postpone doing breaking changes.

I also added a 2d rotation type with the same type of API represented by a simple angle in radians (which I think is the simplest and most convenient representation for 2d). I need 2d rotations in a few places in lyon, some of which are SVG stuff that servo/webrender will eventually need as well.

I am pretty happy with the look of the API and how it follows the same philosophy as other euclid types.
I think that with the addition of a Translation type the whole thing converges towards a coherent (and opinionated) design that has its merits and that should satisfy a different niche from the cgmath and nalgebra crowds.

This change is

nical · 2017-09-14T18:18:52Z

The PR isn't yet in a landable state, it needs more:

tests
documentation

cc @kvark @icefoxen

@icefoxen sorry for the delay on this. If you want to help speed this up, you can write some tests. I did a lot of copy-pasting so there's probably a bunch of stuff that's broken.

nical · 2017-09-27T13:44:17Z

The PR is now in a pretty good shape (tests and doc included).

Other methods could be added, like converting back into euler angles, and also extracting the rotation as quaternion from a matrix (which appears to be used in Gecko), but this PR is already big enough as it is.

r? @kvark

kvark

Great work! A few notes to address.

kvark · 2017-09-27T14:34:48Z

src/rotation.rs

+
+    /// Creates the identity rotation.
+    #[inline]
+    pub fn identity() -> Self where T: Zero {


this is begging for an associated constant

True that. Let's add it for this type and the other ones as a followup.

kvark · 2017-09-27T14:34:48Z

src/rotation.rs

+    pub fn transform_point(&self, point: &TypedPoint2D<T, Src>) -> TypedPoint2D<T, Dst> {
+        let a = self.angle;
+        point2(
+            point.x * Float::cos(a) - point.y * Float::sin(a),


might be worth not computing sin/cos twice

kvark · 2017-09-27T14:34:48Z

src/rotation.rs

+    pub fn euler(roll: Radians<T>, pitch: Radians<T>, yaw: Radians<T>) -> Self {
+        let half = T::one() / (T::one() + T::one());
+
+	    let (sy, cy) = Float::sin_cos(half * yaw.get());


indentation?

kvark · 2017-09-27T14:34:48Z

src/rotation.rs

+    #[inline]
+    pub fn is_normalized(&self) -> bool {
+        // TODO: we might need to relax the threshold here, because of floating point imprecision.
+        (self.i * self.i + self.j * self.j + self.k * self.k + self.r *self.r).approx_eq(&T::one())


might reuse the same code from norm if we have norm2 or something

kvark · 2017-09-27T14:34:48Z

src/rotation.rs

+        }
+
+        // For robustness, stay within the domain of acos.
+        dot = Float::min(Float::max(dot, -one), one);


dot is non-negative at this point, so need to clamp against the lower bound

kvark · 2017-09-27T14:34:48Z

src/rotation.rs

+    ///
+    /// The input point must be use the unit Src, and the returned point has the unit Dst.
+    #[inline]
+    pub fn transform_point(&self, point: &TypedPoint2D<T, Src>) -> TypedPoint2D<T, Dst> {


why is it transform_point as opposed to rotate_point? I'd expect transform_* to be reserved for typed transformations

I hadn't thought about it. rotate_point indeed makes sense.

kvark · 2017-09-27T14:34:48Z

src/rotation.rs

+        T::approx_epsilon()
+    }
+
+    fn approx_eq(&self, other: &Self) -> bool {


should we try to inverse the signs of one of them if they are equal otherwise?

Good point.

kvark · 2017-09-27T14:34:48Z

src/rotation.rs

+    let rm90 = Rotation2D::new(Radians::new(-FRAC_PI_2));
+    let r180 = Rotation2D::new(Radians::new(PI));
+
+    assert!(ri.transform_point(&point2(1.0, 2.0)).approx_eq(&point2(1.0, 2.0)));


case for assert_approx_eq?

kvark · 2017-09-27T14:34:48Z

src/rotation.rs

+    // q2 = numpy.quaternion(0, 1, 0, 0)
+    // quaternion.slerp_evaluate(q1, q2, .0)
+
+    assert!(q1.slerp(&q2, 0.0).approx_eq(&q1));


would be nice to also have tests for the code paths of A) quaternions being very close to each other, and B) quaternions in different hemispheres

kvark · 2017-09-27T14:34:48Z

src/rotation.rs

+    use std::f32::consts::{PI, FRAC_PI_2};
+    let rotations = [
+        Rotation3D::identity(),
+        Rotation3D::around_x(Radians::new(FRAC_PI_2)),


we are missing a test for around_axis?

nical · 2017-09-27T15:53:49Z

First round of review fixes in, I need to think about the missing tests.

nical · 2017-09-27T16:10:18Z

Note: I just renamed normalized_quaternion into unit_quaternion which I find a bit more pleasant. Let me know if you'd prefer that I revert that commit.

nical · 2017-09-27T16:57:49Z

Added some more tests (by messing with with numpy) and squashed the commits.

kvark

Thanks, looks better now. Still a few points to address.

kvark · 2017-09-27T18:09:04Z

src/rotation.rs

+    fn approx_eq(&self, other: &Self) -> bool {
+        (
+            self.i.approx_eq(&other.i)
+            && self.j.approx_eq(&other.j)


nit: could be shorter with using get_vector

D'oh! It would if approx_eq was sensibly implemented for the vector type, but the latter implements ApproxEq instead of ApproxEq as it should.

I'll fix that later but that's techinally a breaking change so I'll do it whenever we have another breaking change to justify it.

kvark · 2017-09-27T18:09:04Z

src/rotation.rs

+        T::approx_epsilon()
+    }
+
+    fn approx_eq(&self, other: &Self) -> bool {


shouldn't this just call approx_eq_eps with a standard epsilon?

That would work too


        Introduce 2d and 3d rotation types.

nical · 2017-09-27T19:55:47Z

I also last-minute-added pre_rotate and post_rotate in TypedRotation2D, which I had forgotten, and a static method radians(T)->Self, which can be less cumbersome than things like Rotation2D::new(Radians::new(PI)).


        Version 0.15.3

kvark

Looks like everything is addressed now?
r=me when ready

nical · 2017-09-27T21:18:36Z

I believe it is ready.

nical · 2017-09-29T12:06:16Z

@bors-servo r=kvark

bors-servo · 2017-09-29T12:06:16Z

📌 Commit 88fb35b has been approved by kvark

bors-servo · 2017-09-29T12:06:20Z

⌛ Testing commit 88fb35b with merge 657b691...


        Auto merge of #226 - nical:rotation, r=kvark

Add 2d and 3d rotations to euclid. The 2d and 3d rotation APIs are modeled using the same principles as the transform types: Use simple terminology but don't try to hide what's happening under the hood (for example that the 3d rotation is a quaternion). This gives something that looks like this: ```rust let r1 = Rotation3D::quaternion(x, y, z, w); // I speak quats fluently let r2 = Rotation3D::euler(raw, pitch, yaw); let r3 = r1.post_rotate(&r2); // just like Transform3D::post_rotate let r4 = Rotation3D::around_y(foo); let p = r3.transform_point(&point2(1.0, 2.0)); ``` Some of the `Transform3D` APIs dealing with rotations should be modified to use the rotation type, but I'd like to postpone doing breaking changes. I also added a 2d rotation type with the same type of API represented by a simple angle in radians (which I think is the simplest and most convenient representation for 2d). I need 2d rotations in a few places in lyon, some of which are SVG stuff that servo/webrender will eventually need as well. I am pretty happy with the look of the API and how it follows the same philosophy as other euclid types. I think that with the addition of a [Translation](#214) type the whole thing converges towards a coherent (and opinionated) design that has its merits and that should satisfy a different niche from the cgmath and nalgebra crowds.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/euclid/226)

bors-servo · 2017-09-29T12:09:30Z

☀️ Test successful - status-travis
Approved by: kvark
Pushing 657b691 to master...

nical force-pushed the nical:rotation branch 2 times, most recently from d3f2248 to c2e9c16 Sep 26, 2017

kvark reviewed Sep 27, 2017

View changes

nical force-pushed the nical:rotation branch from b26ade3 to 9dddc5d Sep 27, 2017

kvark reviewed Sep 27, 2017

View changes

Introduce 2d and 3d rotation types.

Loading status checks…

e65826d

nical force-pushed the nical:rotation branch from 9dddc5d to e65826d Sep 27, 2017

Version 0.15.3

Loading status checks…

88fb35b

kvark approved these changes Sep 27, 2017

View changes

bors-servo merged commit 88fb35b into servo:master Sep 29, 2017
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

nical deleted the nical:rotation branch Sep 29, 2017

servo / euclid

Add 2d and 3d rotations to euclid. #226

Add 2d and 3d rotations to euclid. #226

nical commented Sep 14, 2017 •

edited by larsbergstrom

nical commented Sep 14, 2017 •

edited

nical commented Sep 27, 2017

kvark left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

nical commented Sep 27, 2017

nical commented Sep 27, 2017

nical commented Sep 27, 2017

kvark left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

nical commented Sep 27, 2017

kvark left a comment

nical commented Sep 27, 2017

nical commented Sep 29, 2017

bors-servo commented Sep 29, 2017

bors-servo commented Sep 29, 2017

bors-servo commented Sep 29, 2017

servo / euclid

Join GitHub today

Add 2d and 3d rotations to euclid. #226

Add 2d and 3d rotations to euclid. #226

Conversation

nical commented Sep 14, 2017 • edited by larsbergstrom

nical commented Sep 14, 2017 • edited

nical commented Sep 27, 2017

kvark left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

nical commented Sep 27, 2017

nical commented Sep 27, 2017

nical commented Sep 27, 2017

kvark left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

nical commented Sep 27, 2017

kvark left a comment

nical commented Sep 27, 2017

nical commented Sep 29, 2017

bors-servo commented Sep 29, 2017

bors-servo commented Sep 29, 2017

bors-servo commented Sep 29, 2017

nical commented Sep 14, 2017 •

edited by larsbergstrom

nical commented Sep 14, 2017 •

edited