Trinitycore: Core/Spells: Drain Soul cancels prematurely

[adeutscher]

Description:

Drain Soul cancels prematurely when another enemy dies.

Current behaviour:

Drain Soul will cancel if another enemy dies while it is channeling.

Expected behaviour:

Drain Soul should only be cancelled if the duration runs out or if the target dies.

Steps to reproduce the problem:

1. Create a Warlock
2. Add a rank of Drain Soul (.learn 1120)
3. For convenience:
   
   
   
   • Learn a high-rank Corruption: .learn 11672
   
   
   • Learn Summon Voidwalker for a meatshield: .learn 697
   
   
   • Add Soul Shard for Voidwalker: .additem 6265
   
   
4. Enter combat with 2 experience-granting enemies, which we will call 'Creature A' and 'Creature B'.
5. Cast enough DoTs on Creature A so that it will die.
6. Before Creature A dies, cast Drain Soul on Creature B.
7. When Creature A dies, Drain Soul will be cancelled.
   
   
   
   • This happens with or without Improved Drain Soul
   
   
   • This happens with any rank of Drain Soul
   
   

Branch(es):

3.3.5

TC rev. hash/commit:

7258d00f

Operating system:

Fedora 31

[adeutscher]

These are my findings on the cause of this error so far. I think the problem is caused by the death of any enemy removing the Drain Soul aura prematurely. My current problem is that I'm not sure where this removal is happening.

My attempts to look into this problem brough me to Spell::UpdateChanneledTargetList. When this method returns false, then the spell is cancelled.

I'll be referring to the subject of Drain Soul as the victim, and the extra enemy as the extra.

By my reading, the relevant bits of the Spell::UpdateChanneledTargetList method work like so:

1. Set channelTargetEffectMask to be equal to the spell's m_channelTargetEffectMask.
2. Cycle through TargetInfo items for the spell:
   a. If Spell::IsValidDeadOrAliveTarget returns false for the TargetInfo's unit, then skip to the next loop iteration.
   b. If there's an overlap between the spell's channelAuraMask (a summation of channelTargetEffectMask and the spell's effects on aura apply) and the targetInfo's EffectMask, then if we get an AuraApplication and the caster is the target or the victim is in range, then keep going in the loop. Else, continue to the next loop iteration.
3. The method returns true if channelTargetEffectMask was reduced down to 0.

Observations

• In the case of the working Drain Life, the victim is the only target, and consistently gets the 1 flag removed.
• In the case of Drain Soul channel ticks, 1 and 2 are removed from the Drain Soul victim, and 4 is removed from the caster. Channeling continues on as intended.
• When a Drain Soul finishes as intended with the death of the victim, no values are removed, and the surviving 7 value leads to an a return value of false. This is also intended behavior.
  
  
  
  • Both the victim and the caster are valid targets.
  
  
• When the extra enemy dies while Drain Soul is channeling on the victim, then the victim's 1 and 2 flags are removed, but the caster's is not. The surviving 4 value leads to an incorrect return value of false. The comment seems to think that the aura was dispelled/removed.

[adeutscher]

This only seems to happen when the warlock is the one dealing the killing blow to the extra enemy.

[illfated]

(cosmetic) title suggestion: Core/Spells: Drain Soul cancels prematurely

[adeutscher]

@illfated Edited

[illfated]

OK, fair enough. Improvement achieved.

[adeutscher]

Found another clue to the problem in Aura::TriggerProcOnEvent. When the spell procs, Drain Soul is considered to be using charges, with a charge count of 0. This causes the aura to be removed.

Commenting the Remove() call in a mock build causes drain soul to no longer be interrupted. Will look into how to set Drain Soul to not be considered stackable next sitting.

On a related note, it looks like the Improved Drain Soul proc also happens on any death. Looks to be its own problem, but will look into fixing that as well.

[adeutscher]

After looking into things a bit more, I was going at things incorrectly in my comment. Instead of "How do I make this stackless", I should have been looking to have the aura say "no, this isn't the right time to proc with the death of the extra creature".

I've been having a bit of difficulty with "Is this the original creature?". My next phrasing attempt will be "does the newly-dead unit have a Drain Soul application from the caster?"

[adeutscher]

@illfated I had an interesting surprise in Classic this evening when I experienced this bug on my Warlock. While Classic certainly isn't WotLK (yet), does it change anything about the status of this bug if it can be reproduced on a retail server?

[illfated]

Thank you for asking, even though I am not a real TC member. Well, I guess it is close to what TC would consider a retail bug, which often will classify as something not required to fix in the TC source. Even so, there have been some minor cases where TC has considered the bug to be trivial enough to fix in the TC source (saying something to the effect of "We don't have to keep a retail bug if it is a trivial matter to fix it without breaking something else"). From what I have seen, it depends on what the TC members or owners would like to have fixed in the TC source, even if it is a "blizzlike" bug.

(I would prefer if a TC member or owner could confirm whether they want to have the bug fixed or not, depending on when or if it was solved in the Blizzard retail servers.)

[adeutscher]

After taking a peek at the WoW Wiki page for Improved Drain Soul, I think the retail problem is specific to classic. In 2.1.0, we got the following fix: "This talent will no longer trigger when a creature other than the one you are draining dies.". So the premature proc for this is a vanilla-specific "feature" that should be gone by the WotLK era.

This tracks with the proc handler function for Drain Soul, which always executes and then nopes out if the caster doesn't have Improved Drain Soul.

[illfated]

Thank you for the added information. Sure, as long as that bug was fixed in retail before and up to patch 3.3.5a, it is a valid goal to pursue solving this bug.

[Keader]

2b1b36f5617ac10dc373410b69957eadb2766453