Itens cloning

ID:2099514
 
Jun 12 2016, 8:45 pm
(See the best response by Lummox JR.)
Code: 
        Click()
            if(usr.Loot)
                if(usr.Loot.Inventory.Find(src))
                    if(wear)
                        src:Wear()
                    if(type != /obj/Items/Berry)Drop()
                    if(type == /obj/Items/Berry)src:Drop_All()
                    return
            if(winget(usr,"ShoppingScreen","is-visible") == "true")
                if(usr.Inventory.Find(src)&&!wear)
                    if(type == /obj/Items/Berry)return
                    switch(alert(usr,"Would you like to sell [src] for [num_handle(round(Cost/2,1))]?",name,"No","Yes"))
                        if("No")
                            return
                        if("Yes")
                            for(var/obj/Items/Berry/B in usr.Inventory)
                                B.Amount += round(Cost/2,1)
                                B.Amount()
                                goto End
                            var/obj/Items/Berry/B = new/obj/Items/Berry
                            B.Amount = round(Cost/2,1)
                            B.loc = usr.loc
                            B.Get()
                            End
                            del(src)
                    return
            if(winget(usr,"ShoppingScreen1","is-visible") == "true")
                if(usr.Inventory.Find(src)&&!wear)
                    if(type == /obj/Items/Berry)return
                    switch(alert(usr,"Would you like to sell [src] for [num_handle(round(Cost/2,1))]?",name,"No","Yes"))
                        if("No")
                            return
                        if("Yes")
                            for(var/obj/Items/Berry/B in usr.Inventory)
                                B.Amount += round(Cost/2,1)
                                B.Amount()
                                goto End
                            var/obj/Items/Berry/B = new/obj/Items/Berry
                            B.Amount = round(Cost/2,1)
                            B.loc = usr.loc
                            B.Get()
                            End
                            del(src)
                    return
            if(get_dist(src,usr) <= 1)
                if(usr.Inventory.Find(src))return
                for(var/mob/M in world)
                    if(M.Inventory.Find(src))return
                Get() 
            Get()
                set category = null
                set hidden = 1
                set src in oview(1)
                if(!usr.Conscious)return
                if(Weight > usr.Strength)
                    usr << output("[src] is too heavy for you.","SM")
                    return
                usr.Inventory.Add(src)
                Move(usr)



Problem description:Then two players clicks the item on same time it gets cloned! Two same item on their inventory. There has no sleep or something giving delay so I could not understand what can cause this.

Jun 12 2016, 9:59 pm (Edited on Jun 12 2016, 10:05 pm)
I don't think you even need a loop for this let alone a loop of the world for mobs for something like this
I'm guessing it's getting cloned because your loop isn't instant and if two players are fast enough they can both pick it up before a full loop
Jun 13 2016, 5:31 am
In response to Zagros5000
Hello ! Thanks for the feedback. What you mean with "let alone a loop of the world for mobs"?
Jun 13 2016, 6:54 am
There has no sleep or something giving delay so I could not understand what can cause this.

Also, winget() is a sleeping/blocking proc.
Jun 13 2016, 9:22 am
After looking at this code, I take it berries are your currency.

The if(type != ...) followed by the == check is incorrect. Use if/else here.

I don't see the sense in having a separate Inventory list, unless you have it strictly for sorting purposes. Just use contents and be done with it. 99% of the places you use this list, you can use contents instead. if(usr.Inventory.Find(src)) is much faster as if(src in usr). (Don't forget to put parentheses around the "X in Y" condition if you have another condition in that same if.)

It's sometimes important to check after winget(), input(), and alert() that the client is still valid. Doesn't appear to be an issue here, but keep it in mind. At the very least you probably need to be sure that the proc is still in a good state.

Because winget() sleeps, you should be checking that last (after the check to see if this is an item you want to pick up), or at least avoid checking it until after you've done the inventory check. For safety you should probably check the inventory both before and after.

The loop where you search for a berry can be done much more easily with locate().

Don't calculate round(Cost/2,1) repeatedly. Do it once in a local var, so you're sure the number you're using is consistent. That way if you want to change the price ratio, you can do it in one place.

Your use of goto is wrong. You shouldn't use goto until you have a healthy respect for when to use it and when not to. In this case, you have "goto End", which jumps to a del(src) statement. If you just use del(src) in the first place, then the proc ends immediately. No need for goto here.

This can't possibly be right:

B.Amount += round(Cost/2,1)
B.Amount()

You have a var and a proc with the same name, on the same object? While I think that's legal, you should never do that. You're begging for problems when you do stuff like that. At least call the proc UpdateAmount() or something similar.

You should not have a wear var. Something that's equippable should be checking with the mob that's holding it to see if it's equipped or not; it should not keep track of that status itself. Often with equipment systems people end up having this info in two places: the mob keeps track of what it's equipped, and then the items themselves keep track, and if this info gets out of sync you have a problem. If only the items themselves keep track, then you still have a problem because if the item gets moved out of a mob's contents for any reason, it still thinks it's worn.
Jun 13 2016, 9:47 am
In response to Lummox JR
Wow, that was a lot helpfull and with a lot informations,I read it 2 times and I am trying to fix the bug with informations."For safety you should probably check the inventory both before and after." I did that, later I will see if the bug still (I cant host right now,and it needs two players) but thanks I think this will fix the bug.
Jun 13 2016, 2:56 pm
In response to Lummox JR
Hello,then I tried to loot it not worked anymore.I tried to place:
            if(get_dist(src,usr) <= 1)
                if(usr.Inventory.Find(src))
                for(var/mob/M in world)
                    if(M.Inventory.Find(src))return
                Get()

on begin
Jun 13 2016, 2:58 pm
In response to SoulGamesProductions
for(var/mob/M in world)
if(M.Inventory.Find(src))return

What are you trying to do here? because this is saying that if anyone in the world has this item it cant be picked up
Jun 13 2016, 3:00 pm
In response to Zagros5000
it is for prevent the cloning but it not works how should work
Jun 13 2016, 3:03 pm
In response to SoulGamesProductions
Best response
You have an if() check that's not doing anything there, and that loop does nothing of value either. (I should have made mention of that, but I kind of missed it among all the other stuff).

What you really need is:

if(src in orange(1))
    return Get()

As I mentioned that should go near the beginning of Click(), since it's one of the easiest things to check.

The reason I used orange() is that 1) using range/orange is good solid programming practice in DM when you're not in a speed-critical section, and 2) this covers all the bases in terms of figuring out if the object can be picked up.
Jun 13 2016, 3:06 pm
Wouldn't a get_dist() check be faster and work just as well? It would return 1 in any instance where orange(1) would find src, wouldn't it?
Jun 13 2016, 3:10 pm
In response to Nadrew
Nadrew wrote:
Wouldn't a get_dist() check be faster and work just as well? It would return 1 in any instance where orange(1) would find src, wouldn't it?

Different z levels might be an issue.
Jun 13 2016, 3:11 pm
I always forget that 1,1,1 and 1,1,2 are one tile apart. You're right, that would mess things up.
Jun 13 2016, 4:24 pm
Ohhh thanks ! I fixed the problem with the helpl !