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

Scene serialization doesn't work without field types being registered #8415

Closed
InnocentusLime opened this issue Apr 17, 2023 · 5 comments · Fixed by #8430
Closed

Scene serialization doesn't work without field types being registered #8415

InnocentusLime opened this issue Apr 17, 2023 · 5 comments · Fixed by #8430
Labels
A-Reflection Runtime information about types A-Scenes Serialized ECS data stored on the disk C-Bug An unexpected or incorrect behavior D-Trivial Nice and easy! A great choice to get started with Bevy

Comments

@InnocentusLime
Copy link
Contributor

InnocentusLime commented Apr 17, 2023

Bevy version

Bevy 0.10.1 and c488b70 (latest commit on main so far)

What you did

Consider the following code. Here I am trying to serialize just one entity from the scene -- a text entity I spawned during startup.

For bevy 0.10.1

// For bevy 0.10.1
use bevy::prelude::*;

fn startup(mut commands: Commands, server: Res<AssetServer>) {
    commands.spawn(TextBundle {
        text: Text::from_section(
            "Hello, world!",
            TextStyle {
                font: server.load("my_font.ttf"),
                font_size: 12.0f32,
                color: Color::BLACK,
            },
        ),
        ..default()
    });
}

fn tick(
    text_q: Query<Entity, With<Text>>,
    type_registry: Res<AppTypeRegistry>,
    world: &World,
) {
    let e = text_q.single();
    let mut dyn_scene = DynamicScene::from_world(world, &type_registry);

    dyn_scene.entities.retain(|ent| ent.entity == e.index());
    assert!(dyn_scene.entities.len() == 1); // We do have some entities left

    dyn_scene.serialize_ron(&type_registry).unwrap(); // Try to save
    info!("yes");
}

fn main() {
    App::new()
        .add_plugins(DefaultPlugins)
        .add_startup_system(startup)
        .add_system(tick)
        .run();
}

For bevy 0.11.0-dev (c488b70)

// For bevy 0.11-dev (c488b7089c58c99371af25b7e27551e892df3dcf)
use bevy::prelude::*;

fn startup(mut commands: Commands, server: Res<AssetServer>) {
    commands.spawn(TextBundle {
        text: Text::from_section(
            "Hello, world!",
            TextStyle {
                font: server.load("my_font.ttf"),
                font_size: 12.0f32,
                color: Color::BLACK,
            },
        ),
        ..default()
    });
}

fn tick(
    text_q: Query<Entity, With<Text>>,
    type_registry: Res<AppTypeRegistry>,
    world: &World,
) {
    let e = text_q.single();
    let mut dyn_scene = DynamicScene::from_world(world, &type_registry);

    dyn_scene.resources.clear();
    dyn_scene.entities.retain(|ent| ent.entity == e.index());
    assert!(dyn_scene.entities.len() == 1); // We do have some entities left

    dyn_scene.serialize_ron(&type_registry).unwrap(); // Try to save
    info!("yes");
}

fn main() {
    App::new()
        .add_plugins(DefaultPlugins)
        .add_systems(Startup, startup)
        .add_systems(Update, tick)
        .run();
}

What were you expecting

The app should have been working and spamming "yes" without any problems, since bevy_text::BreakLineOn implements all the needed traits. The struct containing it (bevy_text::Text) is registered

What actually happened

Both respective versions crash with a message saying that bevy_text::BreakLineOn isn't a registered type.

thread 'Compute Task Pool (4)' panicked at 'called `Result::unwrap()` on an `Err` value: Message("no registration found for dynamic type with name bevy_text::text::BreakLineOn")', src\main.rs:29:45

Additional information

My guess is that the serialization implementation can't somehow poke the the serialize implementations of the fields without looking at the type registry.

At first I thought that this is a design choice. But it seems that it's not, since bevy_text is an official crate and BreakLineOn isn't explicitly registered. Vec3 seemingly isn't explicitly registered either. Yet, when I do the same test with a lone TransformBundle, everything just works.

// For bevy 0.10.1
// Spams "yes" without any problem
use bevy::prelude::*;

fn startup(mut commands: Commands) {
    commands.spawn(TransformBundle::default());
}

fn tick(
    text_q: Query<Entity, With<Transform>>,
    type_registry: Res<AppTypeRegistry>,
    world: &World,
) {
    let e = text_q.single();
    let mut dyn_scene = DynamicScene::from_world(world, &type_registry);

    dyn_scene.entities.retain(|ent| ent.entity == e.index());
    assert!(dyn_scene.entities.len() == 1); // We do have some entities left

    dyn_scene.serialize_ron(&type_registry).unwrap(); // Try to save
    info!("Yes");
}

fn main() {
    App::new()
        .add_plugins(DefaultPlugins)
        .add_startup_system(startup)
        .add_system(tick)
        .run();
}

Force-registering bevy_text::BreakLineOn fixes the issue, but that is going to blow the amount of needed to plug things into bevy's reflection.

@InnocentusLime InnocentusLime added C-Bug An unexpected or incorrect behavior S-Needs-Triage This issue needs to be labelled labels Apr 17, 2023
@MrGVSV MrGVSV added A-Scenes Serialized ECS data stored on the disk and removed C-Bug An unexpected or incorrect behavior S-Needs-Triage This issue needs to be labelled labels Apr 17, 2023
@MrGVSV
Copy link
Member

MrGVSV commented Apr 17, 2023

Original message

This is actually expected behavior.

The scene (de)serializer can only handle components and resources that register ReflectComponent and ReflectResource, respectively. This means that if the type is not registered, it will not be able to access those type data to perform the required operations.

The fix here is to just register all types that need to be (de)serialized. It's a bit annoying, but it's the best solution we have given Rust's current tooling.

So in this particular case, the solution would be to register BreakLineOn.


And for reference, Vec3 is registered here:

.register_type::<bevy_math::Vec3>()

But it doesn't need to be for Transform to be (de)serializable in scenes. Again, the important thing is that Transform is registered and also registers ReflectComponent— which it does using the #[reflect(Component)] attribute.


Edit: Seems I'm wrong here. I mistakenly thought BreakLineOn was a Component (and also forgot how reflection's (de)serialization works for a moment haha).

Still, the solution here is to just register the type in bevy_text.

@MrGVSV MrGVSV added C-Bug An unexpected or incorrect behavior A-Reflection Runtime information about types D-Trivial Nice and easy! A great choice to get started with Bevy labels Apr 17, 2023
@InnocentusLime
Copy link
Contributor Author

InnocentusLime commented Apr 17, 2023

Original message

Edit: Seems I'm wrong here. I mistakenly thought BreakLineOn was a Component (and also forgot how reflection's (de)serialization works for a moment haha).

Still, the solution here is to just register the type in bevy_text.

So it is expected from the user to register all the types their crate uses including the "nested" types? I am not an experienced library designer and I don't know the reasoning behind this decision, but tracking down if all types are registered can get difficult quick ☹️

I would like to do the fix-PR if that's ok and will check if any other type in bevy_text has not been registered.

@MrGVSV
Copy link
Member

MrGVSV commented Apr 17, 2023

So it is expected from the user to register all the types their crate uses including the "nested" types? I am not an experienced library designer and I don't know the reasoning behind this decision, but tracking down if all types are registered can get difficult quick ☹️

Yeah that's currently the requirement, and is likely to stay that way until a better solution is found. The main issue is that other crates that "solve" this use life-before-main and linker shenanigans to get it to work, which is not something we want to support right now. Mainly due to the fact that we don't build our reflection logic on top of a "hack" and we want the engine to be cross-platform (these other solutions break on platforms like WASM).

As far as nested types go, there is #5781 which adds the ability to recursively register a type's fields (review very welcome on that PR btw 😉).

I would like to do the fix-PR if that's ok and will check if any other type in bevy_text has not been registered.

Sounds great, thank you!

@InnocentusLime
Copy link
Contributor Author

As far as nested types go, there is #5781 which adds the ability to recursively register a type's fields (review very welcome on that PR btw 😉).

I am just an inexperienced user, but I can provide some feedback if it's welcome!

@MrGVSV
Copy link
Member

MrGVSV commented Apr 17, 2023

I am just an inexperienced user, but I can provide some feedback if it's welcome!

"Experienced" or not, review is generally pretty welcome in this repo. Even if just asking a clarifying question or making a simple suggestion. It's very appreciated! 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Reflection Runtime information about types A-Scenes Serialized ECS data stored on the disk C-Bug An unexpected or incorrect behavior D-Trivial Nice and easy! A great choice to get started with Bevy
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants