+- Cuberite Forum (https://forum.cuberite.org)
+-- Forum: Cuberite (https://forum.cuberite.org/forum-4.html)
+--- Forum: Development (https://forum.cuberite.org/forum-13.html)
+--- Thread: Add Virtual Packet ID Method to Protocols (/thread-3003.html)
Add Virtual Packet ID Method to Protocols - lkolbly - 08-27-2017
I was working on the scoreboard, and noticed that in order to add a network packet I have to duplicate the code several times (protocol 1.8, 1.9, 1.12, and 1.12.1). In each case the only change is the packet ID.
So what if there were a virtual method, cProtocol::GetPacketID(ePacketId a_PacketId), which stored the network packet ID for a packet? Then, instead of:
Quote:cPacketizer Pkt(*this, 0x40); // Update Health packetyou would do:
Quote:cPacketizer Pkt(*this, GetPacketID(packetHealthUpdate)); // Update Health packet
Then, there wouldn't have to be a whole bunch of copying/pasting code every time a new protocol comes out and Mojang changes every packet ID. We just copy/paste the GetPacketID method. It would ensure that nothing gets forgotten when a protocol change happens (look at the currently open PR #3962: https://github.com/cuberite/cuberite/pull/3962, which adds leash packets to the 1.12.1 protocol)
It'd also minimize the amount of code duplication. PR #3908 (https://github.com/cuberite/cuberite/pull/3908/) implemented 1.12.1 support, and added over 800 lines of code. By my count, out of 34 Send methods that were added, these were all just copied/pasted with the packet ID changed:
- SendRespawn
- SendPlayerListAddPlayer
- SendPlayerListRemovePlayer
- SendPlayerListUpdateGameMode
- SendPlayerListUpdatePing
- SendPlayerListUpdateDisplayName
- SendPlayerAbilities
- SendPlayerMoveLook
- SendUseBed
- SendDestroyEntity
- SendRemoveEntityEffect
- SendEntityHeadLook
- SendCameraSetTo
- SendDisplayObjective
- SendEntityMetadata
- SendEntityVelocity
- SendEntityEquipment
- SendExperience
- SendHealth
- SendScoreboardObjective
- SendAttachEntity
- SendDetachEntity
- SendScoreUpdate
- SendTimeUpdate
- SendSetRawTitle
- SendSetRawSubTitle
- SendTeleportEntity
- SendEntityProperties
- SendPlayerMaxSpeed
Or, 29 out of 34. That's just the upgrade from 1.12 to 1.12.1, overall we support 10 distinct protocols (the 1.8 to 1.9 transition is doesn't really apply here, though, since 1.9 doesn't inherit from 1.8).
The only downside I see is possibly performance - it's adding a virtual method call to every sent packet. But, that's basically free, compared to the labor cost of maintaining so much duplicate code, and the cost of sending a network packet anyway.
Anyway, I thought I'd post this on the forum in case anyone has an opinion before I went off and made a PR, since it seems like the sort of change that would warrant some discussion (and presumably has been discussed before, but I couldn't find anything on it).
RE: Add Virtual Packet ID Method to Protocols - lkolbly - 08-28-2017
A rainy day and a packet of M&Ms combined to make a proof of concept: https://github.com/cuberite/cuberite/compare/master...lkolbly:packetIds
That branch's GetPacketId method will call super::GetPacketId for packets which didn't change at all in that version, so there's a bit of a potential performance hit there (my computer is currently too swamped to do any decent performance testing right now, but tomorrow hopefully).
RE: Add Virtual Packet ID Method to Protocols - peterbell10 - 08-29-2017
I don't think super::GetPacketId would be a performance hit, in fact if all the GetPacketId functions were put in one translation unit then it should be easily inlined. The initial virtual call would be the one to worry about.
RE: Add Virtual Packet ID Method to Protocols - lkolbly - 08-29-2017
Hm, so maybe if they were put into a single file, src/Protocol/PacketIDs.cpp?