Messages (Handler/Sender): Refactor Packet Handling and Sending into a DDD approach.

[ovflowd]

Right now all the Messages handled by a Channel, both to Handle packets coming from the Client-Side and also to Send Packets to the Server-Side are being handled by two files:

• Network/ChannelPacketHandler
• Network/ChannelPacketSender

Those two files are already enormously big and already might impact the maintainability of the Melia Core.

The suggestion is to first move the part of the code where messages are being handled and written to a different part of the Emulator. (It is important to mention to leave all-kind of Business-Logic and Application-Logic outside the scope of composing the bodies and reading the bodies of packets.

This means that the refactored methods for handling and sending packets (The part that composes the bodies of the messages and the part that reads them) should be isolated.

Sender-Reader-Handler Approach

• A reader should only contain pieces of code to read the Packet and return the data that was read as a return value of the method
• A Sender should only contain pieces of code to write the bodies of the packets and return the Buffer ready to be sent.
• A handler should invoke a Reader function when it wants to read a packet and Invoke a Sender function when it wants to send something.

An example of this strategy would be:

private readonly CharacterSessionReader Reader = new CharacterSessionReader();
private readonly CharacterSessionSender Sender = new CharacterSessionSender();

[PacketHandler(Op.CZ_LOGOUT)]
public void CZ_LOGOUT(ChannelConnection conn, Packet packet)
{
    var { extra, unkByte } = Reader.CZ_LOGOUT();

    Log.Info("User '{0}' is logging out.", conn.Account.Name);

	Send(Sender.ZC_SAVE_INFO(conn));
	Send(Sender.ZC_LOGOUT_OK(conn));
}

Domain-Driven-Design Folder Structure (Code Separation)

Regarding the DDD part of the refactoring, it would mean that the Readers and Senders and Handlers would be related to their Functionality based in the Game, rather than what they are (Senders/Readers).

Imagine the following folder structure:

- Communication/Messages
  - Character/
    - Session/
      - Reader.cs
      - Handler.cs
      - Sender.cs
    - Inventory
      - Reader.cs
      - Handler.cs
      - Sender.cs
    ...
  ...

This is an example of a possible Folder structure. The naming varies based on how we would separate and group the Messages.

An example of this strategy would be:

• Messages related to the Character should reside in the bigger Group "Character"
• Messages specific to the Character to manage its Inventory should reside inside "Character/Inventory"...
• Messages related to Managing the state/effects of Monsters should reside in the bigger Group "Monsters"
• Messages related to the Character to use Skills should reside inside "Character/Skills"

This is just an illustrative example.

[exectails]

Shouldn't this be a discussion for now? In any case, I've been going back and forth on this for 10 years with other developers, and I still always come back to the current design for its simplicity. Let's go over the problems I see.

First, there are the readers and writers/senders. I agree that, generally, it would make sense to put those into separate classes. It's clean, you only ever see that one packet, you can unit test them, but from a server emulator coder's perspective, it's a really annoying design. Every single time you want to add a packet handler, you have to go create one or two new files for the reader and the writer and implement them. Then you go and create the actual handler and potentially sender. Taking hopping between files into account, you spend what feels like 10x the amount of time on just adding the basic handling for a packet, especially because you often times don't get a packet right on the first try. And this adds up quickly, especially in these early stages, when you're constantly adding new packets. And then, whenever you need to make changes, you're hopping back and forth between files again.

Adding categorization on top of of that makes it even worse, because then you're always asking yourself where you should put something, where you put things in the past, where things should go which's purpose you don't know yet, if maybe you should be moving something, etc.

In my eyes, this doesn't work well for a server emulator, where requirements constantly change and evolve and new discoveries change how packets are used and interpreted. It just adds additional steps, but offers very little benefit in my opinion. It lowers the file size of these classes a little, yes, but is that significant? When I need to find a packet I use the reference search and jump there immediately. The size of the overall file matters little, and I see the entire packet handling at a glance, which in most cases is exactly what I need in our use case. Organizing the packets "properly" also lets you unit test them, certainly a point, weren't it for the fact that packets are in constant flux and whether they're correct isn't determined by our unit tests, but the client. We aren't in control here at all.

Under normal circumstances I wouldn't use our current design. I would use something more or less like you proposed. But if I don't know what's coming and I constantly have to add, remove, modify, and tweak packets, putting everything in one place makes things much easier and straight forward in my opinion. 4000 lines of packet senders is a lot, yes. But personally, I think I'd rather split that class up into partial classes at some point, if actually necessary, instead of adding literally over 1000 separate files.

[Adephx]

Sorry, there might be some misunderstanding on my part. Does the client expect the literal ZC_LOGOUT_OK string or in this case a 0xBC3 OPCode? Was ZC_LOGOUT_OK named by you or by IMC Games? If it's the latter, then I take everything back and agree that we should leave it as it is.

[exectails]

The client doesn't expect the literal string "ZC_LOGOUT_OK", but ZC_LOGOUT_OK is the official name of the packet. The entire opcode list is official and is extracted by me whenever the ops change.

[ovflowd]

you think for developing a server emulator it's more important to be able to quickly and accurately find the packets and opcodes you're looking for?

Isn't why I said that the methods would still be decorated with [PacketHandler(Op.CZ_LOGOUT)] for example?

So the actual method names could be cleaner? 🤔 (You can still nicely and easily search by the OpCodes since the methods are decorated by them)

[Adephx]

Alright, if ZC_LOGOUT_OK is a value that is part of the 0xBC3 package, then we should leave the entire list as it is.

[exectails]

You can still nicely and easily search by the OpCodes since the methods are decorated by them

You can't use a symbol search though. With that, I copy the name of the packet I'm looking for, like from Pale, hit 3 keys in VS, and I'm there. If I have to do a regular search, I need 3 key hits to even start the search, depending on the size of the source code that search might take a few seconds, and then I'm greeted with the results, which I have to parse first, before getting to where I want to go, because they include all instances of that search string in the entire source code, including comments.

Symbol searches are increadibly convenient, because you're searching for what you're actually looking for, a symbol that contains a certain string.

Naturally, this isn't a requirement, but it's a big creature comfort when working with packets at this stage in such a project, because you constantly need to get to specific handlers and senders, and the time those more involved searches take adds up over time. I don't know if you're using symbol searches yourself, but I'm dreading every time I have to search for a packet in some of my bigger C++ projects, where the IDE has trouble properly indexing the whole thing, and I have to resort to normal searches, because the symbol search isn't reliable there.