-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Add backwards compatibility to former (incorrect) Name
reflect serialization
#11485
Conversation
Name
reflect serializationName
reflect serialization
fc533b3
to
8038207
Compare
/// We've since fixed this on [#11447](https://github.com/bevyengine/bevy/pull/11447), but in order | ||
/// to maintain backwards compatibility with data that might have been serialized with the wrong format, | ||
/// we should keep support for deserializing it for a bit. (A couple of releases should be enough.) | ||
fn visit_map<A>(self, mut map: A) -> Result<Self::Value, A::Error> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we mark trait methods as deprecated?
If not, can we call a function that's marked deprecated so we can CTRL+F to find it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No 😕 I tried it but the compiler complained about that.
Let me try this calling other function hack.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, actually calling it was causing us to emit a warning on every build, which isn't nice. I just left it around (starting with a _
to prevent dead code warnings) for searchability.
208f0a0
to
4158533
Compare
deserializer.deserialize_str(EntityVisitor) | ||
deserializer.deserialize_any(EntityVisitor) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could be wrong but I believe this is also a breaking change. IIRC non-self-describing formats do not support deserialize_any
.
Could you verify that this doesn't break non-self-describing formats?
After discussing with @MrGVSV, this might be overkill, and breaks compatibility with non-self-describing formats. (such as Postcard) I'm going to close this PR and just add a migration guide to the other PR. If there's demand for it, I can re-open it under a feature flag (e.g. |
Objective
@MrGVSV pointed out that #11447 inadvertently caused a breaking change, since existing scenes might have been serialized with the broken
{ "name": _, "hash": _ }
format for theName
component. This PR aims to avoids that breakage.Solution
This PR introduces support for deserializing
Name
either from a string or from a{ "name": _, "hash": _ }
map. Once enough time has passed and people have re-exported/fixed their files, we can remove this. Added a warning to make sure people are aware of the change.Changelog
Name
component. This will be removed in a future release, so make sure to update any files that might be using the old format.