[Shadows Adi]

Thank you for feedback!

I have some more questions before pushing the changed code to github:

Originally Posted by HamletEagle 6. Inside PHP Code: if(szData[0] == '[')  (line 258 ) you can add continue to skip the line, after the section counter is increased. Then you can remove the checks for '[' from the switch.

I would prefer to keep this, because it's more readable by other coders.

Originally Posted by HamletEagle 15. For the record, I'm not a fan of caching the player name in a global variable because you have to account for name changing(using ClientUserInfoChanged). Calling get_user_name directly isn't going to cause any performance issues and trying to keep the cache up to date can be a pain in the ass and it's more error-prone(as is the case here since if the player is going to change his name your code will fail).

I am caching player's name in a global variable because I tought it would give some performance boosts. So it's better to keep it as it is and just check if the player is chaning his name using ClientUserInfoChanged or to call the get_user_name() native every time I need it and declare a local static variable to hold player's name?

[HamletEagle]

I don't think it's more readable since it adds one more indentation level and people may potentially need to scroll to the right you see the entire line. 5 indentation levels are a bit on the "too much" side. Plus, you are doing unnecessary checks. Not likely to cause any problems, but branches(ifs) can throw off the CPUs branch predictor and cause a delay if the prediction is wrong. If you want to learn more about how pipelines and branch predictors work(it's nice to get an idea about what happens inside a CPU, even if only a high-level view) you can google it.

You already know when a line is a section header so all I'm suggesting to do is this:

PHP Code:

if(szData[0] == '[')
{
    iSection += 1
    continue
} 


Then you can be sure the code below will only execute if the line doesn't start with '['.

About caching names/auth ids, it doesn't provide any benefit. Any small performance boost it may provide(if any) is instantly negated by the fact that you need more complex code to keep the cache value updated.
It's up to you if you want to maintain the cache and update it or just switch to get_user_name/get_user_authid since you already implemented most of the caching logic. I'm just saying that, in the future, this isn't something you should consider a priority. Personally, I wouldn't bother trying to cache it.

Also, no need to use static.
"static" shouldn't be viewed as an optimization technique primary. Its main use is to provide a variable with the same scope as a global variable(can only be accessed inside the function that defined it) but with the lifetime of a global variable(it doesn't get destroyed when the function exists, thus it can keep its value between function calls). Why it is sometimes used as an optimization technique is because of the latter: the variable isn't allocated and deallocated every time a function is called. For example, if you were to initialize a very big array(not 32 cells) in a function that's called each frame you may use static to avoid zeroing the memory belonging to that variable in every single function call.

Something extra, if you are curious why static variables can retain their values: a binary file contains multiple segments. Usually, we have a text/code segment where the actual code is placed, a data segment where global/static variables are placed, a rodata segment where read-only(constants) are placed, etc. When the binary file is loaded these segments are mapped into memory so they can be used by the program.
When you use static the variable is placed inside the data segment. When you use "new" the variable is placed on the stack. By convention, once a function is done it has to restore the stack to its previous state, the state before the function was called. Therefore, local variables that the function placed on the stack will be "erased". The data segment doesn't work like that so anything that you put there can retain its value during the entire lifetime of the program if needed.
Also check this: https://forums.alliedmods.net/showpo...&postcount=292

[Shadows Adi]

Thank you!
After reading the post you recommended, it makes a lot of sense now about new / static variable declaration. I will update the code soon. Thank you for your time

[HamletEagle]

Another batch of changes:

1.

PHP Code:

public DetectSaveType() 


It doesn't need to be "public". "public" is needed if the function is going to be called from the outside of your plugin(for example amxx forwards like plugin_init, ham hooks, set_task callbacks, etc).

2.

PHP Code:

if(g_iScenario == KILLER_MVP_TERO && g_eMVPlayer[iTopKiller] == id || g_iScenario == KILLER_MVP_CT && g_eMVPlayer[iTopKiller] == id)
    {
        g_eMVPlayer[iTopKiller] = -1
    } 


can be shortened to:

PHP Code:

if(g_eMVPlayer[iTopKiller] == id && (g_iScenario == KILLER_MVP_TERO || g_iScenario == KILLER_MVP_CT)) 


It is easier to read and understand in this forum. && is distributive.

3.

PHP Code:

arrayset(g_iDamage[id], 0, charsmax(g_iDamage)) 


Should be sizeof, not charsmax. Your array is basically g_iDamage[33][2]. With charsmax(sizeof - 1) it's not going to reset the second cell(iHSDmg) for the last player.
You should make this change every place you use arrayset, not going to list all cases.

4. In RG_RestartRound_Post:

• static is not needed, use new
• iPlayer = iPlayers[i] is not really needed because you only use it once. You should avoid indexing when you would need to use iPlayers[i] more than once. Otherwise, it's the same thing: either index once when you do iPlayer = ... or index once directly inside arrayset.

5. Iniside logev_roundstart static is not needed.

6. Inside CalculateTopKiller:

PHP Code:

if(g_bIsBombDefused && g_eMVPlayer[iDefuser] || g_bIsBombPlanted && g_eMVPlayer[iPlanter]) 


Same as 2.
Don't use static.
The function doesn't need to be public.

7. ShowMVP, PlayTrack, LoadPlayerData, SavePlayerData do not need to be public. There are probably more functions, but you get the idea so I'm not going to list all of them.

8. Do not use "stock" inside a sma file, ever. "stock" should only be used inside include files so the compiler does not generate a warning if a function is not used. Anything inside a sma file should be used to stock is not useful.

9. Inside PlayTrack:

• Sarting at line 1027, it looks like you have a potential infinite loop. What happens if all your tracks are VIP-only tracks and the user is not a VIP? You will cycle forever. The proper way to do this is the following: if the user is not VIP, extract all non-VIP tracks in a temporary array. If the array has at least one element, select a random index. If the array is empty then you don't have any non VIP tracks, don't play anything.
• Don't use static.
• goto should be avoided in most cases, as it has the potential to make the code extremely hard to read or obfuscate the control flow(can't predict how the code will execute). It's okay to use goto to get out of multiple nested loops(forward jump), but please don't use it to jump backward(jump at line lower than the line where goto is) and simulate a loop. If you need a loop that runs until a condition is fulfilled use a while loop(potentially while(True) and break once the condition is met). This is just general advice, as you don't really need to use goto here, see the first point.

I think it's enough for now. If you don't understand or don't agree with something I said let me know.

[Shadows Adi]

Update
Version 2.4

• Added full support for AmxModX 1.8.2
• Improved PlayTrack() function
• Fixed a bug in loading settings
• Added support for changing name
• Improved code
• Thanks to HamletEagle

[HamletEagle]

1. In choose_track_handle, at line 920, you can replace the return MenuExit with "goto __EXIT". It's okay to use goto to avoid repeating the same cleanup/exit code, but use it in the entire function, not just some spots.

2. In Clcmd_ChooseTrack, static is not needed. Same in Clcmd_TrackList.

3. In CalculateTopKiller get_players is wrong. If you want to filter by team you should also specify "e" as the flag. Also, it should be "TERRORIST, not "T". I don't remember "T" being supported, but I may be wrong. Please try testing to make sure your code actually works.

4. For your natives, I suggest you add some more checking, to guard against people accidentally modifying the include file and passing the wrong number of arguments. I suggest checking iParamNum against the desired number of params and throw an error if something is not right.

5. Lastly, please try to name your functions in a consistent way. You have "CamelCase" and "snake_case" and "Whatever_ThisIs". Pick a style and stick with it. Any style is fine, just be consistent.

[Shadows Adi]

Update!
Version 2.5

• Fixed a bug when getting players from Terrorist team
• Fixed detection when player is changing his name
• Fixed a bug in showing menu ( it didn't show the MVP_VIP_ONLY in player's selected language
• Fixed a bug in PlayTrack() function
• Modified Load / Save Data code
• Modified functions name

[iclassdon]

Originally Posted by Shadows Adi In which way? The round in a deathmatch will never ends. You have no bomb scenario. If you can explain in which way, I will consider it. Thanks for feedback! Yea, maybe I should do in both ways, and the users can use what they want.

Only way I could think of would be to set a task for it to run and display MVP of the round every x amount of minutes. Also with an adjustable cvar for that timer. I'm sure someone here might have a better idea.

This setup would provide updates through out the map on who the current "MVP of the round/map" at that particular set time or interval. I run 20 min maps. So ideally would be nice to display twice somewhere in between and of course at the end.

I have tested the plugin and I love the it's features, but in my view not useful for DM servers with no rounds.

Nice plugin keep going!

[Shadows Adi]

Originally Posted by iclassdon Only way I could think of would be to set a task for it to run and display MVP of the round every x amount of minutes. Also with an adjustable cvar for that timer. I'm sure someone here might have a better idea. This setup would provide updates through out the map on who the current "MVP of the round/map" at that particular set time or interval. I run 20 min maps. So ideally would be nice to display twice somewhere in between and of course at the end. I have tested the plugin and I love the it's features, but in my view not useful for DM servers with no rounds. Nice plugin keep going!

Thanks for feedback!
I will think about a way that DM servers could use it, until then, this is the way it run.

[HamletEagle]

In your first post, I don't see any description or information about the custom sound tracks(how it works, commands that can be used, etc), please have a proper description.

About your code:

1. In plugin_precache:

In TRACKS_SECTION case you have a small bug. You set g_bNotOnlyVip to true before checking if the file actually exists. So you could end up in a scenario where g_bNotOnlyVipis true, but you don't actually have any (non VIP) tracks added because they were invalid. Set it to true after making sure the track is valid.

In SETTINGS_SECTION case, for SQL_HOSTNAME, SQL_USERNAME, etc you log an error if the value is missing, but then proceed to copy it anyway. Why? You are copying the empty string over the empty string, this doesn't seem useful. You could just do an else and copy only if the value is valid, but this raises another question: how do you handle the case when the SQL data is missing. What I would do is another check inside DetectSaveType. If at least one of the required information for SQL connection is empty then default to sqlite. Thing is, the current code will do this anyway because the connection will fail, but explicitly checking will save you a few connection attempts that are sure to fail.

Here, you also have a few variables declared inside the while loop, they should be declared just before the loop.

2. task_check_scenario is still a different style. Keeping with your convention, it should be Task_Check_Scenario. Same with choose_track_handle, clcmd_tracklist_handle and natives.

3. This is mainly a question, but in the menu, for selecting tracks you already block non VIP players from selecting VIP-only tracks. However, inside PlayTrack I see that you added checks to make sure the currently selected track(line 1346) is not a VIP-only track if the player is not VIP. The only way you could reach this scenario is if someone removes his VIP access during the game and does amx_reloadadmins.
Supporting this case is perfectly fine, I'm just curious if there is another reason for adding this check.

Other than that, everything appears to be in order. Fix these small issues and I'll approve this.