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
Dual addon mounting #188
Dual addon mounting #188
Conversation
This is still pending some more testing to be done on servers, if nothing goes wrong this will be ready for merge. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested on a live server, working well with minimal bugs.
Is this really restricted to 2 add-ons? I know the focus is servers but it would deduplicate a lot of this if folks were able to use "upstream" models. Nice one, btw. |
Technically it can be any number of addons, but each additional one will need yet another reconnect (due to the signon message) and therefore introduce more friction to players trying to join. We thought that for ZE servers having just 1 extra addon with all the server content is enough. This of course strictly applies to our implementation, anyone can use it as a reference to create a separate plugin. |
fadf118
to
a62e094
Compare
35cc5da
to
c7a86b6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly LGTM, some nitpicks & room for future improvement. This is awesome work and will enable so many mods down the line, thank you for making this!
I’m curious why the CNetChan interface is being duplicated, and the server side player should probably be gamedata, but both of those aren’t that big of a deal.
src/serversideclient.h
Outdated
#include "utlstring.h" | ||
#include "netadr.h" | ||
|
||
class CNetChan |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we re-defining net channels here instead of using the HL2SDK? Is INetChannel broken these days?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At the time the hl2sdk definition seemed outdated and I was still hacking around with this stuff. But now that I don't use the remote address there's no reason to keep this definition anymore. Thanks for reminding me about it.
@@ -46,6 +46,10 @@ cs2f_vote_max_nominations 10 // Number of nominations to include per vote, out | |||
// User preferences settings | |||
cs2f_user_prefs_api "" // User Preferences REST API endpoint | |||
|
|||
// Extra addon settings | |||
cs2f_extra_addon "" // The workshop ID of an extra addon to mount and send to clients | |||
cs2f_extra_addon_timeout 10 // How long in seconds until clients are timed out in between connects for the extra addon, requires cs2f_extra_addon to be used |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
10 seconds feels short… Is there any impact to making this longer? (Say, a minute.)
It just feels a little clunky to reject players for being away for 10 seconds, especially when it’s (theoretically) caused by:
- Slow computers
- First-timers clicking “accept download” too slow
- Updated addons on slow connections
- Acts of god
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the player already has the addon, the reconnect is nearly instantaneous so it isn't a big issue. One of the reasons for the timeout is that we've had players who would deny the download prompts then proceed to complain about error models and/or crash.
src/serversideclient.h
Outdated
#ifdef __linux__ | ||
[[maybe_unused]] char pad2[0x10]; | ||
#endif | ||
CNetChan *m_NetChannel; // 80 | 96 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How stable is this class? I know it’s in the engine but it just smells like it should be gamedata.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not very actually, a recent update shifted some members down. Though this should belong in the sdk, but it hasn't been PR'd in there yet (see here).
src/utils/module.h
Outdated
@@ -75,7 +75,7 @@ class CModule | |||
for (size_t i = 0; i < m_size; i++) | |||
{ | |||
size_t Matches = 0; | |||
while (*(pMemory + i + Matches) == pData[Matches] || pData[Matches] == '\x2A') | |||
while (Matches < iSigLength && *(pMemory + i + Matches) == pData[Matches] || pData[Matches] == '\x2A') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this meant to be (less than sig length AND memory matches) OR sig is wildcard
… or is it less than sig length AND (memory matches OR sig is wildcard)
? Because currently it’s the first one.
Regardless should probably make it explicit to avoid confusion.
Also needs an additional check to ensure it doesn’t overrun the targeted module and start scanning unpaged memory.
src/gameconfig.cpp
Outdated
@@ -256,7 +258,7 @@ byte *CGameConfig::HexToByte(const char *src, size_t &length) | |||
} | |||
|
|||
length = strlen(src) / 4; | |||
uint8_t *dest = new uint8_t[length]; | |||
uint8_t *dest = new uint8_t[length + 1]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the final null byte initialized here? I don’t know much about C++ initialization so just making sure.
ecfb06e
to
c22663c
Compare
Now using the proto files included in the sdk, except for the cs ones which are broken for some reason
Also fix a crash when changing maps
Valve has gracefully fixed the auth issue so now we just need to use Hook_ClientConnect for our setup
It broke for some cursed reason that I haven't figured out so I'm settling with this
It's ugly but it ensures that we won't run into issues with clients sharing an IP
GameConfig will need a bit of a rework but that doesn't belong in this branch, we've bloated it too much already.
ec9cc36
to
49aaa71
Compare
Possibly one of the biggest developments for community servers, this PR introduces a simple but quite hacky method that forces servers and clients to mount an extra workshop addon on top of the current map. This works by adding the extra addon to the server's addon string, in which entries can be comma-separated.
We also send connecting clients a fake changelevel signon state that makes them check for the extra addon and prompt to download it if not present. The message isn't sent again until
cs2f_extra_addon_timeout
seconds pass since the last time a client joined.Here I've also updated the build scripts to the new hl2sdk-manifests scheme, and we're now using the protobuf files provided by the sdk (the existing ones are kept for VS though).