Script effeciency

[icecreamassassin]

So I have the following script working to sort classes of items from the player inventory (or another container) into specific other designated containers, but the script runs very slowly and takes awhile to cycle through the lists of items in the formlist. Is there any way I could make this more efficient?

Scriptname DBM_AutoSortScript extends ObjectReference  

ObjectReference Property HoldingContainer Auto

FormList Property IngotsNOre Auto
ObjectReference Property SortChest1 Auto

FormList Property GemList Auto
ObjectReference Property SortChest2 Auto

FormList Property IngredientList Auto
ObjectReference Property SortChest3 Auto

Event OnActivate(ObjectReference akActionRef)
    Int IOSize = IngotsNOre.GetSize()
    Int index1 = 0
    Int ItemCount1 = 0

    Int GemSize = GemList.GetSize()
    Int index2 = 0
    Int ItemCount2 = 0

    Int IngredientSize = IngredientList.GetSize()
    Int index3 = 0
    Int ItemCount3 = 0    

While index1 < IOSize || index2 < GemSize || index3 < IngredientSize
        If HoldingContainer.GetItemCount(IngotsNOre.GetAt(index1)) > 0
        ItemCount1 = HoldingContainer.GetItemCount(IngotsNOre.GetAt(index1))
          HoldingContainer.removeItem(IngotsNOre.GetAt(index1),ItemCount1,False,SortChest1)
          EndIf
        index1 += 1
 
        If HoldingContainer.GetItemCount(Gemlist.GetAt(index2)) > 0
        ItemCount2 = HoldingContainer.GetItemCount(Gemlist.GetAt(index2))
          HoldingContainer.removeItem(Gemlist.GetAt(index2),ItemCount2,False,SortChest2)
          EndIf
        index2 += 1

        If HoldingContainer.GetItemCount(IngredientList.GetAt(index3)) > 0
        ItemCount3 = HoldingContainer.GetItemCount(IngredientList.GetAt(index3))
          HoldingContainer.removeItem(IngredientList.GetAt(index2),ItemCount3,False,SortChest3)
          EndIf
        index3 += 1
    EndWhile


    Debug.messagebox("All Supplies Sorted")
endevent

Edited May 19, 2014 by icecreamassassin

[IsharaMeradin]

You do not need to loop through the form lists and check each item on the list in order to remove said items from the container. You can simply use the form list itself. Only reason to loop through a form list when adding & removing to a container is if you want the stored forms to go to different locations.

 

Try this, should be much faster

 

You will need to test compilation and performance

Scriptname DBM_AutoSortScript extends ObjectReference  

ObjectReference Property HoldingContainer Auto

FormList Property IngotsNOre Auto
ObjectReference Property SortChest1 Auto

FormList Property GemList Auto
ObjectReference Property SortChest2 Auto

FormList Property IngredientList Auto
ObjectReference Property SortChest3 Auto

Event OnActivate(ObjectReference akActionRef)
	If HoldingContainer.GetItemCount(IngotsNOre) > 0
		ItemCount1 = HoldingContainer.GetItemCount(IngotsNOre)
		HoldingContainer.removeItem(IngotsNOre,ItemCount1,False,SortChest1)
	EndIf
 
	If HoldingContainer.GetItemCount(Gemlist) > 0
		ItemCount2 = HoldingContainer.GetItemCount(Gemlist)
		HoldingContainer.removeItem(Gemlist,ItemCount2,False,SortChest2)
	EndIf

	If HoldingContainer.GetItemCount(IngredientList) > 0
		ItemCount3 = HoldingContainer.GetItemCount(IngredientList)
		HoldingContainer.removeItem(IngredientList,ItemCount3,False,SortChest3)
	EndIf

    Debug.messagebox("All Supplies Sorted")
endevent 

 

 

[icecreamassassin]

Oh ok, yeah I was wondering if the list itself could be polled without having to look at the individual indexes... now I know :smile:

 

Thanks again as always, this works much more cleanly, just had to add the INT variables back in and it worked great

Edited May 19, 2014 by icecreamassassin

[jaxonz]

Assuming you want to move all items of those types to their appropriate containers, the conditional phrases are also not necessary, so the following code would also work:

Scriptname DBM_AutoSortScript extends ObjectReference  

ObjectReference Property HoldingContainer Auto

FormList Property IngotsNOre Auto
ObjectReference Property SortChest1 Auto

FormList Property GemList Auto
ObjectReference Property SortChest2 Auto

FormList Property IngredientList Auto
ObjectReference Property SortChest3 Auto

Event OnActivate(ObjectReference akActionRef)
	HoldingContainer.removeItem(IngotsNOre, 1000, False, SortChest1)
	HoldingContainer.removeItem(Gemlist, 1000, False, SortChest2)
	HoldingContainer.removeItem(IngredientList, 1000, False, SortChest3)

	Debug.messagebox("All Supplies Sorted")
endevent  

Remarks:

• The number 1000 is arbitrary, assuming you wouldn't have more than 1000 of any item type in the container. 10,000 also works, but you might avoid ridiculously high numbers for a 32-bit app.
• You say you have this script bound to the player. Does that mean that player inventory is automatically sorted to these 3 containers? If so, this would probably be considered "lore unfriendly" as inventory will be moved even when those containers are worlds away.
• I'm not certain when the OnActivate event fires for the player but suspect that if it happens at all it may occur quite frequently. You might consider OnMenuClose to trigger only when inventory menu is closed (or OnMenuOpen if you never want players to see the inventory.)... Fewer event occurences will keep the script from bogging your game down.
• Another consideration with respect to binding this to the Player is that it may prevent items needed for crafting form being held in player inventory. (e.g., Player goes to SortChest1 and gets some ores, walks over to Smelter only to discover that the ores have been removed back to SortChest1).

In the end, there's much to be said for binding this type of script to a sorting chest rather than a player. If that is your intent, there are both stand-alone mods such as Dynamic Sorting or modder's resources such as Autosorting that have this functionality and much more. I certainly don't want to discourage you from modding. It's a great experience of its own. Might save you some frustration though.

[IsharaMeradin]

@Jaxonz

The OP stated that it was to use the player inventory or other container. It is scripted as if it were on a button or other activation device. As far as the IF statements, yes they are not necessary. However, I personally have had well over 3000 of one type of item in my inventory before. (I fell asleep while mining a quarry pit :P ). Therefore, it has always been my recommendation to obtain the actual count.

[jaxonz]

First let me say that I respect IsharaMeradin and greatly appreciate the responses IsharaMeradin has provided to some of my questions on these forums.

 

Good catch on that this script might also be used for containers... not sure how I missed that, but I think my advice regarding the perils of attaching such a script to the player it is sound nonetheless.

 

I did state clearly that 1000 was an arbitrarily large number and that a larger value could be used if deemed appropriate. One could safely set this value to 1000000. The ad absurdum reasoning for always checking quantities doesn't seem practical to me. Even if there were a bizarre occasion where there were 2 million items in the container, it is not as if the script would break. Simply activating the script twice would still sort everything. This seems very reasonable to me for the benefit of eliminating a substantial portion of the code.

 

 

On principal, I do agree that condition checking is generally a best practice. Papayrus is pretty frail. It has no error handling other than to abort functions. Beginning modders should follow IsharaMeradin's advice and check for all potential error conditions.

 

Nonetheless, I admit that I would use the simpler statement. Not to get philosophical, but I generally subscribe to the notion that less and simpler code is inherently more stable, performant, and manageable. I can get away with the simpler code only because I understand that calling RemoveItem call on a container which has none of the specified objects will simply do nothing, not even throw an error. This makes it safe to call RemoveItem without the conditional checks.

 

Simply put...

HoldingContainer.removeItem(IngotsNOre, 1000000, False, SortChest1)

... moves up to 1 million instances of every item in the IngotsNOre list from HoldingContainer to SortChest, but does nothing whatsoever if HoldingContainer has no ores or ingots.

 

The only opportunity for error here is if references to the list or containers are not set. These are error conditions for which the original code also did not check.

[icecreamassassin]

Yes, this script is used on a trigger box surrounding a pile of static supplies in the corner of the room. You click on it, it polls the counts from the player and removes the items to the destination containers.

 

The thing about Papyrus which is both awesome and annoying at the same time is that there are dozens of ways you can do any one task and though some may function better than others, as long as they function, there's no "wrong" way of doing them. That said I'm not really in the habit of debating papyrus theory. At first glance though it would appear that your version Jaxonz would either bomb out because you don't have 10,000 of the item, or would incorrectly add 10,000 of the item and remove whatever you had up to that amount. I know it probably doesn't function that way, it probably seeds the count automatically, but that's the kind of tiny error potential that having secondary failsafes is good for. Though I know that it does slow down the script to check another set of conditions or poll specific counts into integers, but ultimately it makes it much more foolproof when combining mods together which may have some strange interactions with one another. When delving into the unknown, it's best to know that all your conditions are clearly established in my opinion. At any rate, thanks for both your guys' help, in the end it is moving smoothly, I'm just going to need to figure out how to kill the message posts, because they scroll through for minutes :P

[IsharaMeradin]

To kill the upper left messages when items are transferred via RemoveItem:

 

HoldingContainer.removeItem(IngotsNOre, 1000, False, SortChest1) <-- this one will print messages

HoldingContainer.removeItem(IngotsNOre, 1000, True, SortChest1) <-- this one will not print messages

[icecreamassassin]

Ah so simple :) thx