[RSDK-732] Create Quaternion and corresponding FFI namespace #10

gvaradarajan · 2022-11-03T21:50:00Z

No description provided.

biotinker · 2022-11-03T22:00:15Z

src/spatialmath/quaternion.rs

+    /// represented according to the Tait-Bryan formalism and applied 
+    /// in the Z-Y'-X" order (where Z -> yaw, Y -> pitch, X -> roll). 
+    /// The return value is a list of [roll, pitch, yaw]
+    pub fn to_euler_angles(&self) -> [f64;3] {


It may be worthwhile to allow the user to specify the rotation order for to/from euler angles.

https://github.com/go-gl/mathgl/blob/v1.0.0/mgl64/quat.go#L265

Specifying the rotation order is just a permutation at the very end (or very beginning), yeah? That sounds like a nice-to-have, but not something for the MVP. I wouldn't write anything more than a TODO for it yet (but am open to being wrong!).

we don't currently though, is there a reason for that?

any chance we can leave this as a follow-up? I'm pretty sure the most popular representation is ZYX

The reason we don't currently is because the library I linked exists and does it :)
The other two common (at least, I've seen them be in use) representations are XYZ and ZYZ.
But I don't think it needs to gate this PR, as long as it goes on a to-do list (or a library that already implements this is found)

Here's one for the common extrinsic and intrinsic representations, a quick google found it. I like the idea of having this as our own feature should we need it/ something external causes bugs. I think we should keep it to Tait-Bryan which Gautham has written in for the time being.

penguinland

I haven't looked at the FFI file yet, but here's a start. Looks very good so far!

penguinland · 2022-11-03T22:05:23Z

src/ffi/spatialmath/mod.rs

+pub mod vector3;
+pub mod quaternion;


I worry that I'm sounding like a broken record at this point, but I still think these files should all end in newlines, because not ending in a new line is not POSIX-compliant and some text editors aren't going to like that. I'd still be happy to help you configure your text editor to do this automatically, if you want. If you want to do it on your own, I think a decent starting point is https://thoughtbot.com/blog/no-newline-at-end-of-file

Not a broken record; not ending in a newline is AFAIK explicitly against our internal style guide as well, and will break all sorts of things. Please make sure you keep calling this out wherever you see it.

src/spatialmath/vector3.rs

src/spatialmath/quaternion.rs

penguinland · 2022-11-03T22:20:57Z

src/spatialmath/quaternion.rs

+    /// represented according to the Tait-Bryan formalism and applied 
+    /// in the Z-Y'-X" order (where Z -> yaw, Y -> pitch, X -> roll). 
+    /// The return value is a list of [roll, pitch, yaw]
+    pub fn to_euler_angles(&self) -> [f64;3] {


Specifying the rotation order is just a permutation at the very end (or very beginning), yeah? That sounds like a nice-to-have, but not something for the MVP. I wouldn't write anything more than a TODO for it yet (but am open to being wrong!).

penguinland · 2022-11-03T22:22:59Z

src/spatialmath/quaternion.rs

+            roll = roll_sin_pitch_cos.atan2(roll_cos_pitch_cos);
+        }
+
+


Mild preference to get rid of one of these blank lines. One blank line is enough inside a function.

penguinland · 2022-11-03T22:31:30Z

src/spatialmath/quaternion.rs

+    }
+
+    pub fn conjugate(&self) -> Self {
+        Self { real: self.real, i: self.i * -1.0, j: self.j * -1.0, k: self.k * -1.0 }


DIscussion question: I had expected -1.0 * self.i, rather than self.i * -1.0, because normally when you write math, the numbers come before the letters. Do other folks prefer this ordering instead? I'm not against it, but it did surprise me a little.

penguinland · 2022-11-03T22:32:37Z

src/spatialmath/quaternion.rs

+
+    /// Allocates the vector to the heap with a stable memory address and
+    /// returns the raw pointer (for use by the FFI interface)
+    pub(crate) fn to_raw_pointer(&self) -> *mut Self {


Naive question: does this have to be here, or could we put it in the FFI stuff instead with the code it goes with, so we don't need the comment? Maybe there's a reason it needs to be here, but I don't see it yet.

I can move, l'll do it for vector as well

penguinland · 2022-11-03T22:34:24Z

src/spatialmath/quaternion.rs

+impl std::ops::Add<Quaternion> for Quaternion {
+    type Output = Quaternion;
+
+    fn add(self, _rhs: Quaternion) -> Quaternion {


Normally, having a variable start with an underscore in Rust means "suppress the compiler warnings about unused variables. We know this one isn't used yet." but this one is used. Can we rename it rhs? Same question for several more ops below, too.

I don't know why I made this an _, I can remove

penguinland · 2022-11-03T22:38:29Z

src/spatialmath/quaternion.rs

+        let mut quat = Quaternion::new(
+            0.0,
+            (1.0_f64 / 3.0_f64).sqrt() * 0.5, 
+            (1.0_f64 / 3.0_f64).sqrt() * -0.5, 


I'm mildly surprised that we're multiplying on the right by a constant, and a negative one at that. Could these lines become -0.5 * (1.0_f64 / 3.0_f64).sqrt()) and similar? Maybe that's too nitpicky and this is good as-is.

penguinland · 2022-11-03T22:42:39Z

src/spatialmath/quaternion.rs

+    #[test]
+    fn get_scaled_returns_scaled_quaternion() {
+        let quat = Quaternion::new(0.1, 0.2, 0.3, 0.4);
+        let quat_orig = Quaternion::new(0.1, 0.2, 0.3, 0.4);


Mild preference for let quat_orig = quat.clone() here. By defining it twice, it feels like we don't have a single point of truth on this thing.

src/spatialmath/quaternion.rs

randhid · 2022-11-04T14:53:52Z

src/spatialmath/quaternion.rs

+    /// represented according to the Tait-Bryan formalism and applied 
+    /// in the Z-Y'-X" order (where Z -> yaw, Y -> pitch, X -> roll). 
+    /// The return value is a list of [roll, pitch, yaw]
+    pub fn to_euler_angles(&self) -> [f64;3] {


Here's one for the common extrinsic and intrinsic representations, a quick google found it. I like the idea of having this as our own feature should we need it/ something external causes bugs. I think we should keep it to Tait-Bryan which Gautham has written in for the time being.

src/ffi/spatialmath/quaternion.rs

src/spatialmath/quaternion.rs

src/ffi/spatialmath/quaternion.rs

npmenard · 2022-11-07T21:05:58Z

src/ffi/spatialmath/quaternion.rs

+/// The return value is a pointer to a list of [roll, pitch, yaw]
+/// as C doubles
+#[no_mangle]
+pub unsafe extern "C" fn quaternion_to_euler_angles(quat_ptr: *const Quaternion) -> *const c_double {


is thera a function to free this?

to free the quaternion? Or the euler angles?

to free the *const c_double, I couldn't find the proper fucntion

src/spatialmath/quaternion.rs

npmenard · 2022-11-07T21:19:54Z

src/ffi/spatialmath/quaternion.rs

+
+/// Allocates the quaternion to the heap with a stable memory address and
+/// returns the raw pointer (for use by the FFI interface)
+fn to_raw_pointer(quat: &Quaternion) -> *mut Quaternion {


this is somewhat misleading, you're actually copying the quaternion before boxing it

yeah this is definitely fair, but you asking this brings up something I wish I thought about earlier: should I have done it this way? Should I be copying the quaternion before returning it? I think it's still fine, but I'm not sure

I think it's okay the only type of scenario where I see this being an issue is if you own a reference in some rust data structure and use that reference to change the quaternion you sent to FFI using to_raw_pointer in this case the code calling the FFI won't see the changes made on the Rust side. Given that we use simple data structures we should not run into this so ok just keep this in mind as we grow the library

src/ffi/spatialmath/quaternion.rs

npmenard

Looks great!! Done with first pass.

You should run cargo clippy and fix the errors it surface (could be done in a 2nd PR)

Euler angles are in radians but it's not specified either in the doc or the function

All the unsafe functions should have a #safety section especially since you are working with pointers.

Cloning vs Copy, you have to decide which one you're using mixing them is a bit weird especially since they essentially do the same. I would stick with copying since its only 3/4 f64 !

gvaradarajan · 2022-11-09T15:20:21Z

Looks great!! Done with first pass.

You should run cargo clippy and fix the errors it surface (could be done in a 2nd PR)

Euler angles are in radians but it's not specified either in the doc or the function

All the unsafe functions should have a #safety section especially since you are working with pointers.

Cloning vs Copy, you have to decide which one you're using mixing them is a bit weird especially since they essentially do the same. I would stick with copying since its only 3/4 f64 !

I'm going to correct the clippy errors from vector in this PR, I'll just forget to do it later

npmenard

LGTM

[RSDK-732] Create Quaternion and corresponding FFI namespace

15eb6fe

gvaradarajan requested review from biotinker and randhid November 3, 2022 21:50

gvaradarajan requested review from npmenard and stuqdog as code owners November 3, 2022 21:50

biotinker reviewed Nov 3, 2022

View reviewed changes

penguinland reviewed Nov 3, 2022

View reviewed changes

randhid requested changes Nov 4, 2022

View reviewed changes

randhid reviewed Nov 4, 2022

View reviewed changes

src/spatialmath/quaternion.rs Show resolved Hide resolved

address review comments

efea9cc

gvaradarajan requested review from randhid and biotinker November 4, 2022 19:20