Issue Information
-
#005303
-
2 - Fair
-
Fixed
Issue Confirmations
-
Yes (0)No (0)
0
WZ_WATERBALL does not cancel SC_MAGICPOWER when target dies
Posted by Hercules Bot on 07 February 2012 - 08:56 PM
Originally posted by Wildcard
Synopsis
The wizard skill WZ_WATERBALL, when used in conjuncture with SC_MAGICPOWER (better known as Amplify Magic), will not consume the status effect if it kills the target before the last hit, and can be cast additional times with the benefits of SC_MAGICPOWER until the status timer expires.
Reconstruction
The breakage results from a now false assumption in skill_timerskill. The code assumes that the killing blow to the target will take place within the call to skill_attack. However, skill_attack will in turn evoke battle_delay_damage on any non-aoe skill, thus resulting in no actual damage at the time. The code will thus continue by queueing the next timerskill call. Between the two calls of skill_timerskill, battle_delay_damage_sub will deal the killing blow to the target. The subsequent waterball timer will thus look up a target ID that has already been killed, and exit at the target == NULL check before ever ending the status as originally intended.
History
The issue affects both eAthena and rAthena.
A fix for the exact same issue was introduced by Skotlex in r5601. It is crucial to understand that back then magic damage was *not delayed by amotion*, but instead dealt instantly.
In r6346, Skotlex introducing amotion delay for skills, thus breaking his own assumption about waterball damage. This persists up until today.
Proposed Solution
The original fix remains valid for the scenario of waterball not killing the target. It needs to be replicated, however, should the target indeed die of it. The function most suitable for this cleanup appears to be skill_counter_additional_effect. I have provided a patch with my suggested solution as an attachment.
Additional Concerns
With the recent introduction of magic reflect, and associated problems with the waterball skill, it stands to reason that special scenarios involving reflect might exist wherein the status would also not be canceled. Since the scenario covered in a recent revision however specifically addresses the damage source (in this case the caster) dying, however, I suspect that status cleanup on death should already handle this particular happenstance. I did not, however, investigate the issue of reflection at all.
Another interesting observation is that the reflection issue only ever presented itself due to the fact that reflect damage is applied directly via battle_calc_attack rather than queueing it via amotion, which is how the crash fixed in r15526 even got past the initial check against a dead source at the very top of skill_timerskill. Heh.
I hope I could be of help! Regards,
Wildcard
Synopsis
The wizard skill WZ_WATERBALL, when used in conjuncture with SC_MAGICPOWER (better known as Amplify Magic), will not consume the status effect if it kills the target before the last hit, and can be cast additional times with the benefits of SC_MAGICPOWER until the status timer expires.
Reconstruction
The breakage results from a now false assumption in skill_timerskill. The code assumes that the killing blow to the target will take place within the call to skill_attack. However, skill_attack will in turn evoke battle_delay_damage on any non-aoe skill, thus resulting in no actual damage at the time. The code will thus continue by queueing the next timerskill call. Between the two calls of skill_timerskill, battle_delay_damage_sub will deal the killing blow to the target. The subsequent waterball timer will thus look up a target ID that has already been killed, and exit at the target == NULL check before ever ending the status as originally intended.
History
The issue affects both eAthena and rAthena.
A fix for the exact same issue was introduced by Skotlex in r5601. It is crucial to understand that back then magic damage was *not delayed by amotion*, but instead dealt instantly.
in skill_attack(...) if (attack_type&BF_WEAPON) battle_delay_damage(tick+dmg.amotion,bl,src,0,0,0,rdamage,ATK_DEF,0); else battle_damage(bl,src,rdamage,0);
In r6346, Skotlex introducing amotion delay for skills, thus breaking his own assumption about waterball damage. This persists up until today.
if (dmg.amotion) battle_delay_damage(tick, dmg.amotion,src,bl,dmg.flag,skillid,skilllv,damage,dmg.dmg_lv,dmg.dmotion);
Proposed Solution
The original fix remains valid for the scenario of waterball not killing the target. It needs to be replicated, however, should the target indeed die of it. The function most suitable for this cleanup appears to be skill_counter_additional_effect. I have provided a patch with my suggested solution as an attachment.
Additional Concerns
With the recent introduction of magic reflect, and associated problems with the waterball skill, it stands to reason that special scenarios involving reflect might exist wherein the status would also not be canceled. Since the scenario covered in a recent revision however specifically addresses the damage source (in this case the caster) dying, however, I suspect that status cleanup on death should already handle this particular happenstance. I did not, however, investigate the issue of reflection at all.
Another interesting observation is that the reflection issue only ever presented itself due to the fact that reflect damage is applied directly via battle_calc_attack rather than queueing it via amotion, which is how the crash fixed in r15526 even got past the initial check against a dead source at the very top of skill_timerskill. Heh.
I hope I could be of help! Regards,
Wildcard
Originally posted by Xantara
I like how organized and detailed this report is xD
Dev note: More info (and somewhat a duplicate of) [bug=1249]
I like how organized and detailed this report is xD
Dev note: More info (and somewhat a duplicate of) [bug=1249]
Originally posted by Wildcard
Apologies for the duplicate, I completely missed that bug when searching for related issues. My bad!
Apologies for the duplicate, I completely missed that bug when searching for related issues. My bad!
Originally posted by Ind
I've made the other the duplicate being yours is much more detailed (thank you).
I've made the other the duplicate being yours is much more detailed (thank you).
Originally posted by Ind
fixed in [rev=15548]. thank you for your fix
fixed in [rev=15548]. thank you for your fix
moved issue from Skills