Fix rogue talent Improved Sprint

[cala]

Fix rogue sprint no longer working when talent Improved Sprint was active

This commit 482ca12 is one of those pending backports from old mangos-zero concatenated in: https://github.com/cmangos/issues/wiki/classic_backporting-todo-zero-commits

I tested it and this is my report:

Improved sprint is indeed broken as it prevent sprint from working. This commit fixes it.

[Schmoozerd]

Not entirely sure here - with wotlk at least there are many spells with this spell-icon.
So this might require some more generic look at, and also probably implementation with wotlk/tbc/ and cata

[ghost]

this is correct for classic, I fixed it years ago using the same method

[cala]

I see mangoszero had another approach: mangoszero/server@1781ac3 without using spell-icon. I have not tested it though.

[Schmoozerd]

Ok, the second commit is a horrible, ugly and stupid hack

[cala]

Ok, the second commit is a horrible, ugly and stupid hack

That confirms what I was thinking: I am as good as developing to core than turning lead into gold. :P

[cyberium]

.cast 11305 on classic debug log result with improved sprint talent

WORLD: got cast spell packet, spellId - 11305, data length = 6
Sending SMSG_SPELL_START id=11305
Sending SMSG_SPELL_GO id=11305
Spell 11305 Effect0 : 6 Targets: Player Teab (Guid: 2), -, -
Spell: Aura is: 31
Aura: construct Spellid : 11305, Aura : 31 Target : 1 Damage : 70
Holder of spell 11305 now is in use
Spell 30918 Effect0 : 6 Targets: Player Teab (Guid: 2), -, -
Spell: Aura is: 77
Aura: construct Spellid : 30918, Aura : 77 Target : 1 Damage : 0
Spell 30918 Effect1 : 6 Targets: Player Teab (Guid: 2), -, -
Spell: Aura is: 77
Aura: construct Spellid : 30918, Aura : 77 Target : 1 Damage : 0
Aura 31 now is remove mode 0
Holder of spell 30918 now is in use
WORLD: Received CMSG_FORCE_RUN_SPEED_CHANGE_ACK (227, 0xE3) opcode
WORLD: Received CMSG_FORCE_RUN_SPEED_CHANGE_ACK (227, 0xE3) opcode
Aura 77 now is remove mode 7

.cast 11305 on classic debug log result without improved sprint talent

WORLD: got cast spell packet, spellId - 11305, data length = 6
Sending SMSG_SPELL_START id=11305
Sending SMSG_SPELL_GO id=11305
Spell 11305 Effect0 : 6 Targets: Player Teab (Guid: 2), -, -
Spell: Aura is: 31
Aura: construct Spellid : 11305, Aura : 31 Target : 1 Damage : 70
Holder of spell 11305 now is in use
WORLD: Received CMSG_FORCE_RUN_SPEED_CHANGE_ACK (227, 0xE3) opcode
Aura 31 now is remove mode 7
WORLD: Received CMSG_FORCE_RUN_SPEED_CHANGE_ACK (227, 0xE3) opcode

.cast 30918 on classic debug log

Sending SMSG_SPELL_START id=30918
Sending SMSG_SPELL_GO id=30918
Spell 30918 Effect0 : 6 Targets: Player Teab (Guid: 2), -, -
Spell: Aura is: 77
Aura: construct Spellid : 30918, Aura : 77 Target : 1 Damage : 0
Spell 30918 Effect1 : 6 Targets: Player Teab (Guid: 2), -, -
Spell: Aura is: 77
Aura: construct Spellid : 30918, Aura : 77 Target : 1 Damage : 0
Holder of spell 30918 now is in use
Aura 77 now is remove mode 7
Aura 77 now is remove mode 7

ON 3.3.5
.cast 11305 in debug log

WORLD: got cast spell packet, spellId - 11305, cast_count: 4, unk_flags 0, data length = 10
Sending SMSG_SPELL_START id=11305
Sending SMSG_SPELL_GO id=11305
Spell 11305 Effect0 : 6 Targets: Player Tec (Guid: 2), -, -
Spell: Aura is: 31
Aura: construct Spellid : 11305, Aura : 31 Target : 1 Damage : 70
Holder of spell 11305 now is in use
Spell 30918 Effect0 : 77 Targets: Player Tec (Guid: 2), -, -
Spell ScriptStart spellid 30918 in EffectScriptEffect
Spell 30918 Effect2 : 3 Targets: Player Tec (Guid: 2), -, -
WORLD: Received CMSG_FORCE_RUN_SPEED_CHANGE_ACK (227, 0xE3) opcode
Aura 31 now is remove mode 7
WORLD: Received CMSG_FORCE_RUN_SPEED_CHANGE_ACK (227, 0xE3) opcode

.cast 30198

Sending SMSG_SPELL_START id=30918
Sending SMSG_SPELL_GO id=30918
Spell 30918 Effect0 : 77 Targets: Player Tec (Guid: 2), -, -
Spell ScriptStart spellid 30918 in EffectScriptEffect
Spell 30918 Effect2 : 3 Targets: Player Tec (Guid: 2), -, -

Some conclusions:
We clearly see aura 31 forced to be removed in classic ("Aura 31 now is remove mode 0" line)?
We see 30918 applied twice?
30918 effect0 and effect1 are used in classic where in wotlk it use effect0 and effect2 ?

I tempted to follow the code but has always about aura its hard. Any help is welcome :)
ps: without any hack!

[PatSmuk]

There's this bit in IsNoStackSpellDueToSpell:

// Allow stack passive and not passive spells
if (spellInfo_1->HasAttribute(SPELL_ATTR_PASSIVE) != spellInfo_2->HasAttribute(SPELL_ATTR_PASSIVE))
    return false;

Improved Sprint is passive and Sprint is not, so I don't know how they could interfere with each other.

[cyberium]

@Schmoozerd or @Patman64 what do you think about that -> cyberium@480178d

[wowpsp]

I have a same pull request to submit, and make sure this patch works very well

[wowpsp]

if ((spellInfo_1->SpellIconID == 516) && (spellInfo_2->SpellIconID == 516))

Is it better? Thank you..

[wowpsp]

I invited lot of players to test this patch, and got the same feedback, this patch works very well, @Schmoozerd , could you please merge this pull request?

[Schmoozerd]

@wowpsp Your patch will

1. solve the problem
2. has no sideeffect, as it is explicit.
   (Actually might have some sideeffects on other client-versions, needs to be checked)

However it might not be generic enough and hence not address similar issues for other spell-stacking problems.

On the other hand the approach @cyberium suggests

1. solves the problem as well (i suppose :P )
2. has other sideeffects, and MIGHT have negative sideeffects

Some more thoughts on Cyberium's approach:
Possible problems with then stacking allowed for many increase_speed auras -- which usually must not stack.
So i would assume negative side effects.

In conclusion i think that using the direct approach is a reasonable choice, as this is player spell, and hence is relevant only within the scope of the class.
Though it would be required that somebody confirms that the stacking rule "Can Stack" is valid for all spells of SPELLFAMILY_ROGUE, icon-id 516 of all clients..

[cyberium]

@Schmoozerd yes i confirm my approach have WANTED side effect,
I tried to have more generic approach possible. Then resolve bad side effects.

@wowpsp, your approach will resolve this issue in this client version.
But iam sure there is in this case a better way to resolve this. We simply didnt found it yet.
I think you could understaind why we try to resolve this more generic way, it will resolve other similar case and be probably compatible with other core without adding new check.

conclusion i think also we could add @wowpsp patch till another more generic solution found (who know :) )

[wowpsp]

Thank you @Schmoozerd , thank you @cyberium ...
I'm also looking for a more generic approach ... If I found it, I'll submit a pull request as soon as possible...

[Kelzior]

not working for me

[Argonn]

Isn't it possible to match the 2 types of Aura? (109 & 31) or just based on the Id's?
Something like:

if ((spellInfo_1->Id == 2983 || spellInfo_1->Id == 8696 || spellInfo_1->Id == 11305 ) && spellInfo_2->Id == 13743 || spellInfo_2->Id == 13875)

Or even better (and ofc the check where spell1 and spell2 swap places)

if ((spellId_1 == 2983 || spellId_1 == 8696 || spellId_1 == 11305 ) && spellId_2 == 13743 || spellId_2 == 13875)