Skip to content

Some rearranging of serialize/deserialize code #230

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

Merged
merged 5 commits into from
Feb 8, 2021

Conversation

Rua
Copy link
Contributor

@Rua Rua commented Jan 8, 2021

The following changes are made, some of which are breaking.

  • Canon is moved out of Registry so that it's now a free-standing type. That way there's a clear separation of concerns, between handling Entity and handling component types.
  • The with_entity_serializer methods on WorldSerializer and WorldDeserializer are consequently removed, while World::as_serializable and Registry::as_deserialize(_into_world) take an extra EntitySerializer argument.
  • Canon now handles its locking internally, instead of having to be wrapped in a lock.
  • CustomEntitySerializer's methods take &self, possible due to the change above.
  • EntitySerializer now has a blanket implementation for anything that implements CustomEntitySerializer.
  • The custom run_as_context logic is replaced with the scoped-tls-hkt crate, which does this more cleanly and is better-tested.
  • The ENTITY_SERIALIZER variable, introduced by the change above, is made public and documented. This fixes Allow serializing/deserializing Entity outside World #228.

I think that the EntitySerializer and CustomEntitySerializer traits could do with better names, but I've held off on that as I can't think of names that really work well.

@FluffyCreature
Copy link
Contributor

I think it would probably be better to hide the scoped-tls-hkt interface so there can be less dependency. Maybe consider to wrap ENTITY_SERIALIZER variable inside a function? (similar to the original run_as_context)

@TomGillen
Copy link
Collaborator

These changes look good to me, although I agree with @FluffyCreature that it would be better to hide the dependency from users (even though we don't follow this rule of thumb very consistently elsewhere).

@Rua
Copy link
Contributor Author

Rua commented Feb 8, 2021

I've made ENTITY_SERIALIZER private, and added a set_entity_serializer function as a simple wrapper around it.

@TomGillen TomGillen merged commit 27151ca into amethyst:master Feb 8, 2021
@TomGillen TomGillen added type: breaking API breaking change, will require users to change their code type: improvement An improvement or change to an existing feature labels Feb 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: breaking API breaking change, will require users to change their code type: improvement An improvement or change to an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow serializing/deserializing Entity outside World
3 participants