Optimizing DynDOLOD papyrus scripts

[Borgut1337]

I recently saw a reddit post (https://www.reddit.com/r/skyrimmods/comments/6mp733/thought_i_share_this_how_to_locate_heavy_scripts/) which made me aware of the fact that DynDOLOD does actually use a lot of Papyrus scripting. I just briefly looked through the script source code, and saw at least a few things that can be optimized. Maybe these won't matter a lot for performance, but I suspect they might be noticeable at least, and definitely shouldn't hurt. I'm planning to look into optimizing some of these scripts myself when I have time, maybe later today.

 

Would you mind if I upload these somewhere afterwards (obviously giving credits and all that)? Or would you possibly like to include them in future versions of the mod itself?

[sheson]

I would appreciate if you send any suggestions for improvements to me for discussion / implementation.

[Borgut1337]

I actually only managed to get through a single event so far, can look through more in the following days. I made a few small modifications, and have more suggestions / possibilities worth considering on top of that which I didn't change yet.

 

I made the assumption that the OnTriggerEnter/OnTriggerLeave events can get called relatively often during gameplay (when moving around in exteriors, and/or when transitioning between interior and exterior cells). If this is not the case, it's not really worth optimizing them.

 

I've already changed the following things in the code below:

- Only call Game.GetPlayer() a single time on Event OnTriggerEnter, and cache result, instead of calling same function many times for same result.
- Call self.GetWorldspace() and cast result to String once, instead of doing this twice in the line of code that assigns a value to MyWorld
- Calling JsonUtil.GetStringValue(MyImportFileName, "bunchofnumbers", "x") once and storing the result, instead of calling it twice in a row (I suppose it's safe to assume that the value read from the file is not expected to change in between those two consecutive lines of code)
- fixed indentation in what's now lines 40 and 41. Obviously this isn't an optimization, just figured I'd mention it in case what looked like weird indentation to me was intentional.

 

The following things I didn't change in the code pasted below, but may be worth considering:

- It feels wrong to me to repeat SKSE.GetScriptVersionRelease() and PapyrusUtil.GetVersion() and the DynDOLOD.esp version 2.00 checks frequently during gameplay. In particular the last one could be somewhat expensive too. I suppose a good solution could be to check them all once when the player loads the game, store the result in a bool. Then provide a single globally accessible function to check at once if all three of those versions together are correct, by simply checking if that bool was set to True upon loading the game.
- In lines 51-54, there are four consecutive calls to JsonUtil.StringListGet, with exactly the same lists, but different indices. I would imagine it'd be more efficient to read the list from the file once, save it, and then access it multiple times with the different indices? I'm just not sure if this functionality exists in JsonUtil, not familiar with it.
- I'm not sure I understand what's going on in lines 77-78. The way I read it, if the value in ini is equal to another value, you set the value in the ini file... to that same value again? But they were already equal, so there is no need to do this? I may be missing something here though. The StorageUtil.GetFloatValue() and JsonUtil.GetFloatValue() are also both duplicated in two consecutive lines of code, should be able to call each once and store the results before the IF.

 

I suppose I can just copy/paste the part of the source code that I changed here:

Scriptname SHESON_DynDOLOD_Firstborn extends ObjectReference
;speedrunning is bad mmmkay

ObjectReference MyMaster = None
ObjectReference MyBrother = None
ObjectReference MyBrotherMinion = None
ObjectReference CurrentRef = None
String MyWorld = "None"
String MyImportFileName = "None"
Int MyFirstbornList = 0
Int MyMinionList = 0
Int MyCellList = 0
Int MyFormID
Int Index = 0
Bool MyMasterEnable = FALSE
Bool MyCellDisable = TRUE
Bool MyGridDisable = FALSE

Event onTriggerEnter(objectReference triggerRef)
	Actor PlayerRef = Game.GetPlayer()
	if (triggerRef == PlayerRef)
		if (PlayerRef.GetActorValue("SpeedMult") <= 500)
			if (SKSE.GetScriptVersionRelease() >= 48)
				if (PapyrusUtil.GetVersion() >= 28)
					;Debug.Trace(self + " Player activated cell " + MyMaster)
					if (MyMaster == None)
						if (StringUtil.Find(Game.GetModDescription(Game.GetModByName("DynDOLOD.esp")), "2.00", 0) != -1)
							string selfWorldspace = self.GetWorldspace() as string
							MyWorld = StringUtil.Substring(selfWorldspace, 13, StringUtil.Find(selfWorldspace, "(") - 14)
							if (StorageUtil.GetIntValue(None, "DynDOLOD_Active", 0) == 0)
								Utility.Wait(3)
							endif;
							if (JsonUtil.GetStringValue("DynDOLOD_Worlds", MyWorld, 0) as bool)
								MyImportFileName = "DynDOLOD_" + JsonUtil.GetStringValue("DynDOLOD_Worlds", MyWorld, "None") as string
								if (MyImportFileName == "DynDOLOD_None")
									Debug.Notification("DynDOLOD can not read data from DynDOLOD_Worlds.json")
									StorageUtil.SetStringValue(None, "DynDOLOD_Last_Message", "Can not read data from DynDOLOD_Worlds.json")
								else
									string bunchOfNumbersValue = JsonUtil.GetStringValue(MyImportFileName, "bunchofnumbers", "x")
							        if (bunchOfNumbersValue != (Game.GetFormFromFile(0x00000910, "DynDOLOD.esp") As Keyword).GetString())
								        if (bunchOfNumbersValue == "x")
											Debug.Notification("DynDOLOD can not read data from " + MyImportFileName + ".json")
											StorageUtil.SetStringValue(None, "DynDOLOD_Last_Message", "Can not read data from " + MyImportFileName + ".json")
										else
											Debug.Notification(MyImportFileName + ".json does not belong to this " + "DynDOLOD.esp")
											Debug.Trace("DynDOLOD error: " + MyImportFileName + ".json=" + JsonUtil.GetStringValue(MyImportFileName, "bunchofnumbers", "x") + " DynDOLOD.esp=" + (Game.GetFormFromFile(0x00000910, "DynDOLOD.esp") As Keyword).GetString())
											StorageUtil.SetStringValue(None, "DynDOLOD_Last_Message", MyImportFileName + ".json does not belong to this " + "DynDOLOD.esp")
										endIf
									else
										MyFormID = Math.LogicalAnd(0x00FFFFFF, self.GetFormID())
										if (JsonUtil.StringListGet(MyImportFileName, MyFormID as string, 0) as bool)
											MyFirstbornList = StringUtil.Substring(JsonUtil.StringListGet(MyImportFileName, MyFormID as string, 0), 1, 0) as int
											MyMinionList = StringUtil.Substring(JsonUtil.StringListGet(MyImportFileName, MyFormID as string, 1), 1, 0) as int
											MyCellList = StringUtil.Substring(JsonUtil.StringListGet(MyImportFileName, MyFormID as string, 2), 1, 0) as int
											MyMaster = Game.GetFormFromFile(JsonUtil.IntListGet(MyImportFileName, "master", 0), "DynDOLOD.esp") As ObjectReference
										else
											Debug.Notification("DynDOLOD can not read data from " + MyImportFileName + ".json")
											StorageUtil.SetStringValue(None, "DynDOLOD_Last_Message", "Can not read data from " + MyImportFileName + ".json")
										endIf
									endIf
								endIf
							else
								Debug.Notification("DynDOLOD can not read data from DynDOLOD_Worlds.json")
								StorageUtil.SetStringValue(None, "DynDOLOD_Last_Message", "Can not read data from DynDOLOD_Worlds.json")
							endIf
						else
							Debug.Notification("DynDOLOD requires DynDOLOD.esp version 2.00")
							StorageUtil.SetStringValue(None, "DynDOLOD_Last_Message", "DynDOLOD requires DynDOLOD.esp version 2.00")
						endIf
					endIf
					if (MyMaster != None)
						StorageUtil.SetFormValue(MyMaster, "DynDOLOD_MyCurrentFirstborn", self)
						if (self.GetDistance(StorageUtil.GetFormValue(MyMaster, "DynDOLOD_MyActiveFirstborn", self) as ObjectReference) == 4096)
							StorageUtil.SetIntValue(MyMaster, "DynDOLOD_MyCurrentDistance", 1)
						else
							StorageUtil.UnsetIntValue(MyMaster, "DynDOLOD_MyCurrentDistance")
							if (Utility.GetINIfloat("fminsecondsforloadfadein:Interface") == StorageUtil.GetFloatValue(none, "DynDOLOD_FadeTime", JsonUtil.GetFloatValue("DynDOLOD_Worlds", "fminsecondsforloadfadein", 2.66)))
								Utility.SetINIfloat("fminsecondsforloadfadein:Interface", StorageUtil.GetFloatValue(none, "DynDOLOD_FadeTime", JsonUtil.GetFloatValue("DynDOLOD_Worlds", "fminsecondsforloadfadein", 2.66)))
							endIf
							Index = JsonUtil.IntListCount(MyImportFileName, "c" + MyCellList)
							While Index
								Index -= 1
								CurrentRef = Game.GetFormFromFile(JsonUtil.IntListGet(MyImportFileName, "c" + MyCellList, Index) as Int, "DynDOLOD.esp") as ObjectReference
								if CurrentRef
									if (StorageUtil.GetIntValue(CurrentRef, "CellState") == 0) || (StorageUtil.GetIntValue(CurrentRef, "GridState") == 0)
										StorageUtil.SetIntValue(CurrentRef, "CellState", 1)
										StorageUtil.SetIntValue(CurrentRef, "GridState", 1)
										CurrentRef.Activate(self)
									endIf
								endIf
							endWhile
						endIf
						if !MyMasterEnable && (StorageUtil.GetIntValue(None, "DynDOLOD_Active", 1) == 1)
							(MyMaster as SHESON_DynDOLOD_Master).IBowToTheev2(self, MyFirstbornList, MyMinionList, TRUE, FALSE)
							MyMasterEnable = TRUE
						endIf
					else
						Debug.Notification("DynDOLOD can not find master data in " + MyImportFileName + ".json")
						StorageUtil.SetStringValue(None, "DynDOLOD_Last_Message", "Can not find master data in " + MyImportFileName + ".json")
					endIf
				else
					Debug.MessageBox("DynDOLOD requires PapyrusUtil 2.8 or newer")
				endIf
			else
				Debug.MessageBox("DynDOLOD requires SKSE 1.7.3 or newer")
			endIf
		endIf
	endIf
endEvent

Edited July 12, 2017 by Borgut1337

[sheson]

Those are some great suggestions.

Lets ponder about a few thing:
 

Only call Game.GetPlayer() a single time on Event OnTriggerEnter, and cache result, instead of calling same function many times for same result.

My first question is if Game.GetPlayer() is actually a costly function or not. It could be just as fast as using the variable. You know / any tests? Otherwise I plan to do some timing test sometime soon.

In this case it is only used twice. Assigning it to a variable may just negate the potential time win - if there is one.

If Game.GetPlayer() is costly I would think the best solution would be to set it globally in the script OnInit so all events benefit. The value of Game.GetPlayer() is never going to change in the game, is it.
 

Call self.GetWorldspace() and cast result to String once, instead of doing this twice in the line of code that assigns a value to MyWorld
Calling JsonUtil.GetStringValue(MyImportFileName, "bunchofnumbers", "x") once and storing the result, instead of calling it twice in a row (I suppose it's safe to assume that the value read from the file is not expected to change in between those two consecutive lines of code)

I consequently removed all strings variables and cast to string as much as possible because of the save game string count issue.
 
The same questions as above arise. Is self.GetWorldspace() actually a costly function or not and is assigning it to a variable and reusing it faster? Any tests?
 
Since most of these are inside a if (MyMaster == None) it should only run once ever, so speed optimization is not really required. And not using string variables that count towards the string count seems more important.
 

It feels wrong to me to repeat SKSE.GetScriptVersionRelease() and PapyrusUtil.GetVersion() and the DynDOLOD.esp version 2.00 checks frequently during gameplay.

Yes, this could use some work.
 

In lines 51-54, there are four consecutive calls to JsonUtil.StringListGet, with exactly the same lists, but different indices. I would imagine it'd be more efficient to read the list from the file once, save it, and then access it multiple times with the different indices? I'm just not sure if this functionality exists in JsonUtil, not familiar with it.

The file is only read once and then the data is kept in memory.
 

I'm not sure I understand what's going on in lines 77-78.

 

 It seems silly and can probably be removed. I need to check.

[zilav]

The only thing I know about Papyrus is to avoid using slow Game.GetPlayer() and create an alias with Player ref instead, read it from some guide long time ago

[sheson]

You would think a value that never changes is "fast". I will do some profiling tests with this, anyways. I need to know for sure :)

[Borgut1337]

 

My first question is if Game.GetPlayer() is actually a costly function or not. It could be just as fast as using the variable. You know / any tests? Otherwise I plan to do some timing test sometime soon.

 

In this case it is only used twice. Assigning it to a variable may just negate the potential time win - if there is one.

 

If Game.GetPlayer() is costly I would think the best solution would be to set it globally in the script OnInit so all events benefit. The value of Game.GetPlayer() is never going to change in the game, is it.

One thing I noticed a few years ago when optimizing my own combat mod, is that function calls themselves are already... rather slow in Papyrus. Regardless of what the function does, a function call by itself (even an empty function) takes time. Now of course the circumstances are a bit different, in that for my combat mod performance was REALLY important (if you hit an enemy and he doesn't stagger INSTANTLY, that's noticeable in combat). In the case of DynDOLOD, it's not as essential to have everything completely in real-time... but it doesn't hurt to try to optimize anyway either. And as far as I'm aware, yes, already when you're only at two Game.GetPlayer() calls it technically becomes more efficient to call it once and cache the result, because you avoid the second function call. There's actually some more info, and apparantly better tests than just my anectodal experience under ''Notes'' here: https://ck.uesp.net/wiki/index.php?title=GetPlayer_-_Game#Notes

 

Indeed, the solution I used for avoiding multiple Game.GetPlayer() calls above is not ideal. Like you said, it would be faster to save it once in an OnInit (although this technically increases saved game file size, but by a negligible amount), and it would be even faster to store it in a Property at Creation-Kit time (so that it doesn't need to be set in OnInit at runtime anymore). I imagine the solution with a Property would be difficult in your case though, since you automatically generate these records / the .esp file?

 

However, a significant disadvantage of either of these solutions (Property or setting in OnInit) is that I don't think it will work correctly when updating in an existing playthrough, I think it would require uninstalling the mod, clean saving, and reinstalling. The solution (technically slightly less efficient) I wrote in the code snippet in the previous post would work correctly in an existing playthrough by simply overwriting the compiled script files.

 

The same questions as above arise. Is self.GetWorldspace() actually a costly function or not and is assigning it to a variable and reusing it faster? Any tests?

Same counts for this one, because the problem isn't actually what's happening inside the Game.GetPlayer() or self.GetWorldspace() functions, the problem is just any function call at all.

[sheson]

I did away with any and all properties because they caused a lot of problems also when updating and performance wise (maybe not for for the GetPlayer, but for other things)

 

The clean save already needs to be done when generating a new esp from scratch, because the save file also stores the position of references and other persistent data. It can happen that between two LOD generations the same form ids are used, which then would cause problems if the save is not cleaned. Since I am doing timing tests anyways, I will also try different approaches. So far I only encountered issues with defining Functions in papyrus myself, not with the native ones.

Edited July 13, 2017 by sheson

[sheson]

I found a couple discussions that also report that Game.GetPlayer() is slower than using properties or a cache. So I am just going to take that at face value and since the Firstborn/Minion scripts are only attached to 2 single base records, using a PlayerRef property looks like the best solution. The testing for users doing weird things is what really needed to be addressed.

 

I also did a few tests in another script (LODObject, now caching PlayerRef) where there are a few conditions with kActionRef.GetWorldspace() and PlayerRef.GetWorldSpace() that execute each iteration. Instead of using the functions, I tried using Worldspace / String (although not good for string count) variables instead. With the assignment and all conditions updated, either solution was considerably (twice and more) slower, interestingly. So that function doesn't seem to be as problematic in this case.

 

The next or the second next version will sport these optimized scripts as it requires small changes to patcher of course to add the PlayerRef property. The SKSE/Papyrus tests moved to a once per game load function, etc.