Spell Fire Geysers

[Lightstalker]

Fire Geysers v1.5

FIRE GEYSERS​

My first spell...
Go nuts on the critique but also tell me what to change & improve this is my first vJASS spell...

vJASS, MUI, Leakless, & Lagless

''Technical' description'': creates X amount of fire special effects at random points around the caster, dealing Y damage to all units within range of any of those effects. Every Z seconds for A times, random fire effects will be created at random points around the caster, damaging all units within range.
X, Y, Z, A are all configurable variables.

Screenshots:

Spoiler

Could probably look a lot better with more fire effects, but now you can configure that, too.

Code:

scope FireGeysers initializer Init

globals
    private constant integer SPELLID = 'A000' //ABILITY RAWCODE
    private constant string EFFECT = "Abilities\\Spells\\Other\\Doom\\DoomDeath.mdl" //FIRE EFFECT

    private constant integer EFFECTAMOUNT = 3 //HOW MANY FIRE GEYSERS ARE CREATED DURING A PERIOD
    private constant integer AMOUNT = 3 //HOW MANY TIMES THE EFFECT OCCURS IN TOTAL. TO FIND OUT HOW LONG THE SPELL WILL LAST IN SECONDS: AMOUNT * PERIOD
    
    private constant real PERIOD = 2.0 //HOW OFTEN THE EFFECT OCCURS
    private constant real DAMAGE = 50.0 //DAMAGE DONE BY EACH FIRE GEYSER PER LEVEL
    private constant real RNDMIN = 300.0 //MINIMUM DISTANCE THE EFFECT WILL OCCUR FROM THE CASTER
    private constant real RNDMAX = 600.0 //MINIMUM DISTANCE THE EFFECT WILL OCCUR FROM THE CASTER
    
    private constant attacktype ATTACKTYPE = ATTACK_TYPE_MAGIC //THE TYPE OF ATTACK (MAGIC, HERO, CHAOS, ETC.)
    private constant damagetype DAMAGETYPE = DAMAGE_TYPE_FIRE //THE TYPE OF DAMAGE.
    private constant weapontype WEAPONTYPE = WEAPON_TYPE_WHOKNOWS //FOR DIFFERENT TYPES OF WEAPONS. DON'T RECOMMAND YOU CHANGE IT.
    
    //DON'T CHANGE ANYTHING AFTER THIS POINT UNLESS YOU KNOW WHAT YOU'RE DOING!
    private group TEMPGROUP = CreateGroup()
endglobals

private function Conditions takes nothing returns boolean
    return GetSpellAbilityId() == SPELLID
endfunction

private struct data
    unit u = null
    real ux = 0.0
    real uy = 0.0
    integer lvl = 0
    integer count = 0

    static method create takes unit u returns data
        local data d = data.allocate()
        set d.u = u
        set d.ux = GetUnitX(u)
        set d.uy = GetUnitY(u)
        set d.lvl = GetUnitAbilityLevel(u, SPELLID)
        return d
    endmethod
endstruct

globals
    private data TEMPSTRUCT
endglobals

private function Damage takes nothing returns boolean
    call UnitDamageTarget(TEMPSTRUCT.u, GetFilterUnit(), DAMAGE * TEMPSTRUCT.lvl, false, false, ATTACKTYPE, DAMAGETYPE, WEAPONTYPE)
    return false
endfunction

private function Periodic takes nothing returns nothing
    local data d = data(GetTimerData(GetExpiredTimer()))
    local real rndx = 0.0
    local real rndy = 0.0
    local real angle = 0.0
    local integer i = 0

    if d.count < AMOUNT then
        loop
            set rndx = d.ux + GetRandomReal(RNDMIN, RNDMAX) * Cos(GetRandomReal(0.00,57.29))
            set rndy = d.uy + GetRandomReal(RNDMIN, RNDMAX) * Sin(GetRandomReal(0.00,57.29))
            call DestroyEffect(AddSpecialEffect(EFFECT, rndx, rndy))
            call GroupEnumUnitsInRange(TEMPGROUP, rndx, rndy, 200.0, Condition(function Damage))
        set i = i + 1
        exitwhen i == EFFECTAMOUNT
        endloop
        set d.count = d.count + 1
    else
        call ReleaseTimer(GetExpiredTimer())
        call d.destroy()
    endif
endfunction

private function Actions takes nothing returns nothing
    local timer t = NewTimer()
    call SetTimerData(t, data.create(GetTriggerUnit()))
    call TimerStart(t, PERIOD, true, function Periodic)
endfunction

private function Init takes nothing returns nothing
    local trigger t = CreateTrigger()
    call TriggerRegisterAnyUnitEventBJ(t, EVENT_PLAYER_UNIT_SPELL_EFFECT)
    call TriggerAddCondition(t, Condition(function Conditions))
    call TriggerAddAction(t, function Actions)
endfunction
endscope

Changelog:

Spoiler

v1.5
- Fixed a bug where incorrect units would be damaged.
- Further randomized effects.
v1.4
- Added weapontype, damagetype, and attacktype configurables.
- Made code easier to read.
- Optimized code.
v1.3
- Inlined damage.
v1.2
- Further optimized code.
v1.1
- Optimized code.
- Damage, amount of fire effects, amount of times the spell will run for can now all be easily modified.

 

[Romek]

You don't need to null 'trigger t' nor 'timer t'.

    local data d = data.create(GetTriggerUnit())
    local timer t = NewTimer()
    call SetTimerData(t, d)

Could be:

    local timer t = NewTimer()
    call SetTimerData(t, data.create(GetTriggerUnit()))

On a somewhat related note, you should just inline that create method.

In 'Damage', just use tempstruct directly.

Globals could do with some nice comments.

'Evaluation' should be constant, and at the top of the script.

 

[Viikuna]

call DestroyTimer(GetExpiredTimer())

You need to release your timer, thats the whole point of timer recycling.

Otherwise this looks pretty good. Its a nice start.

edit. Also, you dont need to null neither that trigger t in init function than that timer t in Actions function.

Trigger is never destroyed, so its handle index wont be recycled anyways, which means that theres no need to null it. Its the same thing with those timers when you recycle them properly.

 

[GetTriggerUnit-]

, Gj, please, a map and a pic could help. :shades:

 

[Igor_Z]

Upload a demo map plz... To see how it looks like

 

[Lightstalker]

@Romek

>>>You don't need to null 'trigger t' nor 'timer t'.
So I heard... Any reason why AceHart is doing it here? http://www.thehelper.net/forums/showthread.php?t=105146
(Just curious. )

>>>

    local data d = data.create(GetTriggerUnit())
    local timer t = NewTimer()
    call SetTimerData(t, d)

Could be:

    local timer t = NewTimer()
    call SetTimerData(t, data.create(GetTriggerUnit()))

Cool & fixed.

>>>On a somewhat related note, you should just inline that create method.
What's inlining mean? Using "Tab" to indent it?

>>>In 'Damage', just use tempstruct directly.
Not 100% sure what you mean.

>>>Globals could do with some nice comments.
Done.

>>>'Evaluation' should be constant, and at the top of the script.
How can I make a function constant? Assign it to a variable?
Also, I just thought of something. Instead of doing this:

private function Evaluation takes integer lvl returns real
    return 50.0 * lvl
endfunction

private function Damage takes nothing returns boolean
    local data d = tempstruct
    call UnitDamageTarget(d.u, GetFilterUnit(), Evaluation(d.lvl), false, false, ATTACK_TYPE_MAGIC, DAMAGE_TYPE_FIRE, WEAPON_TYPE_WHOKNOWS)
    return false
endfunction

Couldn't I just do this? (It compiles fine.)

private function Damage takes nothing returns boolean
    local data d = tempstruct
    call UnitDamageTarget(d.u, GetFilterUnit(), 50 * d.lvl, false, false, ATTACK_TYPE_MAGIC, DAMAGE_TYPE_FIRE, WEAPON_TYPE_WHOKNOWS)
    return false
endfunction

@Viikuna

>>>You need to release your timer, thats the whole point of timer recycling.

I thought DestroyTimer was the same as ReleaseTimer

 

[GetTriggerUnit-]

Stil not testmap. :thdown:

 

[Lightstalker]

Stil not testmap. :thdown:

Sorry was working on it. I was trying to find a template so I wouldn't have to make my own.

Anyway, the test map is up.

I will post screenshot as soon as I can fix the problem except I have no idea how to. :banghead:
Basically, when you test the map, you'll notice the the fire effects are always grouped close together at same location. They're suppose to randomly spawn from the ground every 2 seconds.

 

[Kenny]

So I heard... Any reason why AceHart is doing it here? http://www.thehelper.net/forums/showthread.php?t=105146
(Just curious. )

It is good coding practice to null local handles. Lots of people do it. Except in the trigger and timer circumstances, there is no need, as explained by Viikuna.

>>>In 'Damage', just use tempstruct directly.
Not 100% sure what you mean.

He means use the tempstruct directly instead of allocating it to a local variable.

Example:

tempstruct.u instead of d.u.

>>>'Evaluation' should be constant, and at the top of the script.
How can I make a function constant? Assign it to a variable?

Like this:

private constant function Evaluation takes integer lvl returns real
    return 50.00 * lvl
endfunction

Notice the constant keyword BEFORE then function keyword.

I thought DestroyTimer was the same as ReleaseTimer

No. DestroyTimer() destroys the timer while ReleaseTimer() frees the timer for later use.

Now the only problem with it is that the special effects are always created around the same location during a cast. If someone can tell me how to make it so that special effects are created at different locations during a single cast, that would be great.

You need to offset the coordinates using angles, for example:

call DestroyEffect(AddSpecialEffect(EFFECT, d.ux + rndx * Cos(GetRandomReal(0.00,57.29)), d.uy + rndy * Sin(GetRandomReal(0.00,57.29))))
// Notice the Cos() and Sin() parts in the above line, they -should- offset the coordinates randomly.
// Do that for the damage group thing as well

 

[Tom Jones]

Now the only problem with it is that the special effects are always created around the same location during a cast. If someone can tell me how to make it so that special effects are created at different locations during a single cast, that would be great.

You probably have fixed seeds turned on, to turn it off:
Main editor window -> File -> Preferences -> Test Map -> Untick "Use Fixed Random Seed".

 

[Weep]

Basically, when you test the map, you'll notice the the fire effects are always grouped close together at same location.

That's because your X/Y coordinates are being generated not as polar coordinates, but a random value between the minimum and maximum "distances"

private constant real RNDMIN = 300.0 //MINIMUM DISTANCE THE EFFECT WILL OCCUR FROM THE CASTER
private constant real RNDMAX = 600.0 //MAXIMUM .........
...
set rndx = GetRandomReal(RNDMIN, RNDMAX)
set rndy = GetRandomReal(RNDMIN, RNDMAX)

so they'll always be within a square located up and right of the caster, with lower left corner <300, 300> from the caster, and upper right corner <600, 600> from the caster.

 

[Lightstalker]

Fixed everything said by everyone.

Still, one more concern of mine: in the trigger, I am doing this:

private function Evaluation takes integer lvl returns real
    return 50.0 * lvl
endfunction

private function Damage takes nothing returns boolean
    call UnitDamageTarget(TEMPSTRUCT.u, GetFilterUnit(), Evaluation(TEMPSTRUCT.lvl), false, false, ATTACK_TYPE_MAGIC, DAMAGE_TYPE_FIRE, WEAPON_TYPE_WHOKNOWS)
    return false
endfunction

Couldn't I just do this instead?

private function Damage takes nothing returns boolean
    call UnitDamageTarget(TEMPSTRUCT.u, GetFilterUnit(), 50 * TEMPSTRUCT.lvl, false, false, ATTACK_TYPE_MAGIC, DAMAGE_TYPE_FIRE, WEAPON_TYPE_WHOKNOWS)
    return false
endfunction

And what is inlining???

 

[T.s.e]

That would make it harder for someone who uses the spell to modify the damage. What if he wanted to use a different value than 50?

 

[Lightstalker]

That would make it harder for someone who uses the spell to modify the damage. What if he wanted to use a different value than 50?

Well no... I meant for ME. I use the spell too, you know. And I have no need of all these comments and configurables. Anyway, I can just set "50" to a global variable (which I have done in v1.1 ).

Anyway, would it be better for me to do it that way:

private function Damage takes nothing returns boolean
    call UnitDamageTarget(TEMPSTRUCT.u, GetFilterUnit(), DAMAGE * TEMPSTRUCT.lvl, false, false, ATTACK_TYPE_MAGIC, DAMAGE_TYPE_FIRE, WEAPON_TYPE_WHOKNOWS)
    return false
endfunction

 

[Viikuna]

private function Evaluation takes integer lvl returns real
    return 50.0 * lvl
endfunction

You should use this instead, like you were using before.

Inlining means that for example this:

private function Evaluation takes integer lvl returns real
    return 50.0 * lvl
endfunction

private function Damage takes nothing returns boolean
    call UnitDamageTarget(TEMPSTRUCT.u, GetFilterUnit(), Evaluation(TEMPSTRUCT.lvl), false, false, ATTACK_TYPE_MAGIC, DAMAGE_TYPE_FIRE, WEAPON_TYPE_WHOKNOWS)
    return false
endfunction

is shortened to this:

private function Damage takes nothing returns boolean
    call UnitDamageTarget(TEMPSTRUCT.u, GetFilterUnit(), 50 * TEMPSTRUCT.lvl, false, false, ATTACK_TYPE_MAGIC, DAMAGE_TYPE_FIRE, WEAPON_TYPE_WHOKNOWS)
    return false
endfunction

Luckily JassHelper does some inlining for us, so removing that function Evaluation wont make your code any faster ( It should get inlined by JassHelper ). And even without JassHelper, its still worth of one function call to have more readable code.

You can find more about Jasshelper features, such as inlining from JassHelper manual.

 

[Kenny]

static method create takes unit u returns data
        local data d = data.allocate()
        set d.u = GetTriggerUnit()
        set d.ux = GetUnitX(u)
        set d.uy = GetUnitY(u)
        set d.lvl = GetUnitAbilityLevel(u, SPELLID)
        set d.p = GetOwningPlayer(u)
        return d
    endmethod

set d.u = GetTriggerUnit()

Should be:

set d.u = u

call UnitDamageTarget(TEMPSTRUCT.u, GetFilterUnit(), Evaluation(TEMPSTRUCT.lvl), false, false, ATTACK_TYPE_MAGIC, DAMAGE_TYPE_FIRE, WEAPON_TYPE_WHOKNOWS)

Attacktype, damagetype and weapontype should all be configurable options.

set angle = GetRandomReal(0.0, 359.9999)
            call DestroyEffect(AddSpecialEffect(EFFECT, d.ux + rndx * Cos(angle * bj_DEGTORAD), d.uy + rndy * Sin(angle * bj_DEGTORAD)))

If you want it too be truely random do something like this:

call DestroyEffect(AddSpecialEffect(EFFECT, d.ux + rndx * Cos(GetRandomReal(0.00,57.29)), d.uy + rndy * Sin(GetRandomReal(0.00,57.29))))

That gets rid of the need for the local variable for the angle as well.

Also you need to do the same for the GroupEnum function, so I suggest you do something like this:

set rndx = d.ux + GetRandomReal(RNDMIN, RNDMAX) * Cos(GetRandomReal(0.00,57.29))
set rndy = d.uy + GetRandomReal(RNDMIN, RNDMAX) * Sin(GetRandomReal(0.00,57.29))
call DestroyEffect(AddSpecialEffect(EFFECT,rndx,rndy)
call GroupEnumUnitsInRange(TEMPGROUP,rndx,rndy,200.0,Condition(function Damage))

That should work.

Also, this spell would be much better if it was written to last X amount of seconds, instead of AMOUNT * PERIOD seconds.

You should also add a configurable fuction (like you Evaluation function) for the amount of effects created per interval, and the duration.

 

[Lightstalker]

static method create takes unit u returns data
        local data d = data.allocate()
        set d.u = GetTriggerUnit()
        set d.ux = GetUnitX(u)
        set d.uy = GetUnitY(u)
        set d.lvl = GetUnitAbilityLevel(u, SPELLID)
        set d.p = GetOwningPlayer(u)
        return d
    endmethod

set d.u = GetTriggerUnit()

Should be:

set d.u = u

call UnitDamageTarget(TEMPSTRUCT.u, GetFilterUnit(), Evaluation(TEMPSTRUCT.lvl), false, false, ATTACK_TYPE_MAGIC, DAMAGE_TYPE_FIRE, WEAPON_TYPE_WHOKNOWS)

Will WC3 automatically assume 'u' is the triggering unit?

>>>Attacktype, damagetype and weapontype should all be configurable options.
They are now.

set angle = GetRandomReal(0.0, 359.9999)
            call DestroyEffect(AddSpecialEffect(EFFECT, d.ux + rndx * Cos(angle * bj_DEGTORAD), d.uy + rndy * Sin(angle * bj_DEGTORAD)))

If you want it too be truely random do something like this:

call DestroyEffect(AddSpecialEffect(EFFECT, d.ux + rndx * Cos(GetRandomReal(0.00,57.29)), d.uy + rndy * Sin(GetRandomReal(0.00,57.29))))

Also you need to do the same for the GroupEnum function, so I suggest you do something like this:

set rndx = d.ux + GetRandomReal(RNDMIN, RNDMAX) * Cos(GetRandomReal(0.00,57.29))
set rndy = d.uy + GetRandomReal(RNDMIN, RNDMAX) * Sin(GetRandomReal(0.00,57.29))
call DestroyEffect(AddSpecialEffect(EFFECT,rndx,rndy)
call GroupEnumUnitsInRange(TEMPGROUP,rndx,rndy,200.0,Condition(function Damage))

Done!

>>>Also, this spell would be much better if it was written to last X amount of seconds, instead of AMOUNT * PERIOD seconds.
Don't think it would make much of a difference. But anyway, I'm not rewriting it just for that. It's the same spell that is in one of the maps I'm working on right now so I didn't make it for no reason.

>>>You should also add a configurable fuction (like you Evaluation function) for the amount of effects created per interval, and the duration.
Was already done since v1.2.
If you want to create more/less effects, change the value of "EFFECTAMOUNT". The duration is controlled by "PERIOD".

Anyway, thanks a lot. I gave you +REP