alliedmodders / sourcemod

SourceMod - Source Engine Scripting and Administration
http://www.sourcemod.net/
972 stars 420 forks source link

Function int GetClientCount(bool inGameOnly) not accurate #1762

Open GabenManPowered opened 2 years ago

GabenManPowered commented 2 years ago

The internal function int GetClientCount(bool inGameOnly) is not accurate.

I wondered why sometimes I had a difference between the number of players (slots) used by the servers and those returned by the int GetClientCount(bool inGameOnly) function.

This is because the native sm_GetClientCount is not correct. Indeed in the case where we use the boolean inGameOnly at false, we make a loop on the players but SM has not yet recorded that the player was in the process of connection (i.e. state "connecting" in the engine).

playerhelpers (PlayerManager) is updated too late (when the client has reached the state connected => OnClientConnect()).

When player is in this state (connecting), it is not counted in SM whereas it must be, it use a slot!

To fix the case and have the correct count, we must use the function "GetClientCount() "from IServer (baseserver.h), then it will be correct (as it used in the engine ie (sv.GetClientCount()).

I have a fix (tested) wich work if we add iserver to sourcemod_api then use iserver->GetClientCount() (PR will come soon)

JoinedSenses commented 2 years ago

when exactly was it inaccurate? when were you using the function?

GabenManPowered commented 2 years ago

it is inacurrate in this case for example:

hostname: Bongo Gaben Land
version : 6630498/24 6630498 secure
udp/ip  : 124.56.69.21:27015  (public ip: 124.56.69.21)
map     : cs_office at: 0 x, 0 y, 0 z
tags    : startmoney
players : 2 humans, 19 bots (32 max)
edicts  : 595 used of 2048 max
# userid name                uniqueid            connected ping loss state  adr
#    728 "Dave"         BOT                                     active
#    732 "Mike"            BOT                                     active
#     45 "Pardoos�"         [U:1:*******]    00:09    14    0 connecting ***********
#    733 "Striker"           [U:1:******]       02:07      183    1 active **********

It cause problem too when we want :

Actually => GetClientCount(false) => 1 With my fix => GetClientCount(false) => 2

psychonic commented 2 years ago

InGame != connected. For your example above, there is only one human client that has passed ClientPutInServer.

GabenManPowered commented 2 years ago

InGame != connected. For your example above, there is only one human client that has passed ClientPutInServer.

the connecting player must be counted

psychonic commented 2 years ago

I misread a bit. This is still a bit tricky. The behavior of this has always been the count of clients that are addressable by SourceMod. If the client isn't in a connected state, they are not able to be referenced in any meaningful way. Opting all users into changed behavior after it's been this way since the beginning can have unintended consequences.

GabenManPowered commented 2 years ago

The fix i propose is less impact. You can have a look on https://github.com/GabenManPowered/sourcemod/tree/FixGetClientCount

here is a summary:

core\PlayerManager.cpp become:

int PlayerManager::GetNumPlayers(const bool inGameOnly)
{
    int count = 0;

    if (inGameOnly)
    {
        count = m_PlayerCount;
    }
    else
    {
        for ( int i = 0; i < iserver->GetClientCount(); i++ )
        {

            IClient *client = iserver->GetClient( i );

            if ( !client->IsConnected() )
                continue; // not connected yet, maybe challenging

            count++;
        }
    }

    return count;
}

core\logic\smn_players.cpp become

static cell_t sm_GetClientCount(IPluginContext *pCtx, const cell_t *params)
{
    return playerhelpers->GetNumPlayers(params[1]);
}

And in core\sourcemm_api.cpp we expose iserver to get GetClientCount() from engine

JoinedSenses commented 2 years ago

i imagine it would be better to make a new function to serve this purpose rather than modifying an existing one and affecting those who use it. or make use of OnClientConnect to check if a client has initiated a connection

psychonic commented 2 years ago

i imagine it would be better to make a new function to serve this purpose rather than modifying an existing one and affecting those who use it. or make use of OnClientConnect to check if a client has initiated a connection

This, or a new parameter, or something. Bug or not, there are 15+ years of plugins potentially relying on the existing behavior.

GabenManPowered commented 2 years ago

I see 3 possibilities:

1) we use my fix by modifying GetNumPlayers()

2) we add a parameter int GetClientCount(bool inGameOnly) => int GetClientCount(bool inGameOnly, bool FromEngine=false)

3) Make a new function GetEngineClientCount()

I think you will like the 3rd :)

JoinedSenses commented 2 years ago

or keep track of clients in your own plugin by utilizing OnClientConnect / OnClientDisconnected

GabenManPowered commented 2 years ago

or keep track of clients in your own plugin by utilizing OnClientConnect / OnClientDisconnected

dont work, "connecting" clients are not tracked, OnClientConnect is called later

JoinedSenses commented 2 years ago

Theres OnClientConnect and OnClientConnected, i assumed OnClientConnect was called when they were attempting to connect.

GabenManPowered commented 2 years ago

Theres OnClientConnect and OnClientConnected, i assumed OnClientConnect was called when they were attempting to connect.

No, when players are in state "connecting" the OnClientConnect is not called, checked with a trace in

bool PlayerManager::OnClientConnect(edict_t *pEntity, const char *pszName, const char *pszAddress, char *reject, int maxrejectlen)
{
    int client = IndexOfEdict(pEntity);
    CPlayer *pPlayer = &m_Players[client];
    ++m_PlayersSinceActive;

    printf("[DebugConnect] id=%d \"%s\" \"%s\" \n", client, pszName, pszAddress);
Client "Test" connected (**************).
Server waking up from hibernation
status
hostname: Bongo Gaben Land
version : 6630498/24 6630498 secure
udp/ip  : 124.56.69.21:27015  (public ip: 124.56.69.21)
map     : cs_office at: 0 x, 0 y, 0 z
tags    : startmoney
players : 1 humans, 0 bots (32 max)
edicts  : 669 used of 2048 max
# userid name                uniqueid            connected ping loss state  adr
#      2 "Test"   [U:1:**********]    00:14       15   0 connecting **************

As you can see, OnClientConnect is not called

ambaca commented 2 years ago

Hook Event "player_connect", "player_disconnect"