[System] Item Cleanup

Level 11
Joined
Jul 11, 2010
Messages
422
As you already know, powerups are not properly cleaned when they are picked. I have also found two other bugs with that :
1) Items are not properly cleaned when they are attacked to death (not only powerups are concerned),
2) Items are not destroyed when you use "RemoveItem" on a dead item. Indeed, the model is properly hid but the handle still exists and leaks.
So, in order to properly destroy an item, you have to set its life to more than 0.405 before trying to remove it.

This librairy cleans the dead items every 15 seconds ; that takes care of all the leak caused by items. There are two case in which items won't be cleaned when you use it : if you set an dead item's life to more than 0.405 or if you kill an item when it is carried. In those case, you should remove the item manually (you can properly remove an item if it was carried when it died).

You could disguss the use of a periodic cleanup but you have to know that doing a system which cleans items when they die is much more complicated. With this, you can just put the script in your trigger editor and items will be automatically cleaned.

WorldBounds Library
JASS:
library ClearItems requires optional WorldBounds
/*
    This library fixes the leak and the lag caused by unremoved items,
    including powerups and manually-destroyed items.

    The dead items are periodically removed. You can adjust the period by changing
    the constant CLEANING_PERIOD. Note that items' death animations need some time
    to play so adjust the DEATH_TIME delay accordingly.

    If you don't know exactly what you are doing, you shouldn't change the life
    of a dead item; the items are no longer usable after their death but
    you can still change their life.  If you set their life to more than 0.405,
    they won't be properly cleaned. You should also remove items manually
    if you kill them when they are carried by a unit.
*/
    
    globals
        // Interval between item-cleanups.
        private constant real CLEANING_PERIOD = 15
    
        // Time for the item's death animation, optimized for tomes and runes.
        private constant real DEATH_TIME = 1.5
    endglobals
    
    globals
        private keyword S
        private integer N = 0
        private code s_code
        private boolexpr s_bool
        private timer s_timer = CreateTimer()
        private item array I
    endglobals
    
    private function DeleteItems takes nothing returns nothing
        loop
            set N = N - 1
            call SetWidgetLife(I[N], 1)
            call RemoveItem(I[N])
            set I[N] = null
            exitwhen (N == 0)
        endloop
        call TimerStart(s_timer, CLEANING_PERIOD - DEATH_TIME, true, s_code)
    endfunction
    
    private function CleanItems takes nothing returns boolean
        if (GetWidgetLife(GetFilterItem()) < 0.405) then
            set I[N] = GetFilterItem()
            set N = N + 1
        endif
        return false
    endfunction
    
    private function SweepItems takes nothing returns nothing
        static if (LIBRARY_WorldBounds) then
            call EnumItemsInRect(WorldBounds.world, s_bool, null)
        else
            call EnumItemsInRect(S.world, s_bool, null)
        endif
        if (N > 0) then
            call TimerStart(s_timer, DEATH_TIME, false, function DeleteItems)
        endif
    endfunction
    
    private struct S extends array
        static if (not LIBRARY_WorldBounds) then
            static rect world
        endif
        static method onInit takes nothing returns nothing
            static if (not LIBRARY_WorldBounds) then
                set world = GetWorldBounds()
            endif
            set s_code = function SweepItems
            set s_bool = Filter(function CleanItems)
            call TimerStart(s_timer, CLEANING_PERIOD, true, s_code)
        endmethod
    endstruct
    
endlibrary

Thanks to Troll-Brain and Bribe who helped me to do it.
 
Last edited:
Level 14
Joined
Nov 18, 2007
Messages
1,084
Comments on second library:
In function CleanupDeadItems, why don't you just use GetWorldBounds() directly instead of using a variable?
I think you could also do that in the init function for the timer.

Also, couldn't you have done something like this?
JASS:
private function CleanItem takes nothing returns nothing
    if GetWidgetLife(GetEnumItem())<0.405 then
    // Set the item's life to more than 0.405 in order to properly destroy it.
        call SetWidgetLife(GetEnumItem(),1)
        call RemoveItem(GetEnumItem())
    endif
endfunction 
...
call EnumItemsInRect(r,null,function CleanItem)
If you don't follow that suggestion, you should at least fix IsItemDead since it's using GetEnumItem instead of GetFilterItem.
 
Level 11
Joined
Jul 11, 2010
Messages
422
Comments on second library:
In function CleanupDeadItems, why don't you just use GetWorldBounds() directly instead of using a variable?
I think you could also do that in the init function for the timer.
Well, GetWorldBounds doesnt' return a variable so it causes leak. I know the function is called just once but I am used to make as few leak as I can.
For The CreateTimer, you're right even if it doesn't change a lot ^^.

If you don't follow that suggestion, you should at least fix IsItemDead since it's using GetEnumItem instead of GetFilterItem.
Thanks, I thought I fixed it cause I already saw it.
Using a filter and a callback makes the code more readable in my opinion. If you convince me that it is more important than it looks, I'll follow that suggestion :razz:.
 
Level 8
Joined
Oct 3, 2008
Messages
367
This would be much more helpful if it didn't require a custom creation function to utilize, and such a thing is very difficult to pull off.

Also, I do believe there's one or two scripts out there that do the exact same thing. One is on WC3C. Exact copies of scripts for the sake of competition are never a good thing.
 
Level 11
Joined
Jul 11, 2010
Messages
422
Well, as I said, the powerups bug is known but the script in wc3c doesn't fix the leak.
Just put try to put a "BJDebugMsg(GetHandleId)" after the destruction of the item, and you'll see that it is not removed.

I'm not specially for using such kind of librairies (and it's worse when it has already been done) but :
1) The previous librairy only fix the half of what it is said to fix,
2) This kind of strange bugs would normally be fixed/replaced by Blizzard but I'm not sure that they will actually release new patches for now.

I would fully understand that you think these librairies are not enough usefull or simple in use to be approved but I think this bug is enough hard to guess to deserve a place where it is said.
 
Level 8
Joined
Oct 3, 2008
Messages
367
Hmm. True, Vexorian's script only looks for the most common problem.

Well then. Maybe the CreateItem function could have some name other than 2? Something that's still trivial to change, but actually gives some sort of description of what it does?
 
Level 38
Joined
Sep 26, 2009
Messages
8,477
In the top script, you leak the local pointer 'it' within the CreateItemSafe function - the item pointer should be better off as a global.

>> Just put try to put a "BJDebugMsg(GetHandleId)" after the destruction of the item, and you'll see that it is not removed.

This is not a valid test. Handle pointers in JASS are 'smart', as in the handle id is not recycled until all pointers to it are wiped. Since you still have the variable pointing to the handle and pass that pointer into GetHandleId, the returned result will definitely still match the would-be decaying handle id.

Unless you grabbed the item another way, such as from an enumerator, and the item still appeared, my above argument stands.
 
Level 11
Joined
Jul 11, 2010
Messages
422
Bribe said:
In the top script, you leak the local pointer 'it' within the CreateItemSafe function - the item pointer should be better off as a global.
Waow, I didn't believe you when I read but it does leak. That's awesome. I fixed that.

Bribe said:
This is not a valid test. Handle pointers in JASS are 'smart', as in the handle id is not recycled until all pointers to it are wiped.
You can still enum "removed-when-dead" items and its datas (name, item-type, etc...) are not cleared so I confirm the bug. But thanks to warn (I thought only the "==null" was unsafe).
 
Level 8
Joined
Oct 3, 2008
Messages
367
You should use simple item-in-map enumeration exclusively to clear dead items It appears to be the best method, as it doesn't require the user to change their function calls. It's also not terribly inefficient.

Once you get that first script out of the way, you can increase the efficiency and compactness of the second one by jamming it all into the filter function and eliminating the second function. Like how lots of people use conditions instead of actions.

Just do it every 30 seconds to few minutes and everything will be fine.
 
Level 31
Joined
Jul 10, 2007
Messages
6,307
all in the filter function >.> like was suggested in another post

JASS:
private function IsItemDead takes nothing returns boolean
    return GetWidgetLife(GetFilterItem())<0.405
endfunction

private function CleanItem takes nothing returns nothing
    // Set the item's life to more than 0.405 in order to properly destroy it.
    call SetWidgetLife(GetEnumItem(),1)
    call RemoveItem(GetEnumItem())
endfunction

store Filter(function IsItemDead) into a boolexpr and just use the var instead of converting it every time as Condition(function IsItemDead)
 
Level 38
Joined
Sep 26, 2009
Messages
8,477
I made a script from your original code which cleans everything up and has it running as optimally as possible.

Functions CreateItemSafe and NewItem do the same thing.

JASS:
library ClearItems

/*
    This library fixes the leak and the lag caused by non-removed items.
    This includes powerups and items that are destroyed by killing them
    (by attacking them or setting their life to less than 0.405).
    In order to use it, items must be created by the unique functions

        function CreateItemSafe takes integer rawcode, real x, real y returns item
        function NewItem takes integer rawcode, real x, real y returns item        ** NewItem and CreateItemSafe do the same thing.

    This function creates the item and the trigger needed to clean it after its death.
    Items are removed after a few wait (DEATH_TIME) to let them show their death animation.
    The default wait is 1.5 second, which is enough for all the Blizzard item models.
    If you set this value too high, tomes and runes will display a bright point
    until their destruction.
*/

    globals
        private constant real DEATH_TIME = 1.5 // Set to the highest duration of items' death animations.
    endglobals
    
    globals
        private hashtable Hash = InitHashtable() // For typecasting and reverse-array lookups.
        private trigger Trig = CreateTrigger() // A single dynamic trigger to handle all death events.
        private trigger Exec = CreateTrigger() // A semi-dynamic trigger.
        private boolexpr Eval = null
    endglobals
    
    private struct data
        
        static integer Size = 0
        static data array Stack
        static item array Items
        
        /**
         * To deallocate properly, some array searching is required.
         */
        
        static method add takes item it returns item
            if (it != null) then
                set Size = Size + 1
                set Stack[Size] = data.allocate()
                set Items[Stack[Size]] = it
                call SaveInteger(Hash, 1, GetHandleId(it), Size)
                call TriggerRegisterDeathEvent(Trig, it)
            endif
            return it
        endmethod
        
        static method pop takes integer i returns nothing
            local integer id
            
            // clear old material;
            set Items[Stack[i]] = null
            call Stack[i].deallocate()
            call DestroyTrigger(Trig)
            
            // update reverse-lookup data;
            if (i != Size) then
                set Stack[i] = Stack[Size]
                set id = GetHandleId(Items[Stack[i]])
                call RemoveSavedInteger(Hash, 1, id)
                call SaveInteger(Hash, 1, id, i)
            endif
            
            // shrink the stack;
            set Size = Size - 1
            
            // remake the trigger;
            set Trig = CreateTrigger()
            call TriggerAddCondition(Trig, Eval)
            if (Size > 0) then
                set i = Size
                loop
                    call TriggerRegisterDeathEvent(Trig, Items[Stack[i]])
                    exitwhen i == 1
                    set i = i - 1
                endloop
            endif
        endmethod
        
        static method remove takes nothing returns nothing
            local item it = LoadItemHandle(Hash, 0, 0) // item was typecast from widget.
            local integer id = GetHandleId(it)
            
            call data.pop(LoadInteger(Hash, 1, id))
            
            // delete hashtable entries;
            call RemoveSavedHandle(Hash, 0, 0)
            call RemoveSavedInteger(Hash, 1, id)
            
            // Add 0.50 for margin of error;
            call TriggerSleepAction(DEATH_TIME + 0.50)
            
            // Remove the item completely;
            call SetWidgetLife(it, 1.0)
            call RemoveItem(it)
            set it = null
        endmethod

        /**
         * Fires when a powerup is used or any item is removed or killed.
         */
        static method onDeath takes nothing returns boolean
            call SaveWidgetHandle(Hash, 0, 0, GetTriggerWidget()) // widget will be typecast to item.
            call TriggerExecute(Exec)
            return false
        endmethod

        static method onPurchase takes nothing returns boolean
            call data.add(GetSoldItem())
            return false
        endmethod
        
        static method onPawned takes nothing returns nothing
            local integer id = GetHandleId(GetManipulatedItem())
            local integer i = LoadInteger(Hash, 1, id)
            if (i > 0) then
                call data.pop(i)
                call RemoveSavedInteger(Hash, 1, id)
            endif
        endmethod
        
        static method onInit takes nothing returns nothing
            call TriggerRegisterAnyUnitEventBJ(Exec, EVENT_PLAYER_UNIT_PAWN_ITEM)
            call TriggerAddAction(Exec, function data.onPawned)
            
            set Exec = CreateTrigger()
            call TriggerRegisterAnyUnitEventBJ(Exec, EVENT_PLAYER_UNIT_SELL_ITEM)
            call TriggerAddCondition(Exec, Condition(function data.onPurchase))
            
            call TriggerAddAction(Exec, function data.remove)
            
            set Eval = Condition(function data.onDeath)
            call TriggerAddCondition(Trig, Eval)
        endmethod
        
    endstruct
    
    function CreateItemSafe takes integer rawcode, real x, real y returns item
        return data.add(CreateItem(rawcode, x, y))
    endfunction
    
    function NewItem takes integer rawcode, real x, real y returns item
        return data.add(CreateItem(rawcode, x, y))
    endfunction

endlibrary
 
Last edited:
Level 38
Joined
Sep 26, 2009
Messages
8,477
You should use TriggerSleepAction instead of timers, as you simply do not need those timers whatsoever.

A global filterfunc just makes your script execute more quickly. It reduces the need to call the Filter() native over and over.

This revision of your current library has been made as optimal and foolproof as possible.

JASS:
library ClearItems initializer init

/*
    This library fixes the leak and the lag caused by unremoved items,
    including powerups and manually-destroyed items.

    The dead items are periodically removed. You can adjust the period by changing
    the constant CLEANING_PERIOD. Note that items' death animations need some time
    to play so adjust the DEATH_TIME delay accordingly.

    If you don't know exactly what you are doing, you shouldn't change the life
    of a dead item; the items are no longer usable after their death but
    you can still change their life.  If you set their life to more than 0.405,
    they won't be properly cleaned. You should also remove items manually
    if you kill them when they are carried by a unit.
*/

    globals
        // Interval between item-cleanups
        private constant real CLEANING_PERIOD = 15.0

        // Time for the item's death animation, optimized for tomes and runes
        private constant real DEATH_TIME = 1.5
    endglobals
    
    globals
        private item ToDelete = null
        private trigger Delete = CreateTrigger()
    endglobals

    private function DeleteItem takes nothing returns nothing
        local item it = ToDelete
        call TriggerSleepAction(DEATH_TIME + 0.5) // add 0.5 for margin of error.
        call SetWidgetLife(it, 1.0)
        call RemoveItem(it)
        set it = null
    endfunction

    private function CleanItems takes nothing returns boolean
        set ToDelete = GetFilterItem()
        if (GetWidgetLife(ToDelete) < 0.405) then
            call TriggerExecute(Delete)
        endif
        return false
    endfunction
    
    globals
        private rect WorldBounds
        private filterfunc Filt
    endglobals
    
    private function SweepItems takes nothing returns nothing
        call EnumItemsInRect(WorldBounds, Filt, null)
    endfunction

    private function init takes nothing returns nothing
        set WorldBounds = GetWorldBounds()
        set Filt = Filter(function CleanItems)
        call TimerStart(CreateTimer(), CLEANING_PERIOD, true, function SweepItems)
        call TriggerAddAction(Delete, function DeleteItem)
    endfunction

endlibrary
 
Last edited:
Level 8
Joined
Oct 3, 2008
Messages
367
It's not advisable to use TriggerSleepAction because it waits in real time. And it does so with inaccuracy at that. Which means if there's a sudden lag spike, the wait keeps ticking. That results in odd bugs, which is often enough to even desync a replay from what actually happened.
 
Level 38
Joined
Sep 26, 2009
Messages
8,477
The best way to use timers is with TimerUtils, as destroying timers still leaves a leak in memory and so many people use TimerUtils already.

JASS:
library ClearItems initializer init requires TimerUtils

/*
    This library fixes the leak and the lag caused by unremoved items,
    including powerups and manually-destroyed items.

    The dead items are periodically removed. You can adjust the period by changing
    the constant CLEANING_PERIOD. Note that items' death animations need some time
    to play so adjust the DEATH_TIME delay accordingly.

    If you don't know exactly what you are doing, you shouldn't change the life
    of a dead item; the items are no longer usable after their death but
    you can still change their life.  If you set their life to more than 0.405,
    they won't be properly cleaned. You should also remove items manually
    if you kill them when they are carried by a unit.
*/

    globals
        // Interval between item-cleanups.
        private constant real CLEANING_PERIOD = 15.0

        // Time for the item's death animation; optimized for tomes and runes.
        private constant real DEATH_TIME = 1.5
    endglobals
    
    private struct object
        item Item
    endstruct
    
    globals
        private object obj
        private timer tim
    endglobals

    private function DeleteItem takes nothing returns nothing
        set tim = GetExpiredTimer()
        set obj = GetTimerData(tim)
        call ReleaseTimer(tim)
        call SetWidgetLife(obj.Item, 1.0)
        call RemoveItem(obj.Item)
        set obj.Item = null
        call obj.destroy()
    endfunction

    private function CleanItems takes nothing returns boolean
        if (GetWidgetLife(GetFilterItem()) < 0.405) then
            set tim = NewTimer()
            set obj = object.create()
            set obj.Item = GetFilterItem()
            call SetTimerData(tim, integer(obj))
            call TimerStart(tim, DEATH_TIME, false, function DeleteItem)
        endif
        return false
    endfunction
    
    globals
        private rect WorldBounds = null
        private filterfunc Detect = null
    endglobals
    
    private function SweepItems takes nothing returns nothing
        call EnumItemsInRect(WorldBounds, Detect, null)
    endfunction

    private function init takes nothing returns nothing
        call TimerStart(CreateTimer(), CLEANING_PERIOD, true, function SweepItems)
        set WorldBounds = GetWorldBounds()
        set Detect = Filter(function CleanItems)
    endfunction

endlibrary
 
Last edited:
Level 38
Joined
Sep 26, 2009
Messages
8,477
Do not use 'x' as a struct member name when referring to an item and not a coordinate. I realize this may seem trivial, but it would make create a problem for anyone who didn't fully grasp the script.

My book on Python excessively uses x as a generic placeholder, so I've kinda picked up on a bad habit :p I changed it to display Item, instead.

Hold on.. why do you have lifetime on dead items? : |

For tomes and runes to correctly play their death animations. Vexorian's Powerup Sentinel doesn't factor that i.
 
Level 38
Joined
Sep 26, 2009
Messages
8,477
This one is way better than TimerUtils (faster, fewer handles) and it uses WorldBounds by Nestharus.

JASS:
library ClearItems initializer init uses WorldBounds
/*
    This library fixes the leak and the lag caused by unremoved items,
    including powerups and manually-destroyed items.

    The dead items are periodically removed. You can adjust the period by changing
    the constant CLEANING_PERIOD. Note that items' death animations need some time
    to play so adjust the DEATH_TIME delay accordingly.

    If you don't know exactly what you are doing, you shouldn't change the life
    of a dead item; the items are no longer usable after their death but
    you can still change their life.  If you set their life to more than 0.405,
    they won't be properly cleaned. You should also remove items manually
    if you kill them when they are carried by a unit.
*/
    
    globals
        // Interval between item-cleanups.
        private constant real CLEANING_PERIOD = 15
    
        // Time for the item's death animation; optimized for tomes and runes.
        private constant real DEATH_TIME = 1.5
    endglobals
    
    globals
        private integer s_i = 0
        private code s_code = null
        private timer s_timer = CreateTimer()
        private item array a_item
    endglobals
    
    private function DeleteItem takes nothing returns nothing
        local integer i = s_i
        set s_i = 0
        loop
            set i = i - 1
            call SetWidgetLife(a_item[i], 1)
            call RemoveItem(a_item[i])
            set a_item[i] = null
            exitwhen (i == 0)
        endloop
        call TimerStart(s_timer, CLEANING_PERIOD, true, s_code)
    endfunction
    
    private function CleanItems takes nothing returns boolean
        if (GetWidgetLife(GetFilterItem()) < 0.405) then
            set a_item[s_i] = GetFilterItem()
            set s_i = s_i + 1
        endif
        return false
    endfunction
    
    private function SweepItems takes nothing returns nothing
        call EnumItemsInRect(WorldBounds.world, Filter(function CleanItems), null)
        if (s_i > 0) then
            call TimerStart(s_timer, DEATH_TIME, false, function DeleteItem)
        endif
    endfunction
    
    private function init takes nothing returns nothing
        set s_code = function SweepItems
        call TimerStart(s_timer, CLEANING_PERIOD, true, s_code)
    endfunction
    
endlibrary
 
Level 11
Joined
Jul 11, 2010
Messages
422
I never thought to use code variables like that, that's great :goblin_good_job:.

As I'm much more a jasser than a vjasser myself, I don't really know if it worths to use a less known library for this kind of stuff. If people could state which one they prefer, it would be nice.

For now, I put both. Thanks for updating, by the way :).
 
Level 22
Joined
Nov 14, 2008
Messages
3,256
I'd rather use the TimerUtils version but it doesn't matter. I don't know if there are any bigger performance difference between the libraries.

Although I wonder why you do the "integer(obj)", why not use obj directly? Or maybe it's just me not knowing what I'm talking about ...
 
Level 38
Joined
Sep 26, 2009
Messages
8,477
I did integer(obj) because sometimes you just never know with that cranky vJass compiler.

TimerUtils is slower and more wasteful because you honestly don't need a library of that caliber to handle something so simple. A quick loop through a singly-stacked item list is more efficient than all the data loading from TimerUtils.

WorldBounds isn't really a requirement but a better use of the WorldBounds handle.
 
Top