openmultiplayer / open.mp

Open Multiplayer, a multiplayer mod fully backwards compatible with SA-MP
https://open.mp
Mozilla Public License 2.0
424 stars 78 forks source link

Add a new parameter to OnRconLoginAttempt callback #892

Open adib-yg opened 3 months ago

adib-yg commented 3 months ago

Add the playerid parameter after the success parameter to OnRconLoginAttempt callback.

public OnRconLoginAttempt(ip[], password[], success, playerid)
{
    if (!success)
    {
        Kick(playerid);
    }
}

I know we can get the player id by loop through players' ip

NexiusTailer commented 3 months ago

I suggested something similar earlier, but with separate callbacks OnInGameRconLoginAttempt (and OnInGameRconCommand for already logined rcon admins, as OnRconCommand also doen't pass player ID). If opinion about adding this was changed and it could be added, then I still think a separate variants will be much better than adding a playerid parameter to the existing ones, because:

  1. If I'm not wrong, it is called both from remote rcon (without connecting to a server) and in-game rcon, so passing playerid as 65535 half of all cases seems redundant.
  2. Already declared callbacks must be changed and recompiled due to new arguments in them, separate callbacks won't produce such problems.

UPD: Oh btw,

I know we can get the player id by loop through players' ip

actually we cannot be sure which playerid it is even using a loop because if the server allow many players from the same IP, a loop will find only one of the players on this IP address. That's why I also think about adding "playerid"-ful analogue of those callbacks for in-game rcon manipulations is a not so bad idea.

Y-Less commented 3 months ago

Unfortunately adding new parameters to existing callbacks is a compiler error. Weirdly even just changing the names (not types) of existing ones is an error.

The other option is a function like GetRconAttemptPlayer(), returning INVALID_PLAYER_ID when there isn't one. The question with a new callback is how do the semantics work? Do we call only OnInGameRconLoginAttempt for in-game calls? Or both? What about in old scripts with only one? And any attempt to determine which to call could break expectations if a library hooks one or both - you think your script doesn't have the new variant when it does in a library, the server can't tell the difference.

I'm sure there are good answers to all these questions, they just need working out. However, whatever the solution there's still going to be duplicated code too. A Get function would allow people to update exsting scripts at their own pace.

NexiusTailer commented 3 months ago

Well it's also a good variant. Just doing it like with OnPlayerWeaponShot and GetPlayerLastShotVectors, where the second is also updated every shot, being useful inside OPWS and keep containing extended data about the last bullet further (until the new shot rewrite it). So, this seems a good idea for me too.

If considering both OnRconLoginAttempt and OnRconCommand, the naming can be related to "interaction", something like "get player interacted with rcon" (just the idea, not literally this name).

ARKANANTAATAYA commented 3 months ago

Pintar

Y-Less commented 3 months ago

If considering both OnRconLoginAttempt and OnRconCommand, the naming can be related to "interaction", something like "get player interacted with rcon" (just the idea, not literally this name).

Good point. GetRconPlayer?

Edit: GetLastRconPlayer if we want to be more consistent with GetPlayerLastShotVectors.