cmangos / issues

This repository is used as a centralized point for all issues regarding CMaNGOS.
180 stars 47 forks source link

Incorrect Wand Damage Type Reported in Combat Log #1809

Closed lduguid closed 5 years ago

lduguid commented 5 years ago

Current behavior: Using ranged weapon (wand) for damage, the combat log will report inappropriate magic school types, neither of which is the type used by the wand.

Greater Magic Wand (arcane damage) - image

Wand of Decay (shadow damage) - image

Expected behavior:

Log magic school type of wand used.

Steps to reproduce:

  1. Open combat log and wand any creature
  2. For testing i used greater magic wand (arcane damage) and 'wand of decay' (shadow damage)
  3. Only tested TBC

Client version:

1.12.x

Commit hash:

commit 31d25062c28d93bba7d31bfde12f22dcc4648d59

Database version:

commit ab4ead787567f0866cf2651128ec18a80b42cf49

Operating system:

8.1

adellaci commented 5 years ago

I will check on wrath of the lich King tonight

lduguid commented 5 years ago

@Warlockbugs @killerwife - you guys are probably the most knowledage concerning this but please ignore if it better directed at someone else.

I finally got a chance to take closer look at this, the problem seems to go astray in Unit.cpp;

 // !!!
    SpellSchoolMask schoolMask = GetSpellSchoolMask(spell);
    // wand case
    bool wand = spell->Id == 5019;
    if (wand && !!(getClassMask() & CLASSMASK_WAND_USERS) && GetTypeId() == TYPEID_PLAYER)
        schoolMask = SpellSchoolMask(GetWeaponDamageSchool(RANGED_ATTACK));

    // Reflect (when available)

The value of schoolMask after the calling GetSpellSchoolMask(spell);is SPELL_SCHOOL_MASK_NORMAL (1), however after that there is the check for wand and if it meets all the conditions of being a wand weapon, a wand capable class and a player, it attempts to reassign schoolMask. the GetWeaponDamageSchool(RANGED_ATTACK) part will return a SPELL_SCHOOLARCANE (6) which in my case is the appropriate school of magic, as I had the 'greater magic wand' equipped which does arcane damage. Then initializing the enum SpellSchoolMask with 6_ will return SPELL_SCHOOL_MULTI_RADIANT (6).

The Multischools reference the were added to SpellSchoolMask in commit https://github.com/cmangos/mangos-tbc/commit/301152eaeba7704bdd70f11cb2c7171094e5d1b4, these additions appear to cause wand schoolMasks to be incorrectly assigned, as least in the above snippet. I am not sure if this line;

schoolMask = SpellSchoolMask(GetWeaponDamageSchool(RANGED_ATTACK));

can be changed to

schoolMask = GetWeaponDamageSchool(RANGED_ATTACK);

which would fix my immediate issue but that maybe too naive not considering the masking that can take place in SpellSchoolMask(...); ? Regardless, wands needs some additional attention if they are to both GetWeaponDamageSchool and initialize with SpellSchoolMask based on the return value of GetWeaponDamageSchool.

Any suggestions on the best way to resolve this would be appreciated.

Cheers,

adellaci commented 5 years ago

Same in Wrath of the Lich King.

Greater Magic Wand (HolyFire damage) Wand of Decay (Flamestrile damage)

killerwife commented 5 years ago

WB will look at it, but he is currently away for 5 days.

Warlockbugs commented 5 years ago

Fixed in: https://github.com/cmangos/mangos-classic/commit/41624da4155b51d241d89a9a1c638f8606ebb8bf https://github.com/cmangos/mangos-tbc/commit/08a530db10b0b734d84ca8d8ec720d096a000145 https://github.com/cmangos/mangos-wotlk/commit/2ae0f4b4fb7ac66ebc76c0702c0e76622dd2acbe