Is this the correct way to handle item modifiers?

ID:1608226
 
Jun 23 2014, 10:27 am
(See the best response by GhostAnime.)
Code:
mob
    var
        list
            Equipped_Items[0] //the list where it keeps track of what items you have equipped
Equip
    var
        Slot
        Level = 1
        list
            Mods[0]
    parent_type = /obj

    DblClick()
        var/mob/owner = usr
        if(Slot in owner.Equipped_Items) //if item slot in equipped items
            if(!(owner.Equipped_Items[Slot] == src)) //if its not the item we are clicking on
                //usr << output(null, "[Slot]_Equip:1,1")
                var/Equip/item = owner.Equipped_Items[Slot]
                owner.Equipped_Items[Slot] =  null
                owner.Equipped_Items -= Slot
                owner.contents += item
                On_Unequip(item,item.Mods)
                owner.Equipped_Items += Slot
                owner.Equipped_Items[Slot] += src
                owner.contents -= src
                On_Equip(src,src.Mods)
                //owner << output(src, "[Slot]_Equip:1,1")

            else //if it is
                //usr << output(null, "[Slot]_Equip:1,1")
                var/Equip/item = owner.Equipped_Items[Slot]
                owner.Equipped_Items[Slot] =  null
                owner.Equipped_Items -= Slot
                owner.contents += item
                On_Unequip(item,item.Mods)

        else //if there no item equipped already of that type, equip
            owner.Equipped_Items += Slot
            owner.Equipped_Items[Slot] += src
            owner.contents -= src
            On_Equip(src,src.Mods)
            //owner << output(src, "[Slot]_Equip:1,1")

    proc
        Can_Equip(Equip/Item)
            if(usr.Level >= Item.Level) return 1

        On_Equip(Equip/Item, var/list/variables)
            for(var/variablename in variables)
                if(variablename in vars)
                    vars[variablename]  += variablename
                else
                    usr << "ERROR VARIABLE DOES NOT EXIST"

        On_Unequip(Equip/Item, var/list/variables)
            for(var/variablename in variables)
                if(variablename in vars)
                    vars[variablename]  -= variablename
                else
                    usr << "ERROR VARIABLE DOES NOT EXIST"
                    
    Testitem
        Mods = list(
        "Attack" = 50,
        "Haste" = 120,
        )


Problem description:
Just wondering if this would be the correct way to add modifiers to items by storing variables I want to modify in a list. I have a testitem that grants 50 attack and 120 haste stored in a list called Mods. When the item is equipped the users variables get added with the items modifier.
Jun 23 2014, 12:55 pm
That certainly looks correct for the method you want.

Definitely thought ahead with that safety check to make sure that the variables you have in the Mods list exists :)

But there are few things I want to talk about:

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

(1) Does the user have the item?
For your DblClick(), you should check if the item actually belongs to the user (ex: by seeing if it exists in the usr's content).

Otherwise people could DblClick() it on the ground/display area and gain the equip bonus without actually having the item.


(2) Reduce redundancy

I think you should have the if-slot-occupied-unequip check first. This way, what you could do, is unequip the currently equipped item THEN equip the item immediately without calling the equip verb once again (resulting in only one if() statement instead of if..else).

Why is this important? In case you have to fix something, it is much easier to correct in one spot than multiple spots

(3) Do not save ALL the variables :(

I think that you should define the Equip/var has a tmp variables (Equip/var/tmp/...).

The tmp variables tells DM not to save those values - great to use for any temporary variables :P


With tmp, since those variables are not saved, they are reset back to the defined values for that object. Great for fixing stats (boosting/nurfing) for something that's under/overpowered. Without tmp,, you would have to define additional checks to make sure that the Mods value is acceptable.

If you have some variables you DO want saved, be sure it is not under the tmp path.


(4) Are you accessing the correct objects variables?
I noticed that when you are adding the Mods to the variables, you did not mentioned the user.

What this results is that DM thinks you implied src thus you will be modifying src.var (the object's variable list) rather than user.vars.

because this proc can be called indirectly from the user, using usr is not recommended. You should pass who the usr is, probably with something like a variable.

For example, Equip/var/mob/Owner = (The user who owns it; set upon pickup or creation.).


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

So to sum up what I mentioned:
Equip

  var
     Name = "???"   //  Name can be saved since it is not under ../var/tmp/...
     mob/Owner      //  Be sure to set up on pickup or creation of the item. See the mob/Entered() version below.
      tmp           //  [3] /tmp makes the following variables unsavable in a savefile
           Mods = list()  //  Because it is not saved, the Mods list will go to the default/defined value.


   DblClick()
      if(!(src in usr.contents))  //  [1]  Make sure that the item is in the usr's possession
         Item_Info(src)   //  Here, if the item is not in the usr's content/inventory, call up a proc to show the item stats instead!
         return 0         //  Remember to return since the item does not belong to the usr

//  Now [1] is satisfied, meaning src is in usr.contents, let's continue

      var/Equip/item = usr.item_slots[src.slot]

      if(item && item != src)
 /*  If item is true (that is, if the variable is NOT <tt>0, "" or FALSE</tt>), meaning something is equipped
      AND checking if the equipped item is not this item
      = we have to unequip the other item                */

            item.Unequip_Proc()  //  Call the unequip proc on the previous item slot

      src.Equip_Proc()           //  Do your equipping thing
// Because Equip_Proc is NOT nestled underneath the if(), it simplifies our code block (easier for us to read/less complications when needed to modify).


   proc
      Unequip_Proc()
        if(!src.Owner)   //  Safety-check for the owner
          if(!ismob(src.loc))  //  If the item is not in a /mob's inventory/content
              CRASH("Unknown owner for [src.name]")  //  Cut-throat response.
          else src.Owner = src.loc

        var/Equip/item = Owner.item_slot[src.slot]
        if(item && item == src)  //  If something is equipped and that something is this item
              Remove stats


      Equip_Proc()
        if(!src.Owner)
          if(!ismob(src.loc))
              CRASH("Unknown owner for [src.name]")
          else src.Owner = src.loc

        var/Equip/item = Owner.item_slot[src.slot]
        if(item && item != src)  //  Always safety-check
            CRASH("There's still something equipped on [src.Owner.name]!")

        Add Stats



/*  For the following:

Entered() is called when something is added successfully to an object's container.

This means you must Move() the item to the container, NOT change the loc!

Changing the loc forcefully moves an item to a container without calling any of the checks.

So do NOT do item.loc = new_owner

Instead, do either:
var/Equip/Sword/S = new  //  Creates a new /Equip/Sword item while, at the same item, setting up a variable referencing to the new item
S.Move(new_owner)

OR

new/Equip/Sword(new_owner)  //  Will create it in the new_owner's inventory. Please check if this methods does call mob/Entered()

*/

mob
    Entered(Equip/item)  //  Note: Entered() and other movement procs do NOT look for the type you define in the argument. item here could be a /mob so you must double check!

      if(!istype(item))  //  If a path is not defined in istype(), it will look up the path of the variable entered. Here, the istype() is checking if item is NOT an /Equip object
         return ..()  // ..() is calling the parent procedure... think of it as saying "continue checking any other instance of this proc, including the default programmed method"

      item.Owner = src  //  Sets up the item owner immediately

      return ..()  //  Do not forget to call this!



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

That's all I can think of at the moment :)
Jun 23 2014, 1:18 pm
In response to GhostAnime
GhostAnime wrote:
That certainly looks correct for the method you want.

Definitely thought ahead with that safety check to make sure that the variables you have in the Mods list exists :)

But there are few things I want to talk about:

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

(1) Does the user have the item?
For your DblClick(), you should check if the item actually belongs to the user (ex: by seeing if it exists in the usr's content).

Otherwise people could DblClick() it on the ground/display area and gain the equip bonus without actually having the item.


(2) Reduce redundancy

I think you should have the if-slot-occupied-unequip check first. This way, what you could do, is unequip the currently equipped item THEN equip the item immediately without calling the equip verb once again (resulting in only one if() statement instead of if..else).

Why is this important? In case you have to fix something, it is much easier to correct in one spot than multiple spots

(3) Do not save ALL the variables :(

I think that you should define the Equip/var has a tmp variables (Equip/var/tmp/...).

The tmp variables tells DM not to save those values - great to use for any temporary variables :P


With tmp, since those variables are not saved, they are reset back to the defined values for that object. Great for fixing stats (boosting/nurfing) for something that's under/overpowered. Without tmp,, you would have to define additional checks to make sure that the Mods value is acceptable.

If you have some variables you DO want saved, be sure it is not under the tmp path.


(4) Are you accessing the correct objects variables?
I noticed that when you are adding the Mods to the variables, you did not mentioned the user.

What this results is that DM thinks you implied src thus you will be modifying src.var (the object's variable list) rather than user.vars.

because this proc can be called indirectly from the user, using usr is not recommended. You should pass who the usr is, probably with something like a variable.

For example, Equip/var/mob/Owner = (The user who owns it; set upon pickup or creation.).


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

So to sum up what I mentioned:
Equip
>
>   var
>      Name = "???"   //  Name can be saved since it is not under ../var/tmp/...
>      mob/Owner      //  Be sure to set up on pickup or creation of the item. See the mob/Entered() version below.
>       tmp           //  [3] /tmp makes the following variables unsavable in a savefile
>            Mods = list()  //  Because it is not saved, the Mods list will go to the default/defined value.
>
>
>    DblClick()
>       if(!(src in usr.contents))  //  [1]  Make sure that the item is in the usr's possession
>          Item_Info(src)   //  Here, if the item is not in the usr's content/inventory, call up a proc to show the item stats instead!
>          return 0         //  Remember to return since the item does not belong to the usr
>
> //  Now [1] is satisfied, meaning src is in usr.contents, let's continue
>
>       var/Equip/item = usr.item_slots[src.slot]
>
>       if(item && item != src)
>  /*  If item is true (that is, if the variable is NOT <tt>0, "" or FALSE</tt>), meaning something is equipped
>       AND checking if the equipped item is not this item
>       = we have to unequip the other item                */
>
>             item.Unequip_Proc()  //  Call the unequip proc on the previous item slot
>
>       src.Equip_Proc()           //  Do your equipping thing
> // Because Equip_Proc is NOT nestled underneath the if(), it simplifies our code block (easier for us to read/less complications when needed to modify).
>
>
>    proc
>       Unequip_Proc()
>         if(!src.Owner)   //  Safety-check for the owner
>           if(!ismob(src.loc))  //  If the item is not in a /mob's inventory/content
>               CRASH("Unknown owner for [src.name]")  //  Cut-throat response.
>           else src.Owner = src.loc
>
>         var/Equip/item = Owner.item_slot[src.slot]
>         if(item && item == src)  //  If something is equipped and that something is this item
>               Remove stats
>
>
>       Equip_Proc()
>         if(!src.Owner)
>           if(!ismob(src.loc))
>               CRASH("Unknown owner for [src.name]")
>           else src.Owner = src.loc
>
>         var/Equip/item = Owner.item_slot[src.slot]
>         if(item && item != src)  //  Always safety-check
>             CRASH("There's still something equipped on [src.Owner.name]!")
>
>         Add Stats
>
>
>
> /*  For the following:
>
> Entered() is called when something is added successfully to an object's container.
>
> This means you must Move() the item to the container, NOT change the loc!
>
> Changing the loc forcefully moves an item to a container without calling any of the checks.
>
> So do NOT do item.loc = new_owner
>
> Instead, do either:
> var/Equip/Sword/S = new  //  Creates a new /Equip/Sword item while, at the same item, setting up a variable referencing to the new item
> S.Move(new_owner)
>
> OR
>
> new/Equip/Sword(new_owner)  //  Will create it in the new_owner's inventory. Please check if this methods does call mob/Entered()
>
> */
>
> mob
>     Entered(Equip/item)  //  Note: Entered() and other movement procs do NOT look for the type you define in the argument. item here could be a /mob so you must double check!
>
>       if(!istype(item))  //  If a path is not defined in istype(), it will look up the path of the variable entered. Here, the istype() is checking if item is NOT an /Equip object
>          return ..()  // ..() is calling the parent procedure... think of it as saying "continue checking any other instance of this proc, including the default programmed method"
>
>       item.Owner = src  //  Sets up the item owner immediately
>
>       return ..()  //  Do not forget to call this!
>

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

That's all I can think of at the moment :)

So if there were modifiers that added a percentage of someones stat

It would look like this?
Unequip_Proc()
            if(!src.Owner)
                if(!ismob(src.loc))
                    CRASH("Unknown owner for [src.name]")
                else src.Owner = src.loc
                
            var/Equip/item = Owner.item_slot[src.slot]
            
            if(item && item == src)
                for(var/v in src.Mods)
                    if(v in Owner.vars)
                        if(findtext("[src.Mods[v]]","_%"))
                            Owner.vars[v] -= Owner.vars * v
                        else
                            Owner.vars[v] -= v
                    else
                        CRASH("Variable does not exist")
                
        Equip_Proc()
            if(!src.Owner)
                if(!ismob(src.loc))
                    CRASH("Unknown owner for [src.name]")
                else src.Owner = src.loc
            var/Equip/item = Owner.item_slot[src.slot]
            if(item && item != src)
                CRASH("There's still something equipped on [src.Owner.name] !")
                
            for(var/v in src.Mods)
                if(v in Owner.vars)
                    if(findtext("[src.Mods[v]]","_%"))
                        Owner.vars[v] += Owner.vars * v
                    else
                        Owner.vars[v] += v
                else
                    CRASH("Variable does not exist")
Jun 23 2014, 2:08 pm (Edited on Jun 23 2014, 2:43 pm)
In response to Max Omega
Best response
Not quite. There's a serious flaw with that method and here's three examples to illustrate what I mean:

~~~~~~~~~~~
To summarize the different symbol meaning below:

10% Item bonus
% = Equip = +10% = X * 1.10 == X + (X*0.10)
& = Unequipped = -10% = X * 0.90 == X - (X*0.10)


100% temp. bonus (ex: aura boost or some other effect)
* = Bonus granted = +100% = X * 2 == X + X = XX
# = Bonus finished = -100% = X / 2 == XX - X = X

~~~~~~~~~~
Example 1
~~~~~~~~~~
Imagine this scenario: Someone gets a bonus 100% temp. boost in attack. You equip an item that adds 10% strength... what will happen when the temp. boost is done upon unequipping?

Here's how this would go:
Str = 100
Str* = 200 (* = with the 100% bonus)

Str*% = 220 (% = with the 10% item equipped. & = after Unequipping)

Str#% = 220 / 2 = 110 (# = 100% bonus finished)
Str#& = 110 - 11 = 99 (& = unequipped the 10% item boost)


In this specific scenario, we just lost 1 strength point through an unintended consequence (and what some people will call it as a "glitch").

----------

But it is not all beneficial... after all, when you level, your stats increase. So here's an example of someone leveling up with the % item effect.

For simplicity sake, let's assume you gain 10 Str per level.

Values in the bracket beside Str is the current user's level. % = 10% item equip, & = 10% item unequipped afterwards

Str(10) = 100
Str%(10) = 110

Str(20) = 210 (10 levels * 10 Str/level = 100 Str gained)
Str&(20) = 189


So, in this case where you unequip after leveling for a while, you have now LOST 11 Str points... uh, oops!

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

For the final example, this of a combination of the two scenarios:

Str(10) = 100
Str*(10) = 200
Str*%(10) = 220


~~~~~ Unequip at same level ~~~~~

= Boost still there -> Fine =
Str*&(10) = 220 - 20 = 200
Str#&(10) = 200 / 2 = 100 (lose bonus at the SAME level AFTER unequipping)

Difference = 0

== Boost gone -> same scenario as example 1 ==
Str#%(10) = 220 / 2 -> 110
Str#&(10) = 110 - 11 = 99

Difference = -01


~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
~~~~ Unequip at level 20 ~~~~~~
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Perfect unequip will lead to Str(20) = 200

~~~~~~~~~~~~~Assuming no boost obtained~~~~~~~~~~~~
Str%(10) = 110
Str%(20) = 210
Str&(20) = 210 - 21 = 189

Difference = -11


~~~~~~~~~~~~~
~~~~~~~~~~~~~Assuming boost still present here~~~~~~~~~~~~~~
Str*%(10) = 220
Str*%(20) = 320


= Boost present, unequip -> :( =

Str*&(20) = 320 - 32 = 288 [ -> same scenario as example 2]
Str#&(20) = 288 / 2 = 144 [Boost gone now]

Difference = -56


= Boost done at level 20, prior unequip = UH-OH! =

Str#%(20) = 320 / 2 = 160 (after bonus is gone, taking off 100% AT level 20)
Str#&(20) = 160 - 16 = 144

Difference = -56

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

Ouch, right? So what must be done to avoid the issues as shown above? Well, you should keep track of exactly how much of a boost you gave and take away the same amount.


To do this, for the percentages, it is VITAL that you DO have saved variables for them (not through list/Mods though).

The reason is that stats do change over time so you should contain the value that it added initially. not taking this in to account will give you the example scenarios.

And you want to make sure you SAVE them so no under /tmp for this % bonus given variable :)


So here's one example of what you can do:

Equip
  var
     Name = "???"   //  Name can be saved since it is not under ../var/tmp/...
     mob/Owner      //  Be sure to set up on pickup or creation of the item. See the mob/Entered() version below.
     list/Mods_percent_bonus = list()
     tmp           //  [3] /tmp makes the following variables unsavable in a savefile
           Mods = list()  //  Because it is not saved, the Mods list will go to the default/defined value.



Equip()
  if(found "%")
     var/orig_stat = Stat[%BonusStat]
     var/bonus = round(orig_stat * (%BonusStat/100), 0)
                //  [assuming value was given as a percent. Ex: * 200% /100 -> * 2. If 50% = 50/100 = *0.5]

     Owner.Stat += bonus
     src.Mods_percent_bonus[%BonusStat] = bonus


Unequip()

   if(length(Mods_percent_bonus)) [or loop right away]

      Loop in Mods_percent_bonus (= %BonusStatStored)
         Owner.Stat -= Mods_percent_bonus[%BonusStatStored]
         src.Mods_percent_bonus -= %BonusStatStored



And do not forget to round off since you can get decimal places when dealing with %!

---
Edit: Added the bonus in to it AND fixed my math... and even if I forgot something to the math, you can still see that the current method is a no-no :(

To sum up: Save the stat amount you added on equip and remove that amount once done.

And that should be the last of the edits (mainly so I would stop losing the votes :P)