Trinitycore: 3.3.5 Warlock Drain Soul - No Soul Shard

[WOW-Reborn]

Description:

Warlock Drain Soul does not grant a soul shard upon killing a creature that grants XP.

Current behaviour:

Currently when you use Drain Soul on a mob that grants XP all you receive is the random soul shard for hitting the mob. When you kill the mob you do not get a soul shard for its death.

Expected behaviour:

You should get a soul shard when a mob dies that grants XP.

Steps to reproduce the problem:

1. Play using a warlock with the drain soul spell.
2. Use drain soul on mob when their health is low so that you can kill him while the effect is still going.
3. Mob dies but you didn't get a soul shard.

Branch(es):

3.3.5

TC rev. hash/commit:

5cdb53896f89+

Operating system:

Windows Server 2012

[Pharonax]

Confirmed on rev c48a596c66c075c876c860a7d5c036d033de0b9d

[WOW-Reborn]

I fixed the bug on my server but I don't know how "trinity core" likes you to post the fixes.
SpellId SchoolMask SpellFamilyName SpellFamilyMask0 SpellFamilyMask1 SpellFamilyMask2 ProcFlags SpellTypeMask SpellPhaseMask HitMask AttributesMask ProcsPerMinute Chance Cooldown Charges
-1120 0 5 1 0 0 65536 1 2 0 1 0 0 0 0

I'm sure something in this isn't right as I am learning myself LOL but it does produce random shards and produce a shard on a mobs death.

[WOW-Reborn]

I just figure I would at least make some kind of post to guide people in the right direction instead of fixing it myself and not helping the community. Oh and btw the table that needs the change is located in World then Spell_Proc. If someone with more knowledge could look at this and make sure I done it correctly I would appreciate it. It appears to work but I want to make sure its right.

[PolarCookie]

DELETE FROM `spell_proc` WHERE `SpellId` = -1120;
INSERT INTO `spell_proc` (`SpellId`, `SchoolMask`, `SpellFamilyName`, `SpellFamilyMask0`, `SpellFamilyMask1`, `SpellFamilyMask2`, `ProcFlags`, `SpellTypeMask`, `SpellPhaseMask`, `HitMask`, `AttributesMask`, `ProcsPerMinute`, `Chance`, `Cooldown`, `Charges`) VALUES
(-1120, 0, 5, 1, 0, 0, 65536, 1, 2, 0, 1, 0, 0, 0, 0);

Worked on 5cdb53896f89

Macro to delete all shards in bag, for testing convenience.

#show Soul Shard
/run for i=0,4 do for j=1,GetContainerNumSlots(i) do local iID=GetContainerItemID(i,j) if iID~=nil then local name=GetItemInfo(iID) if name =="Soul Shard" then PickupContainerItem(i,j) DeleteCursorItem() end end end end

[illfated]

@WOW-Reborn : as you can see, PolarCookie has posted using the SQL format in TrinityCore.
If you want to post similar suggestions in the future, you can use ```sql on a line above the SQL code
and finish the code block with 3 back ticks (with no text after) if you need to write normal text below the block.

This is known as GitHub flavored MarkDown:

• https://help.github.com/articles/creating-and-highlighting-code-blocks/#syntax-highlighting

[WOW-Reborn]

Ok thanks :) I'm still learning the proper way to do things. Glad to see at least we got some sort of solution to this issue.

[hauspie]

Bug and fix confirmed for me in revision 5cdb53896f89

WOW-Reborn If you want, i can try to make a proper pull request on your behalf soon so that your fix can be merged.

[Aokromes]

sql only fixes don't belongs to pull requests.

[Keader]

About (https://github.com/TrinityCore/TrinityCore/issues/22223#issuecomment-410547766)
Why delete+insert, if entry already exist?
Why not just update?

[illfated]

Good point. I am used to only updating existing data, unless we talk about long, complex SAI scripts or many lines of messy data.
TrinityCore rev. 5cdb53896f89 2018-08-02 19:09:40 +0200 (3.3.5 branch) (Win64, RelWithDebInfo, Static)

SELECT * FROM `spell_proc` WHERE `SpellId` = -1120;

```
SpellId SchoolMask SpellFamilyName SpellFamilyMask0 SpellFamilyMask1 SpellFamilyMask2 ProcFlags SpellTypeMask SpellPhaseMask HitMask AttributesMask ProcsPerMinute Chance Cooldown Charges

---

-1120 0 0 0 0 0 0 0 0 0 1 0 0 0 0

So a more practical `UPDATE` query could look something like this (your codestyle may vary):
```sql
UPDATE `spell_proc`
SET `SpellFamilyName`=5, `SpellFamilyMask0`=1, `ProcFlags`=65536, `SpellTypeMask`=1, `SpellPhaseMask`=2
WHERE `SpellId`= -1120;

[Keader]

Proc flags change? feels like hack :)

[WOW-Reborn]

Well Keader if you know a better solution i'm all ears. Like I said I don't know much about Trinitycore so i'm learning this as I go along lol

[Keader]

@ariel- Is back already?

In Unit::Kill() has:

if (!victim->IsCritter())
        Unit::ProcSkillsAndAuras(attacker, victim, PROC_FLAG_KILL, PROC_FLAG_KILLED, PROC_SPELL_TYPE_MASK_ALL, PROC_SPELL_PHASE_NONE, PROC_HIT_NONE, nullptr, nullptr, nullptr);

It make spell proc, but... This proc make charges of Drain soul become 0 ( Aura::PrepareProcToTrigger), so inside of Aura::TriggerProcOnEvent() we have:

    // Remove aura if we've used last charge to proc
    if (IsUsingCharges() && !GetCharges())
        Remove();

This remove cause REMOVE_BY_DEFAULT.
It all happens before creature change state to death, so when creature use RemoveAllAurasOnDeath(), that should change remove mode to REMOVE_BY_DEATH, drain soul is already removed.
So in the end, Drain Soul is removed using REMOVE_BY_DEFAULT instead REMOVE_BY_DEATH, it make skip SPELL_AURA_CHANNEL_DEATH_ITEM handle.
Checking current rev and old revs i cant find nothing that changed it.

Right now, we can "fix it" using it:

diff --git a/src/server/game/Spells/SpellMgr.cpp b/src/server/game/Spells/SpellMgr.cpp
index ec8d06fa61..0d6d041a1f 100644
--- a/src/server/game/Spells/SpellMgr.cpp
+++ b/src/server/game/Spells/SpellMgr.cpp
@@ -4797,6 +4797,11 @@ void SpellMgr::LoadSpellInfoCorrections()
         spellInfo->Effects[EFFECT_1].RadiusEntry = sSpellRadiusStore.LookupEntry(EFFECT_RADIUS_3_YARDS); // 3yd
     });

+    ApplySpellFix({ 1120, 8288, 8289, 11675, 27217, 47855 }, [](SpellInfo* spellInfo)
+    {
+        spellInfo->ProcCharges = 0;
+    });
+
     for (uint32 i = 0; i < GetSpellInfoStoreSize(); ++i)
     {
         SpellInfo* spellInfo = mSpellInfoMap[i];

But is still hack, because this bug can affect any spell that has PROC_FLAG_KILL and have charges.
So we need Handle with proc first, and remove charges after.
Any idea?

[wow-enthusiast]

Somewhere along the line, drain soul got completely broken - no one can get soul shards, no matter how long they drain soul.

[Jildor]

this still bug? I can't reproduce

[Rushor]

@Jildor @Killyana

https://i.imgur.com/vuyvwJQ.jpg

is fixed, can be closed

[illfated]

Nice. Glad to see that.