Understanding map/packets/basic.h

[StabbyCutyou]

Hello,

I've been spending a bit of time looking into how the packets are constructed and laid out in memory to better understand how I might debug packet dumps and eventually contribute to debugging issues between client / server as I get going with my own server.

I believe I have a good handle on how the memory layout works for the packets themselves (with a huge thanks to https://github.com/atom0s/XiPackets which I used to come up to speed with things), but there was one thing about the LSB codebase was confusing in this regard and I thought I'd ask, as I assume there is a very simple "that's just how c++ works" answer that escapes me:

When I first started reading the codebase, and then again just now as I was checking out this PR #6713 I noticed that type is being represented as a uint8, not a uint16. I see that proliferated in the new ctor as well, and in (what seems to be vestigial) the field declaration itself: https://github.com/LandSandBoat/server/blob/base/src/map/packets/basic.h#L58

My understanding was that it needed a uint16 due to the overflow of the 9th bit from the size field, which we can see reflected here as we try to access it: https://github.com/LandSandBoat/server/blob/base/src/map/packets/basic.h#L133

Obviously, it all works, and so I assume there is something w/r/t the ctor and how ref<>() works just needing the right size of memory overall (2 bytes), and not really worrying about how big either type or size actually need to be, since they sum to the right size.

Appreciate any clarity, thank you!

[zach2good]

I believe yes, the private members are vestigial, but should be uint16s:

    // Mark these members as private, so that they can't be set without using their
    // specialised setters.
private:
    uint8& type;
    uint8& size;

Inheriting packets and external callers are forced to use these accessors:

    /* Getters for the header */

    uint16 getType()
    {
        return ref<uint16>(0) & 0x1FF;
    }

    std::size_t getSize()
    {
        return std::min<std::size_t>(2U * (ref<uint8>(1) & ~1), PACKET_SIZE);
    }

I reckon you could probably remove those private members without doing any harm.

[StabbyCutyou]

Yea, I noticed all real access goes through the accessors, which is why I assumed the private types were vestigial, but then I realized something would still need to tell the struct to have enough space in memory for them behind the scenes for ref to work with (or, without knowing about c++ and trying to figure it out from reading, that's what I'd assume).

[zach2good]

In my other PR, I think I'm going to strip down CBasicPacket - we don't need a lot of the stuff in here. It'll end up looking like this:

class CBasicPacket
{
protected:
    std::array<uint8, PACKET_SIZE> buffer_;

...

With the special getters and setters on top. It'll also get rid of all of this "owner" stuff, which is very unintuitive

[StabbyCutyou]

I will keep an eye out, hopefully I have not sniped you into wasting time here. I figured with the open PR, whatever is going on in here would be fresh in your mind and good for an easy answer.

Appreciate the insight, good to know I wasn't super far off in how I understood it!

[StabbyCutyou]

Actually now that I've bothered to write this up and actually think about it...

The field isn't vestigial, it's job is literally just to carve out a uint8 alongside size to create 2 bytes of space for ref<> to work with, and I assume that's more beneficial than just 1 uint16 called typeandsize, even if the fields themselves are never used directly.

The ref<>s in the ctor likely just need to align with the layout of the struct (again for c++ reasons that I'm not familiar with enough to fully grasp, but can somewhat appreciate all the same).