PDA

View Full Version : Code review: sgTeleporter



Higor
05-24-2012, 03:27 PM
Checking on teleporters now.

First, we start by removing unneeded functions:


simulated function Actor SpecialHandling(Pawn Other)
SpecialHandling is an event that only gets called on NavigationPoint subclasses, it does absolutely nothing here.


simulated function FindTriggerPawn()
This is called by SpecialHandling only, unneeded as well.


simulated function Upgraded()
{
Super.Upgraded();
}
There's absolutely no need to do this.

====
Going to the optimization now.
Finding the teleporter destination shouldn't be performed on every touch, storing it and handling it with user made events is better.
We'll add 2 variables with different purposes:

var bool bHasOtherSide;
var sgTeleporter OtherSide;
OtherSide will be the shortcut to the destination, the destination will have one pointing here as well.
bHasOtherSide is going to be a replicated variable so we must add it to the REPLICATION block, this will work to make clients know when a teleporter is active or not, still needs proper graphical implementation, have you guys decided what to use?


FinishBuilding:

simulated function FinishBuilding()
{
Super.FinishBuilding();
bEnabled=true;
}
Let's add more functionality and setup the initial link if we have one.
We'll also set the TAG variable to make tele finding iterations faster (TeleNetwork).

New code:

simulated function FinishBuilding()
{
Super.FinishBuilding();
bEnabled=true;

if ( Team == 0 ) Tag = 'sgTeleRed';
else if ( Team == 1 ) Tag = 'sgTeleGlue';
else if ( Team == 2 ) Tag = 'sgTeleGreen';
else Tag = 'sgTeleGold';

if ( Level.NetMode == NM_Client )
return;

OtherTele = FindOther();
if ( OtherTele != none )
{
bHasOtherTele = true;
OtherTele.OtherTele = self;
OtherTele.bHasOtherTele = true;
}
}
As soon as the teleporter finishes building it will locate it's destination, it will also set a team specific TAG.
What's with the TAG anyways? AllActors iterator can search by tag in native code, so if we make sure all teleporters in a team use the same tag, we don't even have to check for Team later.
If the destination is found, the Destination's destination is set to this teleporter, so the other doesn't have to bother finding this tele.


FindOther:

simulated function sgTeleporter FindOther(optional Pawn instigator)
{
local Pawn p;
local sgTeleporter teleDest;
local sgTeleNetwork sgTN;
local bool bTeleNetwork;
local bool bNext;
local int iTel;
local bool bFirst;
local sgTeleporter firstteleDest;

if (instigator!=None)
bTeleNetwork = (sgTeleNetwork(instigator.FindInventoryType(class' sgTeleNetwork')) != None);

for ( p = Level.PawnList; p != None; p = p.nextPawn )
if (p.IsA('sgTeleporter'))
{

teleDest=sgTeleporter(p);
if (teleDest.Team == instigator.PlayerReplicationInfo.Team)
{
if (bTeleNetwork)
{
if (!bFirst) { firstteleDest = teleDest; bFirst = true; }
if (teleDest == Self) bNext = true;
else if (bNext == true) return teleDest;
}
else
if (teleDest.URL2==URL1 && teleDest != Self) return teleDest;
}
}
return firstteleDest;
}
Let's program one case for the other tele, and another case for the TeleNetwork, so we make it faster and legible.
Also, no need to TypeCast the resulting sgTeleNetwork search, FindInventoryType always returns NONE or the desired class.
A map can indeed have over 50 pawns, doing an AllActors() iteration will be faster and cleaner than doing a PawnList check.
We'll be doing the usual local variables cleanup as well.

Suggested code:

simulated function sgTeleporter FindOther(optional Pawn instigator)
{
local sgTeleporter sgTele, firstTele;
local bool bNext;

if ( (instigator != none) && (instigator.FindInventoryType(class'sgTeleNetwork' ) != None) )
{
ForEach AllActors ( class'sgTeleporter', sgTele, Tag)
{
if ( sgTele == self )
{
bNext = true;
continue;
}
if ( bNext )
return sgTele;
if ( firstTele == none )
firstTele = sgTele;
}
return firstTele;
}

if ( OtherTele != none )
return OtherTele;

ForEach AllActors ( class'sgTeleporter, sgTele, Tag)
{
if ( (sgTele != self) && (sgTele.URL1 == URL1) )
return sgTele;
}
return none;
}
TeleNetwork destination finding is found in a different iterator, so we avoid extra checks, firstTele can be NONE there so we already cover this special case of having only two teleporters.
It picks the OtherTele if we already saved it, otherwise we try finding one anyways (used by FinishedBuilding).


We must also add a notification at Teleporter destruction so we remove actor references, I'd rather use engine's Destroyed() event rather than the Destruct() function, besides Destruct() calls Destroy() so Destroyed() gets called anyways. (lol words)

event Destroyed()
{
if ( OtherTele != none )
{
OtherTele.OtherTele = none;
OtherTele.bHasOtherTele = false;
}
Super.Destroyed() //Just in case...
}


Side effects:
If destination teleporter is relevant on client machine, player will experience lag-free teleportation!!
EDIT: Only if we also replicate URL1 variable.

What about SuperBoosters and sgTeleporter combos?
Are these not intended to work or is it a bug?

Feralidragon
05-25-2012, 06:38 AM
Side effects:
If destination teleporter is relevant on client machine, player will experience lag-free teleportation!!
EDIT: Only if we also replicate URL1 variable.
Well, that means if you add the builder as instigator of both teleporters, they're always relevant to him. :)
Also, going down that road you can put them with bAlwaysRelevant=True as well. bAlwaysRelevant should be avoided at all costs, but this seems a rare case where its usage would be correct, given the actual teleporter functionality and since you never have many of them in a match and since they don't update often.



What about SuperBoosters and sgTeleporter combos?
Are these not intended to work or is it a bug?
I think it's intended to work, personally I love doing that ingame, and it's only effective if the team gets interested in using it (great for attacking quickly), plus both are easy to kill anytime (and that happens a lot), so either ways it should stay that way imho.

audiosonic
05-25-2012, 06:49 AM
I think it's intended to work, personally I love doing that ingame, and it's only effective if the team gets interested in using it (great for attacking quickly), plus both are easy to kill anytime (and that happens a lot), so either ways it should stay that way imho.

He means the teleporters you build yourself, you can't use boosters through self-made teleporters, not sure whether this was intended this way, or just not able to get it right. However if you can get it to work it would make for some inchereschting gameplay regarding tele's.

Higor
05-25-2012, 06:49 PM
Well, that means if you add the builder as instigator of both teleporters, they're always relevant to him. :)
Also, going down that road you can put them with bAlwaysRelevant=True as well. bAlwaysRelevant should be avoided at all costs, but this seems a rare case where its usage would be correct, given the actual teleporter functionality and since you never have many of them in a match and since they don't update often.


I think it's intended to work, personally I love doing that ingame, and it's only effective if the team gets interested in using it (great for attacking quickly), plus both are easy to kill anytime (and that happens a lot), so either ways it should stay that way imho.

It would indeed be nice to conduct tests by setting bAlwaysRelevant to true and reducing NetUpdateFrequency from 100 (WTF) to 5.

Now that I think about it, isn't NetUpdateFrequency value a bit excessive on these stationary pawns?
The buildings run LOTS of simulated code that actually simulate the Container's healing effect as well as the construction progress so it isn't that necessary to update these little changes all the time in my opinion.

.seVered.][
05-25-2012, 10:48 PM
Why not just base the rotating animation on the bHadOtherSide variable. Only have it spinning if it has a matching teleport. So unmatched teleports don't rotate.

Feralidragon
05-26-2012, 07:13 AM
It would indeed be nice to conduct tests by setting bAlwaysRelevant to true and reducing NetUpdateFrequency from 100 (WTF) to 5.

Now that I think about it, isn't NetUpdateFrequency value a bit excessive on these stationary pawns?
The buildings run LOTS of simulated code that actually simulate the Container's healing effect as well as the construction progress so it isn't that necessary to update these little changes all the time in my opinion.
NetUpdateFrequency is a tricky property, only because it's set to 100, it doesn't mean it will constantly update 100 times per second.
First and foremost, this value is limited by the server tickrate, which by default is 20 (idk the current value on the siege server though).
Second, NetUpdateFrequency only states the *max* frequency of replication updates, so if the actor doesn't have any updates (no variable that was changed server-side and needs to go to the client), then there's no problem, it won't update anyhow.
So running simulated code has nothing to do with NetUpdateFrequency, because it all goes down to what you actually replicate from the server to the client, or vice-versa, which in the case of a teleporter you just replicate a few things on spawn, from there you don't have almost anything to replicate during the whole teleporter lifetime, so that's not an issue imo.

If you still want to reduce it, reduce it to 10 and not just 5, I think 5 is too small (and if something needs to be replicated asap, players will notice the lag).

Higor
05-26-2012, 02:44 PM
I know, thing is, that during construction, when the Energy amount is constantly changing, we don't have to update all these times when we're doing simulation.
Now fire a neutron bomb, and you'll hog the bandwidth channels full of construction updates.

Also, server tickrate is at 65!, a theoretical maximum of 65 updates per second on a stationary pawn seems excessive, and yes, 10 looks fine enough.

Higor
05-26-2012, 03:42 PM
This post will explain how to make the teleporter stop spinning when there is no destination.
It should only stop spinnig for players of that team so enemies won't notice if the team has a working tele or not.
It requires the previous code from this thread to be applied to work.


First, we add another variable:

var PlayerPawn LocalPlayer;


Second, let's make a local player finding function

simulated function PlayerPawn FindLocalPlayer()
{
local PlayerPawn P;
ForEach AllActors (class'PlayerPawn', P)
{
if ( (P.Player != none) && (ViewPort(P.Player) != none) )
return P;
}
return none;
}:


Third, we work on the teleporter graphics:

simulated function TeleporterGraphics( optional bool bReset)
{
local sgMeshFx theFx;

if ( bReset )
{
if ( myFx.RotationRate == rot(0,0,0) )
{
For ( theFx=myFx ; theFx!=none ; theFx=theFx.NextFx )
{
theFx.RotationRate.Pitch = Rand(MFXrotX.Pitch);
theFx.RotationRate.Roll = Rand(MFXrotX.Roll);
theFx.RotationRate.Yaw = Rand(MFXrotX.Yaw);
if ( theFx.NextFx == theFx )
theFx.NextFx = none;
}
}
return;
}

if ( bHasOtherTele && (myFx.RotationRate != rot(0,0,0)) )
return;
if ( !bHasOtherTele && (myFx.RotationRate == rot(0,0,0)) )
return;

For ( theFx=myFx ; theFx!=none ; theFx=theFx.NextFx )
{
if ( bHasOtherTele )
{
theFx.RotationRate.Pitch = Rand(MFXrotX.Pitch);
theFx.RotationRate.Roll = Rand(MFXrotX.Roll);
theFx.RotationRate.Yaw = Rand(MFXrotX.Yaw);
}
else
theFx.RotationRate = rot(0,0,0);
if ( theFx.NextFx == theFx )
theFx.NextFx = none;
}
}


Fouth, we rewrite the Timer() function:

simulated event Timer()
{
Super.Timer();

if ( Level.NetMode != NM_DedicatedServer )
{
if ( LocalPlayer == none )
LocalPlayer = FindLocalPlayer();
else if ( (LocalPlayer.PlayerReplicationInfo != none) && (LocalPlayer.PlayerReplicationInfo.Team == Team) )
TeleporterGraphics();
else
TeleporterGraphics( true);
}

if ( SCount > 0 || Role != ROLE_Authority )
return;
}

Now if you want a different effect, you can work that on TeleporterGraphics() alone without looking too much into the code.
Remember that bHasOtherTele must be replicated.


EDIT: REVIEWED

SilverWing
05-26-2012, 04:41 PM
ill put a poll up if people want the teleporters to be darker when inactive or to stop spinning when inactive

.seVered.][
05-26-2012, 04:42 PM
ill put a poll up if people want the teleporters to be darker when inactive or to stop spinning when inactive

Cool, I think that would be the easier method of knowing.

Feralidragon
05-26-2012, 04:49 PM
The server is at 65?! I thought values up to 35 were good enough, but perhaps for some updates it needs an higher frequency?
Anyway, yeah, I didn't remember about the energy, you're right on that one.

As for the darker tele, it will make it just less visible, and not exactly "darker", in the middle of other buildings, it may not be noticeable and players may build on top of it unaware.
If the rendering style was masked or normal, that would be another story.

SAM
05-26-2012, 05:37 PM
Well after much tweaking and research I found that based on the server hardware, 65 was the best tickrate. This enabled jetpack to work correctly and reliably under heavy long game loads as well as other stuff and just general gameplay. It's extremely high but it works on our server. Just for the people who think they will make their server 65 tick just because uK have, don't. A high tickrate has much more negative implications than what you possibly think. It will possibly max out your server's CPU and make the process much more intense. Which causes HUGE lag. So unless you have dedicated resources, I would not recommend it.

The thing that needs reviewing the most is Jetpack it should be changed from tick dependant. Something similar to TNSE's non tick dependant weapons *may* possibly work.

Higor
05-26-2012, 08:24 PM
Ugh, that's because you didn't take deltatime into account.

Tick( deltatime) if far more reliable for this kind of stuff, and it's precisely the Timers that screw up the timing.
Whatever you do, the server's Timers can never be adjusted so that they hit every 50 milliseconds.
You have to do an approximation with a user made timer on Tick.
Besides, event Timer will always hit the frame past those 50 milliseconds.

With a 65 fps server (assuming you won't get slowdowns).
The theorethical timings since the SetTimer would be:
Every 61,54 ms.

With a user made timer you get (ms):
61,54
107,69
153,84
215,38
261,53
307,68
354,84
The server ticks every 15,38 ms, so if you substract 7.19 ms to all values, every single value will be a multiple of 50 (+-7,19) which is the value we're looking for our timer, a clear indicator this timer always stays in the proper line.
Another strength of this is that the timers will continue to try staying at 50 ms even if the tickrate changes.

UT weapons could have been programmed with this in mind to prevent minigun and pulsegun to become so nerfed in onlineplay.