[iceeedr]

Thanks for your review.

[fum77]

I will try your plugin in a bit!

[DJEarthQuake]

What is the status of this one off iceeedr?

Originally Posted by iceeedr You are right, according to the log it is as if the player was "bugged" in the putinserver and does not pass to the ping test, I honestly do not know what causes this, I will hope that with a little luck the Arkshine, or someone else understood can clarify what happened. PHP Code: L 07/16/2019 - 13:21:34: [admin.amxx] Login: "Player<13><STEAM_0:1:56888062><>" became an admin (account "STEAM_0:1:xxxxxxxx") (access "abcdefghijklstu") (address "191.249.223.253") L 07/16/2019 - 13:21:34: [NewPingKicker.amxx] [HPK]  ?????????????PinG ??? 10ms? L 07/16/2019 - 13:21:37: [NewPingKicker.amxx] [HPK]  ?????????????PinG ??? 10ms? L 07/16/2019 - 13:21:40: [NewPingKicker.amxx] [HPK]  ?????????????PinG ??? 10ms? L 07/16/2019 - 13:21:43: [NewPingKicker.amxx] [HPK]  ?????????????PinG ??? 10ms? L 07/16/2019 - 13:21:46: [NewPingKicker.amxx] [HPK]  ?????????????PinG ??? 10ms? L 07/16/2019 - 13:21:49: [NewPingKicker.amxx] [HPK]  ?????????????PinG ??? 10ms? L 07/16/2019 - 13:21:52: [NewPingKicker.amxx] [HPK]  ?????????????PinG ??? 10ms? L 07/16/2019 - 13:21:55: [NewPingKicker.amxx] [HPK]  ?????????????PinG ??? 10ms? L 07/16/2019 - 13:21:58: [NewPingKicker.amxx] [HPK]  ?????????????PinG ??? 10ms? L 07/16/2019 - 13:22:01: [NewPingKicker.amxx] [HPK]  ?????????????PinG ??? 10ms? L 07/16/2019 - 13:22:04: [NewPingKicker.amxx] [HPK]  ?????????????PinG ??? 10ms? L 07/16/2019 - 13:22:07: [NewPingKicker.amxx] [HPK]  ?????????????PinG ??? 10ms? L 07/16/2019 - 13:22:10: [NewPingKicker.amxx] [HPK]  ?????????????PinG ??? 10ms? L 07/16/2019 - 13:22:13: [NewPingKicker.amxx] [HPK]  ?????????????PinG ??? 10ms? L 07/16/2019 - 13:22:16: [NewPingKicker.amxx] [HPK]  ?????????????PinG ??? 10ms? L 07/16/2019 - 13:22:19: [NewPingKicker.amxx] [HPK]  ?????????????PinG ??? 10ms?

[iceeedr]

Originally Posted by DJEarthQuake What is the status of this one off iceeedr?

https://forums.alliedmods.net/showpo...7&postcount=76

[iceeedr]

Updated

1.4 [23/08/2021] - Final Version (No bug reporting)
* Small optimization in creating cvars.

[HamletEagle]

Can you provide a quick summary about how your plugin is better(in terms of features) than the plugins Arkshine listed here: https://forums.alliedmods.net/showpo...9&postcount=12
Arkshine does not seem to be actively reviewing so I think there is no harm in taking over and reviewing, but I need to know if this plugin is actually worth reviewing in the first place. If you don't provide more features than the already approved plugins then this will be unapproved.

[iceeedr]

Originally Posted by HamletEagle Can you provide a quick summary about how your plugin is better(in terms of features) than the plugins Arkshine listed here: https://forums.alliedmods.net/showpo...9&postcount=12 Arkshine does not seem to be actively reviewing so I think there is no harm in taking over and reviewing, but I need to know if this plugin is actually worth reviewing in the first place. If you don't provide more features than the already approved plugins then this will be unapproved.

First I would like to say that it is an honor to have a plugin reviewed by you, let's get to the facts:

Initially let's see that Ark himself encouraged me to continue the project as he would like to see this type of plugin well written for amxx 1.9+.

#1 plugin:uses idless tasks, just sending the player id, which most of the time caused failures as the player didn't get kicked with high ping and it uses cvar and not pcvars

#2 plugin: It doesn't even use cvars, the values ​​are totally hard coded inside the sma, and it also uses idless tasks, just sending the player id, which most of the time caused failures as the player didn't get kicked with high ping.

#3 plugin: uses idless tasks, just sending the player id, which most of the time caused failures as the player didn't get kicked with high ping and it uses cvar and not pcvars

#4 plugin: uses idless tasks, just sending the player id, which most of the time caused failures as the player didn't get kicked with high ping and it uses cvar and not pcvars

And finally, all are outdated versions and they might not even work right, I don't see any reason not to have an approval since mine uses pcvar, all the optimizations brought by the new versions of amxx and a lang configuration, I thank you in advance for your effort.

[HamletEagle]

1. I don't think using client_putinserver and get_user_flags together is a good idea. If you are using admins by steamid(which everyone should), the steam id may not be available during putinserver, therefore your check will fail. You should be using client_authorized. I wouldn't mind if you also delayed a bit, let's say 0.1 seconds, just to make sure the core had time to properly set the flags.

2. Your cvars are really confusing. For example, VarDelay is described as "Delay between checks" but the only thing it does is to delay the first check after connecting(and you also have another cvar "Time between checks"). Please update them and add proper descriptions. Also have descriptions of cvars in your main post. Why is "taskSetting" called that? Does it involve any settings? I don't think so. Name it properly so people can understand what it is supposed to do.

3. Inside showWarn, you should make sure the client is still connected before displaying the message. Why is this hardcoded at 10 and not configurable by cvar? Same with taskSetting, check if the player is still connected.

4. Why is the number of samples checked at line 78? I don't understand the logic. Shouldn't you check the delay cvar instead?

5. In checkPing, kickPlayer does not need to be repeated in every case. Put it only once, below/after the switch. Also, do not reindex arrays inside a loop(iPlayers[i]).

[iceeedr]

Originally Posted by HamletEagle 1. I don't think using client_putinserver and get_user_flags together is a good idea. If you are using admins by steamid(which everyone should), the steam id may not be available during putinserver, therefore your check will fail. You should be using client_authorized. I wouldn't mind if you also delayed a bit, let's say 0.1 seconds, just to make sure the core had time to properly set the flags. 2. Your cvars are really confusing. For example, VarDelay is described as "Delay between checks" but the only thing it does is to delay the first check after connecting(and you also have another cvar "Time between checks"). Please update them and add proper descriptions. Also have descriptions of cvars in your main post. Why is "taskSetting" called that? Does it involve any settings? I don't think so. Name it properly so people can understand what it is supposed to do. 3. Inside showWarn, you should make sure the client is still connected before displaying the message. Why is this hardcoded at 10 and not configurable by cvar? Same with taskSetting, check if the player is still connected. 4. Why is the number of samples checked at line 78? I don't understand the logic. Shouldn't you check the delay cvar instead? 5. In checkPing, kickPlayer does not need to be repeated in every case. Put it only once, below/after the switch. Also, do not reindex arrays inside a loop(iPlayers[i]).

Some adjustments were made as follows:

1 - I agree, even though the tests and use never failed, I put a security delay.
2 - The names were reanalyzed and adjusted as noted.
3 - We don't check if the player is online as the task is removed on disconnect, I agree with the value being hardcoded, cvar was created, tasksetting was removed.
4 - We check if the amount of player checks is greater than or equal to what is configured in cvar and if the player's average ping is greater than what is configured, if so, we execute the kick.
5 - Done.

Analyzing the code I thought about changing the control of tasks by global entity thinkin, I don't know if it's worth the switch from a resource usage point of view.

1.5 [03/09/2021] - Minor adjustments for approval

[HamletEagle]

What is the reason for adding a version with a thinking entity? I mean, is there a good reason? Your time between checks is in seconds anyway so it's not like tasks can't "think" that fast. Plus, checking faster than 0.1 seconds would be useless anyway.
I think this just adds unneeded complexity for no benefit, but if there is a reason, please let me know.

About the code:

1.

PHP Code:

public client_putinserver(id) 


I already said to use client_authorized. The steamid is guaranteed to be available in client_authorized, not client_putinserver. The task is there just to make sure the core had time to set the admin flags.

2.

PHP Code:

set_task_ex(pCvars[VarDelay], "DoChecks", id, .flags = SetTask_Repeat) 


The purpose of this task is just to delay the initial check, right? Then why is it set to repeat? It should be a single task. From inside DoChecks you start a looping task anyway(checkPing) so there is no point for DoChecks to loop. You basically end up with a useless repeating task that can actually screw over your checks as it can restart(and therefore delay) the checkPing task.
Also, DoChecks is a wrong name, you aren't doing any checks. You are just delaying the ping check, so a better name would be something like DelayPingCheck(feel free to come with a better name).

3. I see you used the WordWord(DoChecks, InitializeChecks) convention but you also have wordWord(showWarn, kickPlayer, checkPing). The style does not matter, but please pick one and be consistent.

4.

PHP Code:

if((ePlayerData[id][g_Samples] >= pCvars[VarTest]) && (ePlayerData[id][g_Ping] / ePlayerData[id][g_Samples]) >= pCvars[VarPing]) 


This condition is a bit too long and kinda hard to read. Maybe split it on the next line? Or maybe compute the average before in a variable and use that variable inside the condition. There is simply too much going on in one single condition.

5.

PHP Code:

client_print_color(iPlayers[i], print_team_default,"%L", id, "KICK_MESSAGE", id, pCvars[VarPing]) 


Should use iPlayer, not iPlayers[i].

6.

PHP Code:

#define KickString 60 


As this is used to size an array/string, it should contain the words "length" or "size". Try something along the lines of KickReasonMaxSize or something.

I know there are a lot of style/naming points, but the coding style and naming things properly are extremely important. It may not be obvious in a short plugin like this one, with only one person working on it(you), but in a bigger project, it will quickly become a pain.

This is pretty much all I have to say, please make the changes and I will approve this.