GetItemCost

[Moderator]

00:35, 11th Feb 2016

Criticism:

One of few possibilities to access the gold, lumber and charges field of item type ids.
The way you've chosen has a few ( small ) flaws, namely:

• Items of type ITEM_TYPE_POWERUP can only be accessed by creating a hero type dummy unit.
• Your system is inefficient on further gold or lumber lookups of the same item type id.

From moderator to user:

You'll not be able to calculate the resource cost of powerup items,
such as tomes or glyphs are.

 

[BPower]

Item costs are absolute values and must not be rounded by using +.5 in R2I.
Why are costs saved as real in the first place? Resouce costs are integer values.

The API should also cover:
function GetItemTypeIdGoldCost takes integer itemTypeId returns integer
function GetItemTypeIdLumberCost takes integer itemTypeId returns integer
function GetItemTypeIdCharges takes integer itemTypeId returns integer

private function GetItemChargesIgnoreZero takes item whichItem returns integer
    return IMaxBJ(1, GetItemCharges(whichItem))
endfunction

Once done you should restore the resource state of the neutral player.
You can't know for what a map maker is using it.
--> Get previous resource state --> Do your stuff --> Restore it.

function UnitAddItemSwapped takes item whichItem, unit whichHero returns boolean
is a wrapper function to UnitAddItem takes unit u, item whichItem returns boolean
Which does not inline, because Blizzard is sometimes stupid.

How does this system work for items of type ITEM_TYPE_POWERUP?
I can tell you want happens. The game season will end in a fatal error,
because adding power ups by function call to non-hero units does crash the game.

costs == 0 can be a valid resource cost information for items.
For example when an item costs 0 lumber.
Better go with HaveSavedInteger(hash, itemId, typeOfCost)

It's very costly to get the item cost in exchange of two unit handles and one item handle.

You might want to compare your code to GetItemCost
This one also has flaws ( I guess ), when the edge of the map is not pathable for units.
In that case the rect enumeration is incorrect, as the shop will not stand in the desired point.
The system never double checks for a properly created shop, hence can fail.

The rect must be draw from the center of the unit with a width and height of 1088
to north, west, south and east.
Also the system must first enumerate all items in the rect and mark them other items.
( SaveBoolean(hash, GetHandleId(enumItem), 0, true)
before ordering the shop command.
The enumerate again all items and find the one which fits in type id and is not marked.
Only then the system works flawless.

Marking an item that it gets ignored is important as items have item-user-data or can
be reference by handle id in other system.

I have a ItemCost library somewhere on my computer.
I can submit it.

Small hint: A not rendered unit ( ShowUnit(u, false) ) can still perform shop orders.

 

[Marebre]

Thanks for the detailed review.

Why are costs saved as real in the first place? Resouce costs are integer values.

I tried to avoid saving default number of charges in the hashtable, and the only way I could think of was saving price per charge instead of price.

The API should also cover:
function GetItemTypeIdGoldCost takes integer itemTypeId returns integer
function GetItemTypeIdLumberCost takes integer itemTypeId returns integer
function GetItemTypeIdCharges takes integer itemTypeId returns integer

Did the first two. Why should GetItemTypeIdCharges be part of API?

Once done you should restore the resource state of the neutral player.
You can't know for what a map maker is using it.
--> Get previous resource state --> Do your stuff --> Restore it.

Fixed.

function UnitAddItemSwapped takes item whichItem, unit whichHero returns boolean
is a wrapper function to UnitAddItem takes unit u, item whichItem returns boolean
Which does not inline, because Blizzard is sometimes stupid.

Fixed.

How does this system work for items of type ITEM_TYPE_POWERUP?
I can tell you want happens. The game season will end in a fatal error,
because adding power ups by function call to non-hero units does crash the game.

I see your point, but I cannot think of a way to calculate power-up cost because it cannot be sold to a shop, and so the value returned will be 0. To fix crashing, dummy is now hero-dummy.

costs == 0 can be a valid resource cost information for items.
For example when an item costs 0 lumber.
Better go with HaveSavedInteger(hash, itemId, typeOfCost)

Fixed.

As for the rest, I think I'm missing something very obvious because it seems I've solved the non-pathable fail by

    call SetUnitPathing(seller,false)
    call SetUnitX(seller,0)
    call SetUnitY(seller,0)

and thus skipped the whole enumeration part. What am I missing?

Small hint: A not rendered unit ( ShowUnit(u, false) ) can still perform shop orders.

Partially true. Shop has still to be shown.

- A good documentation is the first step to proper debugging.

I thought I did proper documentation, or at least placed a lot of comments and tried to make code readable.

 

[Kazeon]

- A good documentation is the first step to proper debugging.

It's his signature btw. I'm not sure tho is it supposed to be a part of of his review or not because somehow your documentation also could use some works Kind of tricky :\

I thought I did proper documentation, or at least placed a lot of comments and tried to make code readable.

You also need to write some documentations as the header of the code. At least it should includes name and short description (and maybe version number) of the resource, required systems, how to import, user API, and credits (depends on the resource tho).

 

[BPower]

Did the first two. Why should GetItemTypeIdCharges be part of API?

Charges directly influence the cost of an item.
An item id can have by default x charges, which determines is base resource cost.
You have to calculcate the default charges anyway, therefore you can also provide an
API to get the default charges set in the object editor.

I see your point, but I cannot think of a way to calculate power-up cost because it cannot be sold to a shop, and so the value returned will be 0. To fix crashing, dummy is now hero-dummy.

ItemResourceCost is a further developed version of Nestharus ItemCost library.
It does allow the calculation of resource cost for items of type powerup.

I thought I did proper documentation, or at least placed a lot of comments and tried to make code readable.

It's my signature and appears everywhere I make a comment.

-------------------------

private function GetItemTypeIdCharges takes integer itemTypeId returns integer
^
This function shouldn't be private.

    //load default number of charges from hashtable
    if (HaveSavedInteger(ItemCost, itemTypeId, ITEM_DEFAULT_CHARGES)) then
        set charges = LoadInteger(ItemCost, itemTypeId, ITEM_DEFAULT_CHARGES)
    else
    //or create new item of the same type and get it's number of charges
        set whichItem = CreateItem(itemTypeId, 0, 0)
        set charges = GetItemChargesProperly(whichItem)
        call SaveInteger(ItemCost, itemTypeId, ITEM_DEFAULT_CHARGES, charges)
        call RemoveItem(whichItem)
        set whichItem = null
    endif
    return charges

^
Returns 1 for items ids with 0 charges and 1 for item ids with 1 charge.

call ShowUnit(seller,false)
^
As you remove the shop inside the function, this line is not required.
You only need it, if the shop is a static unit.

    //just in case that point (0,0) is non-pathable
    call SetUnitPathing(seller,false)
    call SetUnitX(seller,0)
    call SetUnitY(seller,0)

^
In case 0, 0 is not pathable, the shop will also not be on that spot.

Player(PLAYER_NEUTRAL_AGGRESSIVE)
^
Should be stored into a local player.

You have to deal with the case of an item beeing of type power up.
Using a hero unit may not be the best idea. I'm not even sure if heroes
are flushed from the memory once created and removed.

Better disallow to calculate power ups by detecting this item type in the first place.

The system has an unperformant design, as the computation costs are very high for the CreateUnit native.
Especially if one detects unit enter region events via unit indexer systems.

 

[Marebre]

You have to calculcate the default charges anyway, therefore you can also provide an API to get the default charges set in the object editor.

This function shouldn't be private.

Fine. Fine. You win. It's part of the API now.

ItemResourceCost is a further developed version of Nestharus ItemCost library.
It does allow the calculation of resource cost for items of type powerup.

You can calculate item buying price, sure, but certainly not selling price. Except by assuming that Gameplay Constants are not modified, but that's cheating.

^
Returns 1 for items ids with 0 charges and 1 for item ids with 1 charge.

I had no problem with that until I've made GetItemTypeIdCharges public. Now it's fixed and I check for zero charges elsewhere.

^
In case 0, 0 is not pathable, the shop will also not be on that spot.

Fixed that as well. I included "new" test map where (0,0) is not pathable (I used Abyss terrain).

You have to deal with the case of an item beeing of type power up.
Using a hero unit may not be the best idea. I'm not even sure if heroes
are flushed from the memory once created and removed.

Fixed. Now it checks itemType before attempting to calculate its cost.

The system has an unperformant design, as the computation costs are very high for the CreateUnit native.
Especially if one detects unit enter region events via unit indexer systems.

Fixed that as well. Now I have two global dummies.




One more thing. For some reason, it doesn't restore gold/lumber properly (at all). The prices are returned correctly, though.

 

[BPower]

private constant player U = Player(0)?

 

[Marebre]

private constant player U = Player(0)?

Just something I used during testing. Forgot to delete it before uploading.

Here. I've deleted it.

I still have no clue why gold isn't restored.

 

[BPower]

I still have no clue why gold isn't restored.

Because an item can cost gold and lumber,
you however only restore one of the two.
It depends on which resource data you access first for a type id.

 

[Marebre]

Items of type ITEM_TYPE_POWERUP can only be accessed by creating a hero type dummy unit.

I've abandoned idea of using hero dummy. Now I detect item type and return 0 if it's powerup. So dummy isn't hero unit anymore.

Your system is inefficient on further gold or lumber lookups of the same item type id.

Why? I read from hashtable and return result. What more efficient way there is?

While saving gold costs you could also save lumber costs in the same process and vis-à-vis.

I've changed it and now it does exactly that.

Your system code will fail if called inside a module initializer or struct initializer thread.

Why and how to fix that? Could you explain that to me in little more detail?

Currently the neutral player resource is not reset properly. I explained in the comment why.

Fixed that as well. Thanks for pointing it out.

I seldom give such sharp criticism

The point of criticism is to make me improve my code. As long as its precise (e.g. "this here doesn't work properly because of that"), it's very much appreciated.

 

[pred1980]

Whats wrong with this existing library? It exists a long time and work perfect... Just take a few minutes on google... and there it is - Don't wastn your time with coding a system which already exists

Code:

/*
            GetItemCost v1.0.0
               by kingking
               
    GetItemCost is a simple snippet that let you to get the cost 
    of an item without entering the relative cost and item id to
    database, like hashtable.
    
    Function provided :
    GetItemGold(itemId) -> integer
    GetItemLumber(itemId) -> integer
    
    Mechanism description :
    GetItemCost creates a dummy at the middle of map.
    To get item cost, simply add the item to the dummy and ask
    the dummy to buy the item.
    Then store the difference between before and after of the
    lumber and gold.
    
    Requires :
    Table
*/
library GetItemCost requires Table
    
    globals
        private Table Gold
        private Table Lumber
        private unit Dummy
        private integer array ItemIdStack
        private integer StackLevel = 0
    endglobals
    
    private function PreloadItemCost takes integer id returns nothing
        local integer gold
        local integer lumber
        call SetPlayerState(Player(15),PLAYER_STATE_RESOURCE_GOLD,1000000)
        call SetPlayerState(Player(15),PLAYER_STATE_RESOURCE_LUMBER,1000000)
        call AddItemToStock(Dummy,id,1,1)
        set StackLevel = StackLevel + 1
        set ItemIdStack[StackLevel] = id
        call IssueImmediateOrderById(Dummy, id)
        set gold = GetPlayerState(Player(15),PLAYER_STATE_RESOURCE_GOLD)
        set lumber = GetPlayerState(Player(15),PLAYER_STATE_RESOURCE_LUMBER)
        set Gold[id] = 1000000 - gold
        set Lumber[id] = 1000000 - lumber
        // All right, get the item cost here and save to hashtable.
    endfunction
    
    function GetItemGold takes integer id returns integer
        if Gold.exists(id) == false then
            call PreloadItemCost(id)
        endif
        return Gold[id]
    endfunction
    
    function GetItemLumber takes integer id returns integer
        if Lumber.exists(id) == false then
            call PreloadItemCost(id)
        endif
        return Lumber[id]
    endfunction
    
    private struct Initializer extends array
        private static timer t
        private static rect r
        private static integer nearest_id
        private static real nearest_range
        private static item nearest_item
        
        private static method getNearestItem takes nothing returns boolean
            local item i = GetFilterItem()
            local real range
            if nearest_id == GetItemTypeId(i) then
                set range = GetWidgetX(i) * GetWidgetX(i) + GetWidgetY(i) * GetWidgetY(i)
                if range <= nearest_range then
                    set nearest_range = range
                    set nearest_item = i
                endif
            endif
            set i = null
            return false
        endmethod
        
        private static method removeItems takes nothing returns nothing
            loop
            exitwhen StackLevel == 0
                set thistype.nearest_range = 10000000000.
                set thistype.nearest_id = ItemIdStack[StackLevel]
                set thistype.nearest_item = null
                call EnumItemsInRect(r,Condition(function thistype.getNearestItem),null)
                call RemoveItem(thistype.nearest_item)
                set StackLevel = StackLevel - 1
            endloop
        endmethod
        
        private static method addDelay takes nothing returns boolean
            call ResumeTimer(thistype.t)
            //Zero callback, else created item can't be found.
            return false
        endmethod
        
        private static method onInit takes nothing returns nothing
            local trigger trig
            
            set Gold = Table.create()
            set Lumber = Table.create()
            set r = GetWorldBounds()
            
            set Dummy = CreateUnit(Player(15),'nshe',0.,0.,0.)
            call SetUnitPathing(Dummy,false)
            call ShowUnit(Dummy,false)
            call UnitAddAbility(Dummy,'Asid')
            
            set trig = CreateTrigger()
            call TriggerRegisterUnitEvent(trig,Dummy,EVENT_UNIT_ISSUED_ORDER)
            call TriggerAddCondition(trig,Condition(function thistype.addDelay))
            //We have to removed the created item.
            
            set thistype.t = CreateTimer()
            call TimerStart(thistype.t,0.0,false,function thistype.removeItems)
        endmethod
    endstruct
    
endlibrary

 

[Marebre]

Whats wrong with this existing library? It exists a long time and work perfect...

I wanted to be able to detect item selling price, and all the existing libraries I could find detected buying price.

I do realize that it could be done just by multiplying it with Sell Item Return Rate factor (0.5), but I wanted to avoid that.

 

[BPower]

Jass lesson 1.0

Whats wrong with this existing library? It exists a long time and work perfect... Just take a few minutes on google... and there it is - Don't wastn your time with coding a system which already exists

Code:

I was not aware of kingking's library.
After reading the code I have to say it has couple of flaws and can potentially malfunction.

1.) The code will not work if called from a function initalized in a module.
---
This not essentially bad, if you don't use module initializers at all.

2.) The entire map world is enumerated ( You may realise the GetWorldBounds ).
Depending on you map concept that can be
1 up to 1000+ items, which means 1 to 1000+ threads must run.
---
Item pathing checks run up to 1024 units. If there is no valid point in that radius,
the item ignores pathing and is placed directly under the shop.
Therefore only a radius of 1024 + 64 = 1088 must be enumerated.

3.) The library kills the closest item of that item type id, not the item you bought.
Most times it is the same, but for sure not always.
---
Consider what I mentioned above. The pathing thing! If an item of equal type blocks
the pathing for the sold item, that other item will vanish. What is now on the map
is a perfect copy in GetItemTypeId(item), but not in GetHandleId(item) nor GetItemUserData(item).
Systems which reference these values will fail for that item from then on.

4.) Trigger condition + timer timeout + a stack.
---
Seriously not requried complexity and overhead.
Everything can run within one thread as I did it in ItemResource Cost.

5.) It doesn't reset the neutral player resource state.
---
Not a problem for most maps, for others have fun searching until you find the
issue in the library you imported as public resource.

I've abandoned idea of using hero dummy. Now I detect item type and return 0 if it's powerup. So dummy isn't hero unit anymore.

I know, it's just a limit of the code that should be pointed out to users in
the moderator review.

Why? I read from hashtable and return result. What more efficient way there is?

Check how much code is evaluated in my Item Resource Cost to get already
saved lumber and gold cost. You'll it's much shorter. Here you call other
function which evaluate other conditions. Basically you just have to re-arrange the code a bit.
Use Item Resource Cost as template.

Why and how to fix that? Could you explain that to me in little more detail?

This one is intersting. It relates to how the JassHelper is compiling code.

When you make a spell or a system you will end up using
library MyCode initializer Init.
initializer is a key word for the JassHelper that this function should
run on map init after the blizzard variables are initialized.

Now how can we influence what runs before other code.
Obviously you shop must be created before GetItemCost is called.

Four classic initializers are available:
1.) The GUI on map init. This one always runs very last.
2.) library MyCode initializer Init runs before the GUI initializer
3.)

struct MyStruct extends array
    private static method onInit takes nothing returns nothing
    endmethod
endstruct

4.)

private module Inits 
    private static method onInit takes nothing returns nothing
    endmethod
endmodule 
struct MyStruct extends array
    implement Inits
endstruct

Numer 4 runs first before all other initializers.
Therefore code inside that initializers can't use GetItemCost.

You can see Item Resource Cost, how to use a module initializer.

 

[pred1980]

Okay, can someone of the experts write a good "GetItemCost" library pls? I need it too.

Reg.

 

[BPower]

Okay, can someone of the experts write a good "GetItemCost" library pls? I need it too.

Reg.

If you don't need powerup costs, you can go with this one.
Just wait until Marebre changes the initializer to a module initializer

 

[pred1980]

okay, thank you. And thx for the advice of my script. I think I'll change it. I thought its good.

 

[Marebre]

Your system is inefficient on further gold or lumber lookups of the same item type id.

Rearranged the code a bit. I don't think I could make it any more efficient than this.

Items of type ITEM_TYPE_POWERUP can only be accessed by creating a hero type dummy unit.

Correct me if I'm wrong, but it should not be physically possible to determine selling price of power-up items (without knowing the associated Gameplay Constant).

On a related note, I added some fail-safe mechanism because I now use global dummies and they can be killed. So I gave them invulnerability and still check if they're dead whenever calculated cost is 0.

It could happen, right? Supposing someone casts "kill all units in AoE" spell or something.

What's the point of extends array part?

On a loosely related note, can that item selling sound me temporarily muted while dummy sells the item? I tried fiddling with VolumeGroupSetVolume native but couldn't get it to work. Could it be done? And by done I mean done properly, so none of that "replace sound file with empty file" stuff.

 

[BPower]

First of all we have spamming rules. Please do not double post in our forums
in a short interval ~ 72h. Rather edit your previous post via "Edit" button on the right.

Correct me if I'm wrong, but it should not be physically possible to determine selling price of power-up items (without knowing the associated Gameplay Constant).

But the buying price.
It's ok, I approved the system with a note that this is not possible with this type of code.

On a related note, I added some fail-safe mechanism because I now use global dummies and they can be killed. So I gave them invulnerability and still check if they're dead whenever calculated cost is 0.

Give them locust aka 'Aloc' instead. Then you don't have to check if they are dead.

It could happen, right? Supposing someone casts "kill all units in AoE" spell or something.

For locust units not.

What's the point of extends array part?

Ha... I can't explain that one in 2 sentences.
Just copy it from my code.

For more info tutorial section

 

[Marebre]

Give them locust aka 'Aloc' instead. Then you don't have to check if they are dead.

But they could still be killed with triggers if you're really determined. For example by killing all units owned by neutral passive.

Is it probable scenario?

Or I should just go with locust, and if someone really does kill them, it's his fault?

 

[BPower]

But they could still be killed with triggers if you're really determined. For example by killing all units owned by neutral passive.

Is it probable scenario?

Or I should just go with locust, and if someone really does kill them, it's his fault?

The chance is too small that this happens. It's the users fault then.

 

[Marebre]

It's been a week or so, what about:

Your system is inefficient on further gold or lumber lookups of the same item type id.

I rearranged the code a bit, is it efficient enough now?