From e5f5ce5e9738983a1d4e46b864d2714b638e747e Mon Sep 17 00:00:00 2001
From: Trashtalk217 <trashtalk217@gmail.com>
Date: Tue, 10 Oct 2023 00:01:42 +0200
Subject: [PATCH] Migrate `Quat` reflection strategy from "value" to "struct"
 (#10068)

Adopted from #8954, co-authored by @pyrotechnick

# Objective

The Bevy ecosystem currently reflects `Quat` via "value" rather than the
more appropriate "struct" strategy. This behaviour is inconsistent to
that of similar types, i.e. `Vec3`. Additionally, employing the "value"
strategy causes instances of `Quat` to be serialised as a sequence `[x,
y, z, w]` rather than structures of shape `{ x, y, z, w }`.

The [comments surrounding the applicable
code](https://github.com/bevyengine/bevy/blob/bec299fa6e727a59d973fc8ca8f468732d40cb14/crates/bevy_reflect/src/impls/glam.rs#L254)
give context and historical reasons for this discrepancy:

```
// Quat fields are read-only (as of now), and reflection is currently missing
// mechanisms for read-only fields. I doubt those mechanisms would be added,
// so for now quaternions will remain as values. They are represented identically
// to Vec4 and DVec4, so you may use those instead and convert between.
```

This limitation has [since been lifted by the upstream
crate](https://github.com/bitshifter/glam-rs/commit/374625163ef51bbcac1f909f413a068b6ba4d1a0),
glam.

## Solution

Migrating the reflect strategy of Quat from "value" to "struct" via
replacing `impl_reflect_value` with `impl_reflect_struct` resolves the
issue.

## Changelog

Migrated `Quat` reflection strategy to "struct" from "value"
Migration Guide

Changed Quat serialization/deserialization from sequences `[x, y, z, w]`
to structures `{ x, y, z, w }`.

---------

Co-authored-by: pyrotechnick <13998+pyrotechnick@users.noreply.github.com>
Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
---
 crates/bevy_reflect/src/impls/glam.rs | 39 ++++++++---------
 crates/bevy_reflect/src/lib.rs        | 61 ++++++++++++++++++++++++++-
 2 files changed, 80 insertions(+), 20 deletions(-)

diff --git a/crates/bevy_reflect/src/impls/glam.rs b/crates/bevy_reflect/src/impls/glam.rs
index 9a20b098cea5a..64b3a1ea5220a 100644
--- a/crates/bevy_reflect/src/impls/glam.rs
+++ b/crates/bevy_reflect/src/impls/glam.rs
@@ -1,6 +1,5 @@
 use crate as bevy_reflect;
 use crate::prelude::ReflectDefault;
-use crate::{ReflectDeserialize, ReflectSerialize};
 use bevy_reflect_derive::{impl_reflect_struct, impl_reflect_value};
 use glam::*;
 
@@ -310,24 +309,26 @@ impl_reflect_struct!(
     }
 );
 
-// Quat fields are read-only (as of now), and reflection is currently missing
-// mechanisms for read-only fields. I doubt those mechanisms would be added,
-// so for now quaternions will remain as values. They are represented identically
-// to Vec4 and DVec4, so you may use those instead and convert between.
-impl_reflect_value!(::glam::Quat(
-    Debug,
-    PartialEq,
-    Serialize,
-    Deserialize,
-    Default
-));
-impl_reflect_value!(::glam::DQuat(
-    Debug,
-    PartialEq,
-    Serialize,
-    Deserialize,
-    Default
-));
+impl_reflect_struct!(
+    #[reflect(Debug, PartialEq, Default)]
+    #[type_path = "glam"]
+    struct Quat {
+        x: f32,
+        y: f32,
+        z: f32,
+        w: f32,
+    }
+);
+impl_reflect_struct!(
+    #[reflect(Debug, PartialEq, Default)]
+    #[type_path = "glam"]
+    struct DQuat {
+        x: f64,
+        y: f64,
+        z: f64,
+        w: f64,
+    }
+);
 
 impl_reflect_value!(::glam::EulerRot(Debug, Default));
 impl_reflect_value!(::glam::BVec3A(Debug, Default));
diff --git a/crates/bevy_reflect/src/lib.rs b/crates/bevy_reflect/src/lib.rs
index 53304fb65b428..b01adece410bb 100644
--- a/crates/bevy_reflect/src/lib.rs
+++ b/crates/bevy_reflect/src/lib.rs
@@ -575,7 +575,7 @@ pub mod __macro_exports {
 #[allow(clippy::disallowed_types, clippy::approx_constant)]
 mod tests {
     #[cfg(feature = "glam")]
-    use ::glam::{vec3, Vec3};
+    use ::glam::{quat, vec3, Quat, Vec3};
     use ::serde::{de::DeserializeSeed, Deserialize, Serialize};
     use bevy_utils::HashMap;
     use ron::{
@@ -1937,6 +1937,65 @@ bevy_reflect::tests::Test {
     mod glam {
         use super::*;
 
+        #[test]
+        fn quat_serialization() {
+            let q = quat(1.0, 2.0, 3.0, 4.0);
+
+            let mut registry = TypeRegistry::default();
+            registry.register::<f32>();
+            registry.register::<Quat>();
+
+            let ser = ReflectSerializer::new(&q, &registry);
+
+            let config = PrettyConfig::default()
+                .new_line(String::from("\n"))
+                .indentor(String::from("    "));
+            let output = to_string_pretty(&ser, config).unwrap();
+            let expected = r#"
+{
+    "glam::Quat": (
+        x: 1.0,
+        y: 2.0,
+        z: 3.0,
+        w: 4.0,
+    ),
+}"#;
+
+            assert_eq!(expected, format!("\n{output}"));
+        }
+
+        #[test]
+        fn quat_deserialization() {
+            let data = r#"
+{
+    "glam::Quat": (
+        x: 1.0,
+        y: 2.0,
+        z: 3.0,
+        w: 4.0,
+    ),
+}"#;
+
+            let mut registry = TypeRegistry::default();
+            registry.register::<Quat>();
+            registry.register::<f32>();
+
+            let de = UntypedReflectDeserializer::new(&registry);
+
+            let mut deserializer =
+                ron::de::Deserializer::from_str(data).expect("Failed to acquire deserializer");
+
+            let dynamic_struct = de
+                .deserialize(&mut deserializer)
+                .expect("Failed to deserialize");
+
+            let mut result = Quat::default();
+
+            result.apply(&*dynamic_struct);
+
+            assert_eq!(result, quat(1.0, 2.0, 3.0, 4.0));
+        }
+
         #[test]
         fn vec3_serialization() {
             let v = vec3(12.0, 3.0, -6.9);