Packet encapsulation #9052
Merged
Packet encapsulation #9052
+348
−180
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rubidium42
force-pushed
the
packet_encapsulation
branch
from
2f95ec2 to
4dfcc4a
Compare
TrueBrain
reviewed
Apr 20, 2021
There was a problem hiding this comment.
I tried to understand this, but I am not really a C++ programmer .. so I used it to learn a bit about C++, so you have some questions that are just for my education :D Hope you don't mind!
Otherwise, comments are mostly about doxygen comments that are either not clear to me or missing param etc .. code-wise it looks good to me. And of course I assume you tested it :D
JGRennison
reviewed
Apr 20, 2021
rubidium42
force-pushed
the
packet_encapsulation
branch
from
4dfcc4a to
b604b5e
Compare
TrueBrain
reviewed
Apr 20, 2021
rubidium42
commented
Apr 20, 2021
rubidium42
force-pushed
the
packet_encapsulation
branch
2 times, most recently
from
e28c7e1 to
21f83d5
Compare
LordAro
reviewed
Apr 22, 2021
rubidium42
force-pushed
the
packet_encapsulation
branch
2 times, most recently
from
6e187cb to
4dc6df0
Compare
…rite data into the Packet
…vent packet state modifications outside of the Packet
…buffers to prevent packet state modifications outside of the Packet
…om a buffer in to a Packet
…nternal state of Packet private
rubidium42
force-pushed
the
packet_encapsulation
branch
from
4dc6df0 to
5358287
Compare
LordAro
approved these changes
Apr 24, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation / Problem
The pointers and state of the Packet class are updated by other functions all over the place. This makes further changes more difficult as the logic for packet size and reading/writing location is spread all over the network code.
The major benefit at the end is to make it much easier to introduce larger packet sizes, a big first step is in https://github.com/rubidium42/OpenTTD/tree/bigger_packets which makes OpenTTD accept (TCP) packets up to 32767 bytes, and uses that size to send save games as well as to the content server (which is not limited by the current MTU). Later changes could then also be done for e.g. receiving data from the content server or certain admin commands. However, that requires changes to the protocol to determine whether the other side accepts such large packets which is beyond the scope of that patch.
Description
Removes all the direct access to the state variables of a Packet.
Limitations
Any patch messing manually with the Packet state will be broken.
Technically it is possible to extend the Packet size even more, but for save games it does not seem wise to send even larger packets as then it will also take longer before we start with sending data.
Checklist for review
Some things are not automated, and forgotten often. This list is a reminder for the reviewers.