[System] Item Removal System

[deathismyfriend]

Updated and moved to spell section.

 

[Almia]

Your function names are too vauge.

Your coding is JASS-bourne,meaning you code like in JASS. We are in vJASS now bro.

It would be better if you use structs and this:http://www.hiveworkshop.com/forums/jass-resources-412/repo-comment-headers-192184/

I suggest you using Table,so that it would not waste hash tables.

Also,we don't use any TriggerAddAction,it always creates new thread,so we only use TriggerAddCondition because its much faster and doesn't create new thread.

 

[deathismyfriend]

I didn't know tht about triggeraddaction ill have to change them over to triggeraddcondition. And what do u mean by my function names r 2 vague ? I don't like big function names. As for using table I haven't used anything anyone else created so I'm not sure about using table. I may change it so it can use it but 255 has tables is a lot to go through lol.

I didn't think structs would matter to use them here. Not: I have not used structs much at all so I do not know a lot of stuff about there efficiency in coding and everything like tht. I hope to be able to sit down and learn them soon.

I like the commenting thing I have up top short and to the point. Easy to find how to install use and so on. I will take a look at the comment link tht u gave me and I'll see if I like it better I will change it.

 

[Almia]

They are too simple(e.g. Actions)

I suggested you to use Table because there are so many resources that uses hashtables

 

[Maker]

It would be nice to be able to pause the timer, for example during cinematics.

 

[Almia]

^ Its pauses timer in the code right?

 

[Maker]

^ Its pauses timer in the code right?

There could be pauseItemDecay and resumeItemDecay functions that can be accessed from outside the library.

 

[Almia]

Oh now i get it

 

[deathismyfriend]

Being able to pause the timer makes sense tht should be easy enough to do. What do u mean by pauseItemDecay and resumeItemDecay ? We're u just suggesting names to use for pausing the timer ?

On the function names im not sure ill ever change them I like them short it's easy to understand.

And since This is a public resource I think I will switch if over so it uses table.

Edit: I hope to be able to add the pause timer option and to make it useable with table when I get out of work. Thx for your suggestions plz keep them coming lol

 

[deathismyfriend]

added the pause item timer function. lets u pause and unpause timer when u want.

also trying to get it to work w Table by bribe. i have to learn structs tho. if anyone could tell me how to assign items like i did above and how to use the condition like i did above to see if it is in Table ? ik how to create a Table array ( i think thts the one i need ) thx in advance

 

[Maker]

if b then

and don't destroy the timer.

 

[deathismyfriend]

ive been told do not pause repeating timers. if u pause them destroy them and remake them. what should i do then ?

edit: i think i got the table working right

edit2: i have noticed a problem tho. a pre created unit tht drops item on death does not trigger the player unit drops item event. gonna have to fix this b4 it becomes a viable system. any ideas ? i was think of every 1 second i can enumerate all items in map but id rather not do tht since u would have to set a boolean if u dont want tht item to be removed ( quest item ). all items tht were created in the editor would be removed also if i do it this way

i think i found a solution when a unit dies i can enumerate items in area around were the unit died. mybe this will work?

 

[Maker]

ive been told do not pause repeating timers. if u pause them destroy them and remake them. what should i do then ?

If you pause a periodic timer and then resume it, it won't be periodic anymore. I think if you restart it again as a periodic, it will work ok.

 

[deathismyfriend]

If you pause a periodic timer and then resume it, it won't be periodic anymore. I think if you restart it again as a periodic, it will work ok.

ooo ok i thought tht would cause a leak thx

 

[Zwiebelchen]

Seriously, this system is far too inefficient for a public resource, for several reasons:

1) Enuming all items in map area each timer interval AND looping through an array of all stored items at the same time just to decrease the item counter?
Use an item indexing system instead and use the event fired on registering the item instead. Most item indexers have all possible ways to create items on ground covered so you can avoid that enum.

2) You do not need to loop through the arrays of items every time. Instead, sort the items in your array depending on the time remaining with a linked list (that way you only need to loop through the items that decayed). When an item timer is paused, remove that item from the linked list and add it back in as soon as the timer is resumed. That way you only need to loop through all the items when an item is created or removed.

3) Your system does not have any protection against killing or removing items. Killed or removed items should be cleared from the system automaticly

4) You can avoid the search loop in your pickup trigger by having an array storing the current position in the item table (well, in this case, speed doesn't matter that much, I think, so it should be okay to loop here).

5) Improve the structure of your code to improve readability, especially the conditions.

 

[deathismyfriend]

Well tht should help w a couple of things only problem I have is when u kill a unit and the unit drops an item the event unit drops item doesn't fire for them. I meant to have another event tht fires when a unit does then it will enum items. I forgot to out tht in and remove the one tht enum items every second.

When an item gets killed sold or removed by some other means definitely has to be covered in this system. To be honest I just forgot about tht lol thx

I only have one item timer. When the timer goes off it just adds a +1 to each item in the array. Tht way when the item array hits ur specific time u want the items to remove at then the item is removed.

Also can u check and see I'm not sure I'm using table the right way. When I enum items its supposed to check the isnotintable condition the one w enum items. It does work when first adding the item to table but when enumerating through items it still enumerates the items tht r in table. Ill have to check all of these suggestions after work. Thx for the good suggestions. I'm not sure if I will use indexer or anything like tht but I will check what's the best way to do it and go from there.

also next time u comment on something plz say the function name w the comment.

 

[deathismyfriend]

added some new stuff and i plane on making this use structs but i have to learn how to use structs first. hopefully ill have enough time to learn them soon and then implement it into this. if anyone wants to show me how i should do the structs feel free. all the help and criticism is appreciated.

 

[deathismyfriend]

redid the code to use structs may have some bugs. i did not have time to test this so if anyone sees any screw ups plz tell me

 

[Magtheridon96]

Jesus, will someone ever make an Item Indexer and post it in the JASS section?

edit

struct Items extends array
function PauseItemTimer takes boolean b returns nothing
function DoNotRemoveItem takes item itm, boolean b returns nothing
function RemoveItemWithDiffTime takes item i, integer T returns nothing

Honestly, the API isn't very good :/

DoNotRemoveItem isn't a proper function name in any programming/scripting language.

struct Items is too vague. The name should be referring to some sort of recycler or recycling unit.
Something like ItemSweeper or MapItemCleaner or whatever. Anything specific to this library would be good.

PauseItemTimer is also a pretty bad name as it can be referring to any timer that deals with items.

You have 2 options here:

• Move the functions to the body of the struct so you can shorten the names after beefing up the struct name.
• Beef up the function names so that they fit into the library. Also, follow everyone's naming conventions here :v.

Personally, I would just put them in the struct because coming up for a good name for the second function is really difficult.

struct ItemSweeper extends array
    static method start takes nothing returns nothing
    static method pause takes nothing returns nothing
    static method resume takes nothing returns nothing

    static method isPaused takes nothing returns boolean
    static method isRunning takes nothing returns boolean

    static method protectItem takes item it returns nothing
    static method unprotectItem takes item it returns nothing
    static method isItemProtected takes item it returns boolean

    static method setItemExpiration takes item it, real time returns nothing
    static method getItemExpiration takes item it returns real
endstruct

 

[deathismyfriend]

ok tht makes sense. hopefully i can change them like tht soon. i just started learning structs so i think i need to learn a little more about them. hopefully trying to make this a good system will do tht for me. it may take a while since i dont have much time to work on this stuff atm but hopefully soon i can do it. thx for ur suggestions

 

[Almia]

Mag
Nestharus wants an Item Indexer that doesn't use EnumItems.

 

[Magtheridon96]

It would need to hook CreateItem or at least provide a function for creating items that would index the items on its own.

EnumItems would only need to be called once at t = 0 to catch items created by some other source like external .j files (Blizzard.j for example)

This indexer would also listen to Shop events and run code accordingly.
Can't think of anything else that would be needed.

Really, the only thing that's stopping some people is the hooking.
Hook just seems... sloppy.

 

[Almia]

I can't remember a bad reason not to use CreateItem,i also gave that idea to Nes,but he said its a bad idea.

 

[deathismyfriend]

Mybe I could make a function or method tht takes the same fields as create item but also adds it to my item removal system? Tht way instead of doing call createitem they call my custom function ? What are ur thoughts on tht ? It would get rid of the enum items which would be really nice but it would require ppl using this system to call the custom function to create items. I would still need to enumerate items when a unit is killed tho otherwise it will miss the items tht killed units drop. But if these units are made to drop item on death through triggers this would be fine.

 

[deathismyfriend]

mybe something like this ? if u think this is a good idea do u have a better name lol ? i was thinking just createItem since the normal one is CreateItem.

    static method createItemPlusIndex takes integer id, real x, real y returns item // better name ?
        local item itm
        local thistype this
        set itm = CreateItem( id, x, y)
        call this.create( itm)
        return itm
    endmethod

 

[Almia]

We want something that can hook CreateItem

 

[deathismyfriend]

ya ik about hooking i was just thinking of an alternative

 

[Magtheridon96]

globals
    private item tempItem = null
endglobals

function CreateItemEx takes integer itemId, real x, real y returns item
    set tempItem = CreateItem(itemId, x, y)
    call thistype.create(tempItem)
    return tempItem
endfunction

Something of the sort was already done by Vexorian at wc3c.net anyway.

 

[deathismyfriend]

which one is the way i should do it in my system ? i could put it in as a struct or a function.

the other thing to take in as consideration is how fast is enumerating through items with a condition tht checks if its in Table ?
Also how fast is Getwidgetlife ? i am using this every second. so if you think tht is too much to do in a second then i will have to change tht.
It loops through all the structs so its called as many times as there is items on the map.

the only other thing to deal w after this is to decide whats the best way to add items to the system tht drop on unit death. i could put a requirement tht all item drops tht deal w a unit death have to be triggered. which i believe most are triggered anyways. i do not believe this will affect when a unit has an inventory and drops items on death.

 

[Almia]

Mag
ItemDex?

 

[Magtheridon96]

Also how fast is Getwidgetlife ? i am using this every second. so if you think tht is too much to do in a second then i will have to change tht.

Don't be silly, you can do 500 GetWidgetLife checks and it wouldn't matter.

which one is the way i should do it in my system ? i could put it in as a struct or a function.

You don't need to do this. An Item Indexer (a totally other resource) would have to take care of that shit for you.

Mag
ItemDex?

Yes.

the other thing to take in as consideration is how fast is enumerating through items with a condition tht checks if its in Table ?

You wouldn't need to check if they're in the Table with a search, you would have to use another table to store booleans while using their handle Ids as indexes.

the only other thing to deal w after this is to decide whats the best way to add items to the system tht drop on unit death. i could put a requirement tht all item drops tht deal w a unit death have to be triggered. which i believe most are triggered anyways. i do not believe this will affect when a unit has an inventory and drops items on death.

There's a limit to how far the items can spawn from the dead unit right?
Create a rect with a certain size and center it around a dead unit, then enum the items in that rect. You would have to run this code using a 0-second timer to be safe though.

 

[deathismyfriend]

Ok thx for the advice and is there a link to ItemDex ?
@Almia: sry didn't know u were suggesting a system / resource.

The way I check tht there in Table is

ItemTable.has( GetHandleID( item))

Is the the right way ?

And as for the small rect I created one and made a timer .25 seconds after unit death it seems to work good I was hoping there was a better way but thx for clearing tht up for me.
Umm only one problem. Can u tell me how to store things other than integers in Table by Bribe ?

Thx for all ur help. Thx for ur help to Almia

 

[deathismyfriend]

had enough time to rework it. i posted the new code if anyone finds any problems plz tell me. i tested it but only very little. didnt have enough time to test it more. any comments on improvements or suggestions are appreciated. thanks to everyone tht helped.

 

[Almia]

As long as you used that key inside ItemTable.anyhandle[key] in the [] op

 

[deathismyfriend]

Ooo I has multiple things tht can set so I didn't use method operators. Do u think it would be better ?

 

[Zwiebelchen]

I checked your updated system and you still loop through all items in the table every timer interval. I already explained this is not needed because:

1) You can count up the game time instead of decreasing the remaining time of existance for items. It's basicly the "if the prophet does not go to the mountain, the mountain walks to the prophet" kind of idea.
2) Store your items in a table keyed by the expected time removal; every time the global game counter is incremented, you only need to check the items that are keyed to that specific counter value

Easy way to get rid of useless looping in the periodic timer function.


@ offtopic: Yes please, we want some ItemDex!

 

[deathismyfriend]

how do i make Table keyed to a struct and a game time ?

 

[Zwiebelchen]

Ah come on... it isn't that complicated:
You count up game time with a timer iterating a global integer by 1 every second. Then you store your items to a table, parent key is the game time integer when the item is created + amount of time until it disappears. Then you store that item to that parentkey.

Every second you can now check all items inside this particular parent key. Pro solution: use a stack or linked list.


This is the optimized system written in pseudo-code:

Item adding mechanic:
- get global time integer
- add the item to the table:
parent key = global time integer + time until item is removed
child key = number of items listed to that parent key + 1 (better solution: add it to a stack)
- store the parent and child key of that item in the same table by parent key = GetHandleId(item) to avoid search loops when wanting to remove the item before it times out

Remove item mechanic:
- get the parent and child key for that item in the item table by looking them up in the table (use the GetHandleId(item) parent key to get them without a search loop)
- clean the item from the table
- move the last item inside that specific parent key up to the position of the removed item (or simply use a stack to do that automaticly)
- decrement the item count for that parent key
- fire the item removal event (it should throw one!)

Every 1 seconds:
- increment global time integer
- loop through all items inside parent key = global time integer and fire the remove method for them


This is how you could write that system to make it effective. All operations are O(1) complexity instead of the O(n) that you use.

 

[deathismyfriend]

ya i actually found out how to do it thx tho just been to busy to implement it. the thing that kept messing me up was the tableArray but when i reread bribes explanation it was clear on how to do it

 

[deathismyfriend]

now it does what ive been meaning to do. it now removes items when there time expires it does not loop through the items anymore

 

[Zwiebelchen]

Good to see you used the ideas I suggested.

some things I still noticed:

1) the time interval until a dropped item is enumed by a rect seems pretty random. Why 0.25 seconds? did you test that? Make sure the interval is as short as possible, to avoid cross-interactions and possible bugs. I bet that 0.00 seconds is enough.

2) your destroy method should have a protection against double frees (you know, people can remove items by trigger and forget to unregister the item to the system before). Add an if GetItemTypeId != 0 then block around RemoveItem and SetWidgetLife.

3) You can get rid of a lot of methods in your API by combining functionality in similar methods.
- resume and pause should be combined to one method: static method pause takes boolean returns nothing (boolean true = pausing, boolean false = unpausing, just like PauseUnit() native)
- createItem should be removed. addItem is enough imho. It shouldn't be the task of this system to actually create the item that you want to add
- protect and unprotect item should be combined to one method like pause and unpause
- isTimerPaused and isTimerRunning are redundant. Remove isTimerPaused.

4) You never define the size of the rect. You definitely should! Also, why a preplaced rect? Simply create a private rect on init by your system!

5) Something that is not neccessarily connected to your system, but should be included:
People usually have inventory systems that drop items instantly back on the ground if they can not be carried by the specific hero/unit. People can abuse this to reset the timer on the item infinitely even if they are not allowed to pick that item up. The item should only be cleared from the system if it remains in the inventory longer than 0.01 seconds [time span should be configurable, in case people use inventory systems with delays].

 

[deathismyfriend]

Ok some good points the protect item and unprotected item and the pause and resume we're suggested that way by magtheridon that's y there like that. The createitem will be removed the getunittypeid will be put in there and ill check o. The other stuff u suggested and hopefully be able to do all of it. The inventory thing is a great idea that should be easy to detect. Ill have to check on the istimerpaused and the other one I may of not noticed I had the one when I was typing it and thx for all ur help.
about the preplaced rect i didnt know u could create a rect till recently lol. also r u sure that get itemTypeID will work?

 

[deathismyfriend]

updated to hopefully include most of what you suggested Zwiebelchen

 

[Magtheridon96]

You can rely on RegisterPlayerUnitEvent to shorten the Init function and to make things more efficient overall after implementing this into a large map with a lot of triggers running on the same events.

static method addItem takes item itm returns nothing
    local thistype data = thistype.create( itm)
endmethod

->

static method addItem takes item itm returns nothing
    call create(itm)
endmethod

To get rid of the evil red BJ call to get the playable map rect, hold Ctrl and click on it in the editor. Have fun c: (You'll see how you can inline it)

The addManipItem function crashes the thread because data has no value when you call data.create(itm).

These are some of the things I can notice.

 

[deathismyfriend]

i thought that this was a safe BJ event to use ? plz correct me if im wrong

TriggerRegisterAnyUnitEventBJ

this method

private static method addManipItem takes nothing returns boolean
            /* passes manipulated items to AddItem method and checks if timer is running */
            local item itm = GetManipulatedItem()
            local thistype data
            if not itemTable.has(GetHandleId( itm)) and not dontRemoveItem.has(GetHandleId( itm)) then
                call data.create( itm)
            endif
            return false
        endmethod

does have something stored in itm. it gets the dropped item that triggers this to run. it then checks if its in the tables i have made. if it isnt then it adds it to the table through the create method. also its updated if i have to change the other BJs then i will update again.

 

[Magtheridon96]

I was referring to the GetPlayableMapRect() BJ
It's totally fine, but it's useless because it just returns a rect global variable.

When I said that addManipItem is reading invalid variables, I was referring to the data variable.

I just realized that create is a static method so data will be ignored when parsing this to JASS code ;/

In that case, you don't need data. You can simply do call create(itm).

 

[deathismyfriend]

oops sry about that. i didnt understand what u meant i fixed it now lol. also did that in another method lol. As for that getplayeablemaprect function i just never checked on inlining it for some reason lol. updated again


this is bugged still dont have a lot of time to find fix atm.

 

[Magtheridon96]

I don't understand the logic here:

if not itemTable.has( id) or not dontRemoveItem.has( id) then
    call itemTable.remove( id)
    call dontRemoveItem.remove( id)
endif

You want to remove the data from the table if it's not present in the table? :v

 

[TriggerHappy]

I don't see the point in this since there's no (accurate) item indexing.

People should just use a simple timer + item queue (or dynamic timers) orRemoveItemTimedwith TimedHandles.

 

[Almia]

I don't see the point in this since there's no (accurate) item indexing.

People should just use a simple timer + item queue (or dynamic timers) orRemoveItemTimedwith TimedHandles.

Agreed. dimf do this NOW!