Staticify all serialization helpers #878
Open
+2,903
−3,098
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
First of all, why am I changing the whole API so that MinecraftCodecHelper is now static instead of an instance? The reason is that these methods are better used statically. Right now the biggest issue with MCPL I see is its reliance on "serialization instances" that are being passed around everywhere. Why were those even made in the first place? Because there was a belief a few years ago that MCPL could somehow ever support multiple versions of Minecraft. The idea is good however it is unrealistic. Packets change heavily between versions and rarely do serialization types. Serialization instances were made with the idea somehow you could implement multiple codechelpers for each version and "downgrade" packets and bind them together using UnsupportedOperationException. In my opinion this is not a route MCPL should be taking. Instead my plan is to add a module that allows ViaVersion to be used with MCPL to allow the desired multi-version support. Without having to pass serialization instances around, the API becomes a lot simpler. Especially with the removal of the parent class of ItemCodecHelper. (Which previously extended MinecraftCodecHelper for some reason) Now it is clear what you're calling and there are way more lambda/inlining opportunities around the codebase. This makes the API more similar to the approach Velocity and ViaVersion are also taking with their static serialization methods. Classes renamed: - MinecraftCodecHelper -> MinecraftTypes - ItemCodecHelper -> ItemTypes Classes removed: - PacketCodecHelper interface (not needed anymore) - BasePacketCodec (merged into MinecraftTypes) Why was BasePacketCodec not renamed to BaseTypes? Because this library is slowly going to more and more specialize on only minecraft, not needing to retain a special implementation for developers who use it just for PacketLib.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
First of all, why am I changing the whole API so that MinecraftCodecHelper is now static instead of an instance? The reason is that these methods are better used statically. Right now the biggest issue with MCPL I see is its reliance on "serialization instances" that are being passed around everywhere. Why were those even made in the first place? Because there was a belief a few years ago that MCPL could somehow ever support multiple versions of Minecraft. The idea is good however it is unrealistic. Packets change heavily between versions and rarely do serialization types. Serialization instances were made with the idea somehow you could implement multiple codechelpers for each version and "downgrade" packets and bind them together using UnsupportedOperationException. In my opinion this is not a route MCPL should be taking. Instead my plan is to add a module that allows ViaVersion to be used with MCPL to allow the desired multi-version support. Without having to pass serialization instances around, the API becomes a lot simpler. Especially with the removal of the parent class of ItemCodecHelper. (Which previously extended MinecraftCodecHelper for some reason) Now it is clear what you're calling and there are way more lambda/inlining opportunities around the codebase. This makes the API more similar to the approach Velocity and ViaVersion are also taking with their static serialization methods.
Classes renamed:
Classes removed:
Why was BasePacketCodec not renamed to BaseTypes? Because this library is slowly going to more and more specialize on only minecraft, not needing to retain a special implementation for developers who use it just for PacketLib.