Repository: bitshifter/glam-rs
Pull request #49 (2020-03-03 14:57:18)

Added x,y,z,w methods to Quat
This adds methods to read the elements of a quaternion directly. 
You can currently do this by using the various `From` traits implemented on Quat, e.g.: 
```rust
let (x, y, z, w) = quat;
```
or 

```rust
let x = Vec4::from(quat).x();
```
The first of these has the disadvantage that you don't know for sure that you got the order correct without looking up the source. The second is better, perhaps, but again, it's not obvious that it's valid without checking, since the Quat type could well store its elements in the order `w`,`x`,`y`,`z`.

Adding these methods removes the need for any indirection, brings Quat more in line with other data types, and I think it leads to more obviously correct code. 



bitshifter: Hi @Chris-Nicholls , thanks for the PR. The main reason I didn't add accessors to `Quat` was I didn't know of a use case where people might want to access the individual elements. What is your use case for adding these?
Chris-Nicholls: Hey,
So my use case is serialising the quaternions into a binary format for a
multiplayer game. You rarely want to sync four floats for quaternions, and
instead want to compress them in smallest threes or quantize a la
https://gafferongames.com/post/snapshot_compression/

For this you need to access the individual x,y,z,w elements.

On Tue, 3 Mar 2020, 22:35 Cameron Hart, <notifications@github.com> wrote:

> Hi @Chris-Nicholls <https://github.com/Chris-Nicholls> , thanks for the
> PR. The main reason I didn't add accessors to Quat was I didn't know of a
> use case where people might want to access the individual elements. What is
> your use case for adding these?
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/bitshifter/glam-rs/pull/49?email_source=notifications&email_token=AATG4NE4VGMEDOJJUSUV4XDRFWA3FA5CNFSM4LAMH452YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOENVN7MI#issuecomment-594206641>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AATG4NA4CXO4JEQMAADPGVTRFWA3FANCNFSM4LAMH45Q>
> .
>

bitshifter: Makes sense. It is worth keeping in mind that because `Quat` is stored in SIMD vector it is slightly more expensive to access each element individually. As you point out `From` could be ambiguous. On the matrix types I added more explicit `to` methods (e.g. `to_cols_array`) for the same reason. Do you think it would make sense to add a `to_xyzw` here? This would mirror the `from_xyzw` method that's already there.

I can merge this PR regardless, but was interested if you thought that might be useful.
Chris-Nicholls: I like the idea of `to_xyzw` but, at least for my use case, it's very unlikely to provide a noticeable performance difference since the rest of the serialization will dominate. 


Patch:
diff --git a/src/f32/quat.rs b/src/f32/quat.rs
index 39d0112..0a98d75 100644
--- a/src/f32/quat.rs
+++ b/src/f32/quat.rs
@@ -441,6 +441,29 @@ impl Quat {
             Self(Vec4(_mm_add_ps(result0, result1)))
         }
     }
+    /// Returns element `x`.
+    #[inline]
+    pub fn x(self) -> f32 {
+        self.0.x()
+    }
+
+    /// Returns element `y`.
+    #[inline]
+    pub fn y(self) -> f32 {
+        self.0.y()
+    }
+
+    /// Returns element `z`.
+    #[inline]
+    pub fn z(self) -> f32 {
+        self.0.z()
+    }
+
+    /// Returns element `w`.
+    #[inline]
+    pub fn w(self) -> f32 {
+        self.0.w()
+    }
 }
 
 impl fmt::Debug for Quat {


Test patch:
diff --git a/tests/quat.rs b/tests/quat.rs
index eecc137..1ab6357 100644
--- a/tests/quat.rs
+++ b/tests/quat.rs
@@ -230,3 +230,17 @@ fn test_quat_serde() {
     let deserialized = serde_json::from_str::<Quat>("[1.0,2.0,3.0,4.0,5.0]");
     assert!(deserialized.is_err());
 }
+
+#[test]
+fn test_quat_elements() {
+    let x = 1.0;
+    let y = 2.0;
+    let z = 3.0;
+    let w = 4.0;
+
+    let a = Quat::from_xyzw(x, y, z, w);
+    assert!(a.x() == x);
+    assert!(a.y() == y);
+    assert!(a.z() == z);
+    assert!(a.w() == w);
+}