Multiple spell reflection

I know this whole thing is questionable, and I am trying to find a proper way to make Spell Reflection work as intended.

How it should work (clientside view):

  1. You must have Spell Reflection aura at the moment when spell is launched (SpellGo).

  2. If spell is instant Spell Reflection is instantly removed, else Spell Reflection if removed when missile reaches you.

  3. If Spell Reflection aura duration reaches zero while first missile didn’t reach you yet then aura stucks at flashing 0 until this first missile reaches you.

  4. During this period from SpellGo until spell reaches you any spell launched at you should be reflected (if you can reflect it ofc).

Please correct me if I’m wrong.

http://www.wowwiki.com/Spell_Reflection#Multiple_Reflects

http://www.wowhead.com/spell=23920#comments

https://github.com/TrinityCore/TrinityCore/issues/10391

especially:

http://www.wowhead.com/spell=23920#comments:id=114390:reply=174109

video 1 (missiles + instant spell, different casters):

My humble attempts to make Spell Reflection work properly:

first (deprecated) at 53cc37bcec

second at b926039e18

Any thoughts?

As far as I remember ProcDamageAndSpellFor is called on spell hit (missile reaching the target), not on launch, so your fix won’t work properly.

It works, otherwise I wouldn’t post it.

Ok, i see now, ProcDamageAndSpell is called from SpellHitResult. You’ve made a pretty good research on the topic, nice.

Thanks, but for me it looks like ‘non-blizzard hack’ at this moment and I opened this topic so maybe community will give me some clues of how it should be implemented in TC before I make a pull request

The update arg in [COLOR=rgb(0,0,0)]Aura::SetDuration parameter is kinda awkward. Any later call to [COLOR=rgb(0,0,0)]SetNeedClientUpdateForTargets(); will display altered m_duration so your fix is very fragile (in this place).

I see. Seems like this can only be properly implemented with eventmap for spell auras.

Updated first post: added second fix attempt, one more rule and one more video as proof.

You can extract the logic of [COLOR=rgb(0,0,0)]Spell::AddUnitTarget()::L2237 to a separate function in SpellInfo class.

Also, i don’t think you implement 5) properly because new event won’t be re-scheduled after aura duration reaches 0. Is that intended? All other things look good.

If we allow event being rescheduled player will be able to reflect forever against pack of casters spamming on him (ex. something like frostbolts), I didn’t see such thing in the video, that’s why.

Not trying to be smartass, but this delay calculation requires target and caster. If we are to give them as arguments then I guess it will be better to leave this for another time

Well, I’ve interpreted 5) that way.

I don’t understand your rationale for duplicating a formula instead of putting it in a function (SpellInfo.cpp is already coupled to Unit.h so no issue there imo).

Make this a PR and make sure you include clientside description of what should happen in the commit message - it’d be nice to have that info around. I hope someone merges it in, I’m sort of a “retired” dev, I just lurked here because i find the topic (and surprisingly the fix too - TC definitely would benefit by using event queue more rather than direct calls) very interesting.

Then I’ll state this as clearly as I can: If I add a function for this formula it will be illogical to not replace every instance of this code with a function. That will increase amount of code to review and may bring unneccesary questions.

Or maybe it will be better to push two commits with this PR? First one adds this function and second is a fix.

In any case, I hope it will be merged too, thanks for your help, I really appreciate it.

I wonder… I havent found a single proof for 5) . Can you pinpoint it ?

If I remember correctly, the second video shows a bug from bc when if you sat down spell reflect wouldn’t fade normally

I found this:

https://threadmeters.com/wow/3IBp92/sit_spell_reflect_macro/

Well things can have changed from BC to WOTLK.

Actually the point I was trying to make was:

[COLOR=rgb(40,40,40)]5) Every reflected spell delays aura expiration (you can end up reflecting all spells until aura fades normally)

have been taken from the second video (http://youtu.be/ffEkmJfZ5a0?t=6m32s), which is from bc, and its not because spells delay aura expiration, rather it’s because swifty sits down, and that was a bug when you sat down spell reflect would only fade by expiring, but not from reflects.

This makes me sad… how could I forget, I’m sure I’ve seen it… somewhere (never played on offi), so long ago that I cannot recall. Now that you mentioned this I can say you’re right.

But I didn’t try to find more proofs of this bug, instead, I found a video that shows how second reflected spell does not delay spell reflection aura fade (even though this video is not the best example and shitty quality) 0:08 - 0:15

Edit: Updated 1st post. I also made small modifications to the fix so direct charge drop will not conflict with delayed one (just in case)

AFAIK multi-reflect is intended in WotLK

If someone cast an instant spell, and other casters finishes casting their spells, all spells should be reflected.

Check if target has spell reflect aura should be done at the point when the casted spells leave the casters hands

Its like mage’s shatter combo (frostbolt crits, ice lance crits)

I could be wrong, anyway this topic is very interesting

https://github.com/TrinityCore/TrinityCore/pull/11288