What makes certain scripts harmful?

[Magicockerel]

I am looking to release my first mod that includes a script, and I am wanting to make sure that it won't harm people's save games. After rewriting it from scratch multiple times, I've finally got the script to a stage where I'm happy with it (at least mechanically, I'll probably condense it where I'm able). However, I'm unsure whether it will harm people's save games. Could anyone tell me how scripts harm save games and how you detect it? If it's just save game bloat that I have to worry about, then it should be reasonably easy to tell through testing. However, it would obviously be convenient to be able to tell without having to do that (and instead be able to look out for certain things in the script itself).

 

Here's a download link to my mod as it is currently, including the scripts source code in the relevant folder:

 

Dropbox Download

 

For anyone interested in looking at what I've done so far and what the mod actually does:

While the script is done (mechanically), the mod as a whole isn't. I've still got to create a holotape with settings, but beyond that, it should be good to go. In saying that, I'll also have to add it to the players inventory via script, hence the OnPlayerLoadGame event (to make sure that it's in the players inventory each time the game's loaded), but beyond that it's finished. What the mod does is it allows the player to define two sets of headgear, one for when the player's in combat, and one for when the player's out of combat (using the OnCombatStateChanged event). This allows you to equip more defense orientated headgear in combat, and whatever you want outside of combat (for instance you could just remove everything or define a whole other set of headgear). The mod as it is now is just a quest with the script running, and some global variables and form lists (all with the prefix "KE_DEH"). Without F4SE though, I'm unable to hotkey it, and the player has to define exactly what headgear they want to equip.

 

I've given the player 3 slots for headgear when out of power armour, and only 1 slot for headgear when in power armour (as far as I'm aware that's the maximum number of pieces you can equip respectively). For instance, when you're out of power armour you're able to equip a helmet, a bandanna, and some glasses, but when in power armour you're only able to equip a single helmet. As you can only define base objects when equipping and unequipping, this means that the player either can't carry duplicates of what the script is set to equip, or the player has to settle with the script potentially equipping the wrong combat helmet for example. From what I was told in my previous thread, I can't change this unless I want to remove all other base objects from my inventory and apply the correct modifications to a single base object via script, which doesn't sound ideal.

 

The global variables control what headgear you have equipped, and their integer values correspond to entries in their respective form lists. To equip nothing in a certain slot you give the corresponding global variable a value of "-1" or below. There's 109 pieces of headgear, excluding 7 power armour helmets (including the Automatron and Far Harbor DLC, which each add helmets to the game). That means that I'll have to make a holotape with 6 lists that have 109 entries, and 2 lists that have 7 entries (3 slots in and out of combat and out of power armour, and 1 slot in and out of combat and in power armour). So yeah, the holotape will probably take a while to set up as I don't know how I would streamline the process. I'll be busy over the next couple weeks anyway, so it will be a while before I get to that. Beyond the missing settings holotape though, it performs just fine in-game, as far as my testing has shown so far anyway.

[JonathanOstrus]

Obvious things that come to mind that would be harmful:

• Repeatedly spawning things that should be temporary, but specifying them to be permanent. (novice/accidental - Causes savegame bloat)
• Spawning things in hidden cells that are never used. (malicious - causes savegame bloat)
• Making quests or scripts that recursively call each other. (causes lag, memory issues, game instability, savegame bloat - sometimes done by design, maybe accidental)
• Repeatedly calling the api to request a savegame (potentially malicious - causes lag, uses lots of hdd space, overwrites previous save if on survival)
• Calling delete on things that should not be deleted (no clear identifier to figure out which are safe. things you spawn probably are)
• Hooking other quests or registering for events to intercept and change other scripts behaviours. (could be intentional or accidental - can break things and make them irreparable)

There's probably a lot more that don't come to mind right now. If you're conscious about what you're doing you're probably ok. If you don't understand the code, and what it does, you might change something that shouldn't be changed which could be problematic.

 

On a side note, did you try to see if you can get the refid instead of base id from an item in the inventory? Then try to call equipitem with the ref id instead of the base? Actually did you find a way to even have the player specify which items from their inventory to use? I couldn't find suitable commands for that.

[cdcooley]

In addition to the things mentioned above, excessive use of loops and especially using with a goal to monitor things over time. But looking at your script I don't see anything that should be a big problem.

 

I don't have Fallout 4 myself, but unless they've significantly changed from Papyrus in Skyrim the other big problem you can have is simple inefficiency for code that runs often.

 

One good example of inefficiency is:

bool active = KE_DEHActive.GetValueInt()

You've just called a function to convert a float into an int (after retrieving it from a global variable) then converted the int to a bool. Just call GetValue() on global variables don't use the "convenience" functions like GetValueInt() because they just force your script to take extra time. "GetValue() as int" runs significantly faster than GetValueInt(). (And I notice you've used GetValueInt() many times so your script speed will suffer.

 

You've also used "Game.GetPlayer()" quite often. Maybe they've improved it but if not you really don't want to use that function. Use a PlayerRef property instead.

 

Finally you're declaring unnecessary variables.

int aiIndex
			
Form akItemToAdd
int aiCount
bool abSilent
			
Game.GetPlayer().AddItem(akItemToAdd = KE_DEHFormList.GetAt(aiIndex = headgearCount), aiCount = 1, abSilent = true)

Those four variables serve absolutely no purpose. In those function calls you've used the "pass by name" calling convention, but that name on the left side of the equals sign isn't related to variables in the calling function at all. You can remove the four variable declarations and that last line will work just fine.

[Magicockerel]

@BigAndFlabby

 

Thanks for the detailed reply mate :smile:

 

I'm very new to papyrus, but from what I've learnt so far, it appears to be pretty limited. The only "GetEquipped..." functions appear to be for weapons, which is pretty disappointing. Without an equivalent for armour, I'm not sure how I would go about determining the reference ID of the headgear. You appear to be able to get the reference ID using the "OnItem..." events, but that doesn't help me with what I'm doing (I need the reference ID to equip it, not the other way around). I guess I just have to hope F4SE adds something to help with this. They appear to have added some mask related functions in Skyrim, so hopefully they get ported over to Fallout 4.

 

I added all of the headgear in the game to a form list and created a global variable for each slot to identify an entry in the form list (where there's 109 entries for regular headgear, and 7 entries for power armour helmets, split up into two form lists for convenience). You can have a look at it in the source code I provided, or even just load it up in the CK and have a look there. As I said, every form I've added has the prefix "KE_DEH", so they should be easy to find. As the .esp is dependent upon multiple masters, you will have to add the following to your CreationKit.ini file though:

[General]

bAllowMultipleMasterLoads=1

As the form list is just all of the headgear I identified in the "Armor" section of the CK, it limits me to only being able to equip and unequip base objects. So while I could probably find a workaround to get the reference ID of each of the objects using the "OnItem..." events, it would probably require a tedious amount of user input. While the settings holotape I plan to add won't be the most convenient thing in the world, it will probably be a lot more convenient than anything else the current papyrus functions would allow, even with it limiting me to the base object.

Beyond this point might not make much sense, I'm tired, sorry:

After writing this all out, it occurs to me that I may be able to use the "OnItem..." events to get the reference ID after all. I might be able to have an entry for each slot in the settings holotape, and when activated will take the next equipped item as the item to equip when going in or out of combat. I might even add a check to make sure that its base object appears on my existing form list, to make sure that it's actually headgear. This would probably be more convenient than my previous idea for both me and the user. It doesn't appear that you have to register for each instance of these events like the "OnHit" event though, which is annoying because I only want it to be done once. I would probably have to implement some global variable or something to make sure that it's only the next item that the player equips. What I mean is set the global variable to 0 when the user wants to change what is to be equipped in the settings holotape, and set it back to 1 when the event is called to select the item. These are all just my initial thoughts though, this only occurred to me after writing this up. I wont have any free time for the next couple or so weeks, so I'll have to wait until then to actually see if this can be done.

This would depend upon the reference being able to be remembered by the save file though. I'm not sure if that's possible, as I said I'm still new to scripting, and modding in general. As global variables can be remembered by the save file, this would at least be a way to determine the base object. Implementing this would save me from adding all 109 entries for each of the 6 slots though, even if it is just the base object. I'm really tired at the moment, apologies if any of this doesn't make any sense, I only just thought of this while writing this up. I'm not sure if there are any more convenient methods to determine the reference ID, but akReference of the "OnItem..." events should work.

[Surilindur]

I am still just learning, so whatever I say, take it with an overwhelming grain of salt. But based on the little I have learned, what cdcooley just said was pretty much everything there is, I think. I have not done much scripting for Fallout 4 myself, either, but just a few ideas. First, you have this:

    int headgearCount = 0
    int repeatCount = 0
   
    ;Give the player headgear for testing
    while (headgearCount < KE_DEHFormList.GetSize())
        int aiIndex

        Form akItemToAdd
        int aiCount
        bool abSilent
            
        Game.GetPlayer().AddItem(akItemToAdd = KE_DEHFormList.GetAt(aiIndex = headgearCount), aiCount = 1, abSilent = true)
            
        headgearCount += 1
            
        if (headgearCount == KE_DEHFormList.GetSize() && repeatCount >= 1)
            headgearCount = 0
            repeatCount -= 1
        endIf
    endWhile

Which could also be done like the following, incorporating the removal of those unnecessary variables cdcooley pointed out, as well as the PlayerRef property instead of Game.GetPlayer (not visible here, you need to add it to the property list in the script). Function calling also works without passing the name, although I think it is only a matter of preference, and you can do it the way you prefer:

    int index = KE_DEHFormList.GetSize()
    int repeatCount = 0
   
    ;Give the player headgear for testing
    while (index > 0)
        index -= 1
        PlayerRef.AddItem(KE_DEHFormList.GetAt(index), 1, true)
        if (index == 0 && repeatCount > 0)
            index = KE_DEHFormList.GetSize()
            repeatCount -= 1
        endIf
    endWhile

And then you seem to use GetAt with FormLists a lot. I have no idea how taxing that might be, but perhaps it might be handy to grab the form once, and then use that grabbed form instead of separately getting it each time? That loop is a massive one, too, and I did not take a closer look at it.

 

If you have a FormList you want to either unequip or equip, the whole FormList, you could maybe try something like this, again untested, just an idea,

Function EquipUnequipFormList(bool abEquip, FormList akList)
    int index = akList.GetSize()
    Form temp
    While (index > 0)
        index -= 1
        temp = akList.GetAt(index)
        If (PlayerRef.GetItemCount(temp) > 0)
            If (abEquip)
                PlayerRef.EquipItem(temp, False, True)
            Else
                PlayerRef.UnequipItem(temp, False, True)
            EndIf
        EndIf
    EndWhile
EndFunction

and give it the FormList and a boolean value as parameters. Assuming, of course, that you can pass a FormList as a parameter, I have never actually done that and I have no way to check it at the moment. Calling a function will cause it to stack, though, so that is just an idea, and might even turn out to be a stupid one. :P

[Magicockerel]

@cdcooley

 

Thanks mate, I'll make those changes when I get the chance. I originally had the "active" variable defined as an int, but I ended up defining it as a bool to improve the readability of the script. I had no idea that GetValue as int was more efficient than GetValueInt, cheers. I'll also have a look at PlayerRef when I get some free time. Just like with the bool function, I added and defined those parameters to improve the scripts readability, but if it slows the script down I'll make it as bare bones as possible. Thanks for spending the time to read through my script, I definitely appreciate the pointers :).

[Magicockerel]

@Contrathetix

 

Thanks for the additional pointers, I'll make sure to use and add PlayerRef to my properties and cut down on the unnecessary use of repeated functions. Hopefully my script will end up being significantly more efficient, because by the sounds of it, it could be. The while loop only repeats a maximum of 3 times, 1 for each of the slots. I wrote a version of the script yesterday that checked whether every single base object on my form list was in my inventory, whether it was equipped, and if it was equipped and not intended to be, it was unequipped. It was a laggy mess, not that I expected it not to be, I wrote it up more to test how efficient the scripting system is more than anything. The current version just equips the base object in each of the 3 slots, and if there's either none of the item intended to be equipped in the players inventory, or the slot is just empty, the item in the opposite slot is removed if it's currently equipped. It has a couple more checks than that, but those are the basics anyway. By opposite slots I mean slot 1 out of power armour and in combat and slot 1 out of power armour and out of combat. The version of the script that I uploaded only has a very slight delay (less than half a second), but hopefully I can cut that down with the advice you guys have given me. Again, thanks for the tips and reading through my script in the first place, this has been a valuable experience :smile:

[cdcooley]

To be clear

Game.GetPlayer().AddItem(akItemToAdd = KE_DEHFormList.GetAt(aiIndex = headgearCount), aiCount = 1, abSilent = true)

and

Game.GetPlayer().AddItem(KE_DEHFormList.GetAt(headgearCount), 1, true)

and even

Game.GetPlayer().AddItem(KE_DEHFormList.GetAt(headgearCount), abSilent = true)

are all equivalent. None of those are any faster or slower or require any more memory. Those are just different ways of passing the parameters.

 

It was the declaration of akItemtoAdd, etc. that is pointless.

 

They try to disguise that fact with the GetValueInt function but all global variables are always of type float. So are all ActorValues even when actual factional values don't make any technical sense.

 

Getting used to the quirks of a new language always takes time and Papyrus is not quite like any other language.

 

Finally, the advice above about temporary variables is good. You can move large numbers of values between local variables and comparing things using local variables in a single frame. But almost every function call requires at least one frame to run. So if you find yourself calling the same function more than once to get the same value, you should use a temporary local variable. Remember that it will take one whole second (at least) to run 60 functions. When loops are involved you can get to 60 really fast.

 

And the example above that loops from the FormList's size value down to 0 is an example of avoiding calling the GetSize function more than once.

[Magicockerel]

Edit: Sorry, either you edited your message or your entire message didn't load, either way I only read the first couple of code excerpts when I made my reply.

 

I'll be sure to at least ditch the declaration of the parameters. I didn't know functions were tied to the frame rate, I'll be sure to use them as rarely as possible knowing that. I didn't put much thought into the efficiency that different implementations had, so thanks, that's pretty important.

Edited June 16, 2016 by KernalsEgg

[Magicockerel]

This is just an update for anyone interested in the script itself. Here's a link to the script (and the mod as a whole) as it currently is (and of course the source code is included in the relevant folder):

 

Dropbox Download

 

I've taken on board your recommendations and made the required changes to the script. At least, I'm pretty sure I've taken them all on board. I've made a couple other small changes (such as adding back a few debug notifications from previous versions), but they're not important. The script appears to run faster, so thanks for your tips everyone. I will be too busy over the next couple weeks to really do anything more, but I plan on finishing everything off as soon as I can. As the process is the same regardless of whether the player is entering or leaving combat, I plan on making it a function. The only difference is the values the variables take, so it should be fine. To make things easier on me, I've removed the code for power armour. It's pretty much exactly the same as when the player's not in power armour, only with a single slot (and tweaked variable names). Removing it just means that I don't need to make the changes twice. I'll add it back once I've converted the process into a function, hopefully I can set up checks for the number of slots. It should be easy enough, there are only a couple differences, which I can set up if statements for or something. I haven't really looked into how functions work in papyrus, I've only created a couple really basic functions so far, but you should be able to pass variables into them.