Skip to content

Conversation

dktapps
Copy link
Member

@dktapps dktapps commented Sep 10, 2025

Main changes:

  • PacketSerializer is removed, all encode/decode methods previously in there are now static methods of CommonTypes
  • DataPacket->decode() now accepts pmmp\encoding\ByteBufferReader
  • DataPacket->encode() now accepts pmmp\encoding\ByteBufferWriter
  • encode/decode functions all updated to use ext-encoding APIs

Questions/concerns/doubts:

  • Not sure I like CommonTypes being static.
    • CommonTypes::writeString($out, $string) is more boilerplaty than $out->writeString($string)
    • While statics have benefit for the low-level APIs (since they prevent endian mistakes), CommonTypes doesn't need to be static.
    • It is consistent with the low-level API, I suppose...
    • Prevents overriding methods of PacketSerializer to customise packet decoding? but we were mostly gravitating towards final type classes having their own static ::read() and ->write() methods anyway...
  • CommonTypes::writeOptional() looks nasty for objects and could be made prettier
  • Generally, not convinced I made the right design choices for ext-encoding with the static API.

@dktapps dktapps requested a review from a team as a code owner September 10, 2025 13:14
@dktapps dktapps requested review from Copilot and removed request for a team September 10, 2025 13:14
pmmp-admin-bot[bot]
pmmp-admin-bot bot previously approved these changes Sep 10, 2025
Copilot

This comment was marked as outdated.

pmmp-admin-bot[bot]
pmmp-admin-bot bot previously approved these changes Sep 10, 2025
@dktapps dktapps requested a review from Copilot September 10, 2025 14:02
Copilot

This comment was marked as outdated.

pmmp-admin-bot[bot]
pmmp-admin-bot bot previously approved these changes Sep 10, 2025
pmmp-admin-bot[bot]
pmmp-admin-bot bot previously approved these changes Sep 10, 2025
@dktapps dktapps requested a review from Copilot September 10, 2025 15:11
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR implements first pass support for ext-encoding in the BedrockProtocol library. The primary purpose is to migrate from the previous PacketSerializer system to a new encoding system using pmmp/encoding library, which provides better performance and more structured data serialization.

Key changes include:

  • Replace PacketSerializer with ByteBufferReader/ByteBufferWriter throughout the codebase
  • Migrate to pmmp/encoding library for binary data operations (LE, VarInt, Byte, etc.)
  • Introduce CommonTypes class to centralize common serialization operations

Reviewed Changes

Copilot reviewed 300 out of 356 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
Many entity type files Update serialization methods to use new encoding system
Camera-related types Migrate to ByteBufferReader/ByteBufferWriter
Biome generation types Convert to pmmp/encoding format
Core packet types Update to use CommonTypes and new buffer system
Serializer components Replace PacketSerializer with CommonTypes utilities
Multiple packet classes Implement new encoding/decoding with ByteBufferReader/Writer

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

pmmp-admin-bot[bot]
pmmp-admin-bot bot previously approved these changes Sep 10, 2025
pmmp-admin-bot[bot]
pmmp-admin-bot bot previously approved these changes Sep 10, 2025
pmmp-admin-bot[bot]
pmmp-admin-bot bot previously approved these changes Sep 11, 2025
pmmp-admin-bot[bot]
pmmp-admin-bot bot previously approved these changes Sep 11, 2025
pmmp-admin-bot[bot]
pmmp-admin-bot bot previously approved these changes Sep 11, 2025
pmmp-admin-bot[bot]
pmmp-admin-bot bot previously approved these changes Sep 15, 2025
The current game plan is to roll out BedrockProtocol only with ext-encoding first. This ensures that
if there are any major issues that my (extensive) unit tests haven't caught, the damage will be
limited to packet encoding, so no permanent damage will be done.

Since we use NBT in world saves as well as on the network, depending on the version using
ext-encoding here would make this next upgrade very risky, so it's not worth it for now. The time
for that will come in the next month or two.
pmmp-admin-bot[bot]
pmmp-admin-bot bot previously approved these changes Sep 20, 2025
dries-c
dries-c previously approved these changes Sep 20, 2025
@dktapps dktapps dismissed stale reviews from dries-c and pmmp-admin-bot[bot] via 7475989 September 20, 2025 22:32
@dktapps dktapps merged commit 5ad5f12 into master Sep 20, 2025
13 checks passed
@github-project-automation github-project-automation bot moved this from In Progress to Done in ext-encoding integration Sep 20, 2025
@dktapps dktapps deleted the ext-encoding branch September 20, 2025 23:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

2 participants