Migrate Quat reflection strategy from "value" to "struct"

[525c1e21-bd67-4735-ac99-b4b0e5262290]

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 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, 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 }.

[github-actions]

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

[alice-i-cecile]

This looks good to me: great PR description and investigation. Is this intentionally in draft mode?

[MrGVSV]

LGTM!

It would be nice if we added some tests like we do here:

bevy/crates/bevy_reflect/src/lib.rs

Lines 1851 to 1953 in bec299f

	#[cfg(feature = "glam")]
	mod glam {
	use super::*;
	#[test]
	fn vec3_serialization() {
	let v = vec3(12.0, 3.0, -6.9);
	let mut registry = TypeRegistry::default();
	registry.register::<f32>();
	registry.register::<Vec3>();
	let ser = ReflectSerializer::new(&v, &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::f32::vec3::Vec3": (
	x: 12.0,
	y: 3.0,
	z: -6.9,
	),
	}"#;
	assert_eq!(expected, format!("\n{output}"));
	}
	#[test]
	fn vec3_deserialization() {
	let data = r#"
	{
	"glam::f32::vec3::Vec3": (
	x: 12.0,
	y: 3.0,
	z: -6.9,
	),
	}"#;
	let mut registry = TypeRegistry::default();
	registry.add_registration(Vec3::get_type_registration());
	registry.add_registration(f32::get_type_registration());
	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 = Vec3::default();
	result.apply(&*dynamic_struct);
	assert_eq!(result, vec3(12.0, 3.0, -6.9));
	}
	#[test]
	fn vec3_field_access() {
	let mut v = vec3(1.0, 2.0, 3.0);
	assert_eq!(*v.get_field::<f32>("x").unwrap(), 1.0);
	*v.get_field_mut::<f32>("y").unwrap() = 6.0;
	assert_eq!(v.y, 6.0);
	}
	#[test]
	fn vec3_path_access() {
	let mut v = vec3(1.0, 2.0, 3.0);
	assert_eq!(
	*v.reflect_path("x").unwrap().downcast_ref::<f32>().unwrap(),
	1.0
	);
	*v.reflect_path_mut("y")
	.unwrap()
	.downcast_mut::<f32>()
	.unwrap() = 6.0;
	assert_eq!(v.y, 6.0);
	}
	#[test]
	fn vec3_apply_dynamic() {
	let mut v = vec3(3.0, 3.0, 3.0);
	let mut d = DynamicStruct::default();
	d.insert("x", 4.0f32);
	d.insert("y", 2.0f32);
	d.insert("z", 1.0f32);
	v.apply(&d);
	assert_eq!(v, vec3(4.0, 2.0, 1.0));
	}
	}

But I don't think we need to block on that since all the other glam types don't have any tests.

[525c1e21-bd67-4735-ac99-b4b0e5262290]

This looks good to me: great PR description and investigation. Is this intentionally in draft mode?

@alice-i-cecile Wanted to find out more about the test situation before I bothered anyone too much.

LGTM!

It would be nice if we added some tests like we do here:

bevy/crates/bevy_reflect/src/lib.rs

Lines 1851 to 1953 in bec299f

	#[cfg(feature = "glam")]
	mod glam {
	use super::*;
	#[test]
	fn vec3_serialization() {
	let v = vec3(12.0, 3.0, -6.9);
	let mut registry = TypeRegistry::default();
	registry.register::<f32>();
	registry.register::<Vec3>();
	let ser = ReflectSerializer::new(&v, &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::f32::vec3::Vec3": (
	x: 12.0,
	y: 3.0,
	z: -6.9,
	),
	}"#;
	assert_eq!(expected, format!("\n{output}"));
	}
	#[test]
	fn vec3_deserialization() {
	let data = r#"
	{
	"glam::f32::vec3::Vec3": (
	x: 12.0,
	y: 3.0,
	z: -6.9,
	),
	}"#;
	let mut registry = TypeRegistry::default();
	registry.add_registration(Vec3::get_type_registration());
	registry.add_registration(f32::get_type_registration());
	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 = Vec3::default();
	result.apply(&*dynamic_struct);
	assert_eq!(result, vec3(12.0, 3.0, -6.9));
	}
	#[test]
	fn vec3_field_access() {
	let mut v = vec3(1.0, 2.0, 3.0);
	assert_eq!(*v.get_field::<f32>("x").unwrap(), 1.0);
	*v.get_field_mut::<f32>("y").unwrap() = 6.0;
	assert_eq!(v.y, 6.0);
	}
	#[test]
	fn vec3_path_access() {
	let mut v = vec3(1.0, 2.0, 3.0);
	assert_eq!(
	*v.reflect_path("x").unwrap().downcast_ref::<f32>().unwrap(),
	1.0
	);
	*v.reflect_path_mut("y")
	.unwrap()
	.downcast_mut::<f32>()
	.unwrap() = 6.0;
	assert_eq!(v.y, 6.0);
	}
	#[test]
	fn vec3_apply_dynamic() {
	let mut v = vec3(3.0, 3.0, 3.0);
	let mut d = DynamicStruct::default();
	d.insert("x", 4.0f32);
	d.insert("y", 2.0f32);
	d.insert("z", 1.0f32);
	v.apply(&d);
	assert_eq!(v, vec3(4.0, 2.0, 1.0));
	}
	}

But I don't think we need to block on that since all the other glam types don't have any tests.

@MrGVSV I've force-pushed some tests. They failed on Ubuntu since it selected SSE2 but I think they would have passed on MacOS if it were not cancelled.

[PROMETHIA-27]

LGTM

[525c1e21-bd67-4735-ac99-b4b0e5262290]

Hmm, it appears that vec2/vec3 are a bit special in glam.

Bevy deps on glam::Vec3 which doesn't specialise into SSE2/scalar on either Ubuntu or MacOS but glam doesn't seem to provide a Quat that behaves this way; only a Quat that specialises into SSE2/scalar which is why the tests are failing, at least when SSE2 is available.

See:

• https://github.com/bitshifter/glam-rs/blob/main/src/f32/scalar.rs
• https://github.com/bitshifter/glam-rs/blob/main/src/f32/sse2.rs

I guess this was always an issue and is only becoming apparent now that it's being tested.

Not sure whether to:

• Feature-gate the tests i.e. write tests for both when SSE2 is and is not available.
• Change the tests to use TypedReflectSerializer/TypedReflectDeserializer to avoid the specialisations "leaking".
• Drop the tests for now. Actually solve the issue elsewhere and return to testing once the type paths are stable.

[525c1e21-bd67-4735-ac99-b4b0e5262290]

This also broke cargo run --example scene with

WARN bevy_asset::asset_server: encountered an error while loading an asset: Expected identifier at scenes/load_scene_example.scn.ron:16:22

which is expected but wasn't picked up by the CI. I guess CI doesn't test examples?

Should I be testing all examples locally before a PR like this?

[mockersf]

only a Quat that specialises into SSE2/scalar which is why the tests are failing, at least when SSE2 is available.

This was previously discussed in #5803. #7184 should offer a way to fix that

Not sure whether to:

• Feature-gate the tests i.e. write tests for both when SSE2 is and is not available.
• Change the tests to use TypedReflectSerializer/TypedReflectDeserializer to avoid the specialisations "leaking".
• Drop the tests for now. Actually solve the issue elsewhere and return to testing once the type paths are stable.

Can you try to see how it would work with a stable type path?

which is expected but wasn't picked up by the CI. I guess CI doesn't test examples?

CI doesn't test all examples, and this is a warning so it wouldn't even break a test run...

Should I be testing all examples locally before a PR like this?

Maybe not all, but examples relevant to reflection and scene can be interesting to run 👍

[alice-i-cecile]

Can you try to see how it would work with a stable type path?

Seconding this: let's try this before merging :)

[alice-i-cecile]

Blocking on the stable type path changes.

[525c1e21-bd67-4735-ac99-b4b0e5262290]

Technically we could upstream into glam or otherwise shim the stable names/paths ala Vec[2|3|4] but I agree it's worth holding back until stable paths land.

I am personally working on top of soqb's PRs which seem A-OK; they were quick to resolve the only two issues I found so far.

Let us back-burner this until then. In the meantime I've found several more instances this technique should be applied; namely bevy_window::window::WindowPosition::At(IVec2). This endeavour may very well expand to covering those; at least for the core basics such as math & windowing.

[alice-i-cecile]

@pyrotechnick, can you please rebase or otherwise resolve merge conflicts? This is no longer blocked.

[alice-i-cecile]

I've resolved the merge conflicts; can y'all verify that I did so correctly?

[alice-i-cecile]

Looks like it needs formatting following the manual merge conflict resolution.

[MrGVSV]

Nit: Any reason to not use the register method?

Suggested change

	registry.add_registration(Quat::get_type_registration());
	registry.add_registration(f32::get_type_registration());
	registry.register::<Quat>();
	registry.register::<f32>();

[alice-i-cecile]

Putting this up for adoption; there's just a bit more work needed to get over the finish line.

[Trashtalk217]

I've adopted this PR for the final touches in #10068.