[meng]

heres a repeating timer callback. i want it to be as efficient as possible. so i can use it frequently and not worry about adding more checks. heres how it is now.

PHP Code:

// global
new Float:p_Position[28][3];
new p_Team[28];

public Action:RadiusCheck(Handle:timer, any:client)
{
    if (IsClientInGame(client))
    {
        decl Float:tempPlayerPosition[3];
        for (new i = 1; i <= 28 ; i++) // MaxClients
        {
            if (i != client && IsClientInGame(i) && IsPlayerAlive(i) && GetClientTeam(i) == p_Team[client])
            {
                GetClientAbsOrigin(i, tempPlayerPostion);
                if (GetVectorDistance(p_Position[client], tempPlayerPostion) < 512.0)
                {
                    // DO STUFF
                }
            }
        }
    }
    return Plugin_Continue;
} 


after reading the wiki, im thinking something like this.

PHP Code:

// global
new Float:p_Position[28][3];
new p_Team[28];
new Float:tempPlayerPosition[3];
new Float:clientPosition[3];
new clientteam;

public Action:RadiusCheck(Handle:timer, any:client)
{
    if (IsClientInGame(client))
    {
        clientPosition = p_Position[client];
        clientteam = p_Team[client];
        for (new i = 1; i <= 28 ; i++) // MaxClients
        {
            if (i != client && IsClientInGame(i) && IsPlayerAlive(i) && GetClientTeam(i) == clientteam)
            {
                GetClientAbsOrigin(i, tempPlayerPostion);
                if (GetVectorDistance(clientPosition, tempPlayerPostion) < 512.0)
                {
                    // DO STUFF
                }
            }
        }
    }
    return Plugin_Continue;
} 


do any of you gurus disagree?
1. if a variable is sure to be used like hell. can i benefit from storing it globally? such as tempPlayerPosition and clientteam in this example.
2. do i benefit by declaring the array values instead of using the array indexes inside the loop?

[Theme97]

I'm not sure about #1, but if you do end up using local variables, be sure to use decl instead of new.

As for #2, are you using them multiple times in the loop or just once in that comparison? If the former, then yes, you would benefit. If the latter, then no; in fact, you're even hurting performance by taking the extra step to store it in a variable. The purpose behind storing an array value in another variable is to minimize the amount of times you do something like array[key] repeatedly. In both cases, you do that exactly once, but in one, you store it in a variable and in the other you use it directly.

Also, I'm not sure if this is actually beneficial, but I've done stuff like

PHP Code:

if (GetVectorDistance(clientPosition, TempPlayerPostion, true) < 262144.0) { 


and I'm curious if it's really more efficient. Benchmarks would be appreciated.

[Frus]

Why are you declaring p_Team as a float?

If I'm not mistaken, there is no reason why you couldn't use an integer/cell.

It's very minor, but will probably cause tag mismatch warnings if you keep it as-is.

[Kevin_b_er]

I disagree on #1 being a necessarily good thing. However, decl local arrays so they doesn't get initialized twice.

GetVectorDistance would probably greatly benefit from using the squared distance as it means it does not need to run a square root calculation on the result. This will save up to 28 square root calculations per timer call.

Also of note, this will iterate over the client itself. Do you want to "DO STUFF" to the client too? Because they'll likely be within 512 units of themselves.

[meng]

oops, didnt mean mean to decl p_team as float. and i definitely need to check if "i" is the client, thx!!! updated first post.

its a good possibility i didnt quite grasp the wiki article .
so if i only use the array index ONCE in the loop, then no need to declare it beforehand. works for me!

as far as GetVectorDistance goes, im a bit confused. the default behavior is NOT to square it. but it uses the phrase "for optimization" in the api . why make it do any extra math?

[Frus]

The result of a vector distance calculation is squared at the second to last step, as a result of the math (think pythagoras, c2 = a2 + b2). To get the correct distance, the squared distance needs to be 'square-rooted'. But it is more efficient to skip the square root, and compare 2 squared distances.

[Kevin_b_er]

Then we can pull something like this, with two loops which skip over the client. We'll save a tiny iota of time. However, above all else, using squared with getvectordistance will probably save the most time.

Since IsClientInGame(client) returns true as a precondition to doing anything, we also know the client number is sane. If client == MaxClients, things should still work.

PHP Code:

// global
new Float:p_Position[28][3];
new p_Team[28];

public Action:RadiusCheck(Handle:timer, any:client)
{
    if (IsClientInGame(client))
    {
        decl Float:tempPlayerPosition[3];
        new i;
        new clientTeam = p_Team[client];
        decl Float:clientPosition[3];
        
        for (i = 1; i < client; i++)
        {
            if (IsClientInGame(i) && IsPlayerAlive(i) && GetClientTeam(i) == clientTeam)
            {
                GetClientAbsOrigin(i, tempPlayerPostion);
                if (GetVectorDistance(clientPosition, tempPlayerPostion, true) < 262144.0)
                {
                    // DO STUFF
                }
            }
        }
        // skip client
        for( i = client + 1; i <= MaxClients; i++ )
        {
            if (IsClientInGame(i) && IsPlayerAlive(i) && GetClientTeam(i) == clientTeam)
            {
                GetClientAbsOrigin(i, tempPlayerPostion);
                if (GetVectorDistance(clientPosition, tempPlayerPostion, true) < 262144.0)
                {
                    // DO STUFF
                }
            }
        }
    }
    return Plugin_Continue;
} 


[BAILOPAN]

I question the value of literally doubling code complexity for a micro-optimization that probably amounts to a small handful of instructions. You can achieve the same thing by nesting loops. The upper bound needs to be read from a variable so it's reasonable to set it and jump back again (no gotos, thus outer loop needed).

Regardless, unless this code is firing 80 times a frame, doing crazy things with the control flow to save an |if| is pointless. Measure, then optimize, don't assume. We have a profiler, and if it's not working the way people want, we can improve it

[naris]

You should use local variables instead of global variables. global variables aren't any faster than local variables.

The reason for saving the values of p_Position[client] and p_Team[client] is so the array index is only calculated once instead of another time for each client. However, they should be stashed in local variables, not globals.

//Also, don't do like the Microsoft C compiler once did and "optimize" incrementing the loop variable (i) outside of the loop

[naris]

Originally Posted by meng 2. do i benefit by declaring the constants instead of using the array indexes inside the loop?

Declaring constants is a common practice to improve readability and maintainability. There is no performance benefit.

However, if you check if something is < 512.0 in 25 places in the code and then find you need to change it to 1024.0, it is easier to change 1 constant then 25 lines with 512.0.

Also, you can use a more meaningful name like EFFECTIVE_DISTANCE or something similar. This becomes even more useful if there are 2 different kinds of distance checks that start out at 512.0, but only 1 kind needs to be changed.

//Actually, I'm not sure which constant you are referring to. I was discussing the 512.0, but perhaps you meant the 28?