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

Fix reflected serialization/deserialization on Name component #11447

Merged
merged 2 commits into from
Jan 21, 2024

Conversation

coreh
Copy link
Contributor

@coreh coreh commented Jan 21, 2024

Objective

  • This PR makes it so that ReflectSerialize and ReflectDeserialize traits are properly derived on Name. This avoids having the internal hash “leak” into the serialization when using reflection.

Solution

  • Added a conditional derive for ReflectDeserialize and ReflectSerialize via #[cfg_attr()]

Changelog

  • Name now correctly implements ReflectDeserialize and ReflectSerialize whenever the serialize feature is enabled.

Migration Guide

  • Existing scene files containing the former (incorrect) serialization of the Name component will require patching. Search for bevy_core::name::Name, and replace the following structure ( "name": "My Entity Name", "hash": ... ) with just a string: "My Entity Name".

@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior A-Reflection Runtime information about types labels Jan 21, 2024
@MrGVSV
Copy link
Member

MrGVSV commented Jan 21, 2024

Since this is technically a breaking change (i.e. existing scenes could now be invalid and/or expectations of what's included in the serialized output), could you add a migration guide?

@MrGVSV MrGVSV added the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Jan 21, 2024
Copy link
Contributor

It looks like your PR is a breaking change, but you didn't provide a migration guide.

Could you add some context on what users should update when this change get released in a new version of Bevy?
It will be used to help writing the migration guide for the version. Putting it after a ## Migration Guide will help it get automatically picked up by our tooling.

@matiqo15 matiqo15 added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Jan 21, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jan 21, 2024
Merged via the queue into bevyengine:main with commit 18833fa Jan 21, 2024
27 checks passed
@coreh
Copy link
Contributor Author

coreh commented Jan 21, 2024

Since this is technically a breaking change (i.e. existing scenes could now be invalid and/or expectations of what's included in the serialized output), could you add a migration guide?

Just saw this now, but the PR is already merged. 😕 Are we using the reflection (de)serialization for scenes? In this case I can add an entry to the migration guide, will the release script still pick it up even after the PR is merged? Will we need to include it manually later when writing the notes?

@MrGVSV
Copy link
Member

MrGVSV commented Jan 21, 2024

Are we using the reflection (de)serialization for scenes?

Yep! It's what powers the scene system essentially.

In this case I can add an entry to the migration guide, will the release script still pick it up even after the PR is merged? Will we need to include it manually later when writing the notes?

I believe the script is only run towards the end of the cycle. So it should pick it up if you add it now.

@coreh
Copy link
Contributor Author

coreh commented Jan 23, 2024

@MrGVSV Ended up adding backwards compatibility to the old format: #11485

This should give people time to more easily migrate.

Edit: That turned out to be problematic due to non-self-describing serialization format compatibility. Just adding a migration guide instead.

@Testare
Copy link
Contributor

Testare commented Feb 19, 2024

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 C-Bug An unexpected or incorrect behavior M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants