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

Serialize Entity in DynamicScene #8517

Closed
wants to merge 1 commit into from

Conversation

Shatur
Copy link
Contributor

@Shatur Shatur commented Apr 30, 2023

Objective

Extracted part of #7335. I used this mainly to patch my game, but this may also simplify the merging process since this PR fixes only one major.

If DynamicScene scene is serialized with a component that refers to an entity whose generation != 0, then this will cause a panic when mapping entities:

if let Some(map_entities_reflect) = registration.data::<ReflectMapEntities>() {
map_entities_reflect
.map_specific_entities(world, entity_map, &entities)
.unwrap();
}

It's because entities are compared with generation taken into account and since entities from DynamicScene have only id, their generation will be 0 and it causes mismatch.

Solution

Serialize Entity in DynamicScene instead of just u32. And change Entity serialization to u64 to avoid making scene format longer. This way comparsion when mapping entities will be correct and the serialized ron will store plain numbers for dynamic entities as before. The serialized text become even shorter since entities inside components now also plain numbers.

I believe it the simplest change and it's was one of the suggestions from @cart.


Changelog

Changed

  • Serialize Entity as u64.

Fixed

  • Fix DynamicScene deserialization with a component that references entity with generation != 0.

Migration Guide

  • Store Entity instead of u32 in DynamicScene.

This PR also changes `Entity` serialization to `u64`, so `DynamicScene`
will contain just a plain number as before.
@Shatur
Copy link
Contributor Author

Shatur commented Apr 30, 2023

Looks like cargo deny failure is unrelated to the PR.

@MrGVSV
Copy link
Member

MrGVSV commented Apr 30, 2023

Is this PR necessary? #7335 does the same thing and is pretty much fully approved.

@Shatur
Copy link
Contributor Author

Shatur commented Apr 30, 2023

does the same thing and is pretty much fully approved.

But it still not merged. This PR have a smaller scope (fixes the mentioned bug). I assumed this could simplify the merge process (#7335 could be rebased on top of this). Let me know if I should close this one.

@MrGVSV
Copy link
Member

MrGVSV commented Apr 30, 2023

does the same thing and is pretty much fully approved.

But it still not merged. This PR have a smaller scope (fixes the mentioned bug). I assumed this could simplify the merge process (#7335 could be rebased on top of this). Let me know if I should close this one.

I’m not sure if there's a real reason it's not being merged. Probably just fell through the cracks (especially during the jam).

Maybe @alice-i-cecile can consider that one for Merge Train Monday tomorrow? 😁

(Or at least part of the review train haha)

@Shatur
Copy link
Contributor Author

Shatur commented Apr 30, 2023

Would be great!
As far as I know the author need to update the PR to the latest master and address some comments. This is why I extracted this changes: fixes only one bug, easy to review and up to date with the latest master.

@alice-i-cecile alice-i-cecile added A-Reflection Runtime information about types C-Bug An unexpected or incorrect behavior labels Apr 30, 2023
@Shatur
Copy link
Contributor Author

Shatur commented May 1, 2023

#7335 is going to be merged 🎉
Closing.

@Shatur Shatur closed this May 1, 2023
@Shatur Shatur deleted the dynamic-scene-entity branch May 1, 2023 15:50
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants