Overhauling the weapon rack scripts

[taleden]

Ah, that is a convenient function. Thanks for that, I'll see what I can find in my stacks.

 

Meanwhile, in that last thread of yours, it looks like 00102BDC is the object reference (to an IronSword) that PlaceItem() is stuck waiting for Is3DLoaded(). Is it possible to go into that cell and

prid 102bdc
moveto player

or something similar, to force the sword into the cell, so that its 3D will load and the loop will break?

 

EDIT: It looks like most of my threads are similar, the ItemToPlace reference is something that was placed in the cell and supposed to start on the rack, and now I don't know where it is. I tried the above but it didn't help, although I did notice that after 'prid' the console identified the reference as '' [01234567][D] -- does that mean it's been deleted or disabled somehow? Is there a console command to revive it so I can get it loaded in the cell again? I tried 'enable' but no luck.

[Arthmoor]

Yes, 00102BDC is the default iron sword that's normally found on that rack. I pulled it off a very long time ago and sold it, replacing it with the Galdur Blackblade. There's no way to get the thing back after all this time.

[Sclerocephalus]

I'll poke around in your weapon rack overhaul scripts tomorrow probably, for now I'm still trying to see if I can hotfix the looping threads in my save.

 

After applying the fix posted above, the one thread which was looping on a transient reference has exited and is fixed, so I don't get those errors every frame anymore. But, since I've installed your other hotfix, I do still get lots of these on game load:

[10/21/2013 - 09:04:29PM] Warning: Function WeaponRackActivateScript..InitActivator in stack frame 1 in stack 4277881 differs from the in-game resource files - using version from save
[10/21/2013 - 09:04:29PM] Warning: Function WeaponRackActivateScript..HandleStartingItem in stack frame 2 in stack 4277881 differs from the in-game resource files - using version from save
[10/21/2013 - 09:04:29PM] Warning: Function WeaponRackActivateScript..PlaceItem in stack frame 3 in stack 4277881 differs from the in-game resource files - using version from save

78 threads worth of that, actually. They must be stuck in the same loop, but they must not have transient references because they're not spewing the same errors. That also means the defaultDisableHavokOnLoad fix doesn't apply there, because the item they're looping on must not have that script attached.

 

So I figured I'd try something even crazier, and I edited the base ObjectReference.psc that comes with SKSE to redefine Is3DLoaded() from native to scripted and always returning True. I expected that would cause other things to misbehave but should at least break those 78 threads out of their loop, not only because !Is3DLoaded() is the loop condition, but also because according to the CK wiki, "Exception: If a function was native and is now scripted, the stack will be thrown out instead of resumed."

 

But it didn't work; after loading a game with the altered ObjectReference and then re-saving, loading the new save yields exactly the same warnings about threads in the WeaponRackActivateScript being restored from the save. I don't understand how those threads are still running. They aren't generating any errors in the log when they try to evaluate Is3DLoaded(), the way transient or otherwise bad references do, so they must be working with valid non-transient ItemToLoad references. And I can see ~1500 other scripts which extend ObjectReference, but none of them override Is3DLoaded(), so on every iteration, all 78 of those threads ought to be calling the base ObjectReference.Is3DLoaded() which I redefined to always return True. And yet the loops don't break.

 

Anyway, just an update. This scripting engine is crazy. Why didn't they just use Lua or something?

 

 

Ah, that is a convenient function. Thanks for that, I'll see what I can find in my stacks.

 

Meanwhile, in that last thread of yours, it looks like 00102BDC is the object reference (to an IronSword) that PlaceItem() is stuck waiting for Is3DLoaded(). Is it possible to go into that cell and

prid 102bdc
moveto player

or something similar, to force the sword into the cell, so that its 3D will load and the loop will break?

 

EDIT: It looks like most of my threads are similar, the ItemToPlace reference is something that was placed in the cell and supposed to start on the rack, and now I don't know where it is. I tried the above but it didn't help, although I did notice that after 'prid' the console identified the reference as '' [01234567][D] -- does that mean it's been deleted or disabled somehow? Is there a console command to revive it so I can get it loaded in the cell again? I tried 'enable' but no luck.

 

You should take some time and read this thread from the beginning. You would at least stop wondering every time you arrive at an apparently new obstacle. The v2.6 hotfix (USKP 1.3.3c is running v2.5) became necessary, because the GetParentCell function was used as a safety check before the script would try to load the pre-placed item: If GetParentCell returns the same cell for the pre-placed item and the rack, it appeared logical to assume that the pre-placed item was in the same cell. Though, once the pre-placed item physically stops to exist in the game (i.e. sell it to a merchant and wait until his chest resets), GetParentCell always returns the cell of the rack where it started life in the game, no matter whether it really is in that cell or not (in fact, it never was). Ironically, at this time, this particular check was pretty much the only line in the script which had been taken over from the vanilla script and left unaltered. On the other hand, as long as the item still physically exists, GetParentCell returns reliable results ...

 

The problem with papyrus is not that it is unpredictable as a programming language. What's unpredictable are the results obtained from the engine. Every single command is a lucky bag ...

 

EDIT:

If, for whatever reason, you should come to the conclusion that you will have to attach a custom script to the racks, the items or whatever, DO NOT do this directly, but apply the script via a quest alias. Otherwise, you'll never get rid of those scripts again, unless you start a new game.

[Sclerocephalus]

I'm not seeing how Is3DLoaded on the activator script would help, since the activator script is not an ancestor of ObjectReference or defaultDisableHavokOnLoad. When PlaceItem() is in a loop calling ItemToPlace.Is3DLoaded(), why would it try to resolve that function on the activator script?

 

All activators are running the same script which they inherit from their base objects. Thus you could extend the script running on a particular reference. You would have to attach a modified script to the base object, but leave out the PlaceItem function, and extend the parent on the reference with the old version of this function. I don't know whether the parent status matches the ancestor criterion, but it would be worth a try.

[Sclerocephalus]

Any chance the shit hit the fan because it has that disable havok script on it?

 

I wonder whether it is that stupid script that actually makes Zephyr one of the problem weapons. Any chance that we could sack that script entirely ?

[DayDreamer]

Having now read the many messages on this yesterday, I'd like to get back to his premise.

That loop needs some kind of failsafe, because in my game, ItemToPlace has become None and that loop is just going on forever and ever spewing errors into the log.

It doesn't. I has a failsafe - actually, it even has three (two, see edit below) of them:

Taleden is correct. That loop needs a failsafe. Please do not make any loops that run forever without a counter. Also, please never design a loop with a bang ('!') that doesn't check for a None object in the loop.

    Int counter = 0

    While ItemToPlace && !ItemToPlace.Is3DLoaded() && counter < 5
        counter += 1
        Utility.Wait (0.1)
    EndWhile

Personally, I prefer counters that count down as better self-documenting code, but I've coded it in your style. Note that the Bethesda coding tradition -- such as it is -- uses leading lower case for local variables and upper case for Properties....

 

EDIT: Also, in cases where the rest of the code is likely to bomb out, I always add a comment to the log and quit the function; here an example coded in my style:

    if looping <= 0
        debug.trace(self + "Game engine has not rendered 3D data, horse and/or carriage may not display properly!")
        return true
    ;else
    ;    debug.trace(self + "Wait3D() looping=" + looping)
    endIf

[Sclerocephalus]

 

Having now read the many messages on this yesterday, I'd like to get back to his premise.

Taleden is correct. That loop needs a failsafe. Please do not make any loops that run forever without a counter.

 

OK, will do - but without check for ItemToPlace in the while condition. If you are concerned that ItemToPlace could be none when PlaceItem starts running, please specify this, and I will consider adding a check to one of the handling functions.

 

You can't bail out of the placement procedure easily (as it will leave traces in the game which you would have to clean up appropriately). There's a reason why all the item handling is done before this is function is even called. Unfortunately though, you must ckeck for 3D immediately before the motion type is set, since experience tells that 3D can be lost temporarily at any time inbetween. Several commands running on an object reference are known to cause this occasionally (otherwise, the check would not have stayed in there).

 

 

Also, please never design a loop with a bang ('!') that doesn't check for a None object in the loop.

 

Sorry, didn't get this. I assume that something like "If (x != y) ..."  is still OK (if not, what should be the workaround ?)

[DayDreamer]

OK, will do - but without check for ItemToPlace in the while condition.

I'm getting a little irritated myself. I've been trying to be polite and "recommend" and say "please", but enough is enough.

 

Check the damn ItemToPlace in the damn while condition!

 

Checking it earlier doesn't buy you anything -- given the fact that these entries are turning to None, and can change to None at any time because the script and loop itself can be interrupted at any time, and can in fact be interrupted anyplace you reference another object (ItemToPlace) without using an uninterruptible function (see http://www.creationkit.com/Category:Non-delayed_Native_Function), checking here prevents the spurious log entry.

 

So always check! Thank you very much!

 

You probably should also check on the loop exit code, too. (My example was a lazy copy.)

 

If quitting would leave things in a bad state -- CLEAN UP THE STATE before the return!

[DayDreamer]

Sorry, didn't get this. I assume that something like "If (x != y) ..."  is still OK (if not, what should be the workaround ?)

Separately, for pedagogy:

while !foo.bar()
endwhile

If foo.bar() is false, that loops.

 

If foo.bar() is not a valid function, that infinite loops.

 

If foo is None or not a valid object variable, that infinite loops.

 

Always use:

while foo && !foo.bar()
endwhile

Or do what I do, and define your functions so that True is bad and False is good. An old habit picked up from *BSD error conditions (where 0 is no error). There's a reason we picked these coding conventions 30+ years ago.

EDIT: in retrospect, you probably didn't know what I meant: !foo.bar() is spoken "bang foo dot bar", and (x != y) is spoken "x not equal y".

[taleden]

I have begun digging through this entire thread, and doing some experimentation and testing of my own, so I certainly appreciate the difficulty of designing scripts that handle all of the edge cases correctly. This is just one of those edge cases -- Papyrus' multi-threaded nature allows for all sorts of bizarre behavior, so every single line of code (or at least every point where an external call is made) has to be extremely defensive in case things suddenly go wrong while the thread is suspended. Putting "ItemToPlace &&" into your while condition is this kind of defensive strategy; you're right that there should be checks in the item handling code before PlaceItem() is even called, and most of the time those checks will catch problems before they make it that far, but that doesn't spare you from having to put still more sanity checks all the way through til the end. Defense in depth, as they say.

 

Anyway, thanks for all the work you've put into this, Sclerocephalus.

[DayDreamer]

Last night, I tried comparing the current 3.4 debug scripts with 3.1 and with vanilla. Despite the fact that almost all the variables are the same name, they've been gratuitously rearranged. Very difficult to compare and understand.

 

There is an "Auto STATE EmptyRack" in vanilla WeaponRackActivateSCRIPT. It's been renamed "State Inactive" (but not auto), with another "Auto State ActivatorBusy" added to the trigger script. I don't understand how this can be backward compatible.

 

Could it be that at least some of the problems we're experiencing are lack of backward compatibility?

[tyrel68]

DO NOT QUESTION SCLEROCEPHALUS!!!   /slaps daydreamer   

 

Just k8idding

 

though idk i thought the new scripts just overwrote the previous ones so that sovles the compatibility issue

[Sclerocephalus]

Despite the fact that almost all the variables are the same name, they've been gratuitously rearranged.

 

This is because none of the variables fulfills anywhere near the same purpose as in the vanilla scripts (except for attributing the item types to their matching nodes, pretty much everything in the vanilla scripts didn't work - the various issues have all been documented in this thread). Some of the names were only kept because those properties are accessed by the Hearthfires scripts, but even then, I have made sure that those scripts work well with them. Some HF scripts would even fail when using the values from the vanilla scripts.

 

 

Very difficult to compare and understand.

 

They have been rearranged by type and in alphabetical order. What's difficult about this ?

 

 

Could it be that at least some of the problems we're experiencing are lack of backward compatibility?

 

No. Tested very extensively for almost two weeks before even releasing v1.0.

 

Backward compatibility might be suspected to become an issue when there are open tasks running. But then, the engine reloads the old script version from the save (pretty much as with every other changed code which it won't accept until it gets its tasks completed).

[DayDreamer]

A few posts ago, I was trying to be nice, and merely referenced "your style". Actually, "your style" is commonly called a "fencepost error" or "off-by-one error".

 

counter = 0
 
While !ItemToPlace.Is3DLoaded() && counter < 5
    ...

That test is actually executed 6 times, not 5. I was ignoring this potential issue, as it didn't seem to be relevant to the underlying problem. But in future, let's use better coding practices as we're doing code review.

counter = 5
 
While counter > 0 && ItemToPlace && !ItemToPlace.Is3DLoaded()
    ...
    counter -= 1
EndWhile

[Sclerocephalus]

That test is actually executed 6 times, not 5.

 

Learn counting.

[DayDreamer]

In vanilla WeaponRackActivateSCRIPT, there's no OnLoad() event. And I must be confused: isn't the activator invisible? So how could we ever expect to have a valid load event?

I haven't gone back through all the logs posted here, but at least some of the out of order OnLoads are activators. Could it be this is a meaningless event?

[DayDreamer]

Learn counting.

Yes, you need to learn counting. Or the meaning of "&&" (spoken "and then"). Or both.

[DayDreamer]

I've been going through the creationkit documentation yet again, and can find nothing that says a state is discarded by a new script version or a game reload.

So backwardly compatible, the existing vanilla activators are in state "EmptyRack" at the time the script is first called. Not in state "", as seemed to be assumed.

[Sclerocephalus]

I have begun digging through this entire thread, and doing some experimentation and testing of my own, so I certainly appreciate the difficulty of designing scripts that handle all of the edge cases correctly. This is just one of those edge cases -- Papyrus' multi-threaded nature allows for all sorts of bizarre behavior, so every single line of code (or at least every point where an external call is made) has to be extremely defensive in case things suddenly go wrong while the thread is suspended. Putting "ItemToPlace &&" into your while condition is this kind of defensive strategy; you're right that there should be checks in the item handling code before PlaceItem() is even called, and most of the time those checks will catch problems before they make it that far, but that doesn't spare you from having to put still more sanity checks all the way through til the end. Defense in depth, as they say.

 

Anyway, thanks for all the work you've put into this, Sclerocephalus.

 

 

There actually isn't really any multithreading with the weapon racks. The player is the only person in the game who is allowed to activate a rack, and he can't activate two racks at a time. Thus, there's always only one instance of the activator script running. Same for the quest script and the quest alias script, since they are only called by the activator script and only once per activation.

 

No functions on either of the scripts are called by any other script. One Hearthfires script is reading a property from the activator, and the trigger script reads and overwrites properties on the activator script. Though, the way it has been handled in my overhaul, the trigger script is not allowed to do this while the activator is enabled, i.e. it only does when the activator script is not running.

 

While working on versions v3.0/v3.1, I had the trigger script call the activator script to run its initialization procedure, but did rewrite this again because I wasn't really happy with this solution (also because the trigger script would have to wait for the function to return). This is now handled by an update registration.

[Sclerocephalus]

Yes, you need to learn counting. Or the meaning of "&&" (spoken "and then"). Or both.

 

(Counter < 5) returns "true" for Counter = 0, 1, 2, 3 and 4 ... this makes five.

 

 

In vanilla WeaponRackActivateSCRIPT, there's no OnLoad() event. And I must be confused: isn't the activator invisible? So how could we ever expect to have a valid load event?

 

The activator's 3D is not rendered, but this doesn't mean it doesn't have a 3D (this is handled by a node name in the nif, by the way). It will receive OnLoad events like any other object in a cell.

 

 

I've been going through the creationkit documentation yet again, and can find nothing that says a state is discarded by a new script version or a game reload.

 

The CK documentation also doesn't tell you anything about the data baked in your save game, and they are there nonetheless. Why not starting to run your own tests ?

[tyrel68]

Daydreamer

 

Yes, you need to learn counting. Or the meaning of "&&" (spoken "and then"). Or both.

I know your both very passionate and want to state your points of view but comments like that are counter productive and just inflammatory

[Arthmoor]

What tyrel said - guys, please dial it down a notch before things get out of hand.

[DayDreamer]

(Counter < 5) returns "true" for Counter = 0, 1, 2, 3 and 4 ... this makes five.

With all due respect to Arthmoor and others, it seems a basic programming lesson is in order. There's a reason this error has a name, it's that common among beginners.

counter !ItemToPlace.Is3DLoaded() && counter < 5
0              test 1                test 1 true
1              test 2                test 2 true
2              test 3                test 3 true
3              test 4                test 4 true
4              test 5                test 5 true
5              test 6                test 6 false

Sclerocephalus, I don't know who you are. I value the work you've done. But I do not appreciate the disrespect you've repeatedly shown me and others the past few weeks. It's not uncommon for programmers to become wedded to their code. There are entire books concerning "ego-less programming". Please read one. Also structured programming and design.

Likewise, I pointed out the oddness of using OnLoad as you have done some time ago, and it's still biting us in the ass. Please listen to reason....

[Arthmoor]

Let's look at this from another angle. Will the end user be able to detect the difference in the game between 5 and 6 attempts? No. So is this kind of nitpicking truly necessary?

 

As I said, dial it back, ya'll are getting a bit aggressive over this and for no good reason.

[DayDreamer]

They have been rearranged by type and in alphabetical order. What's difficult about this ?

Because we're using diff to check changes. When working with others, an important behavior is to modify only the code that is required to be changed, and to check your work to ensure the diff is clean and clear.

Even code that is no longer used should be left in place, surrounded by

;/ /;

so that the diff lines up nicely.

Changing the names of functions should be documented, and all functions should have a header describing who calls them and what is returned, and whether there are side effects.

Comments should not extend past a reasonable screen width (usually 80), and should be neatly folded.