Skip to content

Commit

Permalink
Migrate Quat reflection strategy from "value" to "struct" (bevyengi…
Browse files Browse the repository at this point in the history
…ne#10068)

Adopted from bevyengine#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](bitshifter/glam-rs@3746251),
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>
  • Loading branch information
3 people authored and Thomas Wilgenbus committed Oct 13, 2023
1 parent 8ada9d8 commit bca8398
Show file tree
Hide file tree
Showing 2 changed files with 80 additions and 20 deletions.
39 changes: 20 additions & 19 deletions crates/bevy_reflect/src/impls/glam.rs
Original file line number Diff line number Diff line change
@@ -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::*;

Expand Down Expand Up @@ -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));
Expand Down
61 changes: 60 additions & 1 deletion crates/bevy_reflect/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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::{
Expand Down Expand Up @@ -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);
Expand Down

0 comments on commit bca8398

Please sign in to comment.