Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Serialization of Name component may not be working correctly #12001

Closed
rparrett opened this issue Feb 20, 2024 · 6 comments · Fixed by #12024
Closed

Serialization of Name component may not be working correctly #12001

rparrett opened this issue Feb 20, 2024 · 6 comments · Fixed by #12024
Labels
A-Core Common functionality for all bevy apps A-Scenes Serialized ECS data stored on the disk C-Bug An unexpected or incorrect behavior

Comments

@rparrett
Copy link
Contributor

rparrett commented Feb 20, 2024

Bevy version

0.13

What you did

Modified the scene example to include Name::new("TEST").

Weirdly, I also had to .register_type::<Name>() in the example's App, despite it being registered in bevy_core.

Diff
diff --git a/examples/scene/scene.rs b/examples/scene/scene.rs
index dac36e96d..3acb19f11 100644
--- a/examples/scene/scene.rs
+++ b/examples/scene/scene.rs
@@ -8,6 +8,7 @@ fn main() {
         .register_type::<ComponentA>()
         .register_type::<ComponentB>()
         .register_type::<ResourceA>()
+        .register_type::<Name>()
         .add_systems(
             Startup,
             (save_scene_system, load_scene_system, infotext_system),
@@ -114,6 +115,7 @@ fn save_scene_system(world: &mut World) {
         component_b,
         ComponentA { x: 1.0, y: 2.0 },
         Transform::IDENTITY,
+        Name::new("TEST"),
     ));
     scene_world.spawn(ComponentA { x: 3.0, y: 4.0 });
     scene_world.insert_resource(ResourceA { score: 1 });

cargo run --example scene

And observe the serialized scene in the logged output.

What went wrong

Name is serialized as:

"bevy_core::name::Name": (
  hash: 778249913144375361,
  name: "TEST",
),

Which is contrary to this comment on name:

#[derive(Reflect, Component, Clone)]
#[reflect(Component, Default, Debug)]
#[cfg_attr(feature = "serialize", reflect(Serialize, Deserialize))]
pub struct Name {
hash: u64, // Won't be serialized (see: `bevy_core::serde` module)
name: Cow<'static, str>,
}

And the stated goal of #11447.

And the migration guide text which suggests that Name will be serialized as a plain string.

Additional information

Scenes will properly deserialize without hash, but only as a struct with a name field. Not as a plain string.

See also: #1037 where we removed the migration guide entry related to #11447.

@rparrett rparrett added C-Bug An unexpected or incorrect behavior S-Needs-Triage This issue needs to be labelled labels Feb 20, 2024
@alice-i-cecile alice-i-cecile added A-Core Common functionality for all bevy apps A-Scenes Serialized ECS data stored on the disk and removed S-Needs-Triage This issue needs to be labelled labels Feb 20, 2024
@alice-i-cecile
Copy link
Member

@coreh, any clues on what happened here?

@MrGVSV
Copy link
Member

MrGVSV commented Feb 20, 2024

Do you have the serialize feature enabled? I wonder if it's causing issues if it's not enabled

@rparrett
Copy link
Contributor Author

rparrett commented Feb 20, 2024

Name seems to serialize the same way (struct with hash and name) with both
cargo run --example scene --features=serialize and
cargo run --example scene.

(but the manual type registration is no longer needed when serialize is enabled.)

@rparrett
Copy link
Contributor Author

rparrett commented Feb 20, 2024

However, deserialization requires the "bevy_core::name::Name": "TEST" format if the serialize feature is enabled.

So with serialize, the serialized output is not deserializable.

@soqb
Copy link
Contributor

soqb commented Feb 21, 2024

i've tracked this down to a combination of things:

  1. allowing different formats of serialization based on a feature flag is inconsistent and flaky behavior. (we should probably make the features required or at the very least, default).
  2. DynamicScene converts all its reflected components into proxy types via clone_value(is it impossible to take by reference? — it would fix everything below).
  3. in bevy_reflect, we have a get_serializable function, which uses the proxy type's TypeId instead of the one we actually need (via get_represented_type_info).

fn get_serializable<'a, E: Error>(
reflect_value: &'a dyn Reflect,
type_registry: &TypeRegistry,
) -> Result<Serializable<'a>, E> {
let reflect_serialize = type_registry
.get_type_data::<ReflectSerialize>(reflect_value.type_id())
.ok_or_else(|| {
Error::custom(format_args!(
"Type '{}' did not register ReflectSerialize",
reflect_value.reflect_type_path()
))
})?;
Ok(reflect_serialize.get_serializable(reflect_value))
}

  1. ReflectSerializable doesn't support proxy types, so even if we did have the right TypeId, the operation would panic.

therefore, we need to fix 3 and come up with a solution to 4. i have some ideas i'll try to throw together this week.

github-merge-queue bot pushed a commit that referenced this issue Feb 26, 2024
# Objective

- Fixes #12001.
- Note this PR doesn't change any feature flags, however flaky the issue
revealed they are.

## Solution

- Use `FromReflect` to convert proxy types to concrete ones in
`ReflectSerialize::get_serializable`.
- Use `get_represented_type_info() -> type_id()` to get the correct type
id to interact with the registry in
`bevy_reflect::serde::ser::get_serializable`.

---

## Changelog

- Registering `ReflectSerialize` now imposes additional `FromReflect`
and `TypePath` bounds.

## Migration Guide

- If `ReflectSerialize` is registered on a type, but `TypePath` or
`FromReflect` implementations are omitted (perhaps by
`#[reflect(type_path = false)` or `#[reflect(from_reflect = false)]`),
the traits must now be implemented.

---------

Co-authored-by: Alice Cecile <[email protected]>
Co-authored-by: Gino Valente <[email protected]>
msvbg pushed a commit to msvbg/bevy that referenced this issue Feb 26, 2024
# Objective

- Fixes bevyengine#12001.
- Note this PR doesn't change any feature flags, however flaky the issue
revealed they are.

## Solution

- Use `FromReflect` to convert proxy types to concrete ones in
`ReflectSerialize::get_serializable`.
- Use `get_represented_type_info() -> type_id()` to get the correct type
id to interact with the registry in
`bevy_reflect::serde::ser::get_serializable`.

---

## Changelog

- Registering `ReflectSerialize` now imposes additional `FromReflect`
and `TypePath` bounds.

## Migration Guide

- If `ReflectSerialize` is registered on a type, but `TypePath` or
`FromReflect` implementations are omitted (perhaps by
`#[reflect(type_path = false)` or `#[reflect(from_reflect = false)]`),
the traits must now be implemented.

---------

Co-authored-by: Alice Cecile <[email protected]>
Co-authored-by: Gino Valente <[email protected]>
msvbg pushed a commit to msvbg/bevy that referenced this issue Feb 26, 2024
# Objective

- Fixes bevyengine#12001.
- Note this PR doesn't change any feature flags, however flaky the issue
revealed they are.

## Solution

- Use `FromReflect` to convert proxy types to concrete ones in
`ReflectSerialize::get_serializable`.
- Use `get_represented_type_info() -> type_id()` to get the correct type
id to interact with the registry in
`bevy_reflect::serde::ser::get_serializable`.

---

## Changelog

- Registering `ReflectSerialize` now imposes additional `FromReflect`
and `TypePath` bounds.

## Migration Guide

- If `ReflectSerialize` is registered on a type, but `TypePath` or
`FromReflect` implementations are omitted (perhaps by
`#[reflect(type_path = false)` or `#[reflect(from_reflect = false)]`),
the traits must now be implemented.

---------

Co-authored-by: Alice Cecile <[email protected]>
Co-authored-by: Gino Valente <[email protected]>
@coreh
Copy link
Contributor

coreh commented Feb 27, 2024

Sorry for missing this, and thanks @soqb for the investigation/solution!

mockersf pushed a commit that referenced this issue Feb 27, 2024
# Objective

- Fixes #12001.
- Note this PR doesn't change any feature flags, however flaky the issue
revealed they are.

## Solution

- Use `FromReflect` to convert proxy types to concrete ones in
`ReflectSerialize::get_serializable`.
- Use `get_represented_type_info() -> type_id()` to get the correct type
id to interact with the registry in
`bevy_reflect::serde::ser::get_serializable`.

---

## Changelog

- Registering `ReflectSerialize` now imposes additional `FromReflect`
and `TypePath` bounds.

## Migration Guide

- If `ReflectSerialize` is registered on a type, but `TypePath` or
`FromReflect` implementations are omitted (perhaps by
`#[reflect(type_path = false)` or `#[reflect(from_reflect = false)]`),
the traits must now be implemented.

---------

Co-authored-by: Alice Cecile <[email protected]>
Co-authored-by: Gino Valente <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Core Common functionality for all bevy apps A-Scenes Serialized ECS data stored on the disk C-Bug An unexpected or incorrect behavior
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants