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

Upgrade packet serializer #1798

Merged
merged 15 commits into from
Oct 30, 2022
Merged

Conversation

SpaceMonkeyy86
Copy link
Contributor

@SpaceMonkeyy86 SpaceMonkeyy86 commented May 10, 2022

Replaces the current serializer, BinaryFormatter, with a modified version of BinaryPack:
https://github.com/SubnauticaNitrox/BinaryPack

This serializer is faster, uses less memory, is more secure, and produces a smaller output.

Resolves a part of #1473.

Copy link
Collaborator

@Measurity Measurity left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code wise is fine, but we should move the changes to binary pack to a repo on this org. I've given you write perms to the https://github.com/SubnauticaNitrox/BinaryPack so you can push to this remote directly.

Edit: code is moved to repo

@Jannify
Copy link
Member

Jannify commented May 21, 2022

Ok, the pr already seems pretty solid.
Things I thought about we need to look into:

  • Reviewing/Merging the parameter constructor pr (Use constructor with parameter during serialization if possible BinaryPack#1)
  • Using parameter constructor where it makes sense (important to double check parameter spelling)
  • Updating README of the BinaryPack repo so it doesn't look like a 1:1 copy
  • Uploading our fork to NuGet (will be done by Meas but acc will take a little to be verified) and replacing the dll file with a reference

@SpaceMonkeyy86
Copy link
Contributor Author

SpaceMonkeyy86 commented Aug 26, 2022

Parameter constructors have been implemented, all that's left to do now is in-game testing, fixing the unit tests, and uploading the library to NuGet.

@SpaceMonkeyy86
Copy link
Contributor Author

It's done 🎉

@Jannify
Copy link
Member

Jannify commented Oct 5, 2022

In NitroxModel.DataStructures.GameLogic.VehicleModel public NitroxTechType TechType { get; } needs to also have the set or json serialization fails.

@Jannify
Copy link
Member

Jannify commented Oct 5, 2022

Doing a quick in-game test it seems like something partially breaks the game/sync. Every time I joined a world both players joined in different (fully repaired) escapepods which weren't visible to the other. While they could see each other flora wasn't synced either. Do you have the same result or something different?

@SpaceMonkeyy86
Copy link
Contributor Author

Doing a quick in-game test it seems like something partially breaks the game/sync. Every time I joined a world both players joined in different (fully repaired) escapepods which weren't visible to the other. While they could see each other flora wasn't synced either. Do you have the same result or something different?

Yes, I got that same issue. I briefly looked into it but couldn't find anything wrong with the class definitions so I concluded that some other part of the codebase was at fault. If it helps there is a NullReferenceException thrown upon joining that might help pinpoint the issue.

@dartasen dartasen added the Area: persistence Related to serialization for long term storage (i.e. files and databases) label Oct 15, 2022
@Jannify Jannify dismissed their stale review October 23, 2022 14:44

Can review as I participate in this pr

@dartasen
Copy link
Member

Tested IG with Jannify
No outstanding errors yet, seems to improve user experience quite a bit.

Might need to do a bigger test sessions with 5+ players doing a casual gameplay

@sarayuuki
Copy link

Really glad development is coming along nicely. I got to test all the new changes

@Jannify Jannify requested review from dartasen and Measurity October 30, 2022 19:26
@Jannify Jannify merged commit 6fd40cf into SubnauticaNitrox:master Oct 30, 2022
@SpaceMonkeyy86 SpaceMonkeyy86 deleted the binary-pack branch October 30, 2022 23:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: persistence Related to serialization for long term storage (i.e. files and databases)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants