Packets cleanup

[xazax]

Meyraw made a nice modification on how to handle packets.

You can check his project there: http://code.google.com/p/eathenanext/

The main point is, the packets are no longer edited with WFIFO macros, but they are structs, with members just like in Aegis.

With such system it would be easier to add support for newer clients, since we can dump those structs from Aegis leaked pdbs ( In his code as far as I remember the struct and member names does not match with aegis' ones, maybe it would worth name matching.). Adding this would be a great deal of work, breaking lot of existing patches, and probably would be better to develop it in a different branch.

Opinions?

[Sirius_White]

Generally pro. I started this about three times, but never got beyond the basic movement / chat packets. It's indeed a lot work, but worth the effort. The eathenanext approach looks fine, definitely better than the current clif.c code. I didn't read through the entire code though, just briefly skimmed it.

We currently support about six different spawn packets, possibly more by now. Structs make it easier to figure out the final packet structure, but the code remains either redundant (one function per packet in each version) or scrambled with #ifdefs (one function per semantic packet [walk, spawn, ...]).

In the process we should definitely merge packet_db versions and PACKETVER. It doesn't make any sense to support dozens of client versions, but have one fixed server protocol.

PACKET(PKT_MC_CUTIN) pkt = { PKT_MC_CUTIN };

This is weird.

[Kenpachi]

Even though my opinion isn't relevant in this discussion I want to support this idea because this will make it easier for noobs like me to get into this part of Athena.

[KeyWorld]

If the cleanup (remove old packets) concern some of the packets send from client to the server, let me know !

roBrowser currently send the oldest possible packet to server to get a maximum compatibility with all pserver (since ea/ra support almost all packets send from client to server).

Well I should improve robrowser to send the packet related to the detected packetver...

Good idea through, It's more clean than the current version of clif.c.

Same thing with chrif.c and login/map-serv.

[GodLesZ]

Totally agree with the clean up.

But we should talk about the implementation of the new struct.

As @Sirius_White pointed out,

PACKET(PKT_MC_CUTIN) pkt = { PKT_MC_CUTIN };

This is weird.

it is.

roBrowser currently send the oldest possible packet to server to get a maximum compatibility with all pserver (since ea/ra support almost all packets send from client to server).

Well I should improve robrowser to send the packet related to the detected packetver...

Well, nice idea but failed by design

[xazax]

I agree that, it is a bit wierd, and I think, meyraws codes are great reference, but maybe we could come up with something that is even cleaner based on that idea .

[Ind]

Totally agree with the clean up.

But we should talk about the implementation of the new struct.

As @Sirius_White pointed out,

PACKET(PKT_MC_CUTIN) pkt = { PKT_MC_CUTIN };

This is weird.

it is.

+1

[CalciumKid]

PACKET(PKT_MC_CUTIN) pkt = { PKT_MC_CUTIN };

This is weird.

Redundant code is redundant.

[Brian]

Is any of the "clif cleanup" Ai4rei is committing in eAthena related to this idea?

I have not merged eAthena r15051-r15058 and I'm wondering if I should proceed or wait.

r15051

r15052

r15053

r15054

r15055 - already merged

r15056

r15057

r15058

[xazax]

Well, it is not related. Ai4rei did small cleanups, you can merge them, they are ok I think.

This one would be about a bigger rewrite how to handle packets.

[Gepard]

I'm totally in favor of improving or even rewriting client interface. But we really need to think it over thoroughly, how we're gonna do this.

[Ind]

reviving this as proposed over #rdev;

---

One thing I'm highly uncomfortable with:

PACKET(PKT_MC_CUTIN) pkt = { PKT_MC_CUTIN };

since each packet version has its own code and length/sizes for its vars (e.g. check the inventory_list1/2/3 crap) I'd like to propose we use the packet id in the name e.g.

struct packet_0xf5 {
       char    offset[5];
       uint32  item_oid;
};
....
func() {
struct packet_0xf5 p;
p.item_oid = 1;
}