[3.3.5] Weapon Damage Calculations when unequipping

[VincentVanclef]

Description:

I noticed some odd values when equipping/unequipping weapons. When i log in with no weapons equipped, my base damage was:

http://prntscr.com/me6oxt
(82-84)

Then after equipping a weapon:

http://prntscr.com/me6p2q

(248-324)

After unequipping it, my new base damage with no weapons becomes:

http://prntscr.com/me6p9t

(99-101)

After some debugging i found the source:

StatSystem CalculateMinMaxDamage calls:

float Unit::GetAPMultiplier(WeaponAttackType attType, bool normalized) const

which has this check:

Item* weapon = ToPlayer()->GetWeaponForAttack(attType, true);
if (!weapon)
return BASE_ATTACK_TIME / 1000.0f;

When logging in with no weapons equipped, weapon is nullptr and it returns. But after unequipping a weapon, this check is never null. Itll still use that weapon. When i equipped a two-hand and unequipped it, my base damage was around 148-150.

I havent found the direct error yet, and will post once i find it. But there might be someone who knows the weapons system better than me. My guess is that the applyweapon damage check is executed before the players item is properly removed from his weapon slot, so that GetWeaponForAttack wont have a chance to return null.

Current behaviour:

Explained above.

Expected behaviour:

Base damage should go back to normal unarmed state, no matter what weapon was previously equipped.

Steps to reproduce the problem:

1. Log in unarmed.
2. Check base damage and equip any weapon.
3. Unequip said weapon and notice the increase in base damage.

Branch(es):

3.3.5

TC rev. hash/commit:

e4658a1

Operating system: CHANGEME OS

Windows 10

[VincentVanclef]

Update:

void Player::RemoveItem(uint8 bag, uint8 slot, bool update)

calls _ApplyItemMods before m_Items[slot] is set to null. So itll always have a weapon to use. But applyitemmods(false) to remove the stats wont be doable if we remove the item before calling it.. So am stuck here. Fixing one breaks the other. :P

[VincentVanclef]

Update again:

I found a fix (unsure if its the proper way to do it tho)

below the CheckTitanGrip i added:

WeaponAttackType attType = Player::GetAttackBySlot(slot);
if (GetWeaponDamageRange(attType, MAXDAMAGE))
    UpdateDamagePhysical(attType);

Fixed it, now base damage is always correct.

Entire function:

void Player::RemoveItem(uint8 bag, uint8 slot, bool update)
{
    // note: removeitem does not actually change the item
    // it only takes the item out of storage temporarily
    // note2: if removeitem is to be used for delinking
    // the item must be removed from the player's updatequeue

    Item* pItem = GetItemByPos(bag, slot);
    if (pItem)
    {
        TC_LOG_DEBUG("entities.player.items", "Player::RemoveItem: Player '%s' (%s), Bag: %u, Slot: %u, Item: %u",
            GetName().c_str(), GetGUID().ToString().c_str(), bag, slot, pItem->GetEntry());

        RemoveEnchantmentDurations(pItem);
        RemoveItemDurations(pItem);
        RemoveTradeableItem(pItem);

        if (bag == INVENTORY_SLOT_BAG_0)
        {
            bool isWeapon = false;

            if (slot < INVENTORY_SLOT_BAG_END)
            {
                // item set bonuses applied only at equip and removed at unequip, and still active for broken items
                ItemTemplate const* pProto = ASSERT_NOTNULL(pItem->GetTemplate());
                if (pProto->ItemSet)
                    RemoveItemsSetItem(this, pProto);

                _ApplyItemMods(pItem, slot, false, update);

                // remove item dependent auras and casts (only weapon and armor slots)
                if (slot < EQUIPMENT_SLOT_END)
                {
                    // remove held enchantments, update expertise
                    if (slot == EQUIPMENT_SLOT_MAINHAND)
                    {
                        if (pItem->GetItemSuffixFactor())
                        {
                            pItem->ClearEnchantment(PROP_ENCHANTMENT_SLOT_3);
                            pItem->ClearEnchantment(PROP_ENCHANTMENT_SLOT_4);
                        }
                        else
                        {
                            pItem->ClearEnchantment(PROP_ENCHANTMENT_SLOT_0);
                            pItem->ClearEnchantment(PROP_ENCHANTMENT_SLOT_1);
                        }

                        UpdateExpertise(BASE_ATTACK);
                    }
                    else if (slot == EQUIPMENT_SLOT_OFFHAND)
                        UpdateExpertise(OFF_ATTACK);
                    // update armor penetration - passive auras may need it
                    switch (slot)
                    {
                        case EQUIPMENT_SLOT_MAINHAND:
                        case EQUIPMENT_SLOT_OFFHAND:
                        case EQUIPMENT_SLOT_RANGED:
                            isWeapon = true;
                            RecalculateRating(CR_ARMOR_PENETRATION);
                        default:
                            break;
                    }
                }
            }

            m_items[slot] = nullptr;
            SetGuidValue(PLAYER_FIELD_INV_SLOT_HEAD + (slot * 2), ObjectGuid::Empty);

            if (slot < EQUIPMENT_SLOT_END)
            {
                SetVisibleItemSlot(slot, nullptr);
                if (slot == EQUIPMENT_SLOT_MAINHAND || slot == EQUIPMENT_SLOT_OFFHAND)
                    CheckTitanGripPenalty();
            }

            if (isWeapon)
            {
                WeaponAttackType attType = Player::GetAttackBySlot(slot);
                if (GetWeaponDamageRange(attType, MAXDAMAGE))
                    UpdateDamagePhysical(attType);
            }
        }
        else if (Bag* pBag = GetBagByPos(bag))
            pBag->RemoveItem(slot, update);

        pItem->SetGuidValue(ITEM_FIELD_CONTAINED, ObjectGuid::Empty);
        // pItem->SetUInt64Value(ITEM_FIELD_OWNER, 0); not clear owner at remove (it will be set at store). This used in mail and auction code
        pItem->SetSlot(NULL_SLOT);
        if (IsInWorld() && update)
            pItem->SendUpdateToPlayer(this);
    }
}

[Shauren]

Hmm not sure about that solution (not the first bug related to removing things recently either)

[VincentVanclef]

Havent found any issues with it yet tho, i do agree that its not a suitable solution as its basically calling the same function twice. But cant think of a way to check if the weapon is about to be removed

[CraftedRO]

c0758ae Still valid :

2022-10-03.12-53-30.mp4