Spell Session 4 - Boil

[Naakaloh]

Boil

Causes water in the area of effect to boil violently, dealing damage to units in the boiling area and causing scalding water to splash into the air. Every time a splash lands it deals damage in an area and if it lands it water it will bounce into the air again.

Modifications:
I've added a function to check for whether or not the area surrounding a point is land or not to try to minimize the amount by which the land moves for the terrain deformations. It seems rather clumsy to me, but I guess it will do.


Naakaloh Spell Session 04 - Boil 1.35.w3x (Updated Version 4)

[Blade.dk]

It is a really nice spell, but there's some things that could be better:

There's no reason or need to use == true on all your booleans in your if statements, == true is a worthless waste of space. Also, you can use "not <boolean>" instead of "<boolean> == false".

Why do you not set the timer variables to null? If you had any problems with it, then I understand why, but it is rare bug, so unless you had problems you should not really care.

JASS:

        set tempGroup = GetUnitsInRangeOfLocMatching( BoilRadius(), inLoc, Condition( function BoilingWaterDamageFilter ))

Blizzard.j group enumeration functions has internal not-set-to-null leaks, better do things directly yourself.

Using ForGroupBJ is kind of worthless when you do not use the bj_destroyGroup boolean or whatever it is called. I suggest you to remove the 'BJ'.

Originally Posted by JESP Standard

- Every single function or handle variables (attached variables) of a public object,
Begins with the spell's codename plus "_" . (This means that replacing the codename with
another value would create a non conflicting new instance of the spell).

Your spell does not follow the JESP Standard, as the functions are not named after this rule.

JASS:

    call TriggerAddCondition( trig, Condition( function BoilAbilityCondition ))
    call TriggerAddAction( trig, function BoilCancelChannel )

This has multiple leaks.

I would have removed debugging messages when the spell was finished, instead of just commenting them out. Not like this matters much, though.

It would be nice if the spell gave you an error and stopped when you try to cast it in an invalid area, where there is no water, instead of making you lose mana and have to wait for the cooldown.

The spell looks great, but if possible, then it would look even better if there was an option to make the color of the water effects fit the color of the water in the map (it looks a bit weird when totally blue water comes up of a green lake).

Please fix the leaks and the error with it not following the JESP Standard even though it claims to do so, and it can be approved. I suggest you to also notice the other things I wrote and care about them :).

[Naakaloh]

Originally Posted by Blade.dk

JASS:

    call TriggerAddCondition( trig, Condition( function BoilAbilityCondition ))
    call TriggerAddAction( trig, function BoilCancelChannel )

This has multiple leaks.

I'm really not sure how I should deal with them. Will TriggerClearActions work for the action?

[Blade.dk]

You will have to save the triggeraction object (it is returned by the TriggerAddAction native), and later destroy it with TriggerRemoveAction.

Same with the triggercondition, just other function names. And the boolexpr must be removed too.

[Naakaloh]

First post updated.

There's no reason or need to use == true on all your booleans in your if statements, == true is a worthless waste of space. Also, you can use "not <boolean>" instead of "<boolean> == false".

Heh... for some reason I was under the impression that WE would give a compile error for that notation. Thanks.

Why do you not set the timer variables to null? If you had any problems with it, then I understand why, but it is rare bug, so unless you had problems you should not really care.

Fixed. I not exactly sure how the bug occurs, so I chose to avoid any situation that might cause it.

Blizzard.j group enumeration functions has internal not-set-to-null leaks, better do things directly yourself.

Using ForGroupBJ is kind of worthless when you do not use the bj_destroyGroup boolean or whatever it is called. I suggest you to remove the 'BJ'.

Fixed. Didn't even realize it was a Blizzard.j function. Didn't notice the BJ.
Edit: Check my on the enumeration function fix; I'm not entirely clear what to clean up.

Your spell does not follow the JESP Standard, as the functions are not named after this rule.

I believe it does comply now. I couldn't find any other names to modify, but it might be good to have another pair of eyes go over it.

JASS:

    call TriggerAddCondition( trig, Condition( function BoilAbilityCondition ))
    call TriggerAddAction( trig, function BoilCancelChannel )

This has multiple leaks.

Please check my fix, I am quite unsure about this one.

I would have removed debugging messages when the spell was finished, instead of just commenting them out. Not like this matters much, though.

As you wish. :)

It would be nice if the spell gave you an error and stopped when you try to cast it in an invalid area, where there is no water, instead of making you lose mana and have to wait for the cooldown.

Done.

The spell looks great, but if possible, then it would look even better if there was an option to make the color of the water effects fit the color of the water in the map (it looks a bit weird when totally blue water comes up of a green lake).

If you have any suggestions on detecting the color of the texture used for the water, I'll be happy to try to implement something that tints the missiles. I had thought about checking the cliff terrain type, but I'm not exactly sure how to do that or if it will work. I'm also not sure if the missiles will tint correctly. But really, it's not so bad, and if you think about it, the water's being boiled, so maybe you could say it's being distilled? (Poor excuse, I know.)

[blu_da_noob]

JASS:

    set x = 0
    set y = 0
    set radiusStep = 0
    set A = 0
    set B = 0
    set stop = 0

endfunction

Integers and reals don't need to be set to 0 at the end of a function. You do it (wastefully) in many places.

JASS:

    call TriggerRegisterAnyUnitEventBJ( gg_trg_Boil, EVENT_PLAYER_UNIT_SPELL_CAST )

You register the Unit Spell Cast event, which can be abused. Use EVENT_PLAYER_UNIT_SPELL_EFFECT instead.

JASS:

        call IssueImmediateOrder( GetTriggerUnit(), "stop" )

Just issueing a stop order isn't fast enough to stop a unit from starting to cast an ability, you need to pause the unit, issue the stop order, and then unpause it.

JASS:

    if( GetSpellAbilityId() == 'A002' ) then

You hardcoded that in instead of using the constant function.

JASS:

        exitwhen A >= R2I(Boil_Radius() / radiusStep ) + 1

You should store the value of Boil_Radius() to prevent having to call the function many times. You should also add an 'or WaterInArea' to that exitwhen, so that if water is found early you don't search the rest of the area anyway.

JASS:

    set trigCondition = TriggerAddCondition( trig, bx )
    set trigAction = TriggerAddAction( trig, function Boil_CancelChannel )
    call SetHandleHandle( trig, "Boil_CancelChannelTriggerAction", trigAction )
    call SetHandleHandle( trig, "Boil_CancelChannelTriggerCondition", trigCondition )

You don't need the variables, as those could be inlined as:

JASS:

        call SetHandleHandle( trig, "Boil_CancelChannelTriggerAction", TriggerAddAction( trig, function Boil_CancelChannel ) )
    call SetHandleHandle( trig, "Boil_CancelChannelTriggerCondition", TriggerAddCondition( trig, bx ) )

Using a 0.01 repeating timer for the actions is overkill. Better change to about 0.04, still smooth enough, but much less strain.

JASS:

    local boolexpr bx = Condition( function Boil_ChannelStopAbilityCondition )

You declare and create this bxpr, and later destroy it, but never use it. Should be removed.

JASS:

call GroupAddUnit( GetHandleGroup( t, "Boil_Missiles" ), tempUnit )

You should store the group in the timer callback function instead of retrieving it from game cache everytime.

JASS:

    call SetHandleReal( tempUnit, "Boil_xVel", inX * Cos(Deg2Rad(GetUnitFacing(tempUnit))))

A Deg2Rad function call isn't needed. Just multiply angle by bj_DEGTORAD.

JASS:

        call GroupEnumUnitsInRange( tempGroup, inX, inY, Boil_Radius(), Condition( function Boil_BoilingWaterDamageFilter ))

Leaks a boolexpr (the filter).

JASS:

        set A = CountUnitsInGroup(tempGroup)

        loop
            exitwhen (A <= 0)
            call Boil_WaterDamageTarget( source, FirstOfGroup(tempGroup) )
            call GroupRemoveUnit( tempGroup, FirstOfGroup(tempGroup) )
            set A = A - 1
        endloop

Inefficient method of looping. Store FirstOfGroup(group) in a variable, do actions and remove it from the group, exitwhen variable == null.

JASS:

(( CountUnitsInGroup(GetHandleGroup( t, "Boil_Missiles")) > 0)) )

Can be replaced with:

JASS:

(FirstOfGroup(GetHandleGroup( t, "Boil_Missiles")) != null)

JASS:

        call TimerStart( t, Period(), false, function Boil_Action)

Instead of using a repeating timer, you use a single expiration timer and restart it each time. Why?

JASS:

    call FlushHandleLocals( tempUnit )
    call DestroyEffect( GetHandleEffect( tempUnit , "Boil_ProjectileEffect" ))

You are flushing the local vars on a unit, then trying to get an effect that was attached to it. That GetHandleEffect will be return null, meaning it is leaking the effect.

JASS:

    call TriggerRemoveAction( trig, GetHandleTriggerAction( trig, "Boil_CancelChannelTriggerAction" ))

Removing the action from a trigger stops the execution of that trigger immediately, meaning that anything below that (including the removing of condition, destroying of trigger and nulling of variables) doesn't happen, making it leak.

JASS:

    local real magnitude = (Boil_Radius() * GetRandomReal( 0.05, 1.00 ))

Getting the magnitude like that doesn't have an equal distribution of points in the circle. Instead it should be:

JASS:

local real magnitude = SquareRoot(GetRandomReal(0,1))*Boil_Radius()

Throughout the projectile move function, you use Period(). Store the value in a variable to prevent extra function calls.


I've probably missed a few things, but I'll have to go over it again after you fix stuff up anyway.

[Naakaloh]

First post updated.

Thanks... I guess it's better to be criticized for being lazy than to never have anyone make you change something. :/

JASS:

    set x = 0
    set y = 0
    set radiusStep = 0
    set A = 0
    set B = 0
    set stop = 0

endfunction

Integers and reals don't need to be set to 0 at the end of a function. You do it (wastefully) in many places.

Ok, just being excessively cautions

JASS:

    call TriggerRegisterAnyUnitEventBJ( gg_trg_Boil, EVENT_PLAYER_UNIT_SPELL_CAST )

You register the Unit Spell Cast event, which can be abused. Use EVENT_PLAYER_UNIT_SPELL_EFFECT instead.

I thought I did use EVENT_PLAYER_UNIT_SPELL_EFFECT... maybe I changed the wrong copy of the map... Fixed.

JASS:

        call IssueImmediateOrder( GetTriggerUnit(), "stop" )

Just issueing a stop order isn't fast enough to stop a unit from starting to cast an ability, you need to pause the unit, issue the stop order, and then unpause it.

Ok, I noticed that without pausing the was the spell was cancelled, but the animation still played. Fixed.

JASS:

    if( GetSpellAbilityId() == 'A002' ) then

You hardcoded that in instead of using the constant function.

I have absolutely no idea why I added that... Fixed.

JASS:

        exitwhen A >= R2I(Boil_Radius() / radiusStep ) + 1

You should store the value of Boil_Radius() to prevent having to call the function many times. You should also add an 'or WaterInArea' to that exitwhen, so that if water is found early you don't search the rest of the area anyway.

Fixed.

JASS:

    set trigCondition = TriggerAddCondition( trig, bx )
    set trigAction = TriggerAddAction( trig, function Boil_CancelChannel )
    call SetHandleHandle( trig, "Boil_CancelChannelTriggerAction", trigAction )
    call SetHandleHandle( trig, "Boil_CancelChannelTriggerCondition", trigCondition )

You don't need the variables, as those could be inlined as:

JASS:

        call SetHandleHandle( trig, "Boil_CancelChannelTriggerAction", TriggerAddAction( trig, function Boil_CancelChannel ) )
    call SetHandleHandle( trig, "Boil_CancelChannelTriggerCondition", TriggerAddCondition( trig, bx ) )

I realized that after I did it and lazily decided not to fix it. Fixed now.

Using a 0.01 repeating timer for the actions is overkill. Better change to about 0.04, still smooth enough, but much less strain.

JASS:

        call TimerStart( t, Period(), false, function Boil_Action)

Instead of using a repeating timer, you use a single expiration timer and restart it each time. Why?

Only the first starting of the timer is 0.01, the rest use 0.04. I'm not sure why I restarted it, I think I was planning on having the period variable at first and decided do to it differently but left it there.

JASS:

    local boolexpr bx = Condition( function Boil_ChannelStopAbilityCondition )

You declare and create this bxpr, and later destroy it, but never use it. Should be removed.

Unless I'm mistaken, it is used.

JASS:

call GroupAddUnit( GetHandleGroup( t, "Boil_Missiles" ), tempUnit )

You should store the group in the timer callback function instead of retrieving it from game cache everytime.

Fixed.

JASS:

    call SetHandleReal( tempUnit, "Boil_xVel", inX * Cos(Deg2Rad(GetUnitFacing(tempUnit))))

A Deg2Rad function call isn't needed. Just multiply angle by bj_DEGTORAD.

Fixed.

JASS:

        call GroupEnumUnitsInRange( tempGroup, inX, inY, Boil_Radius(), Condition( function Boil_BoilingWaterDamageFilter ))

Leaks a boolexpr (the filter).

Ah, didn't realize it, thanks. Fixed.

JASS:

        set A = CountUnitsInGroup(tempGroup)

        loop
            exitwhen (A <= 0)
            call Boil_WaterDamageTarget( source, FirstOfGroup(tempGroup) )
            call GroupRemoveUnit( tempGroup, FirstOfGroup(tempGroup) )
            set A = A - 1
        endloop

Inefficient method of looping. Store FirstOfGroup(group) in a variable, do actions and remove it from the group, exitwhen variable == null.

Clever, I hadn't actually thought of that. Thanks.

JASS:

(( CountUnitsInGroup(GetHandleGroup( t, "Boil_Missiles")) > 0)) )

Can be replaced with:

JASS:

(FirstOfGroup(GetHandleGroup( t, "Boil_Missiles")) != null)

Actually, I had tried to do it but it doesn't seem to work properly. Probably something that I do with the group, but I can't find it.

JASS:

    call FlushHandleLocals( tempUnit )
    call DestroyEffect( GetHandleEffect( tempUnit , "Boil_ProjectileEffect" ))

You are flushing the local vars on a unit, then trying to get an effect that was attached to it. That GetHandleEffect will be return null, meaning it is leaking the effect.

Fixed. I had forgotten about that function, it's actually a backup in the event something causes the missiles to stop prematurely.

JASS:

    call TriggerRemoveAction( trig, GetHandleTriggerAction( trig, "Boil_CancelChannelTriggerAction" ))

Removing the action from a trigger stops the execution of that trigger immediately, meaning that anything below that (including the removing of condition, destroying of trigger and nulling of variables) doesn't happen, making it leak.

Ok, I'm going to leave the triggeractions alone for now, since I can't figure out how to get rid of them. This is just one handle every time the spell is cast, so at the moment I'm not worried about it.

JASS:

    local real magnitude = (Boil_Radius() * GetRandomReal( 0.05, 1.00 ))

Getting the magnitude like that doesn't have an equal distribution of points in the circle. Instead it should be:

JASS:

local real magnitude = SquareRoot(GetRandomReal(0,1))*Boil_Radius()

The intention was to force the distribution to be a little more away from the center. I'm not sure if 0.05 is a large enough factor, but it seems fine to me.

Throughout the projectile move function, you use Period(). Store the value in a variable to prevent extra function calls.

Fixed.

I've probably missed a few things, but I'll have to go over it again after you fix stuff up anyway.

Thanks. Sorry for being lazy. My instructors tend to say similar things about my laziness, though, I could be worse. :)

[Naakaloh]

First post updated

Realized that I didn't need to create the trigger to stop the spell if channelling was cancelled each time the spell was cast. Should have fixed a leak.

[blu_da_noob]

You currently use two generic globals (for passing values to filter functions). While this is technically allowed (as they are generic), it makes importing much easier if you use BJ defined globals temporarily (for example, you could use bj_groupRandomConsidered for your integer and bj_groupRandomCurrentPick for your unit, as they are only use temporarily by their respective BJ functions).

I will look through the spell again some time later today.

Edit: In general, it looks much better.

JASS:

(( CountUnitsInGroup(GetHandleGroup( t, "Boil_Missiles")) > 0)))

You get the group again, even though it is stored in a variable.

JASS:

    set inX = GetRandomReal( Boil_SplashMinVelocity(), Boil_SplashMaxVelocity() )

You change the value of inX here, but then use it later as if it were still the X value of the unit. I don't think that is intentional.


You never destroy (stop) any of your terrain deformations.

Many of your spell functions are in the custom script section instead of in the trigger (most notably, Period). Very generic things like handle variables should preferably not be in the trigger, but other fairly trigger specific functions should be inside. Please also remove things you never use (like the modified TimedLightning function). Those aren't needed and would be useless if the spell user were to copy them as well.

[Naakaloh]

First post updated

Thanks again, you seem to have an incredible eye for catching these things. (Or maybe you've been doing it too long :P...)

Fixed up the things you mentioned, but I was wondering if it has been confirmed that it is necessary to stop terraindeformations if they are temporary.

Edit: Added some stuff I had forgotten.

[blu_da_noob]

Heh, it takes time more than anything else :P.

http://www.wc3jass.com/viewtopic.php?t=2590

That is a pretty inconclusive thread with tests about terrain deformations having to be stopped, but as far as I know, they do need to be (or they will leak). They are handles anyway (extend handle class), and I think just about all dynamically created handles leak if not removed.

[Naakaloh]

Fixed a mistake that was preventing splashes from bouncing.

[blu_da_noob]

This is being approved, but please do stop the terrain deformations to be sure that it does not leak.

[Naakaloh]

Oh, I had already done that when you mentioned it, I think I may have misled you by asking if it was necessary.

[blu_da_noob]

You did kinda, but I should've checked really :P