Trinitycore: [Spell] Item combat spells are being cast twice

[joshwhedon]

Description:

It seems that item combat spells (poison, enchants, etc) are being cast twice per spell.

https://github.com/TrinityCore/TrinityCore/blob/3.3.5/src/server/game/Spells/Spell.cpp#L2488
https://github.com/TrinityCore/TrinityCore/blob/3.3.5/src/server/game/Spells/Spell.cpp#L2524

I think L2488 should be removed because all we should do is prepare the procs, not trigger them here.

Current behaviour:

If you use a rogue, you will notice that the poisons are applied more often than they should.

Expected behaviour:

Each spell attack should proc the weapon enchants only once.

Steps to reproduce the problem:

Take a rogue and hit stuff while poisons are applied

Branch(es):

CHANGEME 3.3.5, master or both

TC rev. hash/commit:

https://github.com/TrinityCore/TrinityCore/commit/267e707fe3404788df8316166bd31ab139909fd1

Operating system: Doesn't matter

[Jildor]

lul yes, seems the same twice, I'm with you, this sould be removed: https://github.com/TrinityCore/TrinityCore/blob/3.3.5/src/server/game/Spells/Spell.cpp#L2486-L2488

[jackpoz]

it seems it was added in https://github.com/TrinityCore/TrinityCore/commit/080d2c6cd439acb2059adc4e24a279de98aa0db6#diff-952c711803610aa78110e4f92ff2516aR2510-R2515 , check if there are cases where that makes sense to exist

[joshwhedon]

I had a look, and tested a bit. Both checks seem to be the same, just with different orders. It really feels like leaving this one was just an oversight when it was reworked. But I can't say with 100% certainty.

[jackpoz]

then please open a PR removing the check that doesn't make sense to you, we can see what cases it breaks :)

[ccrs]

just trying to ping @ariel- for insight

[joshwhedon]

Yeah some insight on the subject would be nice. I am not confortable making a PR with only half baked knowledge of the subject. However I will try removing it on my side and give an update of the situation in the next few days.

[jackpoz]

@joshwhedon open the PR as draft so others can comment on it (code changes in issues are missed, lost and forgotten).

The Draft part shows it's not ready to be merged anyway and requires more research.

[joshwhedon]

I did that pull request : https://github.com/TrinityCore/TrinityCore/pull/24627
Not sure if it is the right format. I don't like doing PR. Too much responsibilities.

[Jildor]

Wait, this should be closed no? XD