Scenes Example isn't Recognizing Vec3 Type #5745
Comments
Carter0
added
C-Bug
|
This is because the scene file is using the incorrect type name. It should be |
MrGVSV
added
D-Trivial
|
This isn't something that should be caught by CI? |
|
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. |
|
To my knowledge, 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 So yeah, if we can find a "better" path, we should take it. But at the time, @PROMETHIA-27 had an idea on discord that might work though! |
|
Potential solution for a PR:
Upsides:
Downsides:
|
|
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) |
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 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 {
} |
|
We could also use that to "force" good / short / stable type names for core types (ex: |
|
Although thats also what the |
…m the logs (bevyengine#5751) # Objective - Fixes bevyengine#5745. ## Solution - Changes the Vec3 and Quat types.
…m the logs (bevyengine#5751) # Objective - Fixes bevyengine#5745. ## Solution - Changes the Vec3 and Quat types.
…m the logs (bevyengine#5751) # Objective - Fixes bevyengine#5745. ## Solution - Changes the Vec3 and Quat types.
# Objective
When deriving `Reflect`, users will notice that their generic arguments
also need to implement `Reflect`:
```rust
#[derive(Reflect)]
struct Foo<T: Reflect> {
value: T
}
```
This works well for now. However, as we want to do more with `Reflect`,
these bounds might need to change. For example, to get #4154 working, we
likely need to enforce the `GetTypeRegistration` trait. So now we have:
```rust
#[derive(Reflect)]
struct Foo<T: Reflect + GetTypeRegistration> {
value: T
}
```
Not great, but not horrible. However, we might then want to do something
as suggested in
[this](#5745 (comment))
comment and add a `ReflectTypeName` trait for stable type name support.
Well now we have:
```rust
#[derive(Reflect)]
struct Foo<T: Reflect + GetTypeRegistration + ReflectTypeName> {
value: T
}
```
Now imagine that for even two or three generic types. Yikes!
As the API changes it would be nice if users didn't need to manually
migrate their generic type bounds like this.
A lot of these traits are (or will/might be) core to the entire
reflection API. And although `Reflect` can't add them as supertraits for
object-safety reasons, they are still indirectly required for things to
function properly (manual implementors will know how easy it is to
forget to implement `GetTypeRegistration`). And they should all be
automatically implemented for user types anyways as long they're using
`#[derive(Reflect)]`.
## Solution
Add a "catch-all" trait called `Reflectable` whose supertraits are a
select handful of core reflection traits.
This allows us to consolidate all the examples above into this:
```rust
#[derive(Reflect)]
struct Foo<T: Reflectable> {
value: T
}
```
And as we experiment with the API, users can rest easy knowing they
don't need to migrate dozens upon dozens of types. It should all be
automatic!
## Discussion
1. Thoughts on the name `Reflectable`? Is it too easily confused with
`Reflect`? Or does it at least accurately describe that this contains
the core traits? If not, maybe `BaseReflect`?
---
## Changelog
- Added the `Reflectable` trait
---------
Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
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::Vec3If I delete the transform component from the ron file then things seem to work as usual and I get
The text was updated successfully, but these errors were encountered: