[LE] Code optimization questions

[jacobpaige]

jacobpaige wrote:

 

"You should really consider picking better names."

That's funny.. did you ever heard about "KISS-Formel" (Keep It Short and Simple). My advice in programming, use short names for function variables and longer names (expressions) for script properties and variables.

 

"The less time people have to spend on remembering what a particular variable is referring to, the faster and easier it will be for them to understand the code."

Most of my comments and also included URLs are made to reflect to the script code snippet you provided us.

 

"Additionally, it drastically reduces the number of comments you need to write and (more importantly) maintain. Its also a good idea to follow Bethesda naming convention."

Not for sure, name convention is important, but it does not make the compiled script better or faster.

 

I have actually, and my suggestion is fully in line with that principle, and fully in line with the best practices of multiple professional programmers that I've known and several different books on writing good/clean code. Still, it's just a suggestion. If it's not your thing, it doesn't matter.

 

You asked also:

"Is there a reason to get rid of EquipPlayer() and move its functionality into SwapEquipment()?"

Do you really understand the code inside the function:

;--------------------------------------------------------
FUNCTION SwapEquipment(Actor aRef, Actor aRef2, Form[] a)
;--------------------------------------------------------
    bool bOK = (aRef2 == Game.GetPlayer())                    ; who is the player, if TRUE then player is Target
    int iMax

    IF ( bOK )
        aRef.RemoveItem(bList as Form, 1, TRUE, aRef2)        ; ## take a formlist to move items in a rush to the player
        iMax = bList.GetSize()
    ELSE
        aRef.RemoveItem(aList as Form, 1, TRUE, aRef2)        ; ## take a formlist to move items in a rush to the NPC
        iMax = aList.GetSize()
    ENDIF

int i = 0
WHILE (i < iMax)
;##    aRef.RemoveItem(a[i], 1, TRUE, aRef2)                    ; ## THIS is obsolete because of formlist usage!
    IF ( bOK )                                                       ; * equip player only
        aRef2.EquipItemEx(a[i], equipSound = false)                  ; SKSE required!
    ENDIF                                                            ; *
    i = i + 1
ENDWHILE
ENDFUNCTION

I removed every single call of RemoveItem() by using a formlist here!! That means, the following loop is empty and ready for EquipPlayer() action.

I'm aware? When I said you were making it needlessly complicated, I was referring to the inclusion of the unnecessary if statements. If you have an EquipPlayer() function, then it can leverage Properties to know which form list was the NPCs, and which actor was the player without a single if statement. At that point, the SwapEquipment() function becomes little more than an alias for Actor.RemoveItem(FormList, 1, true, Actor). Honestly, we could get rid of the SwapEquipment() function altogether. I just like using it to explicitly tell the reader what's going on so that they don't have to think about the fact that RemoveItem actually transfers items from one object to another when a target is specified. That is, it's a way of getting rid of a comment. It also allows for changes in implementation, which came in handy when I made the switch to FormLists.

 

By the way, in your implementation, you should consider making the while loop "while(bOK && i < iMax)" as this will cut the number of loops in half (assuming both actors have the same number of items).

 

jacobpaige wrote:

 

"Also, is there a reason that you're working so hard to avoid FormList.ToArray()? Is it that expensive of an operation?"

No idea.. I never used SKSE functions. But you get filled the formlist for free by implementing into GetWornEquipmentButNotHands().

  form fm = aRef.GetWornForm(iSlot)                            ; SKSE required!
  IF (fm as Armor)
      a[i] = fm
      i = i + 1
      myList.AddForm(fm)        ; fill the specific formlist, myList as function variable is a pointer to hidden script properties "aList" or "bList"
      iMask += fm.GetSlotMask()                                 ; SKSE required!
  ELSE
      iMask += iSlot            ; no armor was found on this slot
  ENDIF
    ENDIF

Cheers..

 

 

Ah, well, I have to use SKSE for EquipItemEX(), to avoid the bugs in EquipItem(), so I'd rather just continue to leverage it for ToArray() to simplify a lot of the code.

 

Thanks for all the suggestions by the way. I realize how much time it must have taken to write all that, and it's helpful to see the approaches that other people would have taken.

 

For reference, here's the current version of the code (still need to add the SKSE check):

 

 

Scriptname EquipmentSwappingScript extends activemagiceffect  
{Swaps equipment between NPC and Player, if that NPC does not have a designated outfit.}

FormList Property playerGearForSwapping Auto

FormList Property npcGearForSwapping Auto

Actor Property PlayerREF Auto

Function GetWornEquipmentButNotHands(Actor akTarget, FormList akWornForms)
{adapted from https://www.creationkit.com/index.php?title=Slot_Masks_-_Armor example}
    int index
    int slotsChecked
    slotsChecked += 0x00100000
    slotsChecked += 0x00200000 ;ignore reserved slots
    slotsChecked += 0x80000000
 
    int thisSlot = 0x01
    while (thisSlot < 0x80000000)
        if (Math.LogicalAnd(slotsChecked, thisSlot) != thisSlot) ;only check slots we have not found anything equipped on already
            Armor thisArmor = akTarget.GetWornForm(thisSlot) as Armor
            if (thisArmor)
                akWornForms.AddForm(thisArmor)
				index += 1
                slotsChecked += thisArmor.GetSlotMask() ;add all slots this item covers to our slotsChecked variable
            else ;no armor was found on this slot
                slotsChecked += thisSlot
            endif
        endif
        thisSlot *= 2 ;double the number to move on to the next slot
    endWhile
EndFunction

Function SwapEquipment(Actor akFirst, Actor akSecond, FormList akListOfItems)
	akFirst.RemoveItem(akListOfItems, 1, true, akSecond)
EndFunction

Function EquipPlayer()
	int idx = 0
	Form[] listOfItems = npcGearForSwapping.ToArray() ;EquipItemEx will not take FormLists
	while(idx < listOfItems.Length)
		PlayerREF.EquipItemEx(listOfItems[idx])
		idx += 1
	endWhile
EndFunction

Function FindAndSwapGear(Actor akNPC)
	GetWornEquipmentButNotHands(PlayerREF, playerGearForSwapping)
	GetWornEquipmentButNotHands(akNPC, npcGearForSwapping)
	
	SwapEquipment(PlayerREF, akNPC, playerGearForSwapping)
	SwapEquipment(akNPC, PlayerREF, npcGearForSwapping)
	
	EquipPlayer()
	
	;cleanup
	playerGearForSwapping.Revert()
	npcGearForSwapping.Revert()
EndFunction

Event OnEffectStart(Actor akTarget, Actor akCaster)
	if (akCaster == PlayerREF && akTarget.GetActorBase().GetOutfit().GetNumParts() == 0)
	;to prevent item duplication
		FindAndSwapGear(akTarget)
	elseif(akTarget == PlayerREF)
		FindAndSwapGear(akCaster)
	endif
EndEvent

 

 

[foamyesque]

 

I find downcounting generally easier to work with, but it's pretty minor all told. If you find upcounting easier to understand and maintain absolutely use that.

 

FormLists need to be created in the CK but they don't need to have anything added to them at that time. Skyrim supports GetSize(), AddForm(), RemoveAddedForm(), Revert(), GetAt(), Find(), and HasForm() commands for manipulating the data in them. They are often the best way to pass information to and from major game systems, such as alias conditions, or to Add/RemoveItem calls, or things of that nature, where arrays -- because they were retrofitted in after release -- aren't accepted.

Formlists are known to get slower the bigger they are in size, where as arrays are just fast period, except in situations that you've already outlined. I learned this the hard way when I worked with 100+ forms in a list. It took about 10 seconds to go through the whole thing - mileage of course varies depending on the system(I use a laptop).

 

 

Certainly. If I had to iterate through a FormList of any substantial size -- and I've had cause to go through a set of FormLists containing every single item you can put in an inventory, so it's not a case I'm unfamiliar with -- I'd use ToArray and iterate through the array. The reason to use a FormList here is to avoid the RemoveItem loop as well as needing to use SKSE functions to dynamically size an array. The likely case of the FormLIst is that it'll contain six (helm body boots gloves ring amulet) to fourteen (circlet, cloak, bandoleer [x4], underwear, forearm) items (or one more if jacob includes shields), which is reasonably small to work with.

 

It might be worth investigating the array system and seeing which one is actually faster, though. Jacob's saying the scan is really slow, so perhaps working with an array and then down-sizing it, avoiding AddForm, is a better call.

 

Also, jacob, I notice your current implementation still retains the scan of both player & npc. You could cut the time of the scan down in half by doing the preload, and on the activator side even further -- you say it's linked to a mannequin, and so if that's a static link and not a dynamic target like the spell, you can also track exactly what the mannequin has equipped. That pulls your activator case to the two RemoveItem calls and then the Equip loop.

 

With an SKSE check you can also have your script fail gracefully via using non-SKSE functions as a backup option. They'll be slower but your use of SKSE is relatively limited (ToArray and EquipItemEx) and there are vanilla ways to make those work. You'll need an alias on the player to listen for OnPlayerLoadGame() to check for it, though.

[jacobpaige]

 

It might be worth investigating the array system and seeing which one is actually faster, though. Jacob's saying the scan is really slow, so perhaps working with an array and then down-sizing it, avoiding AddForm, is a better call.

 

Also, jacob, I notice your current implementation still retains the scan of both player & npc. You could cut the time of the scan down in half by doing the preload, and on the activator side even further -- you say it's linked to a mannequin, and so if that's a static link and not a dynamic target like the spell, you can also track exactly what the mannequin has equipped. That pulls your activator case to the two RemoveItem calls and then the Equip loop.

 

With an SKSE check you can also have your script fail gracefully via using non-SKSE functions as a backup option. They'll be slower but your use of SKSE is relatively limited (ToArray and EquipItemEx) and there are vanilla ways to make those work. You'll need an alias on the player to listen for OnPlayerLoadGame() to check for it, though.

 

I actually noticed a very clear difference after switching to FormLists. Before, the NPC would be naked for a while. Now, the clothing swap is basically instantaneous once it happens. The only slow part is the actual compilation of the lists.

 

I'm trying to make it affect all mannequins in the game. So, if someone were rearranging their displays by swapping gear with various mannequins, and changing a few items they themselves were wearing inbetween mannequins, then both lists would be changing constantly. Or is there a way to create a FormList linked to every mannequin in the game (that wouldn't waste massive amounts of memory)? If so, that would dramatically reduce the number of checks I'd need to do since I could just adjust the lists in the OnAddItem and OnRemoveItem events. Though, in this case, I'd be back to the problem of keeping the lists straight during the swap and subsequent EquipPlayer().

 

The non-SKSE version is a good idea. I'll work it in once I've taken care of the other, higher priority items. Though, I'll have to leave the player naked if I don't use SKSE, or specifically ignore player enchanted items. Or was that bug fixed in EquipItem()?

 

Speaking of mannequins, I was checking how hard it would be to uninstall my mod, and even with a complete reinstall of the game the changes to mannequins didn't go away. So, I manually reverted the changes and I'm currently writing a script that inherits from the mannequin script that I'll attach to the mannequins instead. And since I'm doing it this way, it occurred to me that I could add a menu and have one option be Swap and the other be Normal Activation (or whatever), which would call the original activation method. This would allow the possibility for other mannequin related mods (posing or whatever) to function as intended, and give players the option to dress the mannequins without stripping themselves.

 

Is there a way to pass the activation event to the parent script, or do I just need to duplicate that part of the code and not worry about compatibility with other mods?

 

Edit: Actually, just tried it in game and, for whatever reason, the parent script's OnActivate took precedence over the child's. Is there a way to fix that without altering the parent script?

Edited May 7, 2018 by jacobpaige

[foamyesque]

This is intended specifically to work only with mannequins, to be clear? What's your goal exactly; what are you trying to change about the default activation setup?

[jacobpaige]

This is intended specifically to work only with mannequins, to be clear? What's your goal exactly; what are you trying to change about the default activation setup?

 

At a minimum, I want the activation to result in the mannequin casting the spell with the effect we've been discussing on the player. I can do this quite easily if I alter the activation script directly, but then the mod can never be removed, so that seems like a really bad idea. This is why I was trying to simply make a child of the activation script and override the OnActivation event, which failed for unknown reasons. Here's the code for reference. It's basically just the original code with a lot of the commented code removed, a single line changed and an additional property.

 

 

Scriptname MannequinGearTradeOnActivateScript extends MannequinActivatorSCRIPT  

Spell Property EquipmentSwappingSpell Auto

EVENT OnActivate(ObjectReference TriggerRef)
	PlayCurrentPose()
	EquipmentSwappingSpell.Cast(self, Game.GetPlayer())
	
	;Trace("DARYL - " + self + " Moving to my linked ref")
	MoveTo(GetLinkedRef())
	
	;Trace("DARYL - " + self + " Waiting a second to give me some time to animate to my pose")
	utility.wait(0.1)
	
	;Trace("DARYL - " + self + " Disabling my AI so i'll freeze in place")
	self.EnableAI(FALSE)

EndEVENT	

 

 

 

If possible, I'd also like to make it so that other mods affecting mannequin behavior can coexist with mine. Though, I'm not really sure what that would entail. I was kind of hoping that kicking control back to the parent script would solve the issue for at least some of them, but I honestly don't know if it would since I don't (and can't) know how they all are ( or will be) implemented.

Edited May 7, 2018 by jacobpaige

[jacobpaige]

As I was coding up the non-SKSE version, I realized that I was also using SKSE to figure out what the two actors are equipped with. As far as I can tell, there is no vanilla equivalent. Is that right? If so, then my script is basically just going to have to print a message/notification to the player letting them know that they need to get SKSE if they want my mod to work.

[foamyesque]

As I was coding up the non-SKSE version, I realized that I was also using SKSE to figure out what the two actors are equipped with. As far as I can tell, there is no vanilla equivalent. Is that right? If so, then my script is basically just going to have to print a message/notification to the player letting them know that they need to get SKSE if they want my mod to work.

 

If you're working *strictly* with mannequins you should be able to pull the list of forms from their mannequin script and check those with IsEquipped.

[jacobpaige]

 

As I was coding up the non-SKSE version, I realized that I was also using SKSE to figure out what the two actors are equipped with. As far as I can tell, there is no vanilla equivalent. Is that right? If so, then my script is basically just going to have to print a message/notification to the player letting them know that they need to get SKSE if they want my mod to work.

 

If you're working *strictly* with mannequins you should be able to pull the list of forms from their mannequin script and check those with IsEquipped.

 

Honestly, the most common use case I expect for this is people casting it on a mannequin and then on a follower and then on the mannequin again. So much so that I considered making it three spells, one to target self, one to target an NPC and one to actually carry out the swap, but it ran into a problem with not being able to hit mannequins with spells.

 

I also considered just doing everything through a dynamically created menu that would list the player and all the player's followers when clicking on a mannequin, but that didn't allow the player to swap with the followers directly, and would have added a SkyUI dependency.

[jacobpaige]

I figured out why it wasn't working. It was a load order issue. But now I have a new problem. My mod is overriding the re-texturing from another mod now that I've moved it down in the order, and I really hate the way vanilla mannequins look. Is there a way to attach my script to every mannequin in the game (not just vanilla) that will work with other mannequin mods? I considered using a quest, but, as far as I can tell, that won't allow me to affect all mannequins period, nor would it actually allow me to remove the vanilla activator script from the mannequins so that I could attach my own, so that I don't have to worry about both scripts trying to run at the same time.

[foamyesque]

How you approach this is going to depend on what exactly you're after. As I understand it, you're trying to do the following:

 

1. Allow the player to swap their equipped gear with that on a mannequin or follower (but not NPCs in the general case?);

2. Allow them to do so either through activating the mannequin or casting a spell on either the mannequin or the follower. (Maybe also through follower dialogue? Not specified yet);

3. Do so in a way that will play relatively nicely with other mannequin-modifying mods;

4. Still allow vanilla activate->show inventory mannequin inventory management to occur.

 

Is that correct? Are there any other pieces to this? Am I missing anything?