AddItem Method

[Dexter_Lexia]

I don't know if anyone knows this, but a method was added to BaseCreature a while back. Its called SetWearable. I'll show examples shortly. Most scripts use AddItem method to equip items on NPC's and monsters. This is wrong. Most client crashes are caused by mobiles having multiple items added to a specific layer. You could use EquipItem method, however if it returns false, that item will be leaked onto the internal map unless it is handled properly. Using the SetWearable method will automatically handle layer conflicts, and even has some handy arguments for making the item movable and setting the hue.

public virtual void SetWearable(Item item)
        {
            SetWearable(item, -1, 0.0, true);
        }

        public virtual void SetWearable(Item item, int hue)
        {
            SetWearable(item, hue, 0.0, true);
        }

        public virtual void SetWearable(Item item, double dropChance)
        {
            SetWearable(item, -1, dropChance, true);
        }

        public virtual void SetWearable(Item item, int hue, double dropChance)
        {
            SetWearable(item, hue, dropChance, true);
        }

        public virtual void SetWearable(Item item, bool removeConflicting)
        {
            SetWearable(item, -1, 0.0, true);
        }

        public virtual void SetWearable(Item item, int hue, bool removeConflicting)
        {
            SetWearable(item, hue, 0.0, true);
        }

        public virtual void SetWearable(Item item, double dropChance, bool removeConflicting)
        {
            SetWearable(item, -1, dropChance, true);
        }

        public virtual void SetWearable(Item item, int hue, double dropChance, bool removeConflicting)
        {
            if (!EquipItem(item))
            {
                if (removeConflicting)
                {
                    Item i = FindItemOnLayer(item.Layer);

                    if (i != null)
                    {
                        if (Backpack == null)
                            i.Delete();
                        else
                            PackItem(i);
                    }

                    if (!EquipItem(item))
                    {
                        if (Backpack == null)
                            item.Delete();
                        else
                            PackItem(item);
                    }
                }
                else
                {
                    if (Backpack == null)
                        item.Delete();
                    else
                        PackItem(item);
                }
            }

            if (hue > -1)
                item.Hue = hue;

            item.Movable = dropChance > Utility.RandomDouble();
        }

So, using the first method while just passing in the item, and any conflicts will be removed to make room for the new item.

So, in conclusion, STOP USING ADDITEM for equipping equipment on NPCs/Monsters!!!

 

[Voxpire]

Since it causes the client to crash when used the "wrong" way, why not adjust AddItem() to compensate?

Asking users to make changes to the way they've been doing something for 20 years is probably not going to work.

AddItem() could easily handle layer conflicts and bouncing items to the target mobile's pack when they are replaced.

 

[Dexter_Lexia]

I thought about that, but then I ran into EquipItem, which does this. I'm assuming it's been around since AddIten or at least a long time. The problem with EquipItem is if it returns false, the item will leak to the internal map.

 

[Voxpire]

I know EquipItem does that, but what I'm saying is, AddItem should do the same, but force the equipment switch if something already exists, rather than fail. At the moment, as you've said, it forces the equipment to be equipped, but it simply doesn't remove an existing piece. EquipItem just fails if there is an existing piece.

It's a similar concept to TryDropItem() vs DropItem() where the latter is an assertion that the result will be as expected.

Making an edit so that using AddItem guarantees that the item gets equipped as a priority, independent to EquipItem, might just save a lot of trouble.

 

[PyrO]

This is just my opinion but wouldnt it make more sense to have AddItem add the item to the mobiles backpack while Equipitem would equip the item with putting the current equipment on that layer into the backpack?

 

[Dexter_Lexia]

AddItem, with respect to a mobile, adds it to thier person and actually equips it. This is where all the packet stuff is handled and sent to the client. I believe EquipItem was originally meant to safely handle all the layer conflicts, however was never used. Instead, literally hundreds of scripts bypassed this, which is why Voxpire suggests making the changes to AddItem.

 

[Ravenwolfe]

Have to agree with Voxpire, easier to change methods than habits

 

[Dexter_Lexia]

Should I put the anti-conflict code in Mobile.cs, or should I mark AddItem virtual and override. My first thought is to mark is virtual, and override it so if there is a conflict, we can utilize PackItem to put the item in. This auto-creates a backpack for the creature.

 

[Talow]

I'd virtual and override, allows for more changes with less effort.

 

[Voxpire]

It allows for more changes, but that also allows for more bugs.
If AddItem is improperly overridden, it could cause issues or even a stack overflow if the intent of the original method is misunderstood.

AddItem must always assert that the item is successfully added to the Mobile, but the problem is ensuring the Backpack is not null from the core, because the Backpack class isn't available in that context.

The only real option we have would be with making AddItem virtual anyway.

With that being said, if the AddItem method is overridden by BaseCreature, it should also be sealed to prevent child classes from overriding it again, same with PlayerMobile.

The override and sealed keywords can both be used in conjunction.