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

Remove lots of unused logic in game server messaging #1463

Merged
merged 13 commits into from
Aug 15, 2023

Conversation

dgelessus
Copy link
Contributor

@dgelessus dgelessus commented Aug 13, 2023

Follow-up to a few things I mentioned in #1377. This simplifies the game server message send/receive code by removing a lot of unused logic. Specifically:

  • plNetMessage had some incomplete code for reading/writing messages from/to memory buffers instead of streams - presumably to reduce copying? The actual message IO is still based on hsStream though, so the fixed-size buffers just complicated the API and logic for no reason. I changed it to use hsStream all the way through.
  • plNetMessages could theoretically be sent to the auth server instead of the game server, but this was never done - every single message was explicitly set to go to the game server, so I hardcoded that globally. (DIRTSAND and MOSS don't even implement the auth server PropagateBuffer messages necessary to support this.)
  • I removed all remaining peer-to-peer code and related stuff like "channels" and "transport groups". None of this was actually used - the current network protocol is strictly client/server. Even if the P2P code could be made to work again, it would be pointless nowadays where everyone is behind a firewall.
  • The plNetClientComm message receiving code supported dynamically un-/registering message handlers, with special logic for multiple handlers for the same message and optional fallthrough. This was completely unused though - all the real message handling happens in a single "default handler" with a big static switch. I removed all the dynamic message dispatching code and left only the single global message handler callback.

I think all of these things will never be useful anymore (at least not without some serious reworking) so there's no reason to keep them - but if anybody disagrees or was planning to use any of this code, let me know 🙂

Yes, all the plNetClientMsgHandler::PeekMsg calls really did nothing,
because plNetMessage::fNetCoreMsg was never set anywhere.
This simplifies the writing process especially, where the previous
implementation actually had to write the message twice: first to
calculate the required buffer size, then another time to actually fill
in the buffer. Now the message is written directly into a hsRAMStream.
The channel number was ignored when sending messages to the server. The
plNetTransportMember subscription lists were updated in various places,
but also never actually used for anything.
Now there's only one single message handler, previously called the
"default handler", which takes care of all message dispatching.
The previous type-based message handling was already completely unused.
Copy link
Member

@Hoikas Hoikas left a comment

Choose a reason for hiding this comment

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

Lots of good cruft clearing, I think. I am a little nervous about the channel group removal, though, and I just to check... Did you test or make sure that voice chat will still only go out to the players who are in the listen list?

@dgelessus
Copy link
Contributor Author

The channel/transport groups were one of the hairier removals, yeah. I'm relatively confident about that though - all the code that adds channel subscriptions is guarded behind if (IsPeerToPeer()), and I still can't find any place that reads the channel group subscription lists.

I have to admit my multiplayer testing wasn't very in-depth - I mainly just checked that everything was still working at all. I suppose this could be a good thing to test on TrollLand with a small group of people.

The voice chat filtering is particularly difficult to test though, because if all goes well, the filtering is done by both the sender (talk list) and the receiver (listen list). So even if I broke the filtering on send, I wouldn't notice, as long as the filtering on receive still works. Testing this properly would require looking at packet captures I guess...

That said, the voice talk/listen lists are their own lists that are maintained separately from the channel group stuff. I didn't touch the voice code except for removing the P2P branches, so hopefully that works as before...

Nothing used the option to send a plNetMessage to the auth server.
DIRTSAND and MOSS don't support handling such messages either.
The parsed plNetMessage is now passed directly to the actual handler,
making the code much easier to follow.
@Hoikas Hoikas merged commit 49eb827 into H-uru:master Aug 15, 2023
14 checks passed
@dgelessus dgelessus deleted the netclient_simplification branch November 29, 2023 21:30
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.

2 participants