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

Core/Spells: Fixed double heal reduction on Earth Shield (issue #4032). #11915

Merged
merged 1 commit into from
Aug 24, 2014

Conversation

Numielle
Copy link
Contributor

The SpellScript for Earth Shield applies healing reducing auras once when the spell is casted on the target and every time when the buff procs. The suggested code revokes the application of healing reduced auras at cast time which yields the desired behavior of 50% healing reduction when instead of 75% when Earth Shield was cast while any such aura was active on the target.

Edit: Tested in duels with an arms warrior applying Mortal Strike to a Resto Shaman.

@Numielle Numielle changed the title Core/Spells: Fixed double heal reduction on Eart Shield (issue #4032). Core/Spells: Fixed double heal reduction on Earth Shield (issue #4032). Apr 20, 2014
@Warpten
Copy link
Contributor

Warpten commented Apr 20, 2014

My understanding of this is, the aura's amount gets updated with spell bonus done/taken on every proc, meaning that the value is always going sequentially down. Did you try setting amount = GetSpellInfo()->Effects[effIndex].CalcValue(GetCaster()) before applying spell taken/done bonus?

@Numielle
Copy link
Contributor Author

The aura's base amount does not get updated on every proc, neither before nor after my patch. I tested this by equipping a weapon with bonus healing instead of the starter mace while the Earth Shield was active on the casting Shaman. This also means that there is no sequential decrease. It seems to me that instead of re-evaluating the base amount per proc, the precalculated value is used to determine the effective heal.

Since I'm new to Trinity it will take me some time to evaluate your suggestion, once I know where it belongs, I will get back to you on this matter.

Edit: For HoTs like Renew it is actually the case that the aura's amount gets updated every tick. Maybe it is possible to change the script of Earth Shield to not use a precalculated value, I don't know however what the correct behavior is (precalculated vs updated), Maybe I can find a source on this.

@Warpten
Copy link
Contributor

Warpten commented Apr 20, 2014

That's what i meant. amount is passed by reference since it is used in AuraEffect itself. By overwriting it with the default value (from DBCs with the shaman's talents, which is what my suggestion does) and then calling spellhealingbonusdone/taken just like it is in the current code, there should not be any double heal reduction.

Unless i totally misunderstood you, and you meant that healing reductions were applied in both spellhealingbonusdone and spellhealingbonustaken. If so, it should probably be only in the latter.

(I'm on my phone, forgive the big chunk of text)

@Warpten
Copy link
Contributor

Warpten commented Apr 20, 2014

I could be wrong about the fact that DoCalcAmount hooks are not called on proc; not touching the code for over five months doesn't help remember. But if it is not, doesn't it mean that if earth shield procs after ms faded, ms would still be considered (incorrectly) in the calculation?

I'll fiddle with it once I get my devenv set up again, shouldn't take more than a week.

@Numielle
Copy link
Contributor Author

No worries, I'm glad for any kind of feedback. The healing reduction is only applied in SpellHealingBonusTaken.

I commented the call to SpellHealingBonusTaken in CalculateAmount to "simulate" overwriting the value. This however leads to Amplify Magic not having any effect on Earth Shield anymore. Hence I believe with the current implementation, only partial calculations have to be reverted.

Your theory about Earth Shield still being reduced after MS faded is correct.

Let me know if I can be of any assistance when you get your hands dirty! ;-)

@Warpten
Copy link
Contributor

Warpten commented Apr 23, 2014

Well looks like I won't be able to toy with this for some time. I hate my life.

Clearing the assignee; maybe someone will take a look.

Edit: f'ing phone doesn't want to clear it.

@Warpten
Copy link
Contributor

Warpten commented Apr 27, 2014

@Numielle:

amount = GetSpellInfo()->Effects[effIndex /* replace for real effect index. I dont remember if this hook feeds it as a parameter */].CalcValue(GetCaster());
amount = caster->SpellHealingBonusDone(GetUnitOwner(), GetSpellInfo(), amount, HEAL);
amount = GetUnitOwner()->SpellHealingBonusTaken(caster, GetSpellInfo(), amount, HEAL);
canBeRecalculated = true;

Please try this.

@joschiwald
Copy link
Contributor

@Warpten i think that will not work, the problem is that spellbonus is calc for both spells, dummy and the heal effect, same like life bloom, the done bonus part is hacked to not apply it twice

@Warpten
Copy link
Contributor

Warpten commented Apr 27, 2014

@joschiwald: god that's retarded

@joschiwald
Copy link
Contributor

it must be related to SPELL_DAMAGE_CLASS_NONE, to not apply bonuses, same issue like some trinkets and other stuff

@Aokromes
Copy link
Member

Any new on this?

@Shauren Shauren closed this Jun 23, 2014
@Aokromes Aokromes reopened this Jun 23, 2014
@DoodleZero
Copy link

Works for me ^^

@LuqJensen
Copy link
Contributor

its part of a bigger issue with double dipping and this is a hack

jackpoz added a commit to jackpoz/TrinityCore that referenced this pull request Aug 24, 2014
This is a workaround required by the current spell system limitations.
Close pull request TrinityCore#11915 .
@jackpoz jackpoz merged commit 91827e8 into TrinityCore:master Aug 24, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants