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

First version of NetworkCodec #819

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

AlexProgrammerDE
Copy link
Contributor

No description provided.

@AlexProgrammerDE
Copy link
Contributor Author

The goal is to move all custom data types such as Knownpack to migrate to NetworkCodec in the future. Right now only KnownPack uses this system. Follow-up PRs will implement this system for other data types as well as move some simple data types from the codechelper classes to NetworkCodec.

@Camotoy
Copy link
Member

Camotoy commented Jun 1, 2024

This PR, if merged, will define how future classes are set to encode/decode. Pistonmaster, please do note down for the record on GitHub why you think MCPL should move to a Codec system. I will try to get more input on this.

@AlexProgrammerDE
Copy link
Contributor Author

The main reason for NetworkCodec is to debloat the MinecraftCodecHelper class. It has proven itself to not be a future-proof approach for serializing Minecraft data structures. Using a dedicated codec helper class has added way too many methods into a single class and it's hard to just find the serialization methods associated with a data structure. This NetworkCodec class follows Mojangs approach of moving from a centralized serialization class to the class itself to contain everything inside it that is needed to serialize it.
This has become obvious to me when the maintainers stopped using MinecraftCodecHelper and instead made a second class called ItemCoodecHelper that just takes care of item-related data structures. The reason MinecraftCodecHelper even existed instead of a superior approach like this is that "it could allow potentially multi-version serialization just like in cloudburstmc", I disagree that this is the way to go. Mainly MCPL is not anywhere near targeting multi-version support and the project should rather into the direction of supporting only the latest major version and allow being used together with a third-party translator like ViaVersion.
NetworkCodec can also be incrementally ported to thanks to the nature of how it works. People just call the networkcodec instead of the MinecraftCodecHelper. As seen in this PR only KnownPack uses it with more coming (I'll take care of it) once the team decides this as the new default way to write MCPL network code.

@basaigh
Copy link
Member

basaigh commented Jun 2, 2024

I can see the merit in moving serialisation within the data classes, but for that I'd prefer to stick to our current format with a read and a write function, largely just because I don't see any benefits of using a network codec to surround the same code that was going to be there in the first place.
I'm still open to codecs for json serialisation, but at the least, I'd like to stay away from network codecs until after that's implemented.

@AlexProgrammerDE
Copy link
Contributor Author

Well I've given benefits of this approach above. Additionally differences in the read/write implementation are far easier to see because they are right next to each other and there is no surrounding code that may distract readers. But if you'd like to see json codecs first and then come back to network codecs then fine I can wait, but I'll leave this PR open and come back once json codecs are in use.

@onebeastchris onebeastchris marked this pull request as draft September 8, 2024 03:34
@onebeastchris
Copy link
Member

Given that this is a concept, ill mark this as a draft

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants