Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Packet encapsulation #9052

Merged
merged 10 commits into from Apr 24, 2021
Merged

Conversation

rubidium42
Copy link
Contributor

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.

  • The bug fix is important enough to be backported? (label: 'backport requested')
  • This PR affects the save game format? (label 'savegame upgrade')
  • This PR affects the GS/AI API? (label 'needs review: Script API')
    • ai_changelog.hpp, gs_changelog.hpp need updating.
    • The compatibility wrappers (compat_*.nut) need updating.
  • This PR affects the NewGRF API? (label 'needs review: NewGRF')

Copy link
Member

@TrueBrain TrueBrain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

src/network/core/packet.cpp Show resolved Hide resolved
src/network/core/packet.cpp Outdated Show resolved Hide resolved
src/network/core/packet.cpp Outdated Show resolved Hide resolved
src/network/core/packet.cpp Outdated Show resolved Hide resolved
src/network/core/packet.cpp Outdated Show resolved Hide resolved
src/network/core/packet.cpp Outdated Show resolved Hide resolved
src/network/core/packet.cpp Outdated Show resolved Hide resolved
src/network/core/packet.cpp Outdated Show resolved Hide resolved
src/network/core/packet.cpp Show resolved Hide resolved
src/network/core/packet.cpp Outdated Show resolved Hide resolved
Copy link
Member

@TrueBrain TrueBrain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two minor nitpicks, but it looks good to me. Doesn't mean JGR's question isn't a valid one, but I leave that to others to jump in :D

src/network/core/packet.cpp Outdated Show resolved Hide resolved
src/network/core/packet.h Outdated Show resolved Hide resolved
src/network/core/udp.cpp Outdated Show resolved Hide resolved
@rubidium42 rubidium42 force-pushed the packet_encapsulation branch 2 times, most recently from e28c7e1 to 21f83d5 Compare April 21, 2021 05:59
src/network/core/packet.h Show resolved Hide resolved
src/network/core/packet.h Outdated Show resolved Hide resolved
src/network/core/packet.h Show resolved Hide resolved
@rubidium42 rubidium42 force-pushed the packet_encapsulation branch 2 times, most recently from 6e187cb to 4dc6df0 Compare April 22, 2021 16:12
@rubidium42 rubidium42 merged commit 7538687 into OpenTTD:master Apr 24, 2021
@rubidium42 rubidium42 deleted the packet_encapsulation branch April 24, 2021 18:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants