[System] [Needs work] Item Drop System

[deathismyfriend]

moved

 

[Zwiebelchen]

Hmm... Isn't this a little bit too simple? Besides, why even write a system for it?
You can totally just use a hashtable and store all the items to the GetUnitTypeId() with the percentages and then retrieve one randomly with 2 or 3 function calls.

But well, ... why not?

The issue I get with this is how the methods are used.
For example, I have over 200 different unit types in my RPG, all with different droptables.
If I would set up my droptables using your system (well, I wouldn't, because I already have an optimized one, but that's not the point), I could not write a simple registry without thousands of lines.
All my unit types have at least 12 available items to drop. This makes 13 method calls each per registry entry. Every single one of them takes a new line and plenty of characters, making a massive registry when you do that with every unit type.

It may look ugly, but it's way more practical to have the create method take a pre-defined number of items and percentages and put everything into one single line. Especially since item rawcodes are not long, so you can fit a lot of them in one single line before the folding point.
This or at least make a wrapper for this.

Besides, I don't get why you even need to loop in your addItem method. I don't even get what you actually do there. Just store the item IDs to the table and the percentage to the table and you're done. Creating an item on the death event doesn't have to be lightning fast, but adding the item at map init should be O(1) for operation limit's sake and ease-of-use.

Also, locust units should be excluded from dropping items.

Percent should be real, not integer. What if I want a drop chance below 1%?

 

[deathismyfriend]

I could make the add item take multiple item. I left it w one item and the percent chance for that item to drop because I don't know what is a good number mybe 5 or 6 or 10. I also don't use Table array in this because it seemed like a little more work the way I did it but much faster in the long run. The looping in the add item is just to set the indexes since I use arrays in this instead of table array. The desth event is very fast for this since it only does one ite which ill need to make slightly bigger but thats still fast. it also does a getrandom integer for the random and creates the item by punching in that number so mo extra function calls to set the chance of an item drop. also i always try to make death events as fast as possible since it id probably one of the most used events for systems. so the faster it is the better. i also think I will change the add item to 5 as that seems like a good number to me. You have a very good point on the addItem calls with how many times it would have to be called and all of the lines of code. Hopefully I can change this and upload a better updated version tomorrow

 

[Zwiebelchen]

The looping in the add item is just to set the indexes since I use arrays in this instead of table array.

Use table. Really, there's no way why you should use arrays here. It limits the amount of registry entries, destroys flexibility and causes an unneccessary complicated and inefficient add method. Have you benchmarked this? I highly doubt your array approach is faster than simple table calls. Sure, table calls require roughly twice the time of an array read, but this doesn't matter in this situation as the additional overhead totally overshadows the minor speed gain.

 

[deathismyfriend]

updated w protection from op limit. easy to add item Tables now. and protection from overwriting old item Tables.

Edit: gonna redo system like u said zwiebelchen after bowling if I have time lol.

 

[deathismyfriend]

updated

 

[Magtheridon96]

It should be struct ItemDropSystem.

The API isn't very intuitive :/

static method create takes integer unitTypeId returns thistype
static method specificUnitCreate takes unit u returns thistype
method changeMultiDrop takes integer multi returns nothing
static method returnTable takes integer id returns thistype

->

static method createForType takes integer unitTypeId returns thistype
static method createForUnit takes unit u returns thistype

// to replace the multidrop functions
method getDropCount takes nothing returns integer
method setDropCount takes integer i returns nothing

static getDataTable takes integer id returns thistype

The addItem function should only take 1 item and 1 weight (Not perfect)
You decide what item to drop based on weights rather than percentages. You can, however keep using percentages. You would let the user pass in weight values and you would adjust percentages of all item registered for the unit/unit-type so that they are consistent and add up to a percentage of 100.

Example:

// add 'I000' drop with weight of 5
gnollDrop.addItem('I000', 5)

// at this point:
// 'I000' -> 5/5 -> 100% drop chance

// add 'I001' drop with weight of 10
gnollDrop.addItem('I001', 10)

// at this point:
// 'I000' -> 5/15 -> 33.333% drop chance
// 'I001' -> 10/15 -> 66.666% drop chance

That's how your system would handle it. This works awesomely because it makes it much more dynamic in terms of registration

By the way, for encapsulation purposes, the user should not have access to the table

 

[deathismyfriend]

i changed the things u suggested. now w the weight system u dont have to worry about ever going over 100 percent for items as it isnt possible w a weight system.
i added another method to add a weight for no item drops. updated

 

[deathismyfriend]

added a test map

 

[deathismyfriend]

bump

 

[Adiktuz]

Pretty neat, though I still like mine better... hahahahaha...

mainly because its easier for me to use (coz I made it, lol)

and because it supports both %chance and weight registrations...

and also because aside from the drop chances of the items, you can also specify the chance that a unit can drop an item... (like a unit should only drop an item from the table, 15% of the time) well, yours can too I guess using the noWeight drop though I think that's a bit slower since you would still go through the table and also, the user will need to adjust the weights in order to achieve his desired "NO DROP" chance...

 

[deathismyfriend]

Pretty neat, though I still like mine better... hahahahaha...

mainly because its easier for me to use (coz I made it, lol)

and because it supports both %chance and weight registrations...

weight drop is a percent chance. lets say i have 5 items with a weight of 20 each. They now have a 20 Percent chance.
If i have 2 Items one w 10 weight and 1 w 20w. The one w 10 weight has a 33.33% drop chance. The 2nd item has a 66.66% drop chance.

and also because aside from the drop chances of the items, you can also specify the chance that a unit can drop an item... (like a unit should only drop an item from the table, 15% of the time)

that can be done in mine see.

method addZeroDropWeight takes integer weight returns nothing
- this is how u add a weight for no item drops.

 

[Adiktuz]

yes, I know that dude... What I just wanna say is that, on mine, you can directly specify chances or weight, depending on what version ur using... and of course, weight is always safer than direct chances...

as for 2), I realized it and edited my post above earlier than your post

well, yours can too I guess using the noWeight drop though I think that's a bit slower since you would still go through the table and also, the user will need to adjust the weights in order to achieve his desired "NO DROP" chance...

btw, I have a suggestion... can you add a copy function? so that it will be easier for two units to have the same drop table...

something like

static method copyForType takes integer unitID, integer copyFromID returns thistype

 

[deathismyfriend]

i could easily add a copy method to make it easier. Thats a great idea. +rep

i will have to make this tomorrow tho lol gotta go to sleep very soon lol.

thanks for the suggestion.

 

[Adiktuz]

uhm,

/*struct itemDropSystem extends array
/ *  
/ *             static method createForType takes integer unitTypeId returns thistype
/ *                 -   this is what you need to start an item drop table. 
/ *                 -   Below is how u add the items to the table
/ *
/ *             static method createForUnit takes unit u returns thistype
/ *                 -   this is used for creating item tables for specific units do not call the create function for these units.
/ *
/ *             static method destroy takes nothing returns nothing
/ *                  -   to destroy an item drop table for the unit type id
*/

destroy is an instance method, not a static method...

method destroy takes nothing returns nothing
            if specificUnit then
                call unitIdTable.remove( this.unitTypeId)
            else
                call unitTypeTable.remove( this.unitTypeId)
            endif
            call this.deallocate()
        endmethod

 

[deathismyfriend]

lol thats what i get for not checking the API close enough haha.
Not sure how i did that one lol.

 

[Adiktuz]

bro, I think that you should let the user define the chance that a unit shall process item drops... because that will make it easier for the user to manage the weights of the actual drops...

I mean that would result to less calculations in his part to achieve his desired drop chance of the actual drops

example:

I want a 25% chance that a unit drops an item: there are two possible items one with 25% chance, the other 75%

on the proposed system:

User defines drop chance of 25% for the unit
Now for the 2 items, he can just use weights of 1 and 3 respectively

on the current system:

He'll need to compute the actual percent for each item to drop (taking into account the 25% drop chance of the unit)

so item 1 will need to have a drop chance of 25%*25% while item2 has a drop chance of 75%*25%

Now that would be: 6.25% for item1, 18.75% for item2 and (100 - 6.25 - 18.75)% for no-drop

Then the user will still need to translate those into weights...

now that is user-wise, for the actual system running, it also has a perk:

You won't need to loop thru the drop table if the unit doesn't need to drop an item. this is because you will first check if the unit will have a drop and if not, you finish the processing already.

Of course that is helpful only if most units have a non-100% chance to drop any item. But I think only bosses normally have a 100% chance to drop items.

 

[deathismyfriend]

my system already protects from units that have a 100 percent chance to not drop an item lol

Should have this updated soon lol

Edit: updated

 

[Adiktuz]

of course... but I never said anything about 100% no drop... maybe you misunderstood this line?

non-100% chance

All it says is that units that don't have a 100% chance to drop, not a 100% chance not to drop...

Anyway what I'm saying in the last post is that right now, its tedious to the user to define the right weights when you take into account the non-100% chance to process a drop as I explained on the above post...

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

Also, with your current set-up that loops thru the item drop processing until you reach the multidrop amount limit + the fact that the NO DROP is part of the drop table, you can actually have a unit that drops from 0 up to its max drop limit...

Now that can be either good or bad depending on the user... But a better way to achieve that and maintain the capability for exact drop amount is to just assign a min and max amount instead of just a single amount... Of course assuming that you take my earlier suggestion to add the %chance to drop any item to the unit registration.

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

Also, since you use if-then-elseif for determining the table upon death, its either the per type table or the table specific to the unit... But what if the user wants both?

            if unitTypeTable.has( id) then
                set this = unitTypeTable[ id]
            elseif unitIdTable.has( GetHandleId( u)) then
                set this = unitIdTable[ GetHandleId( u)]
            else
                return false
            endif

Anyway, if you want to keep it like that, I believe that the perUnit table should precede the perType... but I really suggest that your system should process both drop tables if they both exist...

 

[deathismyfriend]

ya i will switch them around the perUnit should precede the perType. Umm if they both exist i can see about changing this. Ill have to see if its viable to do lol.

for ur earlier thing i dont think im understanding correctly what ur saying. Can u explain it more clearly plz.

 

[Adiktuz]

You can have them both, check if the perUnit exists, then process it... Afterwards, check the perType and process it too... but you would need to use two tables to separate the perUnit and the perType tables...

right now you have

itemDropTable
itemIdTable

//you'll just need to add another set of that

itemDropTableUnit
itemIdTableUnit

//something like that

You know what? I think you can keep the current weight system... To provide alternatives... It will just force users to use not so decimal percentages anyway... And so that your system works differently than my Pool version...

Though since you don't have a %chance per unit, its harder to implement a drop chance bonus system [something that mine has] into this than if you have a %chance per unit...

PS: I also realized some perks of the current system... XD...

EDIT: I found something:

                loop
                    exitwhen L > this.multiDrop
                        set r = GetRandomInt( 1, this.ttlWeight)
                        loop
                            exitwhen L1 > end
                            if r < weights[L1] then
                                call CreateItem( itemIdTable[this][L1], x, y)
                                set u = null
                                return false //THIS!!!
                            endif
                            set L1 = L1 + 1
                        endloop
                    set L = L + 1
                endloop

You have a return false there after an Item is made... That means that whenever an item is dropped, the system will already stop, making the multiDrop loop useless...

I believe that should be

                loop
                    exitwhen L > this.multiDrop
                        set r = GetRandomInt( 1, this.ttlWeight)
                        loop
                            exitwhen L1 > end
                            if r < weights[L1] then
                                call CreateItem( itemIdTable[this][L1], x, y)
                                set L1 = end
                            endif
                            set L1 = L1 + 1
                        endloop
                    set L = L + 1
                endloop

also, you have a local leak here:

local unit u = GetTriggerUnit()
            local integer id = GetUnitTypeId( GetTriggerUnit())
            local thistype this
            local integer r
            local real x = GetUnitX( u)
            local real y = GetUnitY( u)
            local integer array weights
            local integer L = 1
            local integer L1 = 1
            local integer end = 0
            if unitTypeTable.has( id) then
                set this = unitTypeTable[ id]
            elseif unitIdTable.has( GetHandleId( u)) then
                set this = unitIdTable[ GetHandleId( u)]
            else
                return false 
                //Since this terminates the process already
                //you should have a set u = null here
            endif

 

[deathismyfriend]

Lol thx for the review I will change these when I can. I don think I will have to add more tables I could just make the double loop and I must've misplaced the return after the create item lol. Can't believe I forgot to null the local lol.

What are the perks of my system lol ? Since u put it in a PS statement lol.

 

[Adiktuz]

Well, using two tables just for safety... XD...

One is that, its somehow like the default wc3 item drop tables...

 

[deathismyfriend]

Ooo lol I tried to make it easy to add items for ppl that only know GUI. I believe all my examples in the test map are in GUI so they would know how to use it. Downside is that not every GUIer has JNGP downloaded already. I may make an easier method for a GUIer to be able to use the system easily but I'm not sure. I hope to make those changes that u suggested when I'm done with work.

 

[Adiktuz]

I think it's pretty easy enough to use... As long as they can follow instructions or learn from given samples...

unless you want to do something like:

• Set Unit = MyUnit
• Set Amount = 1
• Custom script: call itemDropSystem.registerTypeGUI
• Set Item = Blades of attacks
• Custom script: call itemDropSystem.addItemGUI

addItemGUI takes the unit in MyUnit and uses it's UnitTypeID to find the right table to add the item to...

but if you ask me, this looks more tedious than just typing out the rawcodes themselves... just have a good how to or a good sample and they can do it...

 

[deathismyfriend]

Ya that's true lol ill have to try some different options to see what's the best way. Thanks for all ur help

Edit: updated the system. Fixed the local leaks i missed. Also changed so items from both drop tables can be dropped when unit dies.
Also allows the user to decide if he has a item table based on a units type that it will not drop the item from the unit type table with a simple boolean.

 

[Adiktuz]

method destroy takes nothing returns nothing
            if specificUnit then
                call unitIdTable.remove( this.unitTypeId)
            else
                call unitTypeTable.remove( this.unitTypeId)
            endif
            call this.deallocate()
endmethod

uhm, your unit can now have both tables right? and the current destroy method won't remove both...

just use the .has method to determine if you can use .remove...

or maybe you can just use .remove right away, not sure if that's safe though...

and also, I think its better if the unit specific table gets removed when the unit dies

and on this part:

local integer id = GetHandleId( u)
            set this.unitTypeId = id

just make it

set this.unitTypeId = GetHandleId( u)

since you only used the id variable once in that function... its useless to have a local for that...

 

[deathismyfriend]

true lol. i will have to make these changes when i can. thanks again lol

updated. Thanks again Adiktuz