PDA

View Full Version : Code review: sgProtector



Higor
05-12-2012, 07:00 PM
Looking into sgProtector and sgSProtector's enemy finding methods.


ShouldAttack:

function bool ShouldAttack(Pawn enemy)
{
local UT_Invisibility inv;
local bool bInv;
local Inventory RubberSuit;

RubberSuit = enemy.FindInventoryType(class'WildcardsRubberSuit' );

if ( enemy == None )
return false;

if ( !FastTrace(enemy.Location) )
return false;

if ( ScriptedPawn(enemy) != None && enemy.Health > 10 )
return true;

if ( sgBuilding(enemy) != None )
{
if ( sgBuilding(enemy).Team == Team || sgBuilding(enemy).Energy < 0 )
return false;
}
else if ( enemy.PlayerReplicationInfo == None ||
enemy.PlayerReplicationInfo.Team == Team ||
enemy.Health <= 10 || RubberSuit != none ||
!enemy.bProjTarget)
return false;
return true;
}
This function is called within a RadiusActors iterator, so we don't need to check for (Enemy == none).
Second, we shouldn't try finding the rubbersuit until all the other checks have failed.
Third, we should use FastTrace as final positive check, so we use it the least amount of times.
We'll also remove some unused variables.

Suggested code:

function bool ShouldAttack(Pawn enemy)
{
if ( ScriptedPawn(enemy) != None )
{
if ( enemy.Health < 10 )
return false;
return FastTrace( enemy.Location);
}

if ( sgBuilding(enemy) != None )
{
if ( sgBuilding(enemy).Team == Team || sgBuilding(enemy).Energy < 0 )
return false;
}
else if ( enemy.PlayerReplicationInfo == None ||
enemy.PlayerReplicationInfo.Team == Team ||
enemy.Health <= 10 || !enemy.bProjTarget || (enemy.FindInventoryType(class'WildcardsRubberSuit ') != none) )
return false;
return FastTrace(enemy.Location);
}
No new variablies initialized, RubberSuit is found right if when need to find it and the trace checks are done as final condition.

EDIT 1:
FindEnemy:

function Pawn FindEnemy()
{
local Pawn p, target;

SightRadius = 1000 + 1750*Grade;

foreach RadiusActors(class'Pawn', p, SightRadius)
if ( ShouldAttack(p) && (target == None ||
VSize(p.Location - Location) < VSize(target.Location - Location)) )
target = p;

return target;
}
ShouldAttack requires more resources than the distance checks, we should invert these checks.
We shouldn't search further away from current enemy if we have one.

Suggested code:

function Pawn FindEnemy()
{
local Pawn p;
local float eDist;

//Should be applied during upgrade, does the function Upgraded() properly work?
// SightRadius = 1000 + 1750*Grade;

if ( Enemy != none )
{
eDist = VSize( Enemy.Location - Location);
if ( (eDist > SightRadius) || !ShouldAttack(Enemy) )
Enemy = none;
else
{
eDist -= 1;
foreach RadiusActors(class'Pawn', p, eDist)
if ( (VSize(p.Location - Location) < VSize(Enemy.Location - Location)) && ShouldAttack(p) )
Enemy = p;
return Enemy;
}
}

foreach RadiusActors(class'Pawn', p, SightRadius)
if ( (Enemy == None || VSize(p.Location - Location) < VSize(Enemy.Location - Location)) && ShouldAttack(p) )
Enemy = p;

return Enemy;
}
We store Enemy for future uses, if enemy is still valid, we try finding one at a closer distance.
Expensive checks placed last as well.

Other changes:
Add:


simulated function Upgraded()
{
SightRadius = 1000 + 1750*Grade;
}

And in the defaultproperties block add:


SightRadius = 1000;

Checking on sgSProtector:

function bool ShouldAttack(Pawn enemy)
{
return Super.ShouldAttack(enemy);
}

sgSProtector is subclass of sgProtector, you should remove this.

SAM
05-22-2012, 03:10 PM
I believe we lost some posts on this subject due to the erratic downtime which has now been resolved. Please feel free to discuss!

Feralidragon
05-23-2012, 11:28 AM
The only thing I think it's missing in that, is not about optimization, is simply about flexibility:

enemy.FindInventoryType(class'WildcardsRubberSuit' )
Could be:

enemy.FindInventoryType(RubberSuitClass)

where the RubberSuitClass would be a:

var() Class<Inventory> RubberSuitClass;

Just to avoid it to be hardcoded, which isn't very good (although admittedly I did it and still do it myself sometimes).
As for the rest, totally agree with the optimization. :D

Higor
05-23-2012, 02:53 PM
Another discussion point here:

From what can be seen in the Protector's behaviour is that they always pick the nearest enemy.
What if this nearest enemy is stored in a variable (enemy?), and for the following enemy checks, only check the distance to that enemy - 1.0 unit? Of course, never forgetting of checking if current enemy is still eligible.

This way if the sgProtector finds an enemy at distance 500, all following enemy-finding iterations must be performed with a radius of 499 units, after all, if no new enemy is found we keep the old enemy.

The rubbersuit check was already a hardcoded class, but indeed it would be good to have a DontShootIfCarrying variable for that inventory class.

Feralidragon
05-24-2012, 05:53 AM
From what can be seen in the Protector's behaviour is that they always pick the nearest enemy.
What if this nearest enemy is stored in a variable (enemy?), and for the following enemy checks, only check the distance to that enemy - 1.0 unit? Of course, never forgetting of checking if current enemy is still eligible.

This way if the sgProtector finds an enemy at distance 500, all following enemy-finding iterations must be performed with a radius of 499 units, after all, if no new enemy is found we keep the old enemy.
That's a very good idea, just don't forget 2 things:
1 - you need to make an extra fast trace to the current enemy before you do a smaller radius check, since he can go out of sight at any given moment;
2 - you need to check if that distance is within the max protector current range (since they vary with level).

Also, another very minor thing, but still quite important nonetheless, although I don't see any direct problems with that solution, don't forget that UT works with floating point variables, which means lack of precision. On a direct floating point context, 1unit is precise enough, but in a radial perspective (Pitagoras Theorem) the calculations become less precise and precision will be lost, so that 1 unit of difference may not be 1 exact unit at some checks and may still detect the enemy as well, or not, depending on the final value of the calculation at c++ level (which I believe uses floating point as well?).

Higor
05-24-2012, 01:57 PM
Edited first post, added another function rewrite.

PD: In case we indeed have a float precision error, the distance check should prevent ShouldAttack() being called twice on it, ugly but safe.
PD2: I notice sgBuilding has an Upgraded() method, SightRadius could be set to a default of 1000, and modified on each Upgraded() call (only needed serverside).

Check again, now Upgraded() sets sight radius.

.seVered.][
05-24-2012, 02:27 PM
function Shoot(Pawn target){
local vector fireSpot,
projStart;
local Projectile
proj;
local rotator shootDirection;


if ( target == None || VSize(target.Location - Location) > SightRadius )
return;


shootDirection = rotator(target.Location - Location);


PlaySound(FireSound, SLOT_None, 0.75);


if ( Grade > 2 )
{
fireSpot = target.Location + FMin(1, 1.1 + 0.6 * FRand()) *
target.Velocity * VSize(target.Location - Location) /
ProjectileType.default.Speed;


if ( !FastTrace(fireSpot, ProjStart) )
fireSpot = 0.5 * (fireSpot + target.Location);


shootDirection = Rotator(fireSpot - Location);
}


Spawn(ProjectileType, Self,, Location, shootDirection);
}

I Love how big the CodeBox is BTW (< thank you $carface)

Pythagorean theorem for calculating the point in 3d space is only half of it. Finding the fireSpot also requires "bearing". Does it base this on the direction you are facing when you build it, like the teleport is or is it the same for all SP's?


It appears that the SP is trying to anticipate where the target is "going" to be (fireSpot=). Even at level 5 the ProjectileType.default.Speed is not fast enough, as I can outrun even 2 SP's close together. (unless player velocity were actually adjusted according to health status ( less health, less speed) which it is not. So increase the ProjectileType.default.Speed or adjust the forumla.

The fast trace is done in the Shoot(), why is it done again in the ShouldAttack()?

The unused variables could have been used to decrease the accuracy of the Spawn(ProjectileType, Self,, Location, shootDirection) if the target had invisibility suit on, but was removed prior; and the bInv; variable might have also part of that. Perhaps this could be RE-inserted?


Why remove the subclass of sgSProtector?.

Higor
05-24-2012, 02:32 PM
It would appear that the FastTrace check was a sanity check left by the previous coder, we should see under what contexts is Shoot called, if under every circumstance this function is called, the target was already checked by a FastTrace, we can remove it from here.
Anticipation has a randomized effect here, I think it was intended, so I won't touch it.

EDIT: We're not removing sgSProtector, only a function that's not necessary.

.seVered.][
05-24-2012, 02:52 PM
Can we move these and other posts to their own Department, sort of create a wiki.Siege Thread collection? These posts could prove quite useful to future targets. It would be nice to have them ALL in one place.

|uK|B|aZe//.
05-24-2012, 03:21 PM
i personally dont wanna read all this

but as long as we can trust you and you have the intentions of doing good i dont see why we cannot use your help

also please just make sure siege runs optimally and not like a huge turd as it is now

im sure you will do a good job! thanks

.seVered.][
02-26-2013, 02:30 PM
Perhaps add Enemy.Health check in 'function Pawn FindEnemy()' of sgProtector.uc


function Pawn FindEnemy()
{
local Pawn p;
local float eDist;

//Should be applied during upgrade, does the function Upgraded() properly work?
// SightRadius = 1000 + 1750*Grade;

if ( Enemy != none )
{
eDist = VSize( Enemy.Location - Location);
if ( (eDist > SightRadius) || !ShouldAttack(Enemy) )
Enemy = none;
else
{
eDist -= 1;
foreach RadiusActors(class'Pawn', p, eDist)
if ( (VSize(p.Location - Location) < VSize(Enemy.Location - Location)) && ShouldAttack(p) )
Enemy = p;
if ( Enemy.Health <= 10 )
Enemy = none;
return Enemy;
}
}


Also the projectile should have a Health check in simulated function 'ProcessTouch' of sgProtProj.uc



else if ( Pawn(other) != None && Pawn(other).PlayerReplicationInfo != None && Pawn(other).PlayerReplicationInfo.Team == sgProtector(Owner).Team && Pawn(other).Health <= 10)
return;

Crsk
02-26-2013, 03:49 PM
Perhaps add Enemy.Health check in 'function Pawn FindEnemy()' of sgProtector.uc

The protector need to detect enemy activity 'function Pawn FindEnemy()' then decide if send projectiles or not 'function bool ShouldAttack(Pawn enemy)', the rest is projectiles work (visual effect) and the SiegeGI work (damage part).

I don't know if combining Protector functions will work but if you are trying to fix the Protector issue, i think is necesary modify the SiegeGI class adding new rules for the excepcional cases, the currently rule for the players is good i guess but maybe is needed start from there



class SiegeGI extends TeamGamePlus config(SiegeUltimateRC16);
.
.
.

if ( (DamageType == 'sgSpecial') && (Injured.Health - Damage <= 10) )
{
injured.Health = 10;
return 0;
}


In old siege versions 'injured.Health = 10' doesn't exist, in replacement they have 'Damage= 0' what is most realistic though but doesnt matter, nobody will see the chagnge but since what Siege version the protector bug is killing people? maybe this change is taking credit idk :s

][X][~FLuKE~][X][
02-26-2013, 04:47 PM
or it could be the player going around with the name SgProtector, saw him/her playing today.

Crsk
02-26-2013, 05:24 PM
[X][~FLuKE~][X][_{HoF};52719]or it could be the player going around with the name SgProtector, saw him/her playing today.

AHHAHA

personally I never see this bug before, I go by what the players say and I don't think this is what happens taking in consideration this bug history but Hahahah

terminator
02-26-2013, 05:35 PM
[X][~FLuKE~][X][_{HoF};52719']or it could be the player going around with the name SgProtector, saw him/her playing today.

<------- :troll:

|uK|B|aZe//.
02-27-2013, 09:49 AM
its been happening occasionally in every siege version ever made, neve happened to me weirdly enough. but maybe thats cos the protectors know im the BOSS

|uK|kenneth
02-27-2013, 01:43 PM
happend once to me.