Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🐛 [Bug Report] [TBC] Attack Power mods with negative value (debuff) cause zero total Attack Power for Player #3872

Open
CyberBarr opened this issue Feb 27, 2025 · 4 comments · May be fixed by cmangos/mangos-tbc#730
Labels
Expansion: TBC (2.4.3) Issues relating to the TBC Expansion (2.4.3). Info: Needs Replication Issue needs replication before further action. System: Player Data saved stats, items, achievements, queststatus System: Spells e.g. spell scripting, spell editing, and missing spells

Comments

@CyberBarr
Copy link

Bug Details

In TBC core, UNIT_FIELD_ATTACK_POWER_MODS field is set as a signed 16 bit integer within Player::UpdateAttackPowerAndDamage(bool ranged) within StatSystem.cpp.

SetInt16Value(index_mod, 1, m_attackPowerMod[size_t(mod)][size_t(AttackPowerModSign::MOD_SIGN_NEG)]); // m_attackPowerMod is a 2-D floating point array

The field value is ONLY ever retrieved once and as an unsigned 16 bit integer in Unit::GetTotalAttackPowerValue(WeaponAttackType attType) within Unit.cpp. The resulting outcome is integer overflow whenever a debuff/negative attack power mod is applied to a Player. When applied, the Player's total attack power value becomes 0.

int32 ap = GetInt32Value(UNIT_FIELD_ATTACK_POWER) + GetUInt16Value(UNIT_FIELD_ATTACK_POWER_MODS, 0) - GetUInt16Value(UNIT_FIELD_ATTACK_POWER_MODS, 1); // overflow results in subtracting a large positive 16 bit integer value

Steps to Reproduce

  1. Cast any spell with AP reduction onto a Player, such as Demoralizing Shout.
  2. Check DPS stats on Player's original in-game stat interface. Notice DPS is exactly reduced to weapon damage amount.
  3. Alternatively, set breakpoint within GetTotalAttackPowerValue to inspect field values retrieved.
  4. Re-affirm by attempting to do melee damage to an enemy while negative mod is applied. Result is drastically reduced melee damage done.

Expected behavior

DPS on Player's in-game stat interface is exactly reduced to weapon damage amount. Damage done by the Player is reduced more than it should be while a negative mod is active.

Suggested Workaround

Add the following function within Object.h:

int16 GetInt16Value(uint16 index, uint8 offset) const { MANGOS_ASSERT(index < m_valuesCount || PrintIndexError(index, false)); MANGOS_ASSERT(offset < 2); return *(((int16*)&m_uint32Values[index]) + offset); }

Change the following:

int32 ap = GetInt32Value(UNIT_FIELD_ATTACK_POWER) + GetUInt16Value(UNIT_FIELD_ATTACK_POWER_MODS, 0) - GetUInt16Value(UNIT_FIELD_ATTACK_POWER_MODS, 1);

to:

int32 ap = GetInt32Value(UNIT_FIELD_ATTACK_POWER) + GetUInt16Value(UNIT_FIELD_ATTACK_POWER_MODS, 0) + GetInt16Value(UNIT_FIELD_ATTACK_POWER_MODS, 1); // addition of the stored (as negative) value

Crash Log

No fault occurs.

Core SHA1 Commit Hash

a9c89d6

Database SHA1 Commit Hash

0caae67b319ae0e31e256eb7edd3e784d127249a

Operating System

Windows 10

Client Version

2.4.3 (The Burning Crusade)

@CyberBarr CyberBarr added the Info: Needs Replication Issue needs replication before further action. label Feb 27, 2025
@insunaa
Copy link
Contributor

insunaa commented Feb 27, 2025

I don't think this is ultimately the correct solution. The UNIT_FIELD_ATTACK_POWER_MODS should never ever be set to a negative value, so instead we should investigate why/if that occurs.

@insunaa
Copy link
Contributor

insunaa commented Feb 27, 2025

Try with this:

diff --git a/src/game/Entities/Unit.cpp b/src/game/Entities/Unit.cpp
index cd79cac70..622515ff1 100644
--- a/src/game/Entities/Unit.cpp
+++ b/src/game/Entities/Unit.cpp
@@ -9437,6 +9437,7 @@ bool Unit::HandleStatModifier(UnitMods unitMod, UnitModifierType modifierType, f
             if (modifierType == TOTAL_VALUE)
             {
                 auto sign = amount > 0 ? AttackPowerModSign::MOD_SIGN_POS : AttackPowerModSign::MOD_SIGN_NEG;
+                amount = std::abs(amount);
                 if (unitMod == UNIT_MOD_ATTACK_POWER)
                     m_attackPowerMod[size_t(AttackPowerMod::MELEE_ATTACK_POWER)][size_t(sign)] += apply ? amount : -amount;
                 else if (unitMod == UNIT_MOD_ATTACK_POWER_RANGED)

@CyberBarr
Copy link
Author

CyberBarr commented Feb 27, 2025

I agree about it not being a negative value. The proposed solution is likely sub-optimal but it worked as a hotfix for my case.

Edit: I should have clarified, I was unaware of the potential downstream impacts of removing the negative signage, specifically being unaware about usage of m_attackPowerMod elsewhere by the core.

@insunaa
Copy link
Contributor

insunaa commented Feb 27, 2025

I just checked and you're correct, the number should be negative. my change is incorrect.

Correct would be instead

int32 ap = GetInt32Value(UNIT_FIELD_ATTACK_POWER) + GetUInt16Value(UNIT_FIELD_ATTACK_POWER_MODS, 0) + static_cast<int16>(GetUInt16Value(UNIT_FIELD_ATTACK_POWER_MODS, 1));

@insunaa insunaa added System: Player Data saved stats, items, achievements, queststatus System: Spells e.g. spell scripting, spell editing, and missing spells Expansion: TBC (2.4.3) Issues relating to the TBC Expansion (2.4.3). labels Feb 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Expansion: TBC (2.4.3) Issues relating to the TBC Expansion (2.4.3). Info: Needs Replication Issue needs replication before further action. System: Player Data saved stats, items, achievements, queststatus System: Spells e.g. spell scripting, spell editing, and missing spells
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants