Fix packet code

[psu-de]

The code of packets is super messy and weird. Lets standardize how packets should be structured, and handle differences in minecraft versions.

My suggestion is the following:
Have a base class for every packet in the PacketType enum implementing the IPacket interface.
If packets use enum's or have nested fields, create the new type nested within the packet class.
For nested classes: always implement the ISerializable<T> interface.

If the packet has differences between versions:
In this case, the base class should be abstract.
The PacketType field is implemented as usual, since it should never change between versions.
The Write method is declared as abstract.

Create a new nested class for each version where the packet changed, deriving from the base class.
Every version specific packet class implements its required members, and the correct Read and Write methods.

The base class then implements the read method, by reading the correct subclass for the current version.

Example:

public abstract record MyPacket : IPacket
{
    public PacketType Type => StaticType;
    
    public static PacketType StaticType => PacketType.CB_Play_MyPacket;

    public abstract void Write(PacketBuffer buffer, MinecraftData version);

    public static IPacket Read(PacketBuffer buffer, MinecraftData data)
    {
        return version.Version.Protocol switch
        {
            >= ProtocolVersion.V_1_19_2 => Since192._Read(buffer, version),
            < ProtocolVersion.V_1_19_2 => Before192._Read(buffer, version)
        };
    }

    // packet used before minecraft 1.19.2
    public sealed record Before192 : MyPacket
    {
         // fields for versions before 1.19.2

        public override void Write(PacketBuffer buffer, MinecraftData data)
        {
             // ... writing logic
        }

       
        internal static Before192 _Read(PacketBuffer buffer, MinecraftData version)
        {
            return new(...buffer.read());
        }
    }

    // packet used after minecraft 1.19.2
    public sealed record Since192 : MyPacket
    {
         // fields for versions since 1.19.2

        public override void Write(PacketBuffer buffer, MinecraftData data)
        {
             // ... writing logic
        }

       
        internal static Since192 _Read(PacketBuffer buffer, MinecraftData version)
        {
            return new(...buffer.read());
        }
    }
}

Posted by @Klotzi111 in #71 (comment) :

[...] But all the possible version specific sub classes need to implement a common interface and need to be registered in a lookup table (per packet).

Why do you want register all subclasses in a lookup table? The reading logic can deal with reading the right packet, and for writing, inheritance does the job.

[Klotzi111]

First of all I do not like the naming of your version specific sub classes. Before192 and Since192 is not reall a good pattern when there are more than 2 sub classes (different versions). I would probably go with something like MyPacketV1_18_0 and MyPacketV1_19_2. So the base class' name followed by the minecraft version. The underscores are not typicall C# Naming convention but it avoids problems that could occur in the future when its ambiguous. And it also avoids the problem that when you do type checking for sub versions in your class for multiple packets that you get ambiguous usings (and would currently need to use the namespace to reference the correct ones).

Second I would let those sub classes implement an common interface for example: IPacketVersionSubClass<TSelf, TBasePacket>. This interface then has static abstract members for the minecraft protocol version they first appeared. And I would also let the sub classes implement ISerializableWithMinecraftData (interface I added in my PR) so that they have the same exposed methods as the Packet base class. (I would also let all the Packet classes implement that interface).

This then leads to third: I would create a lookup dictionary in very packet base class that are ordered by the protocol version (key) and map to the versioned sub class' read delegate (the static method) (value). The Read method of the base packet class then gets the matching read delegate by "asking" with the current protocol version. The dict then returns the corresponding read delegate for the sub class.
So why would I create this dict for the version specific sub classes? I think it makes registering them easier. There is just a simple Register<TPacketSubClass> line somewhere in the base class (I do not like the pattern matching switch too much). And it also makes it possible to dump/get all version specific sub classes of a packet at runtime (not that I think this will be important by you can).

Finally all of these boilder plate stuff like adding the interfaces and creating the dict and registering all the sub clases can be done with a source generator. The ones that run directly with your dotnet build in one go.

I would like to implement all this for a single packet to better demonstrate this with an example. But I do not know a packet that has more than 2 versions. If you know one please tell me.

[psu-de]

The packet LoginStartPacket has changed from

• 1.18 --> 1.19
• 1.19 --> 1.19.2
• 1.19.2 --> 1.19.3
• 1.19.3 --> 1.20.2

[Klotzi111]

Oh I am sorry for falsely closing this issue (because of the commit message). Before you edited it it would have been resolved by the PR.

[Klotzi111]

I implemented the pattern I thought of in a branch of my forked repo. I did it for LoginStartPacket but then noticed that this packet is never read, only written.
So I did the same pattern for SetExperiencePacket.
The next step would be to create a source generator that does the boiler plate code for us. Look at the file SetExperiencePacket.cs.sourceGenTarget how the packet file would look like once the source generator is done.

What do you think?

[psu-de]

Seems good to me, the version with the SourceGenerator is not too different to my approach.
To be honest, I don't quite understand SourceGenerator. Is it a big performance improvement to use a dictionary lookup instead of pattern switching?

[Klotzi111]

Is it a big performance improvement to use a dictionary lookup instead of pattern switching?

No. It is not even an improvement it is actually slightly slower. But with a dictionary you can query all versions at runtime. With a switch you can not.

[psu-de]

okay, lets try it out then

[Klotzi111]

Ok. I will start implementing the source generator tomorrow then