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

Overhauled serialization system #3535

Merged
merged 141 commits into from
Aug 10, 2019
Merged

Overhauled serialization system #3535

merged 141 commits into from
Aug 10, 2019

Conversation

eviltak
Copy link
Member

@eviltak eviltak commented Oct 29, 2018

Contents

  • Clean serialization API for use in the engine and in modules
  • Add serialization support for various types previously unsupported
  • Modularize serialization to make it easier to add custom type serialization
  • Rework serialization to serialize objects via Gson or Protobuf by changing a single line of code (see AbstractSerializer, GsonSerializer and ProtobufSerializer)

Closes #3490.

Review guide

This PR is finally ready to review and merge! Since there are a lot of changes to review, I'd recommend taking a look at the user-facing API introductions/changes first:

Since there are a lot of files changed by this PR (still no conflicts, surprisingly!), I'd suggest you initially skip the ones with few (< 20) changed lines and a similar number of additions and deletions -- almost all of these files were changed by the IDE during a refactor and have no real reviewable changes.

After you finish reviewing the API changes in the files listed above, you can take a look at the logic introduced by the code in this PR: almost all of it resides in the persistence.typeHandling package under a TypeHandler or a TypeHandlerFactory, or the ReflectionUtil class. However, I am pretty confident that all these classes/methods have been tested thoroughly to ensure they work as intended, so this step can be skipped if needed.

I'd then recommend looking at the tests to see how the new serialization system would be used; the main ones to look at here are TypeSerializerTest and TypeHandlerLibraryTest, since these test most of the user-facing APIs.

Testing

Check out branch MovingBlocks/newSerialization and run ./gradlew tests

TODO

  • Prevent modules for adding handlers for types they do not declare (compare module declaring TypeHandler<T> and module declaring T).
  • Add TypeHandlerLibrary.getRuntimeTypeHandler that returns the retrieved type handler wrapped in a RuntimeDelegatingTypeHandler to support subtype serialization
  • Create a reflection abstraction over ModuleEnvironment/Reflections with more AbstractClassLibrary-esque resolution features (if made with a ModuleEnvironment).
  • Flatten the RuntimeDelegatingTypeHandler serialization format by adding the class entry to the serialized object itself, if the serialized representation is a map

Module Fixes

@eviltak eviltak added Topic: Architecture Requests, Issues and Changes related to software architecture, programming patterns, etc. Api Size: L Very big effort likely requiring a lot of research and work in many areas across the codebase labels Oct 29, 2018
@eviltak eviltak self-assigned this Oct 29, 2018
@GooeyHub
Copy link
Member

Hooray Jenkins reported success with all tests good!

4 similar comments
@GooeyHub
Copy link
Member

Hooray Jenkins reported success with all tests good!

@GooeyHub
Copy link
Member

Hooray Jenkins reported success with all tests good!

@GooeyHub
Copy link
Member

Hooray Jenkins reported success with all tests good!

@GooeyHub
Copy link
Member

Hooray Jenkins reported success with all tests good!

@GooeyHub
Copy link
Member

Uh oh, something went wrong with the build. Need to check on that

@GooeyHub
Copy link
Member

Hooray Jenkins reported success with all tests good!

@GooeyHub
Copy link
Member

GooeyHub commented Nov 1, 2018

Hooray Jenkins reported success with all tests good!

3 similar comments
@GooeyHub
Copy link
Member

GooeyHub commented Nov 5, 2018

Hooray Jenkins reported success with all tests good!

@GooeyHub
Copy link
Member

GooeyHub commented Nov 5, 2018

Hooray Jenkins reported success with all tests good!

@GooeyHub
Copy link
Member

GooeyHub commented Nov 6, 2018

Hooray Jenkins reported success with all tests good!

@eviltak eviltak requested review from Cervator and immortius November 6, 2018 06:05
@GooeyHub
Copy link
Member

Hooray Jenkins reported success with all tests good!

3 similar comments
@GooeyHub
Copy link
Member

Hooray Jenkins reported success with all tests good!

@GooeyHub
Copy link
Member

GooeyHub commented Dec 2, 2018

Hooray Jenkins reported success with all tests good!

@GooeyHub
Copy link
Member

Hooray Jenkins reported success with all tests good!

@syntaxi
Copy link

syntaxi commented Dec 16, 2018

@eviltak, is this ready to be reviewed and then subsequently merged? Or is there still outstanding work blocking this?
Would love to get this in as part of the merge-ocalypse

@eviltak
Copy link
Member Author

eviltak commented Dec 19, 2018

@jellysnake unfortunately there is still some work to be done, potentially involving related changes in gestalt. I would say that this should be merged only after all modules have PRs fixing code that has been broken by these changes. I shall update the PR description with clear todo items so that progress can be easily gauged!

@GooeyHub
Copy link
Member

Hooray Jenkins reported success with all tests good!

@Cervator
Copy link
Member

This pull request introduces 2 alerts when merging 0ae28ce into b6590b7 - view on LGTM.com

new alerts:

  • 1 for Potential output resource leak
  • 1 for Dereferenced variable may be null

Comment posted by LGTM.com

@GooeyHub
Copy link
Member

Hooray Jenkins reported success with all tests good!

@GooeyHub
Copy link
Member

Uh oh, something went wrong with the build. Need to check on that

1 similar comment
@GooeyHub
Copy link
Member

Uh oh, something went wrong with the build. Need to check on that

@GooeyHub
Copy link
Member

Uh oh, something went wrong with the build. Need to check on that

@GooeyHub
Copy link
Member

Uh oh, something went wrong with the build. Need to check on that

@GooeyHub
Copy link
Member

Uh oh, something went wrong with the build. Need to check on that

@GooeyHub
Copy link
Member

Hooray Jenkins reported success with all tests good!

1 similar comment
@GooeyHub
Copy link
Member

Hooray Jenkins reported success with all tests good!

@GooeyHub
Copy link
Member

Hooray Jenkins reported success with all tests good!

@Cervator
Copy link
Member

Testing & merge report

Tried out the following modules to some degree, including their dependencies & fix PRs if ready:

  • Tasks & QuestExamples - found a minor bug in the tasks type handler, actually managed to fix it woo
  • MedievalCities - mostly to exercise a smaller set of modules at first, fixed the Perlin->Simplex thing
  • LightAndShadow (GSOC) - still had a soft dependency on Tasks, restored it for now
  • MetalRenegades (GSOC) - no module fix PR yet, managed to put one together with a bunch of tweaks
  • MasterOfOreon (TSOC)
  • WildAnimalsMadness (GSOC)
  • Apiculture (GSOC)

I think that's the critical stuff for now. JoshariasSurvival and others need some attention in the near future as well, but there's no GSOC or TSOC impact on there, so less time sensitive. Phiew. This might be happening!

/itshappeninggif

@Cervator Cervator merged commit 419b85a into develop Aug 10, 2019
Cervator added a commit that referenced this pull request Aug 10, 2019
Cervator added a commit to Terasology/MetalRenegades that referenced this pull request Aug 10, 2019
- MovingBlocks/Terasology#3535 - new serialization
- Terasology/Tasks#12 - goes with the new serialization
- MovingBlocks/Terasology#3716 - updated world gen
@Cervator Cervator added this to the v2.3.0 milestone Aug 10, 2019
@Cervator Cervator deleted the newSerialization branch October 15, 2019 04:25
@skaldarnar skaldarnar removed the Api label May 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Size: L Very big effort likely requiring a lot of research and work in many areas across the codebase Topic: Architecture Requests, Issues and Changes related to software architecture, programming patterns, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Recover and finish parked serialization / type handling overhaul
7 participants