Trinitycore: Scripts/Spells: Ebon Gargoyle - Attack Power too low

[tje3d]

Description:

Ebon Gargoyle hitting too low, should hit around 2K ( and its damage currently does not scale with the owner attackpower )

Branch(es): 3.3.5a

TC rev. hash/commit: d3d0640

TDB version: TDB 335.63

Operating system: Debian9

[ariel-]

Rev. 7e71b4535a0e927ecdee30489e5a3c0b6f247559

Cannot reproduce

Ebon Gargoyle's Gargoyle Strike hits Heroic Training Dummy for 1554 Nature.
Ebon Gargoyle's Gargoyle Strike hits Heroic Training Dummy for 1554 Nature.
Ebon Gargoyle's Gargoyle Strike hits Heroic Training Dummy for 1399 Nature.(155 Resisted)
Ebon Gargoyle's Gargoyle Strike hits Heroic Training Dummy for 1554 Nature.
Ebon Gargoyle's Gargoyle Strike hits Heroic Training Dummy for 1554 Nature.
Ebon Gargoyle's Gargoyle Strike hits Heroic Training Dummy for 1554 Nature.
Ebon Gargoyle's Gargoyle Strike hits Heroic Training Dummy for 1399 Nature.(155 Resisted)
Ebon Gargoyle's Gargoyle Strike hits Heroic Training Dummy for 1554 Nature.
Ebon Gargoyle's Gargoyle Strike hits Heroic Training Dummy for 1554 Nature.
Ebon Gargoyle's Gargoyle Strike misses Heroic Training Dummy.
Ebon Gargoyle's Gargoyle Strike hits Heroic Training Dummy for 1554 Nature.
Ebon Gargoyle's Gargoyle Strike hits Heroic Training Dummy for 1554 Nature.
Ebon Gargoyle's Gargoyle Strike hits Heroic Training Dummy for 1554 Nature.
Ebon Gargoyle's Gargoyle Strike hits Heroic Training Dummy for 1554 Nature.

[tje3d]

its works as expected now, thanks

[TheWinchesters]

TC rev. hash/commit: 2ebaa9f
It seems that this problem is back.
No matter how much AP the caster has, the Ebon Gargoyle hits 140.
Could someone else confirm?

[Strazhnik]

Confirmed on 858782f68a33ca8f2aacee3e1d9b1435b2edfb30

[illfated]

@Strazhnik : please edit your comment to add the TC commit rev. hash, indicating (for future reference) where you confirmed this issue.

[sirikfoll]

It broke again with 8051ff13dfcf3819b58adfcec1608c08dcbcd080
Changing the SpellScript's hook to OnEffectLaunch and using SetEffectValue(damage); instead of SetHitDamage(damage); makes it scale again, but the damage is still a little bit lower than it was with the spell hardcoded in Spell::EffectSchoolDMG

[Strazhnik]

@sirikfoll @illfated, any ideas?

[illfated]

This is mostly something real coders (who also occasionally play PvP) know something about.
Comments from @Keader , @sirikfoll or @ariel- would be useful here.

[Strazhnik]

@Aokromes, @Killyana, may be revert?

[Aokromes]

ofc no, the previous code was hack.

[Shauren]

What about OnHit + SetHitDamage? The way damaging spells work is that damage effect handlers only add damage value to total damage done to target, not deal it directly - it happens much later in processing, between OnHit and AfterHit hooks

[Strazhnik]

I don't have ideas how this is realize.
This is damage formula correct?

[D0d0]

This small patch can fix Gargoyle low damage

Index: src/server/scripts/Pet/pet_dk.cpp
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- src/server/scripts/Pet/pet_dk.cpp   (revision 97c5f86c17bac6123323f28230bc0d9e48449049)
+++ src/server/scripts/Pet/pet_dk.cpp   (revision b6bd398a2c58056b282e298c3a4c6633134bd441)
@@ -148,6 +148,8 @@
         {
             if (caster->getLevel() >= 60)
                 damage += (caster->getLevel() - 60) * 4;
+
+            damage = caster->SpellDamageBonusDone(GetHitUnit(), GetSpellInfo(), damage, SPELL_DIRECT_DAMAGE, EFFECT_0, { });
         }

         SetHitDamage(damage);

[Strazhnik]

@Sorikoff, can you comment this fix?

[Sorikoff]

Hi. I'm sorry, but I'm no expert here. Probably you should ask TC members.

[illfated]

@xvwyh : would you like to comment on the code above?

[xvwyh]

@Shauren correctly pointed out that damage is calculated not in OnEffectHitTarget phase, but rather upon launch at target, just look at Spell::EffectSchoolDMG:

    if (effectHandleMode != SPELL_EFFECT_HANDLE_LAUNCH_TARGET)
        return;

So 8051ff13dfcf3819b58adfcec1608c08dcbcd080 is incorrect in that the code was moved to OnEffectHitTarget everywhere. If you only care about rewriting the damage of the spell entirely with your script - change all hooks in the aforementioned commit to either one of these:

• OnEffectLaunchTarget+PreventDefaultEffect(effIndex) at the end to prevent Spell::EffectSchoolDMG from being called
• OnEffectHitTarget
• OnHit <- this is the preferred method used in the vast majority of scripts

EDIT: Actually, looking at it, it seems like OnEffectHitTarget is still being called before the damage was dealt, so, um, seems like you can keep using it. The real problem is described below anyway.

Regardless of what option you choose, know that you're changing the final outgoing damage of the spell. Whatever you put in SetHitDamage will be the number that will be subjected to absorbs/resists and end up being dealt to the enemy. So if you're choosing to completely overwrite the damage this way - you need to execute the rest of the code from Spell::EffectSchoolDMG manually, namely:

        if (unitCaster && damage > 0 && apply_direct_bonus)
        {
            damage = unitCaster->SpellDamageBonusDone(unitTarget, m_spellInfo, (uint32)damage, SPELL_DIRECT_DAMAGE, effIndex, { });
            damage = unitTarget->SpellDamageBonusTaken(unitCaster, m_spellInfo, (uint32)damage, SPELL_DIRECT_DAMAGE);
        }

to account for damage buffs on the caster, damage reductions on the target and so on. You will have to put these two lines in every single script you intend to extract from the hacks inside Spell::EffectSchoolDMG. And yes, they will have to be placed before every SetHitDamage added in 8051ff13dfcf3819b58adfcec1608c08dcbcd080, unless the original hack was setting apply_direct_bonus to false. Think about whether it's worth it. Maybe it'd be better to create another script hook, say, OnDamageCalc?

Now specifically about the Gargoyle. From what I see in Pet.cpp, you're already calculating the part of the damage, that's supposed to come from AP, there:

                    SetBonusDamage(int32(GetOwner()->GetTotalAttackPowerValue(BASE_ATTACK) * 0.5f));

I'm sure this by itself is a hack, and Gargoyle Strike should be set to scale from AP via `spell_bonus_data`, and the AP should be shared from owner to gargoyle via "pet scaling" auras, but I can't really help with the latter because I myself have no idea how to properly implement scaling auras, or, rather, where to take the data about their scaling from. Anyway, this is all beside the point. And the point is - bonus damage added via SetBonusDamage is applied to spell's damage inside Unit::SpellDamageBonusDone:

    if (HasUnitTypeMask(UNIT_MASK_GUARDIAN))
        DoneAdvertisedBenefit += static_cast<Guardian const*>(this)->GetBonusDamage();

Since the call to SpellDamageBonusDone is missing in the scripts - it ends up not being accounted for.

P.S. I hate pets. I dread working on pet issues. When you subject yourself to that - you're plunging into an abyss full of despair.

[D0d0]

@xvwyh agree with you, it is hack, but we needed it cause unholy was “unplayable” without it

[Strazhnik]

@jackpoz, can you apply this hack fix?

[jackpoz]

Maybe it'd be better to create another script hook, say, OnDamageCalc?

this looks like the proper way to fix the issue

[jackpoz]

actually a revert is the best choice as the spell effect checks spell family and we don't have any hook for spell families. It makes no sense to assign spellscripts to single spell ids in this case

[Strazhnik]

I agree :)

[xvwyh]

@jackpoz, spell families in the effect handler don't matter in the slightest. They are only used there as a way to categorize hacks so that programmers can at a glance figure out that these are hacks for druid spells, and these are hacks for paladin spells, and these are for shaman, and so forth. For scripts we have separate .cpp files for that - spells_druid.cpp, spells_paladin.cpp etc. And if somewhere they're used in combination with spell family flags test, then the same result can be achieved by assigning the spell script to every spell with that family name and flags. It's even better to have granular control like that, because way too often blizzard copies class spells for use by mobs and they don't even bother to change the family and flags, resulting in class spell hacks erroneously affecting them as well.

But yes, a revert is a better choice if nobody wants to take on the task of making more script hooks.

[jackpoz]

the point is the hook will be inside a single spell effect Spell::EffectSchoolDMG and will only handle a part of the body function.

Your point doesn't make much sense for cases like https://github.com/TrinityCore/TrinityCore/blob/8051ff13dfcf3819b58adfcec1608c08dcbcd080/src/server/game/Spells/SpellEffects.cpp#L328

// Meteor like spells (divided damage to targets)
if (m_spellInfo->HasAttribute(SPELL_ATTR0_CU_SHARE_DAMAGE))
{
  uint32 count = 0;
  for (auto ihit = m_UniqueTargetInfo.begin(); ihit != m_UniqueTargetInfo.end(); ++ihit)
  {
    if (ihit->MissCondition != SPELL_MISS_NONE)
      continue;

    if (ihit->EffectMask & (1 << effIndex))
      ++count;
  }

  // divide to all targets
  if (count)
    damage /= count;
}

This code applies to every spell that has (or will have in new WoW versions) attribute SPELL_ATTR0_CU_SHARE_DAMAGE , making it a nice generic solution. If we were to implement a script for every spell, it will require to build and maintain a list of spells that have the attribute and update them at every WoW patch iteration.

But maybe a mix of both solution would be the best then ? Moving some of the cases to spell scripts with a new hook and leave some other cases in the current switch.

[xvwyh]

Whoa, slow down there. Have you not noticed "CU" in SPELL_ATTR0_CU_SHARE_DAMAGE? That's not a blizzard's attribute, that's TrinityCore`s attribute. You have complete control over what spells is it applied to.

Not to mention that I don't like that the aforementioned piece of code is in case SPELLFAMILY_GENERIC:. What, are there no class spells that need to split damage over all targets? Even it there aren't any, why shouldn't there be an ability to apply this feature on class spells? Once again, you have complete control over this attribute, if you don't want it applied on non-SPELLFAMILY_GENERIC spells - then don't apply it in SpellMgr.cpp or in `spell_custom_attr`! Simple. That SPELL_ATTR0_CU_SHARE_DAMAGE handler needs to be pulled out of the switch and placed somewhere in-between the switch and the codeblock that calls SpellDamageBonusDone and the like.

It's true that there would be need to keep track of spell changes, remove scripts as spells are removed, apply scripts as spells are added. But then again, isn't there a need for that already? The meaning of spell family flags can change across patches as blizzard removes, adds, removes-and-then-readds class spells. Also this issue is for 3.3.5a branch, which I'm pretty sure isn't getting patched anytime soon. 😃

[jackpoz]

Please check/test https://github.com/TrinityCore/TrinityCore/pull/23228 and see if the damage is better with that PR. In the end I didn't have to add any new hook, I just had to use the "right" one :)

[xvwyh]

That's neat, never seen it being used that way. Yeah, effect hooks are executed before effect handlers, so as long as the effect handler doesn't override the value of damage in some way, I can see it working.

[Strazhnik]

Nice work :) I will test later