GeyserMC / MCProtocolLib

A library for communication with a Minecraft client/server.
MIT License
724 stars 200 forks source link

First version of NetworkCodec #819

Open AlexProgrammerDE opened 4 months ago

AlexProgrammerDE commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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.

AlexProgrammerDE commented 2 months ago

Here is an example codebase that makes full use if this concept: https://github.com/SkinsRestorer/SkinsRestorer/blob/dev/shared/src/main/java/net/skinsrestorer/shared/codec/NetworkCodec.java https://github.com/SkinsRestorer/SkinsRestorer/blob/dev/shared/src/main/java/net/skinsrestorer/shared/gui/SRInventory.java

onebeastchris commented 3 weeks ago

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