Can't unequip!

ID:142852
 
Feb 19 2008, 5:29 am
Code:
obj/item/beginner_helmet
    icon = 'items.dmi'
    icon_state = "beghelm"
    value = 0
    layer = MOB_LAYER+3
    Click()
        if(src in oview(1,usr))
            src.Move(usr)
            usr << "You pick up a item."
        if(src in usr.contents)
            if(usr.helmeq == 0 && src.weared == 0)
                suffix = "-Equiped-"
                usr.overlays+=src
                usr.helmeq = 1
                src.weared = 1
            else if(usr.helmeq == 1 && src.weared == 1)
                suffix = ""
                usr.overlays-=src
                usr.helmeq = 0
                src.weared = 0
    verb/Drop()
        if(src.weared == 0)
            src.loc = locate(usr.x,usr.y-1,usr.z)


Problem description:
Equiping works fine but when I'm unequiping it doesn't remove the overlay :S

Feb 19 2008, 5:51 am (Edited on Feb 19 2008, 6:27 am)
You need to read up on boolean. Instead of using:

if(status == 1) //True
if(status == 0) //False


You should use:

if(status) //True
if(!status) //False


Feb 19 2008, 5:56 am
In response to Siientxx
Why? There is no difference >_>
Feb 19 2008, 6:25 am
In response to Syltmunk
Feb 19 2008, 6:35 am (Edited on Feb 19 2008, 6:40 am)
In response to Siientxx
lol, still doesn't fix my problem and as I said, both are fine if you know what you're doing. I prefer "==" over "!" (Althought it could mess up as it say in the text but I still prefer ==)
Feb 19 2008, 6:41 am
In response to Syltmunk
Well, you're free to continue going about using the amateur and incorrect way of using true/false if checks.
Feb 19 2008, 12:15 pm
"equipped" has two Ps, and "weared" is not a word, you're thinking of "worn." More importantly, however, you've got your code designed wrong, both backwards and forwards.

Which is to say that it's wrong backwards and wrong forwards and having the two of them doesn't make it any more right.

First off: Storing boolean (1 or 0) values for whether something is equipped is the wrong way to go about things. What you should have instead is one mob variable which keeps track of WHICH item is equipped. That's MUCH more useful than knowing if something is equipped or not, because you can then FIND it.

Second: You need to be designing things with a sense of object-orientation. For example:

        if(src in oview(1,usr))
            src.Move(usr)
            usr << "You pick up a item."


This is something that should be defined under all items. You can pick up anything in range by clicking on it. Which reminds me that your first programming error comes on the line after that:

        if(src in usr.contents)


You're attempting to equip something the moment you pick it up, which is a problem. But, anyway...

Similar to the picking up items thing, ALL equipment should be equipped when you click on it (while holding it). Basically, the point of all this is to prevent you from writing the same code over and over and over and over and over and over and over and over and over and over and over and over and over when you have fifty different items and each and every one requires you to re-type out the Click() proc.

Third: I could trump up a third thing you've gotten wrong, but I think those first two are enough.

So, let's start from the beginning: items you can pick up and drop. You've already written this anyway, though there's a couple changes you should make

obj
  item
    icon = 'items.dmi'
    Click()
      if(src in oview(1,usr))
        //We have this if() statement here because Move() could fail, and we don't want to say we picked something up if we didn't
        if(src.Move(usr))
          //We gave a more informative message hwen players pick something up
          usr << "You pick up \a [src]"
    verb/Drop()
      //Now we're trying to move this to where we're standing
      if(Move(usr.loc))
        usr << "You drop \a [src]"


Simple enough? Now anything derived from /obj/item has the icon 'items.dmi', and can be picked up by clicking on it and dropped by using the Drop verb. Now, let's make a piece of equipment:

obj/item
  equipment
    layer = MOB_LAYER+3
    proc
      equip(var/mob/M)
      unequip(var/mob/M)


Hmm... there's not much there. All we have done is define the layer (all equipment goes above the player) and the (empty) procs equip() and unequip(). However, being derived from /obj/item, it can also be picked up and dropped. But why did we define these empty procs? Because that's what all equipment has in common: it can be equipped or unequipped. The thing is, everything is going to go around doing that in a different manner. So we have to go one type deeper:

obj/item/equipment
  helmet
    //these procs were already defined, so there's no need for proc
    equip(var/mob/M)
      //obviously, only allow people to equip things they have
      if(loc == M)
        //only allow us to equip something if nothing's there
        if(!M.helmet)
          //equip the item
          M.helmet = src
          M.overlays += src
          suffix = "-Equipped-"

    unequip(var/mob/M)
      if(loc == M)
        if(M.helmet == src)
          M.helmet = null
          M.overlays -= src
          suffix = ""

mob
  var/obj/item/equipment/helmet/helmet
  //well, that's long.  We've defined a variable named helmet, or type /obj/item/equipment/helmet


Now we have a helmet that can be equipped and unequipped. We do need a way to do this, however, as they are procs, not verbs. We'll use Click(), like you did:

obj/item/equipment/helmet
  Click()
    //we also perform this check in the equip() and unequip() procs, but it doesn't hurt to do it again
    if(loc == usr)
      if(!usr.helmet)
        //if we have no helmet, equip this one
        equip(usr)
      else if(usr.helmet == src)
        //otherwise, if we have this helmet equipped, unequip it
        unequip(usr)
    
    //recall, however, that Click() also picks up items.  So, we need to call ..() in order to let it do that.
    ..()


There. Now we have an equippable helmet, which can also be picked up and dropped. However, it's not quite YOUR helmet. Fortunately, it's very easy to make a specific type of helmet:

obj/item/equipment/helmet
  beginner_helmet
    icon_state = "beghelm"
    value = 0


That's it. Want another helmet?

obj/item/equipment/helmet
  loser_helmet
    icon_state = "loserhelm"
    value = -100


BAM! Easy. You could make a dozen helmets now with the effort it would take you to have made just one the way you were doing it.

Of course, hopefully you are paying enough attention to ask, "But what keeps you from dropping an equipped helmet?" And that's a damn good question. Fortunately, we used Move(), which calls the helpful procs Enter() and Exit() to determine whether something can enter or exit something else. The first something will be our items, and the second something will be our mob. So, we want to keep a player from dropping their equipped helmet:

mob
  Exit(atom/movable/M)
    if(M == helmet)
      return 0
    return ..()


That was easy. If what is trying to exit is your helmet (and you could add more if() statements to check if it's your pants, or shoes, or whatever), it will return 0, which will prevent it from exiting. Otherwise, it's going to return the default value returned from ..(), which is going to be 1 (unless if you mess with Exit() some more) allowing the thing to exit.
Feb 21 2008, 4:44 am
In response to Garthor
Thanks for that detailed "tutorial" but the "usr.overlays += src" still doesn't work, it isn't removed from the overlays :S
Feb 21 2008, 9:43 am
In response to Syltmunk
That would be because += adds to overlays, not removes from. That's -=.
Feb 21 2008, 10:38 am
I suspect it has to do with the order of operations:

if(usr.helmeq == 0 && src.weared == 0)


Replace the values of the vars and you have this expression: (0 == 0 && 0 == 0)

1. 0 == 0 && 0 == 0
0 == 0 is evaluated 1st, resulting in true or 1
Plug this in and see what happens next:
2. 1 && 0 == 0
1 && 0 is evaluated next, resulting in false, or 0
Plug that in for the last part:
3. 0 == 0
The final result of the expression is true, or 1

So the If is true and you equip the item, then change usr.helmeq=1 and src.weared=1.

Now time to unequip. Look at the order of operations again:
if(usr.helmeq == 0 && src.weared == 0)


Look up the values, it means this: (1 == 0 && 1 == 0)

1. 1 == 0 && 1 == 0
1 == 0 is evaluated 1st, resulting in false or 0
Plug this in and see what happens next:
2. 0 && 1 == 0
0 && 1 is evaluated next, resulting in false, or 0
Plug that in for the last part:
3. 0 == 0
The final result of the expression is true, or 1

So all it does is try to equip again. You never even get to the else part.

What you want to do is this:
if( (usr.helmeq == 0) && (src.weared == 0) )


Then when you equip, you start with:
((0 == 0) && (0 == 0))
because of the parenthesis, the evaluation results in this:
1. 1 && (0 == 0)
2. 1 && 1
3. 1 or true.

Then when you unequip:
((1 == 0) && (1 == 0))
1. 0 && (1 == 0)
2. 0 && 0
3. 0 or false

So now you get to the else clause... yay!!!

Now, I'll let you figure out if you need () or not to order your operations in the else if... ;)
Apr 19 2008, 12:04 pm
In response to Siientxx
amateur? yes. Incorrect? well, if it works just as well and doesn't cause any extra use of memory, it cant be entirely incorrect.
Apr 19 2008, 2:37 pm
In response to Jehubuddaka
Fixed:
Jehubuddaka wrote:
amateur? yes. Incorrect? quite so.

Using the proper boolean checking is more robust and efficient, as well as being slightly faster too (checking for true values at least).

Why? There is no difference >_>

It can make all the difference. You've been linked into an article from a BYOND Staff member and known coding guru, that talks about the difference.
At any case, you're getting better code in exchange for actually having shorter code! So I don't know why you're insisting on typing more code ( == 1 .... == 0...) all the time.