[Merged by Bors] - bevy_reflect: Remove `ReflectSerialize` and `ReflectDeserialize` registrations from most glam types #6580

MrGVSV · 2022-11-13T08:36:26Z

Objective

Part of #6573

When serializing a DynamicScene we end up treating almost all non-value types as though their type data doesn't exist. This is because when creating the DynamicScene we call Reflect::clone_value on the components, which generates a Dynamic type for all non-value types.

What this means is that the glam types are treated as though their ReflectSerialize registrations don't exist. However, the deserializer does pick up the registration and attempts to use that instead. This results in the deserializer trying to operate on "malformed" data, causing this error:

WARN bevy_asset::asset_server: encountered an error while loading an asset: Expected float

Solution

Ideally, we should better handle the serialization of possibly-Dynamic types. However, this runs into issues where the ReflectSerialize expects the concrete type and not a Dynamic representation, resulting in a panic:

bevy/crates/bevy_reflect/src/type_registry.rs

Lines 402 to 413 in 0aa4147

    
           impl<T: Reflect + erased_serde::Serialize> FromType<T> for ReflectSerialize { 
        
               fn from_type() -> Self { 
        
                   ReflectSerialize { 
        
                       get_serializable: |value| { 
        
                           let value = value.downcast_ref::<T>().unwrap_or_else(|| { 
        
                               panic!("ReflectSerialize::get_serialize called with type `{}`, even though it was created for `{}`", value.type_name(), std::any::type_name::<T>()) 
        
                           }); 
        
                           Serializable::Borrowed(value) 
        
                       }, 
        
                   } 
        
               } 
        
           }

Since glam types are so heavily used in Bevy (specifically in Transform and GlobalTransform), it makes sense to just a quick fix in that enables them to be used properly in scenes while a proper solution is found.

This PR simply removes all ReflectSerialize and ReflectDeserialize registrations from the glam types that are reflected as structs.

Changelog

Remove ReflectSerialize and ReflectDeserialize registrations from most glam types

Migration Guide

This PR removes ReflectSerialize and ReflectDeserialize registrations from most glam types. This means any code relying on either of those type data existing for those glam types will need to not do that.

This also means that some serialized glam types will need to be updated. For example, here is Affine3A:

// BEFORE
(
  "glam::f32::affine3a::Affine3A": (1.0, 0.0, 0.0, 0.0, 1.0, 0.0, 0.0, 0.0, 1.0, 0.0, 0.0, 0.0),

// AFTER
  "glam::f32::affine3a::Affine3A": (
    matrix3: (
      x_axis: (
        x: 1.0,
        y: 0.0,
        z: 0.0,
      ),
      y_axis: (
        x: 0.0,
        y: 1.0,
        z: 0.0,
      ),
      z_axis: (
        x: 0.0,
        y: 0.0,
        z: 1.0,
      ),
    ),
    translation: (
      x: 0.0,
      y: 0.0,
      z: 0.0,
    ),
  )
)

This only affects glam types that are treated as structs

TheRawMeatball · 2022-11-14T07:37:32Z

This feels like it might break semver... Are we sure it's OK to do in a patch release?

alice-i-cecile

Can we add more tests to catch this sort of issue better in the future?

MrGVSV · 2022-11-23T01:04:01Z

Can we add more tests to catch this sort of issue better in the future?

Yes! I actually meant to create an issue for this but forgot lol. Basically we should probably have a more "real world" scene that contains usages of Transform, Visibility, and other common (or uncommon) components. I imagine we can create this in the repo's tests folder and ensure any changes to reflection/scenes doesn't break that test(s).

That should be its own PR, though.

MrGVSV · 2022-11-23T01:05:46Z

This feels like it might break semver... Are we sure it's OK to do in a patch release?

I’m not sure what does or doesn't break semver tbh. Why might this be problematic? Isn't fixing an unintended behavior okay for a patch release?

PROMETHIA-27

LGTM!

Edit: And this doesn't break semver since we're pre-1.0. I know cargo doesn't care, but imo that's cargo's problem, not ours. And even if we overlook that, this changes a broken feature into a working one, which imo is worth it.

alice-i-cecile · 2022-11-25T21:58:36Z

bors r+

…strations from most glam types (#6580) # Objective > Part of #6573 When serializing a `DynamicScene` we end up treating almost all non-value types as though their type data doesn't exist. This is because when creating the `DynamicScene` we call `Reflect::clone_value` on the components, which generates a Dynamic type for all non-value types. What this means is that the `glam` types are treated as though their `ReflectSerialize` registrations don't exist. However, the deserializer _does_ pick up the registration and attempts to use that instead. This results in the deserializer trying to operate on "malformed" data, causing this error: ``` WARN bevy_asset::asset_server: encountered an error while loading an asset: Expected float ``` ## Solution Ideally, we should better handle the serialization of possibly-Dynamic types. However, this runs into issues where the `ReflectSerialize` expects the concrete type and not a Dynamic representation, resulting in a panic: https://github.com/bevyengine/bevy/blob/0aa4147af6d583c707863484d6a8ad50ed0ed984/crates/bevy_reflect/src/type_registry.rs#L402-L413 Since glam types are so heavily used in Bevy (specifically in `Transform` and `GlobalTransform`), it makes sense to just a quick fix in that enables them to be used properly in scenes while a proper solution is found. This PR simply removes all `ReflectSerialize` and `ReflectDeserialize` registrations from the glam types that are reflected as structs. --- ## Changelog - Remove `ReflectSerialize` and `ReflectDeserialize` registrations from most glam types ## Migration Guide This PR removes `ReflectSerialize` and `ReflectDeserialize` registrations from most glam types. This means any code relying on either of those type data existing for those glam types will need to not do that. This also means that some serialized glam types will need to be updated. For example, here is `Affine3A`: ```rust // BEFORE ( "glam::f32::affine3a::Affine3A": (1.0, 0.0, 0.0, 0.0, 1.0, 0.0, 0.0, 0.0, 1.0, 0.0, 0.0, 0.0), // AFTER "glam::f32::affine3a::Affine3A": ( matrix3: ( x_axis: ( x: 1.0, y: 0.0, z: 0.0, ), y_axis: ( x: 0.0, y: 1.0, z: 0.0, ), z_axis: ( x: 0.0, y: 0.0, z: 1.0, ), ), translation: ( x: 0.0, y: 0.0, z: 0.0, ), ) ) ```

bors · 2022-11-25T22:02:19Z

Build failed:

build-android

alice-i-cecile · 2022-11-25T23:30:02Z

bors retry

…strations from most glam types (#6580) # Objective > Part of #6573 When serializing a `DynamicScene` we end up treating almost all non-value types as though their type data doesn't exist. This is because when creating the `DynamicScene` we call `Reflect::clone_value` on the components, which generates a Dynamic type for all non-value types. What this means is that the `glam` types are treated as though their `ReflectSerialize` registrations don't exist. However, the deserializer _does_ pick up the registration and attempts to use that instead. This results in the deserializer trying to operate on "malformed" data, causing this error: ``` WARN bevy_asset::asset_server: encountered an error while loading an asset: Expected float ``` ## Solution Ideally, we should better handle the serialization of possibly-Dynamic types. However, this runs into issues where the `ReflectSerialize` expects the concrete type and not a Dynamic representation, resulting in a panic: https://github.com/bevyengine/bevy/blob/0aa4147af6d583c707863484d6a8ad50ed0ed984/crates/bevy_reflect/src/type_registry.rs#L402-L413 Since glam types are so heavily used in Bevy (specifically in `Transform` and `GlobalTransform`), it makes sense to just a quick fix in that enables them to be used properly in scenes while a proper solution is found. This PR simply removes all `ReflectSerialize` and `ReflectDeserialize` registrations from the glam types that are reflected as structs. --- ## Changelog - Remove `ReflectSerialize` and `ReflectDeserialize` registrations from most glam types ## Migration Guide This PR removes `ReflectSerialize` and `ReflectDeserialize` registrations from most glam types. This means any code relying on either of those type data existing for those glam types will need to not do that. This also means that some serialized glam types will need to be updated. For example, here is `Affine3A`: ```rust // BEFORE ( "glam::f32::affine3a::Affine3A": (1.0, 0.0, 0.0, 0.0, 1.0, 0.0, 0.0, 0.0, 1.0, 0.0, 0.0, 0.0), // AFTER "glam::f32::affine3a::Affine3A": ( matrix3: ( x_axis: ( x: 1.0, y: 0.0, z: 0.0, ), y_axis: ( x: 0.0, y: 1.0, z: 0.0, ), z_axis: ( x: 0.0, y: 0.0, z: 1.0, ), ), translation: ( x: 0.0, y: 0.0, z: 0.0, ), ) ) ```

bors · 2022-11-25T23:49:14Z

Pull request successfully merged into main.

Build succeeded:

…strations from most glam types (bevyengine#6580) # Objective > Part of bevyengine#6573 When serializing a `DynamicScene` we end up treating almost all non-value types as though their type data doesn't exist. This is because when creating the `DynamicScene` we call `Reflect::clone_value` on the components, which generates a Dynamic type for all non-value types. What this means is that the `glam` types are treated as though their `ReflectSerialize` registrations don't exist. However, the deserializer _does_ pick up the registration and attempts to use that instead. This results in the deserializer trying to operate on "malformed" data, causing this error: ``` WARN bevy_asset::asset_server: encountered an error while loading an asset: Expected float ``` ## Solution Ideally, we should better handle the serialization of possibly-Dynamic types. However, this runs into issues where the `ReflectSerialize` expects the concrete type and not a Dynamic representation, resulting in a panic: https://github.com/bevyengine/bevy/blob/0aa4147af6d583c707863484d6a8ad50ed0ed984/crates/bevy_reflect/src/type_registry.rs#L402-L413 Since glam types are so heavily used in Bevy (specifically in `Transform` and `GlobalTransform`), it makes sense to just a quick fix in that enables them to be used properly in scenes while a proper solution is found. This PR simply removes all `ReflectSerialize` and `ReflectDeserialize` registrations from the glam types that are reflected as structs. --- ## Changelog - Remove `ReflectSerialize` and `ReflectDeserialize` registrations from most glam types ## Migration Guide This PR removes `ReflectSerialize` and `ReflectDeserialize` registrations from most glam types. This means any code relying on either of those type data existing for those glam types will need to not do that. This also means that some serialized glam types will need to be updated. For example, here is `Affine3A`: ```rust // BEFORE ( "glam::f32::affine3a::Affine3A": (1.0, 0.0, 0.0, 0.0, 1.0, 0.0, 0.0, 0.0, 1.0, 0.0, 0.0, 0.0), // AFTER "glam::f32::affine3a::Affine3A": ( matrix3: ( x_axis: ( x: 1.0, y: 0.0, z: 0.0, ), y_axis: ( x: 0.0, y: 1.0, z: 0.0, ), z_axis: ( x: 0.0, y: 0.0, z: 1.0, ), ), translation: ( x: 0.0, y: 0.0, z: 0.0, ), ) ) ```

…strations from most glam types (#6580) # Objective > Part of #6573 When serializing a `DynamicScene` we end up treating almost all non-value types as though their type data doesn't exist. This is because when creating the `DynamicScene` we call `Reflect::clone_value` on the components, which generates a Dynamic type for all non-value types. What this means is that the `glam` types are treated as though their `ReflectSerialize` registrations don't exist. However, the deserializer _does_ pick up the registration and attempts to use that instead. This results in the deserializer trying to operate on "malformed" data, causing this error: ``` WARN bevy_asset::asset_server: encountered an error while loading an asset: Expected float ``` ## Solution Ideally, we should better handle the serialization of possibly-Dynamic types. However, this runs into issues where the `ReflectSerialize` expects the concrete type and not a Dynamic representation, resulting in a panic: https://github.com/bevyengine/bevy/blob/0aa4147af6d583c707863484d6a8ad50ed0ed984/crates/bevy_reflect/src/type_registry.rs#L402-L413 Since glam types are so heavily used in Bevy (specifically in `Transform` and `GlobalTransform`), it makes sense to just a quick fix in that enables them to be used properly in scenes while a proper solution is found. This PR simply removes all `ReflectSerialize` and `ReflectDeserialize` registrations from the glam types that are reflected as structs. --- ## Changelog - Remove `ReflectSerialize` and `ReflectDeserialize` registrations from most glam types ## Migration Guide This PR removes `ReflectSerialize` and `ReflectDeserialize` registrations from most glam types. This means any code relying on either of those type data existing for those glam types will need to not do that. This also means that some serialized glam types will need to be updated. For example, here is `Affine3A`: ```rust // BEFORE ( "glam::f32::affine3a::Affine3A": (1.0, 0.0, 0.0, 0.0, 1.0, 0.0, 0.0, 0.0, 1.0, 0.0, 0.0, 0.0), // AFTER "glam::f32::affine3a::Affine3A": ( matrix3: ( x_axis: ( x: 1.0, y: 0.0, z: 0.0, ), y_axis: ( x: 0.0, y: 1.0, z: 0.0, ), z_axis: ( x: 0.0, y: 0.0, z: 1.0, ), ), translation: ( x: 0.0, y: 0.0, z: 0.0, ), ) ) ```

…strations from most glam types (bevyengine#6580) # Objective > Part of bevyengine#6573 When serializing a `DynamicScene` we end up treating almost all non-value types as though their type data doesn't exist. This is because when creating the `DynamicScene` we call `Reflect::clone_value` on the components, which generates a Dynamic type for all non-value types. What this means is that the `glam` types are treated as though their `ReflectSerialize` registrations don't exist. However, the deserializer _does_ pick up the registration and attempts to use that instead. This results in the deserializer trying to operate on "malformed" data, causing this error: ``` WARN bevy_asset::asset_server: encountered an error while loading an asset: Expected float ``` ## Solution Ideally, we should better handle the serialization of possibly-Dynamic types. However, this runs into issues where the `ReflectSerialize` expects the concrete type and not a Dynamic representation, resulting in a panic: https://github.com/bevyengine/bevy/blob/0aa4147af6d583c707863484d6a8ad50ed0ed984/crates/bevy_reflect/src/type_registry.rs#L402-L413 Since glam types are so heavily used in Bevy (specifically in `Transform` and `GlobalTransform`), it makes sense to just a quick fix in that enables them to be used properly in scenes while a proper solution is found. This PR simply removes all `ReflectSerialize` and `ReflectDeserialize` registrations from the glam types that are reflected as structs. --- ## Changelog - Remove `ReflectSerialize` and `ReflectDeserialize` registrations from most glam types ## Migration Guide This PR removes `ReflectSerialize` and `ReflectDeserialize` registrations from most glam types. This means any code relying on either of those type data existing for those glam types will need to not do that. This also means that some serialized glam types will need to be updated. For example, here is `Affine3A`: ```rust // BEFORE ( "glam::f32::affine3a::Affine3A": (1.0, 0.0, 0.0, 0.0, 1.0, 0.0, 0.0, 0.0, 1.0, 0.0, 0.0, 0.0), // AFTER "glam::f32::affine3a::Affine3A": ( matrix3: ( x_axis: ( x: 1.0, y: 0.0, z: 0.0, ), y_axis: ( x: 0.0, y: 1.0, z: 0.0, ), z_axis: ( x: 0.0, y: 0.0, z: 1.0, ), ), translation: ( x: 0.0, y: 0.0, z: 0.0, ), ) ) ```

Remove ReflectSerialize/ReflectDeserialize from glam types

0aa4147

This only affects glam types that are treated as structs

MrGVSV added C-Bug An unexpected or incorrect behavior A-Reflection Runtime information about types A-Scenes Serialized ECS data stored on the disk M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide labels Nov 13, 2022

cart added this to the 0.9.1 milestone Nov 13, 2022

sQu1rr mentioned this pull request Nov 21, 2022

DynamicScene (de)serialization to postcard and bincode fails for some components. #6713

Closed

alice-i-cecile requested changes Nov 23, 2022

View reviewed changes

alice-i-cecile approved these changes Nov 23, 2022

View reviewed changes

sQu1rr mentioned this pull request Nov 25, 2022

Scene fails to serialize #6627

Closed

PROMETHIA-27 approved these changes Nov 25, 2022

View reviewed changes

alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Nov 25, 2022

bors bot changed the title ~~bevy_reflect: Remove ReflectSerialize and ReflectDeserialize registrations from most glam types~~ [Merged by Bors] - bevy_reflect: Remove ReflectSerialize and ReflectDeserialize registrations from most glam types Nov 25, 2022

bors bot closed this Nov 25, 2022

This was referenced Dec 3, 2022

Scene not loading when entity has GlobalTransform or Children #6573

Closed

Add better scene tests #6834

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Merged by Bors] - bevy_reflect: Remove `ReflectSerialize` and `ReflectDeserialize` registrations from most glam types #6580

[Merged by Bors] - bevy_reflect: Remove `ReflectSerialize` and `ReflectDeserialize` registrations from most glam types #6580

MrGVSV commented Nov 13, 2022

TheRawMeatball commented Nov 14, 2022

alice-i-cecile left a comment

MrGVSV commented Nov 23, 2022 •

edited

Loading

MrGVSV commented Nov 23, 2022

PROMETHIA-27 left a comment •

edited

Loading

alice-i-cecile commented Nov 25, 2022

bors bot commented Nov 25, 2022

alice-i-cecile commented Nov 25, 2022

bors bot commented Nov 25, 2022

	impl<T: Reflect + erased_serde::Serialize> FromType<T> for ReflectSerialize {
	fn from_type() -> Self {
	ReflectSerialize {
	get_serializable: \|value\| {
	let value = value.downcast_ref::<T>().unwrap_or_else(\|\| {
	panic!("ReflectSerialize::get_serialize called with type `{}`, even though it was created for `{}`", value.type_name(), std::any::type_name::<T>())
	});
	Serializable::Borrowed(value)
	},
	}
	}
	}

[Merged by Bors] - bevy_reflect: Remove ReflectSerialize and ReflectDeserialize registrations from most glam types #6580

[Merged by Bors] - bevy_reflect: Remove ReflectSerialize and ReflectDeserialize registrations from most glam types #6580

Conversation

MrGVSV commented Nov 13, 2022

Objective

Solution

Changelog

Migration Guide

TheRawMeatball commented Nov 14, 2022

alice-i-cecile left a comment

Choose a reason for hiding this comment

MrGVSV commented Nov 23, 2022 • edited Loading

MrGVSV commented Nov 23, 2022

PROMETHIA-27 left a comment • edited Loading

Choose a reason for hiding this comment

alice-i-cecile commented Nov 25, 2022

bors bot commented Nov 25, 2022

alice-i-cecile commented Nov 25, 2022

bors bot commented Nov 25, 2022

[Merged by Bors] - bevy_reflect: Remove `ReflectSerialize` and `ReflectDeserialize` registrations from most glam types #6580

[Merged by Bors] - bevy_reflect: Remove `ReflectSerialize` and `ReflectDeserialize` registrations from most glam types #6580

MrGVSV commented Nov 23, 2022 •

edited

Loading

PROMETHIA-27 left a comment •

edited

Loading