strange code in Flashlight.uc

[Barbie]

By reworking and searching for the reasons of "Access None" I stumbled over the following code in stock Flashlight.uc:

Code: Select all

04 class Flashlight extends Pickup;
...
23 state Activated
24 {
...
31	function Tick( float DeltaTime )
32	{
...
52		if (Charge<-0) {
53			s.Destroy();
54			Pawn(Owner).ClientMessage(ExpireMessage);		
55			UsedUp();		
56		}
57		
58		if (Charge<400) s.LightBrightness=byte(Charge*0.6+10);
59
60		GetAxes(Pawn(Owner).ViewRotation,X,Y,Z);
...
63		s.SetLocation(HitLocation-vector(Pawn(Owner).ViewRotation)*64);
...

I found several issues there:
1) In line 52 is a typo? I guess the coder has meant "<=" instead of "<-".
2) The code block in lines 52-56 checks if the Flashlight has run out. Isn't there a RETURN missing after UsedUp() in line 55? *s* is destroyed in this block and accessed again below (line 58/63).
3) For beauty of code and efficiency I'd used an "Else If" in line 58.

Please correct me if I'm wrong, thanks.

[Wormbo]

Line 52 apparently isn't "wrong enough" to be a problem. -0 is just 0, so the flashlight will run out of charge when Charge becomes negative instead of when just becoming "non-positive".

Yes, a return statement would have made sense after line 55. The alternative could have been an else block from line 57 to the end of the function.

Else-If on line 58 would have been "not quite correct" either, as there's still an access to s below it on line 63. When an If block ends with a statement that unconditionally leaves the block's outer scope (not just a return, also a break or continue in a loop), any following else statement is redundant. Remember that the UnrealScript compiler does not optimize anything, so you're generate a dead unconditional "jump" bytecode after the unconditional block-ending statement. It won't hurt performance because it's never executed, but an Else here wouldn't necessarily improve "code beauty".


In summary: The only thing really missing here would have been a return statement after line 55.

[sektor2111]

1st check
if (bDeleteMe) return; //nothing more to execute if I'm sent to trash - I think all important ticks need this
2nd
if ( Charge < 1 ) //irrelevant 0 - zero it's just 0 it won't be a load ever - Logic
Client message //tell to client that we run out
s.Destroy();
UsedUp() //see what else need - perhaps look for a duplicate spam message - if happens there do rewrite this
3rd
After calls and occurrences I think a "return" would be priceless. Flashlight as I know so far it's just in deletion process after exhausting charge so then nothing is supposed to happen. In mean-time I'll do a check into "flashlights"...

Just thoughts.

Edit:
Oh, I've added more check (Owner included) + A.I. code.

[Barbie]

Thanks for the response.

Else-If on line 58 would have been "not quite correct" either

Did you notice that there is only one statement (setting "LightBrightness") depending on "Charge<400"? So an Else-If won't affect code below line 59.

if (bDeleteMe) return;

Yep, that's an nice enhancement. The tests for Owner == None were already done in my version of Flashlight.

I'm going to recode my recoded "FlashlightSB" then. ;o)

[Wormbo]

1st check
if (bDeleteMe) return; //nothing more to execute if I'm sent to trash - I think all important ticks need this

bDeleteMe checks seem pretty redundant. TBH I have yet to see a place where they really make sense. Sure, Destroy() could have been called deep within the event callstack, but you might want to consider the stuff that happens during the Delete() call as well, which bDeleteMe doesn't cover. (You can detect this only starting with Unreal Engine 2, which sets bPendingDelete at the beginning of the Destroy() function.) Events like Tick() and Timer(), for example, are unlikely to be called during or after Destroy(). (I think it's outright impossible for Tick().)
Also, Inventory uses the Destroyed() event to make sure it is no longer associated with a Pawn or its inventory chain after it is destroyed. A simple check for an Owner should be enough and more flexible than a destruction check.

[sektor2111]

bDeleteMe checks seem pretty redundant. TBH I have yet to see a place where they really make sense. Sure, Destroy() could have been called deep within the event callstack, but you might want to consider the stuff that happens during the Delete() call as well, which bDeleteMe doesn't cover. (You can detect this only starting with Unreal Engine 2, which sets bPendingDelete at the beginning of the Destroy() function.) Events like Tick() and Timer(), for example, are unlikely to be called during or after Destroy(). (I think it's outright impossible for Tick().)

All right, then enlight me about GuidedWarshell postrender errors. There I'm very convinced about a state doing nasty things after explosion. If I well recall was even another boolean added but code never sanitized. Firing a GuidedWarshell inside a monsters group (or whatever) PostRender Errors occurs as long as the rocket still try to do something when it shouldn't. Not a single time I have prevented even attitude problems for bDeleteMe stuff and things suddenly went better. Aside, recently I've been developing that NavigationTool when some dude opened my eyes again... oh well... something spawned has been sent in removal for some unknown map reason. SUCH Actor-Object sent to deletion process combined with "SetLocation" (attempting to relocate that thing) will successfully crash Unreal Engine 1 (in cause not 2 or 3 not others which are not in my attention) as long as such actors spending a bit of time in Level which later and supposed to be moved are not a good thing. So I'll keep my way by wrapping states with ticks as long as they have trash behavior in this situation. I'm sorry if I'm wrong but I'm using such a wrapped flashlight for 3 years and I don't see anything evil this way.
I'm gonna recreate a crash type using a small mutator as tester for confirmation. I don't know if this master check is harmful more than helpful.

Edit: No, wait, it's done... first mutator from topic "Little A.I. modifier" and it was map which was crashing. I fixed it using a check against bDeleteMe and a few tiny things. Crash has gone...

[Wormbo]

Yes, if Destroy() happens deep in some event callstack, you will get some unexpected stuff happening due to the way UnrealScript works with to-be-deleted actors.

However, in this particular Flashlight case, Destroy() happens right inside the Tick() event, which is not called from within some deep UnrealScript callstack, but right from the engine's main look via some native code calles and no intermediate UnrealScript. The easiest way to prevent errors in such case is to set all relevant references to None to prevent anything else from accidentally doing invalid stuff to the deleted actor. Flashlight does this through the Inventory default behavior of removing itself from the Pawn's inventory chain during its Destroyed() event.
GuidedWarshell, on the other hand, is a Projectile that is set as the player's ViewTarget. Its PostRender() call is not a native event, but routed through Weapon.PostRender(), which in turn is called by HUD.PostRender(). Any errors there are related to improper cleanup. bDeleteMe only detects such improper cleanup, but you should have really been more thorough earlier on.

I'm not saying bDeleteMe is redundant in all cases, but usually there are other things going wrong if you find yourself needing it. Granted, your Destroy()-during-spanwing example where a SetLocation afterwards happened further down the initialization sequence is a valid case. However, that's only so because SetLocation() apparently doesn't do the check itself.
Remember that UT is a game that came out when the engine itself was still very new. Epic didn't know it in and out themselves yet. Things have improved by a giant margin from the state of UT v436 to the state of UT2004 v3369. Often, when I look back at UT UnrealScript code, I can help but notice how bad it really ist. I mean, just look at that Flashlight script. As simple as it is, there are errors left and right. That might be due to the fact that it's actually not even UT code, but from the original Unreal. The GuidedWarshell does something projectiles aren't supposed to do. If you compare it to how vehicles and the Redeemer work in UT200x or UT3, you'll notice just how big of a hack it really ist.

Bottom line: If your code is clean, you only need bDeleteMe in very special situations to work around engine bugs.