WZ_WATERBALL does not cancel SC_MAGICPOWER when target dies

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 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