Code review: Supplier ammo-giving optimization.

[Higor]

This one reduces the processing time of the suppliers when giving ammo and upgrading armor.
It still does not regenerate thrown weapons, that can be added at the expense of losing speed.

First thing first, we need an extra variable in the class sgSupplier:

var int iWeaponClasses;

This adds a lot of flexibility in code by reducing iteration count if a weapon class fails to load (mods?).


Now, when loading the weapon classes, we add a some new code after the function:

static function LoadWeaponClasses()
{
local int i;
local class<Weapon> weaponClass;

default.ClassesLoaded = true;
for ( i = 0; i < 9; i++ )
if ( default.Weapons[i] != "" )
default.WeaponClasses[i] =
class<Weapon>(DynamicLoadObject(default.Weapons[i],
class'Class'));

//Keep array compacted, faster if weapon fails to load
default.iWeaponClasses = 9;
for ( i=0 ; i<default.iWeaponClasses ; i++ )
if ( default.WeaponClasses[i] == none )
{
default.WeaponClasses[i] = default.WeaponClasses[--default.iWeaponClasses];
default.WeaponClasses[default.iWeaponClasses] = none;
--i; //Loop this element again
}
}


That's part of the process of keeping the weapon class table compacted and with a ceiling index (weapon class count), it sorta makes it work like a dynamic array.


Now we go to the main Supply function:

function Supply(Pawn target)
{

local int numWeapons, i, j;
local Inventory inv;
local sgArmor theArmor;

if ( bDisabledByEMP )
return;

if ( !default.ClassesLoaded )
LoadWeaponClasses();

numWeapons = min(GetWeaponCount(), default.iWeaponClasses);
for ( inv=target.Inventory ; inv!=none ; inv=inv.Inventory )
{
if ( (theArmor == none) && (sgArmor(Inv) != none) )
{
theArmor = sgArmor(Inv);
continue;
}

if ( (Weapon(Inv) == none) || (Weapon(Inv).AmmoType == none) )
continue;

for ( i=0 ; i<numWeapons ; i++ )
{
if ( Inv.class == default.WeaponClasses[i] )
{
j++;
if ( Weapon(inv).AmmoType.AmmoAmount < Weapon(inv).PickupAmmoCount / 2 )
Weapon(inv).AmmoType.AmmoAmount = Weapon(inv).PickupAmmoCount / 2;
else if ( FRand() < float(Weapon(inv).AmmoType.MaxAmmo) / 400 )
Weapon(inv).AmmoType.AmmoAmount = FMin(
Weapon(inv).AmmoType.AmmoAmount + 1 +
int(FRand()*Grade/2*EquipRate),
Weapon(inv).AmmoType.MaxAmmo);
if ( j >= numWeapons )
Goto WEAPONS_READY;
break;
}
}
}

WEAPONS_READY:
if ( theArmor != none )
Goto UPGRADE_ARMOR;

while ( inv != none )
{
if ( sgArmor(Inv) != none )
{
theArmor = sgArmor(Inv);
Goto UPGRADE_ARMOR;
}
inv = inv.Inventory;
}

theArmor = Spawn( class'sgArmor');
if ( theArmor == none )
Goto PLAY_SOUND;
theArmor.GiveTo( Target);

UPGRADE_ARMOR:
if ( FRand() < 0.1 + (Grade/15) && inv.Charge < 25 + Grade*25 )
++theArmor.Charge;


PLAY_SOUND:
if ( FRand() < 0.2 )
target.PlaySound(sound'sgMedia.sgStockUp', SLOT_Misc,
target.SoundDampening*2.5);
}

Code here can result illegible to some, it contains jumps to ensure maximum speed, it's also very compacted and adding an extra item to upgrade would require a reformulation of some loops.

Explanation step by step:
- Instead of looping the entire Inventory chain for each weapon/armor, we simply go through the Inventory chain once and start making our weapon checks there.
If we happen to catch the armor we also store it...
Now we check in the first loop if it's a valid suppliable weapon, if it is, we start checking among our weapon table (limited by Supplier level), if weapon is in the table, we add ammo and count J up.
If the J counter reaches the maximum weapons to supply (depends on Supplier level), we immediately leave the main loop and the subloop.
Notice that when we leave both loops, INV variable doesn't change or anything, we're still in the middle of the inventory chain...

- We left the loop, either by having it finished (entire inventory chain) or by jumping out into WEAPONS_READY, let's check if we got the armor during these iterations...
We got it!, skip some code by jumping to UPGRADE_ARMOR.
We didn't get it... remember INV? We continue looping through the Inventor chain in a much simpler loop meant to find this armor.
If we found the armor, we instantly jump to UPGRADE_ARMOR.
If we didn't, we create the armor, if armor fails to be created, we jump to PLAY_SOUND.

- We reach UPGRADE_ARMOR, either by finding it or by making it.
The code difference here? There's already a maximum armor check on the conditional, so using the ++ preoperator to add one is entirely the same and much faster than the previous method.

- Code keeps going, we now reach PLAY_SOUND.
This is the same as before, PLAY_SOUND is added here to skip armor upgrade if we fail to create it.


=======
Super supplier has identical code for both functions, replace them as well, add the extra variable and you're set.


Trivia:
Did you know this works in unrealscript?


if ( aNumber == 6 )
Goto DO_SOMETHING;

if ( false ) //Never enter this code chunk by normal means
{
DO_SOMETHING:
TestFunction();
}


or



if ( aNumber == 6 )
Goto AFTER_FUNCTION;

return;

AFTER_FUNCTION:
TestFunction();
}

[.seVered.][]

if ( aNumber == 6 )
Goto DO_SOMETHING;

if ( false ) //Never enter this code chunk by normal means
{
DO_SOMETHING:
TestFunction();
}


or



if ( aNumber == 6 )
Goto AFTER_FUNCTION;

return;

AFTER_FUNCTION:
TestFunction();
}


Damn, that ALMOST looks like visual basic... I should just use my VB editor for uScript. What about intrinsic subroutine's? (GOSUB). What other c++ routine's can you apply to the NukeLauncher routine's, perhaps a DODGE while your flying the nuke... :encouragement:

I already saw your post about the the nested dynamic array ...nice and clean, except your comment is not INDENTED :emmersed: (*wink)


static function LoadWeaponClasses()
{
local int i;
local class<Weapon> weaponClass;

default.ClassesLoaded = true;
for ( i = 0; i < 9; i++ )
if ( default.Weapons[i] != "" )
default.WeaponClasses[i] =
class<Weapon>(DynamicLoadObject(default.Weapons[i],
class'Class'));

-> //Keep array compacted, faster if weapon fails to load <--
default.iWeaponClasses = 9;
for ( i=0 ; i<default.iWeaponClasses ; i++ )
if ( default.WeaponClasses[i] == none )
{
default.WeaponClasses[i] = default.WeaponClasses[--default.iWeaponClasses];
default.WeaponClasses[default.iWeaponClasses] = none;
--i; //Loop this element again
}
}


Your a lawyer right... seem's like that should have been your FALLBACK career, instead of your primary. Your quite good at this ... stuff.

[Feralidragon]

Oh noes.... I asked this in the other topic, but looking at this one it seems you're actually serious in using goto's...

Let me explain the following: goto's are BAD. Period. Don't use them, don't build algorithms with them, as you stated it makes the code confusing (unnecessarily I must add), which leads inevitably to VERY UNSTABLE code (you can argue back saying your code "works" with these goto's, but it's not correct anyway as the code looses structure and readability later, which is what enhances our human nature of making mistakes while building something, thus by loosing structure you loose stability). You can build exactly the same algorithms in a much more simpler way and have the same speed (sometimes a bit slower, others a bit faster, but always along the same) using proper algorithmic ways using simple "if", "switch", "while", "for", etc.

In uscript in particular, goto should only be used in state code for "latent code" only (since "while" and "for" do not support at all latent instructions within them in uscript), and that's the only place you see goto's used, and even there their usage is as brief as possible.

So, if you're going to seriously reviewing and show SW and others new algorithms to optimize things up in Siege and potentially other things, PLEASE don't use nor promote the usage of goto's (if you want to keep using them yourself, fine, but don't make it an example to follow), that's just a bad algorithmic example (and this isn't just a matter of opinion, just put "why goto is bad" in google and you have tons of places explaining why).

[.seVered.][]

That's what I said, what about the intrinsic subroutine's?


[;28326']Damn, that ALMOST looks like visual basic... I should just use my VB editor for uScript. What about intrinsic subroutine's? (GOSUB). What other c++ routine's can you apply to the NukeLauncher routine's, perhaps a DODGE while your flying the nuke... :encouragement:

When you "jump" out of the ForNext loop using a gosub (aka Function), the stack pointers are maintained and a new stack does not need to be initialized, reducing overhead and maintaining stability (there are memory restrictions and overhead costs to stacks, aren't there?).

[Higor]

Readability depends on what the programmer feels more confortable with after all, I use both gotos and For, While loops.
Since optimization was the goal, I simply chose gotos, if SilverWing or any other Siege developer doesn't want these gotos (maybe they want to implement more into the supplier), I'll reformulate the functions, excepting for that first goto that leads out of a nested loop.

[Feralidragon]

Readability depends on what the programmer feels more confortable with after all, I use both gotos and For, While loops.
Nope, you're mistaken on what "readability" is.
Readability is defined from the standard and generic point of view, when you say something is readable, then it means anyone should be able to read it (in case of programming, anyone with minimal knowledge should be able to read it perfectly fine).
It doesn't matter what you're more comfortable with personally, that doesn't necessarily make it "readable" at all.
Example:

_ = (0x0012 + \027 + (~15 + 1)) << 1;

What just did I do there? Is it "readable"? Let's say that's how I code (obviously not, but just for exemplification).
For me is perfectly readable because I built it with everything in my mind and since I was comfortable with it, for you, perhaps you're not used to that kind of notation but you will still understand it nonetheless, but it will make you think slightly rather than interpret it right away, to others they may or not even figure out what I just put there and why when there are more simple and correct approaches.

In resume (for the ones that don't now what I did there): "_" is an integer var, but let's substitute it for "_var" for readability:

_var = (3 + 23 - 15) * 2;
Now it's "readable".



Since optimization was the goal, I simply chose gotos, if SilverWing or any other Siege developer doesn't want these gotos (maybe they want to implement more into the supplier), I'll reformulate the functions, excepting for that first goto that leads out of a nested loop.
You're keeping the "goto"...? You know, you can achieve exactly the same objective without goto's without loosing performance, "while" and "for" are your best friends for it and they exist for the sole purpose of not having to use goto's for cycles and such (that's why there's even a "do while"), plus breaking out from a "for" loop without being with a "return" is not good, that's what "while" is for (although I must admit I slipped in that mistake myself in some rare occasions).

Anyway, goto's aren't the holy grail of code optimization, using goto's as "max performance" excuse, well, that's really nothing but an excuse to not write what should be good and clean code, they're bad as they can be overused and make the code unreadable, buggy and unnecessarily complex (and you've already shown you abuse them quite a bit, 4 goto's in a single function....), and they're often a sign of bad written algorithms.
If you reformulate the code without goto's, you will see that you will make a better and cleaner algorithm, which is actually the correct thing to do, but if you want still to use them and shove it down SW if he accepts it as it is, well, sorry but you're serving a very bad coding example for a potential future developer (or developers) and I think you should seriously rethink what you're posting up.
Personally, I'm already happy you will take out most of them, but you should really take *all* of them out.

From there, if you use them personally, no one has anything to do with it but you, and I have a few bad habits myself as well which I am seriously working out and trying to correct them (and the ones I corrected so far made me a much better programmer than I was months or years ago, "good practices" sure make a lot of difference), but when I teach or show up something for people to understand, I make always sure I show the correct thing to do, and not necessarily how I do it personally.

[terminator]

maybe outta thread but please if u know how to, fix PLATFORMS! Thanks :))

[|uK|Grimreaper]

wtf since when does higor know coding XD

AND FIX THE PLATFORM BUG AFTER IT GOT REMOVED

[Higor]

I haven't been around for much to know what happens with platforms... mind telling me what's wrong with it?

[|uK|Grimreaper]

when a platform is build and it gets removed the spot where the platform was is buggy you lag trough it try it out on the server

[][X][~FLuKE~][X][]

its basically like the platform is removed/destroyed but the collision is still there.

[Feralidragon]

Just checked the code, the problem is obvious.
The platform spawns an hidden collision (because as platform is a pawn, thus it cannot serve as platform as you would keep jumping on it instead of staying), then it uses a simulated non-native event to destroy it (-> unreliable way to destroy actors on client).

Plus, given that the ICH is bNetTemporary=False, it cannot be destroyed in the client, and when you forcefully try (like it is happening now), once the server destroys it, odd things may happen client-side.

Use the native destroyed event to destroy the ICH together with the platform, take out the reference from the ICH to the platform (only the reference platform->ICH is needed) and do it server-side only, and problem solved.

[|uK|Grimreaper]

whatever he said ^

[SAM]

Grim can you stop posting useless spammy posts in constructive discussions like this please?

[|uK|Grimreaper]

huh what i was one of the few people that asked about this bug

[Higor]

Yeah, the platform should be a normal, replicated actor.
That means extra bandwidth but maybe there's another way.

Feralidragon, there's one thing I've been wondering for a long time and never decided to try it out:
On clientside replicated actors, do their PostBeginPlay(), PostNetBeginPlay(), and Destroyed() get called every time they enter relevancy or does that happen once?

If it happens everytime, we could make the platform collision appear in a totally clientside fashion and serve the same purpose the non-replicated collision serverside, then destroy it just like the graphic effects.
As a matter of fact, if the events don't happen, we can just make an extra graphic effect that works as collision so we don't have to rewrite a lot of code.
DELETED TEXT: Stupid statement.

PD: Was the forum domain name hijacked for the last days or two? The site was showing something completely different for me a few hours ago.


EDIT:
I noticed that due to this extra collision, the platform can't take non-explosive damage from above.
I suggest using a workaround so that the platform itself doesn't receive damage, while the collision (which should be bigger both above and below than the actual platform) forwards TakeDamage calls to the platform.

[SAM]

PD: Was the forum domain name hijacked for the last days or two? The site was showing something completely different for me a few hours ago.

No it wasn't. I pointed the NameServers to a wrong IP /amateur mistake.

[Feralidragon]

Feralidragon, there's one thing I've been wondering for a long time and never decided to try it out:
On clientside replicated actors, do their PostBeginPlay(), PostNetBeginPlay(), and Destroyed() get called every time they enter relevancy or does that happen once?
It only happens once.



If it happens everytime, we could make the platform collision appear in a totally clientside fashion and serve the same purpose the non-replicated collision serverside, then destroy it just like the graphic effects.
As a matter of fact, if the events don't happen, we can just make an extra graphic effect that works as collision so we don't have to rewrite a lot of code.
Yes, as long the building runs simulated functions/events, it could just make a Level.NetMode != NM_DedicatedServer and spawn the effects.
Personally I did that a lot, the only reason to not do it is when the spawn context of an effect happens server-side (well, but even for that we can define replicated functions, but most of the times just spawning the effect in the server isn't problematic as long as the actual processing and variables doesn't get replicated as well, which can occupy a lot more bandwidth over time than the spawn alone).



I noticed that due to this extra collision, the platform can't take non-explosive damage from above.
I suggest using a workaround so that the platform itself doesn't receive damage, while the collision (which should be bigger both above and below than the actual platform) forwards TakeDamage calls to the platform.
Yes, that would be the best approach (actually that's pretty much the "always-correct" kind of approach for anything using an external collision hull).

[.seVered.][]

Couldn't we make the regular supplier supply everyone standing in it?

[|uK|B|aZe//.]

what would be the point of having a super sup then