User Tag List

Page 1 of 3 123 LastLast
Results 1 to 10 of 21
  1. #1
    Whicked Sick Higor's Avatar
    Join Date
    Apr 2012
    Location
    Full sail ahead!
    Posts
    3,676
    Country:

    Code review: Mines

    Hope this is the right section, it is discussion after all, in a technical way...

    Been talking to ChanClan about posting technical reviews, gonna start here.
    Optimizing code and reducing processing time, starting with mines.

    ====
    First of all, we have the mine Timer:
    Code:
    simulated event Timer()
    {
        	local Pawn p;
    	Super.Timer();
    	
    
    	if ( SCount > 0 || Role != ROLE_Authority )
            	return;
    
    		foreach RadiusActors(class'Pawn', p, 75)
    			if ( p.bIsPlayer && p.Health > 0 &&
            			p.PlayerReplicationInfo != None &&
              			p.PlayerReplicationInfo.Team != Team && !p.PlayerReplicationInfo.bIsSpectator || ScriptedPawn(p) != None )
    				if (p != None && !bDisabledByEMP) 
    					Damage();
    }
    There is also a RadiusActors iteration, this checks the entire actor list, besides, the iterator doesn't include a break so the Damage() function can be called multiple times lagging even more the server or making the mine deal over 500 damage on lower levels.
    Also, bDisabledByEMP is a global conditional, check it outside of the iterator and to sum up, ForEach iterators ALWAYS return a valid actor, otherwise the code isn't even called, so we don't need to check for (p != none).

    Suggested code:
    Code:
    simulated event Timer()
    {
        	local Pawn p;
    	Super.Timer();
    	
    
    	if ( SCount > 0 || bDisabledByEMP || Role != ROLE_Authority) //EMP check prevents iterations
            	return;
    
    	ForEach VisibleCollidingActors (class'Pawn', p, 75,,true) //This true makes the iterator ignore actors with bHidden=true property
    		if ( p.bIsPlayer && p.Health > 0 &&
           			p.PlayerReplicationInfo != None &&
           			p.PlayerReplicationInfo.Team != Team && !p.PlayerReplicationInfo.bIsSpectator || ScriptedPawn(p) != None )
    		{
    			Damage();
    			return; //Prevent multiple detonation on same frame
    		}
    }
    VisibleCollidingActors is faster, it checks the collision hash for colliding actors instead of the entire actor list, is faster when used with small radius (< 300).
    - It checks if the actors have a valid collision (players have, spectators don't so it works)
    - It checks if they are in sight so there is no need to call FastTrace on them
    - Checks visibility if specified (bHidden).


    Next function: Damage
    Code:
    function Damage()
    {
    	local int i;
        	local Pawn p;
    	local Pawn pAward;
    
        	local vector dir;
    	local int x;
    	local int y;
    	local int z;
    	local string sMessage;
    	local string sName;
    
    	Self.PlaySound(Sound'SharpExplosion',, 4.0);
    
    	pAward = GetAwardedPlayer();
    
            for ( i = 0; i <= Grade; i++ )
    	{
    		dir.x = Rand(80)-40;
    		dir.y = Rand(80)-40;
    		dir.z = Rand(80)-40;
    		spawn(class'MineExplosion',,,Location + dir); 
    		foreach RadiusActors(class'Pawn', p, 75+(i*25))
    			if (p != None) 
    				if (ShouldAttack(p))
    				{
            				dir = Location - p.Location;
            				dir = normal(dir); 
    					if (p.Health>0)
    					{
            					p.TakeDamage((Grade+1) * 15, instigator, 0.5 * (p.CollisionHeight + p.CollisionRadius)*dir, vect(0,0,0), 'mine');
    						sgPRI(pAward.PlayerReplicationInfo).AddRU((Grade+1) * 4);
    					}
    				}
    	}
    
    	sgPRI(pAward.PlayerReplicationInfo).AddRU(50);
    
            Destroy();
    
    }
    Ok, we have here: unneeded variables, slow iterators inside fast iterators and sets of functions that can be replaced with faster stuff.
    - Calling a function in unrealscript is VERY EXPENSIVE, unrealscript functions are 40 times slower than their c++ counterparts, so we should focus on reducing the unrealscript function calls even if some functionality is wasted...
    - Look how I generate a random dir: (dir = VRand() * 40), 3 functions total: equal, VRand() and *. If i do this without storing the variable, it will only be 2 functions total.
    - How it's done in the main code: (dir.x = Rand(80)-40; 3 times) 12 functions total: equal, dot (accessing X in vector struct), Rand(), - (minus), 3 times a row.
    - Second, no need to check (p != none) this always returns true here.
    PD: Why call SELF.Function if you can simply call function?

    Suggested code:
    Code:
    function Damage()
    {
    	local int i;
        	local Pawn p, pAward;
    
    	PlaySound(Sound'SharpExplosion',, 4.0);
    	pAward = GetAwardedPlayer();
    
    for ( i = 0; i <= Grade; i++ )
    {
    	Spawn (class'MineExplosion',,, Location + VRand() * 40);
    	ForEach VisibleCollidingActors (class'Pawn', p, 75+(i*25),,true)
    	{
    		if ( (p.Health > 0) && ShouldAttack(P) ) //Health check first, faster
    		{
    			p.TakeDamage((Grade+1) * 15, instigator, normal( Location - p.Location) * 0.5 * (p.CollisionHeight + p.CollisionRadius), vect(0,0,0), 'mine');
    				sgPRI(pAward.PlayerReplicationInfo).AddRU((Grade+1) * 4);
    		}
    	}
    }
    
    	sgPRI(pAward.PlayerReplicationInfo).AddRU(50);
    
            Destroy();
    }
    Here we call a collision hash iterator *grade* times, we also removed a lot of unused variables and reduced the amount of function calls.


    Next function: ShouldAttack
    Code:
    function bool ShouldAttack(Pawn enemy)
    {
        if ( ScriptedPawn(enemy) != None )
    		return true;
    	if ( enemy == None )
            return false;
        if ( !FastTrace(enemy.Location) )
            return false;
        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.bProjTarget )
            return false;
        return true;
    }
    This one is simple, we remove the most expensive function here: FastTrace.
    Why? VisibleCollidingActors already performs this check.
    We also remove the (enemy == none) check, seeing the context this function is called, enemy ALWAYS exists so this check will always return FALSE.

    Suggested code:
    Code:
    function bool ShouldAttack(Pawn enemy)
    {
        if ( ScriptedPawn(enemy) != None )
    		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.bProjTarget )
            return false;
        return true;
    }
    Supermine optimization in next post...
    Last edited by Higor; 05-12-2012 at 11:33 PM. Reason: Fixed typo, fixed Timer function

  2. #2
    Whicked Sick Higor's Avatar
    Join Date
    Apr 2012
    Location
    Full sail ahead!
    Posts
    3,676
    Country:

    Supermine optimization

    Now we deal with the SuperMine subclass:

    Function: Timer
    We replace the timer as well, with the difference that radius is greater and that supermine is allowed to detonate multiple times.
    For that we remove the break inside the iterator.
    Suggested code:
    Code:
    event Timer()
    {
        	local Pawn p;
    	Super.Timer();
    	
    
    	if ( SCount > 0 || bDisabledByEMP) //EMP check prevents iterations
            	return;
    
    	ForEach VisibleCollidingActors (class'Pawn', p, 100,,true) //This true makes the iterator ignore actors with bHidden=true property
    		if ( p.bIsPlayer && p.Health > 0 &&
           			p.PlayerReplicationInfo != None &&
           			p.PlayerReplicationInfo.Team != Team && !p.PlayerReplicationInfo.bIsSpectator ) //Original supermine didn't check for monsters, so we do the same?
    			Damage();
    }

    Function: Damage
    Same logic here, we just modify it with the supermine parameters.
    Code:
    function Damage()
    {
    	local int i;
        	local Pawn p, pAward;
    
    	PlaySound(Sound'SharpExplosion',, 4.0);
    	pAward = GetAwardedPlayer();
    
    for ( i = 0; i <= Grade; i++ )
    {
    	Spawn (class'MineExplosion',,, Location + VRand() * 40);
    	ForEach VisibleCollidingActors (class'Pawn', p, 100+(i*35),,true)
    	{
    		if ( (p.Health > 0) && ShouldAttack(P) ) //Health check first, faster
    		{
    			p.TakeDamage((Grade+1) * 15, instigator, normal( Location - p.Location) * 0.5 * (p.CollisionHeight + p.CollisionRadius), vect(0,0,0), 'supermine');
    			sgPRI(pAward.PlayerReplicationInfo).AddRU((Grade+1) * 4);
    		}
    	}
    }
    
    	sgPRI(pAward.PlayerReplicationInfo).AddRU(50);
    
    	Energy -= 200;
    	if ( Energy <= 0 )
    	        Destruct();
    }
    Since we use the new iterators here, the ShouldAttack function defined in Mine will work as well here.

    The code does EXACTLY THE SAME as before, except that it solves a rare case of normal mines detonating multiple times if more than one player approached it at the same time, not to mention the reduced overhead in the timer and the detonation.

  3. #3
    Administrator SAM's Avatar
    Join Date
    Jan 2011
    Posts
    8,296
    Country:
    Thanks very much for taking your time to make such an informative post here Higor. I do believe it is SW's intention to optimise Siege however his time constraints with University is proving a challenge.

    I'm sure he will take your comments into account. Please continue posting. I'd love to see just where siege is going wrong and I already know it is all the timer functions as well as actors...

  4. #4
    Whicked Sick SilverWing's Avatar
    Join Date
    Jan 2011
    Posts
    1,140
    Country:
    Wow... kk... if u got anymore fixes plz post... and ill add them in if it works...

    Im deving Siege right now ( hopefully not for long xD)... and im only able to fix few bugs here and there... so any help i can get would be amazing

    do u have xfire/msn ?
    Last edited by SilverWing; 05-11-2012 at 06:15 PM.


  5. #5
    Unstoppable audiosonic's Avatar
    Join Date
    Jan 2011
    Posts
    734
    Country:
    Quote Originally Posted by |uK|SilverWing View Post
    so any help i can get would be amazing
    UNIVERSITY MATERIAL RIGHT THERE
    Last edited by SilverWing; 05-11-2012 at 06:15 PM.

  6. #6
    Whicked Sick SilverWing's Avatar
    Join Date
    Jan 2011
    Posts
    1,140
    Country:
    FIXED


  7. #7
    Dominating |uK|Cube's Avatar
    Join Date
    Jan 2011
    Location
    Netherlands
    Posts
    527
    Country:
    Things like this u should pm me about (SW)
    Last edited by SilverWing; 05-11-2012 at 06:57 PM.

  8. #8
    The Big Ticket Moskva's Avatar
    Join Date
    Feb 2011
    Location
    Siege Hall of Fame
    Posts
    2,347
    Country:
    Things like this u should pm me about(SW)

    DONT EDIT MY POSTS SILVERPRICK
    Last edited by Moskva; 05-12-2012 at 12:50 AM.
    Season ranks: MLSG - #45 Semis, MLSG II - #38 Finals, MLSG III - #34 Finals, GU.Siege - #32 5th Place, IST - #11 CHAMPION

  9. #9
    Moderator .seVered.]['s Avatar
    Join Date
    Jun 2011
    Location
    Near a River and Under a Bridge
    Posts
    2,125
    Country:
    Please, let keep these posts semi-public... some of us would like to see these discussion's about the code.

  10. #10
    Rampage Feralidragon's Avatar
    Join Date
    Jan 2011
    Posts
    374
    Country:
    Great review Higor, but I want to point a very few things up as part of this discussion:


    Quote Originally Posted by Higor View Post
    'Simulated' keyword to prevent the event from even being processed and removing the conditional (Role != ROLE_Authority) since removing 'Simulated' already makes this one useless (returns TRUE always on server).
    In this case you're absolutely right, but for the sake of SW or other devs not starting to modify that wherever they see this kind of code:
    - if your function "Supers" it's counterpart in the superclass (not the case here), keep it simulated and with that check, because if the upper super function needs to be simulated, making it not simulated in your subclass will break the "simulation" chain (since non-simulated only call functions server side, no matter if they are simulated or not, there are just a few exceptions to this rule though).
    Only if you're absolutely sure that "Super." doesn't call any simulated function in the chain, then you can discard the "simulated" and the check.


    Quote Originally Posted by Higor View Post
    VisibleCollidingActors is faster, it checks the collision hash for colliding actors instead of the entire actor list, is faster when used with small radius (< 300).
    I believe that limit is way higher, up to 2000 VCA is faster than RA, from there RA is faster. But that's debatable.


    Quote Originally Posted by Higor View Post
    - Calling a function in unrealscript is VERY EXPENSIVE, unrealscript functions are 40 times slower than their c++ counterparts, so we should focus on reducing the unrealscript function calls even if some functionality is wasted...
    Sorry, but I cannot agree with that at all. Yes, function calls shouldn't be excessively used (the time taken as to do with the needed instructions to even initialize it, specially with arguments and returns, thus is one of the slowest instructions you can give), however avoiding them at all costs is not the solution either and I think that's an over-dramatization of it.

    If we go that way we start to have redundant code, which is very bad programming practice, for example:
    Code:
    a = b + c;
    If that is only once or at max twice in your code, just do it that way, but if you use the exact same expression over and over (let's say 5x or more), the best thing to do is:
    Code:
    function doMyThang(){
          a = b + c;
    }
    And call doMyThang() where needed. It has the overhead of the function call, and yes it would be faster to just repeat "a = b + c;", but that's plain wrong in good programming practices, as if you have to update the algorithm, good luck, if you have a problem related to it, good luck again.

    So here's the summary:
    - don't use functions if there's already something that does the same thing (like Higor rightfully pointed out);
    - however, every time you have a repeatable pattern of instructions, don't just copy and paste them to not create functions, create a function for them and reuse that function whenever it's needed (and if your function becomes too big, it's also acceptable and sometimes encouraged to split it into smaller functions as smaller steps of the same algorithm).

    Also, 40x slower than C++? That's nothing to worry about, C++ is absurdly fast, and languages like javascript and php are way slower than uScript specially since they're interpreted languages (whereas uscript is byte code like java), and I sure you what you will find there the most are functions, classes and instances (way heavier than functions), in the case of javascript you can even store functions in variables, yet your browser runs them just fine, servers use it, etc, and php for instance was made in C++ and, again, is interpreted (much worse than byte-code in most occasions).
    It's not function calls by themselves that will endanger your code into running considerably slower, there are way more weighting problems for drops in performance such as:
    - Usage of heavy functions (not the call itself);
    - Instantiation of classes (objects): spawns and "new something"
    - Graphical processing (which in UT is mostly done in CPU and not GPU);
    - Squared and cubic algorithms (cicles within cicles).


    Also, there's a better way than that "VisibleCollidingActors" of making the awareness of the mines if we are heading for absolute optimization: in my mod (purely as a practical example), I spawn a custom built "trigger" actor that once touched, it triggers the mine.
    Why spawn a seperate trigger and not use the mine itself? For 2 reasons:
    - Mine collision to be destroyed (which differs from the sensing);
    - Translocator doesn't like any actor other than triggers, inventory and a few others (as for it "touching" other actors is the same as hitting a wall, not sure why they did this, but it's the way it is).

    That way instead of relying in a timed iterator, you use once-triggered events, if they aren't triggered, they're just occupying memory space and not processing time.

    Just my 2 cents, as for everything else, really nice and I agree.

Thread Information

Users Browsing this Thread

There are currently 2 users browsing this thread. (0 members and 2 guests)

Similar Threads

  1. Code review: Mines 2
    By Higor in forum Code Reviews
    Replies: 6
    Last Post: 07-18-2012, 04:39 PM
  2. Code review: Jetpack
    By Higor in forum #siegepug Discussion
    Replies: 7
    Last Post: 06-01-2012, 04:19 PM
  3. Code review: sgHUD
    By Higor in forum #siegepug Discussion
    Replies: 6
    Last Post: 06-01-2012, 02:45 PM
  4. Code review: Mines 2
    By Higor in forum #siegepug Discussion
    Replies: 5
    Last Post: 05-26-2012, 02:49 PM
  5. Code review: Mines
    By Higor in forum #siegepug Discussion
    Replies: 20
    Last Post: 05-21-2012, 09:36 PM

Tags for this Thread

Posting Permissions

  • You may not post new threads
  • You may not post replies
  • You may not post attachments
  • You may not edit your posts
  •