Scenes Example isn't Recognizing Vec3 Type

[Carter0]

Bevy version

I just pulled down main yesterday.

What you did

I ran the bevy scene example with cargo run --example scene.

What went wrong

I get this error and it prevents any of the data in the ron file from being uploaded.

WARN bevy_asset::asset_server: encountered an error while loading an asset: No registration found for glam::vec3::Vec3

If I delete the transform component from the ron file then things seem to work as usual and I get

2022-08-19T15:50:03.689570Z  INFO scene:   Entity(3)
2022-08-19T15:50:03.689615Z  INFO scene:     ComponentA: { x: 3 y: 2 }

2022-08-19T15:50:03.689641Z  INFO scene:   Entity(4)
2022-08-19T15:50:03.689652Z  INFO scene:     ComponentA: { x: 3 y: 4 }

[MrGVSV]

This is because the scene file is using the incorrect type name. It should be glam::f32::vec3::Vec3 not glam::vec3::Vec3. Same goes for glam::quat::Quat which should be glam::f32::scalar::quat::Quat.

[afonsolage]

This isn't something that should be caught by CI?

[DJMcNab]

Do we document that scenes shouldn't be used across compiler versions - I can't find this documented anywhere?

If not, we shouldn't be using type_name anyway.

Frankly, this is the first time I've looked at our reflect code, and I'm slightly horrified by our gung-ho usage of type_name.

[cart]

To my knowledge, type_name is our best and only option when it comes to identifying a type across compiler versions.

We could mitigate this by adding CI to detect type_name changes early and couple that with either an alias database or migration tools. Not a full solution, but probably would work pretty well in practice, given how rare rustc-driven type_name changes are. This is literally the first time this has come up, and it may have been because glam actually changed their module path.

The alternatives (manually defining TypeUuid on every reflected type, parsing rust files in a pre-build step and generating our own ids, etc) are not reasonable in the context of "reflect everything". Thats a level of complexity I'm not willing to foist on users.

@DJMcNab called out that module_path exists, which might work for our scenarios. When I wrote Bevy Reflect, I wasn't aware of that api. However it doesn't handle the name of the type itself (which would need to either be extracted from type_name or within the Reflect proc macro). Notably, the macro approach won't handle generic type names properly. Theres also the question of the static lifetime, which could be lost if we need runtime string ops.

So yeah, if we can find a "better" path, we should take it. But at the time, type_name was the only real viable option from my perspective. And from my perspective, its still our best option.

@PROMETHIA-27 had an idea on discord that might work though!

[PROMETHIA-27]

Potential solution for a PR:

• use the module_path macro, which provides a stable module path
• create a new trait, ReflectTypeName, which has a function type_name() to return the module-path-qualified type name, with generic parameters
• this trait gets derived alongside Reflect, so no extra derives needed.
• For simple types, it just returns module_path + the ident of the type name
• For generic types, it returns module_path + ident of the type name + <ReflectTypeName of each parameter>
  • To do this, it also needs to put a constraint on each parameter to implement ReflectTypeName

Upsides:

• Perfectly stable
• Allows us to completely forgo type_name()

Downsides:

• No lifetimes are represented in the type names (meh)
• Reflect is now even more infectious. Any type parameters to a reflected type must be reflect as well in order for that type to have a reflectable type name. Maybe introduce a #[only_reflect_name] helper macro in the case that something needs a ReflectTypeName impl but can't be reflected fully?

[cart]

Actually, even if we do find a better path forward, if we continue using type names / module paths as identifiers, we should do a dump of the default Bevy type registry for the last stable version, compare that to a dump of the type registry for main, and have some sort of policy around defining aliases / migrations.

(validated in CI)

[cart]

ReflectTypeName

I think I like this. It also has the benefit (or downside, depending on your perspective), of enabling library authors to define their own type_names. Ex: maybe they don't want to deal with migrations when the module changes, so they make it return my_lib::MyType instead of my_lib::some::path::MyType. It would open to doors to unstructured names like BlAH@blah blah, but we could add validation if we really want to.

If this is a pattern we want to encourage, we could add a convenience to the Reflect derive like:

#[derive(Reflect)]
#[reflect_type_name("my_crate::Foo")]
struct Foo {
}

[cart]

We could also use that to "force" good / short / stable type names for core types (ex: Vec3 instead of glam::f32::vec3::Vec3)

[cart]

Although thats also what the short_name system was designed for. We might still want to encourage module-like paths for ReflectTypeName, then use short_name where appropriate.