Side note: This truncates 4 bytes to 1 byte 👀

How does the reader recognize the length of this text? Surely this must be either null terminated or bundled with a count? I wonder if this is an original bug.

Should this be addressed in another PR or here?
A potential fix:

{
    return sizeof(UnsignedByte) + m_text.getByteCount(); 
}

This needs to be addressed separately, and it needs to consider retail compatibility.

I suspect it works in original game because message buffer is zero initialized and therefore string is zero terminated when nothing comes after it.

You can add a TheSuperHackers @todo ... comment here.

I see what you did now:

struct NetPacketDisconnectChatCommandHeader {
	NetPacketCommandTypeField commandType;
	NetPacketRelayField relay;
	NetPacketPlayerIdField playerId;
	NetPacketDataFieldHeader dataHeader;
	UnsignedByte textLength;
};

You put the textLength into the header struct.

It is a bit confusing that it is detached from the string data.

Perhaps move it? Same in the other struct.

Looking at this again, I think we need to make another pass on this.

NetChatCommandMsg inherits from NetCommandMsg. All these messages inherit from it.

So what we can do is add

size_t NetCommandMsg::getByteCount() const
{
  return sizeof(PackedNetCommandMsg);
}

It can then be combined with its descendent classes:

size_t NetChatCommandMsg::getByteCount() const
{
  return NetCommandMsg::getByteCount() + ...;
}

Alternatively the Packed structs can also inherit. Not sure what the best approach is because of the variable length data.

Ok I went with packed structs inheritance, let me know what you think

It still is confusing. We have 25 messages that inherit the base message:

class NetGameCommandMsg : public NetCommandMsg
class NetAckBothCommandMsg : public NetCommandMsg
class NetAckStage1CommandMsg : public NetCommandMsg
class NetAckStage2CommandMsg : public NetCommandMsg
class NetFrameCommandMsg : public NetCommandMsg
class NetPlayerLeaveCommandMsg : public NetCommandMsg
class NetRunAheadMetricsCommandMsg : public NetCommandMsg
class NetRunAheadCommandMsg : public NetCommandMsg
class NetDestroyPlayerCommandMsg : public NetCommandMsg
class NetKeepAliveCommandMsg : public NetCommandMsg
class NetDisconnectKeepAliveCommandMsg : public NetCommandMsg
class NetDisconnectPlayerCommandMsg : public NetCommandMsg
class NetPacketRouterQueryCommandMsg : public NetCommandMsg
class NetPacketRouterAckCommandMsg : public NetCommandMsg
class NetDisconnectChatCommandMsg : public NetCommandMsg
class NetChatCommandMsg : public NetCommandMsg
class NetDisconnectVoteCommandMsg : public NetCommandMsg
class NetProgressCommandMsg: public NetCommandMsg
class NetWrapperCommandMsg : public NetCommandMsg
class NetFileCommandMsg : public NetCommandMsg
class NetFileAnnounceCommandMsg : public NetCommandMsg
class NetFileProgressCommandMsg : public NetCommandMsg
class NetDisconnectFrameCommandMsg : public NetCommandMsg
class NetDisconnectScreenOffCommandMsg : public NetCommandMsg
class NetFrameResendRequestCommandMsg : public NetCommandMsg

Are not all messages using all the fields of the base message? It would have been nice if this could be constructed with consistency. But maybe that is not the case. What deviation patterns do we have?

The pattern is:
Variable data -> getByteCount() function
Fixed data -> sizeof(struct) directly

We've got 5 variable-length classes using getByteCount() with packed struct inheritance.
and 20 fixed-size classes using sizeof(struct) directly

The 5 that need custom size calculations are:
NetGameCommandMsg
NetChatCommandMsg
NetDisconnectChatCommandMsg
NetFileCommandMsg
NetFileAnnounceCommandMsg

These packets have dynamic content (text, file data, game arguments), so they use custom size calculations. They use a header + variable data pattern

The confusion I see here is that the packed structs have different inheritance patterns than the non packed structs. It would be nice if these were consistent, but that seems not possible because the first few data fields are not always the same, for whatever reason.

I am inclined to say if we cannot make the packed ones use inheritance consistently, then make none inherited - for consistency.

But what we could do is add getByteCount() functions to all non packed structs, and then make them static where they simply return sizeof(PackedStructXYZ).

Also, I think we better name the functions getPackedByteCount().

And then in next changes we add functions toPackedStruct and fromPackedStruct to convert between packed and non-packed.

Ok I added getByteCount() functions to all of them, and used static where appropriate, and renamed the functions getPackedByteCount, in separate commits.