[vJASS] Do I need all of the code?

[TheGoldenGoblin]

Hi! I used BPower's http://www.hiveworkshop.com/forums/spells-569/simple-spell-collection-1-v1-0-0-0-a-245192/ as help to do my first vjass spell.
I made a simple Mind Crush spell. So here is what it does: It deals damage, reduces armor and attack damage.

I feel like there is alot of unnecessary code and checking since I will be using all of the systems like RegisterPlayerUnitEvent and DummyCaster, but I'm not sure what to delete..

Mind Crush Code:

scope MindCrush


    globals
        
        private constant integer    AID          = 'A00A'
        
        private constant attacktype ATTACK_TYPE  = ATTACK_TYPE_NORMAL      
        private constant damagetype DAMAGE_TYPE  = DAMAGE_TYPE_MAGIC
        
        private constant integer    BUFF_CAST_ID = 'A00C'
        private constant integer    ORDER_ID     = 852189
        private constant integer    BUFF_CAST_ID2 = 'A00B'
        private constant integer    ORDER_ID2     = 852149

        
    endglobals

    native UnitAlive takes unit id returns boolean
    
    
    private constant function GetDamage takes integer shadowpower, integer spellpower returns real
        return 100. + (1*shadowpower) + (0.5*spellpower)
    endfunction



    private module init 
        private static method onInit takes nothing returns nothing
            static if LIBRARY_SpellEffectEvent then
                call RegisterSpellEffectEvent(AID, function thistype.run)
            else
                local trigger t = CreateTrigger()
                call TriggerRegisterAnyUnitEventBJ(t, EVENT_PLAYER_UNIT_SPELL_EFFECT)
                call TriggerAddCondition(t, Condition(function thistype.check))
                set t = null
            endif
            
            static if not LIBRARY_DummyCaster then
                static if LIBRARY_UnitIndexer then
                    set UnitIndexer.enabled = false
                    set c = CreateUnit(NEUTRAL,DUMMY_CASTER,0,0,0)
                    set UnitIndexer.enabled = true
                else
                    set c = CreateUnit(NEUTRAL,DUMMY_CASTER,0,0,0)
                endif
                call SetUnitPosition(c,2147483647,2147483647)
                call UnitAddAbility(c, BUFF_CAST_ID)
            endif
        endmethod
    endmodule

    
    
    
    
    private struct Spell extends array
        implement optional Alloc
        
        unit caster
        unit aim
        real dmg
        
        static if not LIBRARY_DummyCaster then
            static unit c
        endif
        
        static if (not thistype.allocate.exists) then
            static integer array rn
            static integer ic = 0
        endif
            
        private static method applyDamage takes nothing returns nothing
            local timer t = GetExpiredTimer()
            local thistype this = GetTimerData(t)
            
            if (UnitAlive(.aim)) then
                call UnitDamageTarget(.caster, .aim, .dmg, false, false, ATTACK_TYPE, DAMAGE_TYPE, null)
            endif
            


            static if (thistype.deallocate.exists) then
                call this.deallocate()
            else
                set rn[this] = rn[0]
                set rn[0] = this
            endif
            
            set .caster = null
            set .aim    = null
        endmethod

        
        
        private static method run takes nothing returns boolean
            local integer shadowpower
            local integer spellpower
            
            static if (thistype.allocate.exists) then
                local thistype this = allocate()
            else
                local thistype this = rn[0]
                if this == 0 then
                    set ic = ic + 1
                    set this = ic
                else
                    set rn[0] = rn[this]
                endif
            endif
            set .caster = GetTriggerUnit()
            set .aim    = GetSpellTargetUnit()
            set spellpower = 10
            set shadowpower = 10
            set .dmg    = GetDamage(shadowpower, spellpower)
            static if DummyCaster.castTarget.exists then
                call DummyCaster[BUFF_CAST_ID].castTarget(GetTriggerPlayer(), 1, ORDER_ID, .aim)
                call DummyCaster[BUFF_CAST_ID2].castTarget(GetTriggerPlayer(), 1, ORDER_ID2, .aim)
            else
                call SetUnitOwner(c, GetTriggerPlayer(), false)
                call IssueTargetOrderById(c, ORDER_ID, .aim)
                call SetUnitOwner(c, NEUTRAL, false)
            endif
            call TimerStart(NewTimerEx(this), 0.00, false, function thistype.applyDamage) 
            return false
        endmethod
    
        static if not LIBRARY_SpellEffectEvent then
            private static method check takes nothing returns boolean
                if (GetSpellAbilityId() == AID) then
                    call thistype.run()
                endif
                return false
            endmethod
        endif
    
        implement init
        
    endstruct
endscope

 

[PurgeandFire]

You just need to know what static if does. static if is read when the vJASS is compiled, not during execution. It'll check if libraries exist or not. Therefore, something like this:

static if LIBRARY_SpellEffectEvent then
    // Code A
else
    // Code B
endif

It will lookup and see if there is a library named "SpellEffectEvent". If there is, then it will compile "Code A" (Code B will be commented out). If it cannot find that library, then it will compile "Code B" (Code A will be commented out).

As such, if you know you will include all of the libraries (Alloc, SpellEffectEvent, DummyCaster, UnitIndexer, etc.) then you just need to remove the parts that compile when those libraries don't exist. Try it yourself, but you should probably end up with something like this:

scope MindCrush


    globals
        
        private constant integer    AID          = 'A00A'
        
        private constant attacktype ATTACK_TYPE  = ATTACK_TYPE_NORMAL      
        private constant damagetype DAMAGE_TYPE  = DAMAGE_TYPE_MAGIC
        
        private constant integer    BUFF_CAST_ID = 'A00C'
        private constant integer    ORDER_ID     = 852189
        private constant integer    BUFF_CAST_ID2 = 'A00B'
        private constant integer    ORDER_ID2     = 852149

        
    endglobals

    native UnitAlive takes unit id returns boolean
    
    
    private constant function GetDamage takes integer shadowpower, integer spellpower returns real
        return 100. + (1*shadowpower) + (0.5*spellpower)
    endfunction



    private module init 
        private static method onInit takes nothing returns nothing
            call RegisterSpellEffectEvent(AID, function thistype.run)
        endmethod
    endmodule

    
    private struct Spell extends array
        implement Alloc
        
        unit caster
        unit aim
        real dmg
            
        private static method applyDamage takes nothing returns nothing
            local timer t = GetExpiredTimer()
            local thistype this = GetTimerData(t)
            
            if (UnitAlive(.aim)) then
                call UnitDamageTarget(.caster, .aim, .dmg, false, false, ATTACK_TYPE, DAMAGE_TYPE, null)
            endif
            
            call this.deallocate()
            
            set .caster = null
            set .aim    = null
        endmethod

        
        
        private static method run takes nothing returns boolean
            local integer shadowpower
            local integer spellpower
            local thistype this = allocate()

            set .caster = GetTriggerUnit()
            set .aim    = GetSpellTargetUnit()
            set spellpower = 10
            set shadowpower = 10
            set .dmg    = GetDamage(shadowpower, spellpower)

            call DummyCaster[BUFF_CAST_ID].castTarget(GetTriggerPlayer(), 1, ORDER_ID, .aim)
            call DummyCaster[BUFF_CAST_ID2].castTarget(GetTriggerPlayer(), 1, ORDER_ID2, .aim)
            
            call TimerStart(NewTimerEx(this), 0.00, false, function thistype.applyDamage) 
            return false
        endmethod
    
        implement init
        
    endstruct
endscope

 

[BPower]

I see. The use of modules, static ifs, ... are not very tutorial friendly
I've completly cut them out and added a lot of comments. Feel free to ask for help or any questions.

//Spells should most likely be scopes. Google "JASS HELPER MANUAL" for very detailed information. 
scope MindCrush


    globals
        private constant integer    AID          = 'A00A'//AID as possible shortcut for ability id.
       
        private constant attacktype ATTACK_TYPE  = ATTACK_TYPE_NORMAL      
        private constant damagetype DAMAGE_TYPE  = DAMAGE_TYPE_MAGIC
       
        private constant integer    BUFF_CAST_ID = 'A00C'
        private constant integer    ORDER_ID     = 852189
        private constant integer    BUFF_CAST_ID2 = 'A00B'
        private constant integer    ORDER_ID2     = 852149   
        
        private unit DUMMY_CASTER
    endglobals

    //native UnitAlive has proven to be very effective compared to others is unit death checks.
    native UnitAlive takes unit id returns boolean
   
    private constant function GetDamage takes integer shadowpower, integer spellpower returns real
        return 100. + (1*shadowpower) + (0.5*spellpower)
    endfunction

    // Go on
    private struct Spell extends array
        //Those are your struct members
        unit caster
        unit aim
        real dmg
       
        
        static integer array rn
        static integer ic = 0
           
        private static method applyDamage takes nothing returns nothing
            local timer t = GetExpiredTimer()
            local thistype this = GetTimerData(t)
           
            if (UnitAlive(.aim)) then
                call UnitDamageTarget(.caster, .aim, .dmg, false, false, ATTACK_TYPE, DAMAGE_TYPE, null)
            endif
            
            //recycle the allocation, so it can be used again
            set rn[this] = rn[0]
            set rn[0] = this
            //caster instead of .caster works aswell.
            set caster = null
            set aim    = null
        endmethod

       
        //trigger conditions always return boolean.
        private static method run takes nothing returns boolean
           local thistype this//thistype equals your struct name, always use thistype within the struct. 
           //local Spell this   thistype equals Spell, as the struct name is Spell. (The private keyword adds a few extra chars)
           
           
           //Check if the casted spell is our spell.
           //Not required with SpellEffectEvent.
           if (GetSpellAbilityId() == AID) then
                //--------------------------------------------
                //Make your spell MUI. This is called allocation.
                set this = rn[0]
                if this == 0 then
                    set ic = ic + 1
                    set this = ic
                else
                    set rn[0] = rn[this]
                endif
                //--------------------------------------------
                //Now for better understanding 
                //  You can also write this.caster or just caster instead of .caster. 
                //  However you need either this. or . if you also have a local in this function labeled with caster.
                set .caster = GetTriggerUnit()
                set .aim    = GetSpellTargetUnit()
                set spellpower = 10
                set shadowpower = 10
                set .dmg    = GetDamage(shadowpower, spellpower)
                
                //Dummy caster block
                call SetUnitOwner(DUMMY_CASTER, GetTriggerPlayer(), false)
                call IssueTargetOrderById(DUMMY_CASTER, ORDER_ID, .aim)
                call SetUnitOwner(DUMMY_CASTER, NEUTRAL, false)
                
                //this is actually an integer value for exmaple 4
                //  --> this.caster equals caster[4]
                //TimerUtils API. You start a timer and link it with an integer.
                //  --> NewTimerEx(4)
                //  --> this = GetTimerData(timer) --> this = 4 --> this.caster equals caster[4]
                call TimerStart(NewTimerEx(this), 0.00, false, function thistype.applyDamage)
            endif
            return false
        endmethod
   
        private static method onInit takes nothing returns nothing
            //This is your trigger, which fires on spell effect. 
            local trigger t = CreateTrigger()
            call TriggerRegisterAnyUnitEventBJ(t, EVENT_PLAYER_UNIT_SPELL_EFFECT)
            call TriggerAddCondition(t, Condition(function thistype.run))
            //set t = null -->  Can be nulled, but you don't have to.
        
            //Now you need a "dummy caster". In my spell I used a library DummyCaster
            //  - pro: Imagine you have another spell, which requires a dummy caster
            //         You can now use this caster created in DummyCaster aswell. --> Just one unit.
            // 
            //In order to present a nice example we create our very own dummy caster for this spell.
            //  --> Add a private unit variable to the global block
            set DUMMY_CASTER = CreateUnit(Player(15), 0,0,0)
            call UnitAddAbility(DUMMY_CASTER, BUFF_CAST_ID)
        
            //---------------------------- DONE -------------------------------------------------
        
            //optional features found in simple spells
            //  1. SpellEffectEvent: This library makes spell casting very effective, if you use many custom spells
            //      - why? Because it only fires the one trigger connected to the ability id, while the common way
            //             fires all triggers --> call TriggerRegisterAnyUnitEventBJ(t, EVENT_PLAYER_UNIT_SPELL_EFFECT)
            //             those are stopped at the first condition if AbilityId == AID then.
            //  2. DummyCaster: This library provides one dummy caster unit which can be used in all other resources of your map.
            //  3. UnitIndexer.enabled = .... forget about this one for now. If you have UnitIndexer you can disable the indexing 
            //     for units which do not have to be indexed
            //      - what is an Unit Indexer: UnitIndexer assgins every unit entering the map with an unique number i.e. 3
            //        you now have a reference to each unit for exmaple GetUnitById(3) --> is your indexed unit 3.
            //  4. call SetUnitPosition(DUMMY_CASTER, 2147483647, 2147483647) moves the dummy unit to the edge of the map. 
        
        endmethod
       
    endstruct
endscope

 

[TheGoldenGoblin]

Thank you alot, I haven't been coding spells in vjass before, I have only done it in gui and this was very helpful
So if I understood this correctly shouldn't I change this:

private static method onInit takes nothing returns nothing
            //This is your trigger, which fires on spell effect. 
            local trigger t = CreateTrigger()
            call TriggerRegisterAnyUnitEventBJ(t, EVENT_PLAYER_UNIT_SPELL_EFFECT)
            call TriggerAddCondition(t, Condition(function thistype.run))

to this?:

private static method onInit takes nothing returns nothing
            call RegisterSpellEffectEvent(AID, function thistype.run)

 

[BPower]

You can use SpellEffectEvent if you want. It's basically just a matter of efficiency.

What's the difference:
- The library SpellEffectEvent uses either Table or a hashtable ( depends if you have Table)
- Now the child key is the ability raw, the trigger handle is on the which handle field.
- On spell effect the correct trigger is loaded and fired (only this trigger).
- call TriggerEvaluate(LoadTriggerHandle(.ht, 0, GetSpellAbilityId()))

- Creating local triggers per spell inside the spell library/scope (upper example)
- On spell effect each of these triggers are fired
- The first condition is GetSpellAbility() == AID
- All triggers where this condition is false are now stopped.

Assume you have 20 custom spells and you use the upper way.
All 20 triggers are fired on spell effect and 19 of these don't pass the first condition
while SpellEffectEvent will only fire the one correct trigger.

Edit: Yes use it, it's freakin' awesome.

 

[TheGoldenGoblin]

I want to use it since efficiency matters alot especially in an rpg map
btw why does it say NEUTRAL doesn't exist while it works with every other trigger? :O

 

[BPower]

Because there is no such variable declared in that scope and no public global variable labeled NEUTRAL either I guess.

 

[TheGoldenGoblin]

I made an Arcane Explosion spell, what it does is that it deals damage to every unit within 500 of casting point. But is this code fine or is there better ways of making it?

Spell

scope ArcaneExplosion
        
        
        globals
        
        private constant integer AID                = 'A011'
        
        private constant attacktype ATTACK_TYPE        = ATTACK_TYPE_NORMAL
        private constant damagetype DAMAGE_TYPE        = DAMAGE_TYPE_MAGIC
        
        private constant real AOE                      = 500
        
        private constant integer EXPLODE                = 'h004'
        private constant integer EXPLODE2               = 'h005'
        endglobals


        private function TargetFilter takes unit target, player owner returns boolean
           return UnitAlive(target) and IsUnitEnemy(target, owner) and not IsUnitType(target, UNIT_TYPE_STRUCTURE)
        endfunction
            
        
        
        
    private struct Spell extends array
           unit caster
           unit aim
           real dmg
           real x
           real y
           player owner
           unit dummy
           unit dummy2
           
           static integer array rn
           static integer ic = 0
           
           
           
           
           
        private static method applyDamage takes nothing returns nothing
            local timer t = GetExpiredTimer()
            local thistype this = GetTimerData(t)
            local unit u
            local group g = CreateGroup()
            
            call KillUnit(.dummy)
            call KillUnit(.dummy2)
            call GroupEnumUnitsInRange( g, .x, .y, AOE, null )
             loop
                set u = FirstOfGroup(g)
                exitwhen u == null
                call GroupRemoveUnit(g, u)
                if (TargetFilter(u, .owner)) then
                    call UnitDamageTarget(.caster, u, .dmg, false, false, ATTACK_TYPE, DAMAGE_TYPE, null)
                endif
             endloop
           
            
            //recycle the allocation, so it can be used again
            set rn[this] = rn[0]
            set rn[0] = this
            //caster instead of .caster works aswell.
            set caster = null
            set aim    = null
         
        endmethod
        
        
        private static method run takes nothing returns boolean
        local thistype this
        
        
        set this = rn[0]
        if this == 0 then
            set ic = ic + 1
            set this = ic
        else
            set rn[0] = rn[this]
        endif
        
        
        set .owner  = GetTriggerPlayer()
        set .caster = GetTriggerUnit()
        set .dmg    = 100
        set .x      = GetLocationX(GetSpellTargetLoc())
        set .y      = GetLocationY(GetSpellTargetLoc())
        set .dummy  = CreateUnit(.owner, EXPLODE, .x, .y, 360 )
        set .dummy2  = CreateUnit(.owner, EXPLODE2, .x, .y, 360 )
        
        call TimerStart(NewTimerEx(this), 0.00, false, function thistype.applyDamage)
        return false
        
        endmethod


        private static method onInit takes nothing returns nothing
            call RegisterSpellEffectEvent(AID, function thistype.run)
        endmethod

     endstruct
endscop

 

[deathismyfriend]

Use

GetSpellTargetX
GetSpellTargetY
// not 
GetSpellTargetLoc // Also every time you call this it creates a location which needs to be cleaned.

you have to null timers and groups.

I don't see you doing anything with dummy or dummy2 so it is better to get rid of them.

 

[TheGoldenGoblin]

I use dummy and dummy 2 when I create the effect.

set .dummy  = CreateUnit(.owner, EXPLODE, .x, .y, 360 )
call KillUnit(.dummy)
set .dummy2 = CreateUnit(.owner, EXPLODE2, .x, .y, 360 )
call KillUnit(.dummy2)
set .dummy  = null
set .dummy2 = null

I nulled the group, the timer and units, is it better now?

Update

scope ArcaneExplosion
        
        
        globals
        
        private constant integer AID                = 'A011'
        
        private constant attacktype ATTACK_TYPE        = ATTACK_TYPE_NORMAL
        private constant damagetype DAMAGE_TYPE        = DAMAGE_TYPE_MAGIC
        
        private constant real AOE                      = 500
        
        private constant integer EXPLODE                = 'h004'
        private constant integer EXPLODE2               = 'h005'
        endglobals


        private function TargetFilter takes unit target, player owner returns boolean
           return UnitAlive(target) and IsUnitEnemy(target, owner) and not IsUnitType(target, UNIT_TYPE_STRUCTURE)
        endfunction
            
        
        
        
    private struct Spell extends array
           unit caster
           unit aim
           real dmg
           real x
           real y
           player owner
           unit dummy
           unit dummy2
           
           static integer array rn
           static integer ic = 0
           
           
           
           
           
        private static method applyDamage takes nothing returns nothing
            local timer t = GetExpiredTimer()
            local thistype this = GetTimerData(t)
            local unit u
            local group g = CreateGroup()
            
            call GroupEnumUnitsInRange( g, .x, .y, AOE, null )
             loop
                set u = FirstOfGroup(g)
                exitwhen u == null
                call GroupRemoveUnit(g, u)
                if (TargetFilter(u, .owner)) then
                    call UnitDamageTarget(.caster, u, .dmg, false, false, ATTACK_TYPE, DAMAGE_TYPE, null)
                endif
             endloop
           
            
            //recycle the allocation, so it can be used again
            set rn[this] = rn[0]
            set rn[0] = this
            //caster instead of .caster works aswell.
            set caster = null
            set aim    = null
            set g      = null
            set u      = null
            set t      = null
        endmethod
        
        
        private static method run takes nothing returns boolean
        local thistype this
        
        
        set this = rn[0]
        if this == 0 then
            set ic = ic + 1
            set this = ic
        else
            set rn[0] = rn[this]
        endif
        
        
        set .owner  = GetTriggerPlayer()
        set .caster = GetTriggerUnit()
        set .dmg    = 100
        set .x      = GetSpellTargetX()
        set .y      = GetSpellTargetY()
        set .dummy  = CreateUnit(.owner, EXPLODE, .x, .y, 360 )
        call KillUnit(.dummy)
        set .dummy2 = CreateUnit(.owner, EXPLODE2, .x, .y, 360 )
        call KillUnit(.dummy2)
        
        set .dummy  = null
        set .dummy2 = null
        
        call TimerStart(NewTimerEx(this), 0.00, false, function thistype.applyDamage)
        return false
        
        endmethod


        private static method onInit takes nothing returns nothing
            call RegisterSpellEffectEvent(AID, function thistype.run)
        endmethod

     endstruct
endscope

 

[deathismyfriend]

Looks a lot better. Only thing is the unit creation. If you can use static units and create them at run time. That way even if you cast this spell 100 times it only created 2 units. Were the way you have it it would create 200 units. In a big / semi big or medium to long map that could really hurt the maps performance. If you could instead create effects at a point then you should go that route.

 

[TheGoldenGoblin]

Since I can't change the size of a special effect I need to use dummy units..
But what do you mean with creating 2 units? Can I create 2 units and use the over and over again or?

 

[deathismyfriend]

yes i mean use the same units over and over again.

Edit:

        private struct Spell extends array
            static unit Dummy1
            static unit Dummy2
            
            static method onInit takes nothing returns nothing
                set Dummy1 = CreateUnit( Player(12), 'hpea', 0, 0, 0)
                set Dummy2 = CreateUnit( Player(12), 'hpea', 0, 0, 0)
            endmethod

Something like the above. Then in your spell move that unit to the location needed and attach effect to it.

 

[TheGoldenGoblin]

but the unit is the effect, I need to create the unit and then kill it to get the effect :O

 

[deathismyfriend]

If you kill the unit just for the effect then you can use that effect and cast it at a point by creating the effect. Which effect are you using ?

 

[TheGoldenGoblin]

http://www.hiveworkshop.com/forums/models-530/arcane-nova-229286/?prev=search=arcane&d=list&r=20
http://www.hiveworkshop.com/forums/...229773/?prev=search=shadow&r=20&d=list&page=4
I use these models and destroy them. But I have set their scaling to 3.0 and that's why I can't use normal special effects.

 

[deathismyfriend]

Ya i think you will have to stay with the way you have it then.

To bad they didn't think we may want to scale effects when making the special effect class lol.

 

[PurplePoot]

All of these systems seem to massively overcomplicate what you're actually trying to do with no apparent benefit. One might say I'm somewhat of a grumpy old type shooing people off his lawn, but I'd just toss the whole thing out and use a more traditional approach. If you really want to worry about trivial code size/efficiency hacks, you should be doing it when you know a lot more about JASS than you currently do. The whole "let's make everything static methods in structs" thing is just nonsensical, overcomplicated, and dumb.

Start with a basic spell template, which should look something like this:

scope MySpellName initializer init
private function Actions takes nothing returns nothing
    //do stuff
endfunction
private function Conditions takes nothing returns boolean
    return GetSpellAbilityId() == '0000'
    //replace '0000' with your spell ID.
    //You may want to put this in the top in a constant global for readability
    //or maintenance purposes, but I left it this way for simplicity.
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

Notice how there is no unnecessary code: everything there directly contributes to the spell. The only non-strictly necessary thing there is the scope, but that saves the headache of having to name everything with really long prefixes like MY_COOL_SPELL_TOTALLY_UNIQUE_EDITION_ which is a road you just don't want to go down. You can technically fold everything into the condition, but once again I'd definitely not do that unless you know what you're doing much more than you currently do, and even then it's dumb and unnecessary in most situations and does little other than negatively impact readability for newer coders.

And then... do stuff, as the comment says. It sounds like you already know your way around basic ins and outs of making dummy units and such, so it should be pretty straightforward from here on out until you start dealing with messier stuff like simulating projectiles.

As for creating models of different sizes, I'd advise against just using scaled dummies. On top of spell effects just being easier to manage, particles won't scale properly, which makes a lot of effect models look really bad when scaled. It's a bit of a pain to learn at first, but learning to do extremely basic model editing in War3ModelEditor goes a long way in situations like this.

Also don't use 0 second timers without good reason. Just... don't. Yet another case of overcomplicated nonsense with no real point unless you have a clear idea of what you actually want to accomplish.

Here's a cleaned up version of your orb spell (assumes a custom SFX model rather than a scaled dummy). I used global constants this time, once again non strictly necessary, because unlike the struct/timer nonsense they're actually important good practice to learn:

scope MySpellName initializer init
globals
    private constant int SPELL_ID = 'A000'
    private constant real DAMAGE = 100.
    private constant real RADIUS = 500.
    private constant string EFFECT = "someeffectpath"
    private constant attacktype ATTACK_TYPE = ATTACK_TYPE_NORMAL
    private constant damagetype DAMAGE_TYPE = DAMAGE_TYPE_NORMAL
 
    private group g = CreateGroup()
endglobals
private function Callback takes nothing returns nothing
    //danger: not TriggerSleepAction-safe
    if GetWidgetLife(GetEnumUnit())>0. then
        call UnitDamageTarget(GetTriggerUnit(), GetEnumUnit(), DAMAGE, false, false, ATTACK_TYPE, DAMAGE_TYPE, null)
    endif
endfunction
private function Actions takes nothing returns nothing
    call GroupEnumUnitsInRange(g, GetSpellTargetX(), GetSpellTargetY(), RADIUS, null)
    //for effects with a death animation, unless you want to play the death animation,
    //you'll need to use a timer or somesuch to get the effect you want. This is a huge
    //pain in the ass. Luckily if you edited a custom model like I suggested above you can
    //just delete the unwanted death animation, if applicable.
    call DestroyEffect(AddSpecialEffect(EFFECT, GetSpellTargetX(), GetSpellTargetY()))
    call ForGroup(g, function Callback)
endfunction
private function Conditions takes nothing returns boolean
    return GetSpellAbilityId() == SPELL_ID
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

Notice how much less boilerplate code and general overcomplicated fluff there is if you skip the whole song and dance with structs, timers etc. Should work fine but I might've typoed somewhere since I didn't compile it. Includes warnings on TriggerSleepAction-safety, but you should almost never use TSA since it's a bad function which causes all kinds of problems.

I'm also about 75% sure that you can just use GetTriggerUnit() in the enum callback and skip the caster global, but it's been long enough since I modded that I wrote it that way just to be safe, and I can't be bothered to get my editor set up again to test.

Edit: Actually, I'm 99.9% sure, because that way makes sense for GUI. Fixed.

 

[TheGoldenGoblin]

But the struct extends array is what is making the spell mui isn't it?

This is what makes the spell mui right?

static integer array rn
static integer ic = 0
set rn[this] = rn[0]
set rn[0] = this

Why is everyone so concerned about using dummy units as effect? I think my effect looks cool and since I'm nulling the units it shouldn't make any bigger difference.
For the timers, I didn't know how else to call for the applyDamage function. But now I guess I can change it to

call ForGroup(g, function applyDamage)

 

[muzzel]

Effects are faster than Units (less fps drop). However.. if you have to move the effects you need to destroy them and create them again which is still faster than units but looks different (especially if the models have particle emitters).

 

[TheGoldenGoblin]

but lets say 2 players got this spell and can use it every 6 sec, that would mean 4 units max every 6 sec. I don't think that is a problem.

 

[PurplePoot]

But the struct extends array is what is making the spell mui isn't it?

Nope: the struct is just fluff. What makes the spell MUI is not holding any globals over a wait time, and since the spell doesn't involve any waiting time it's pretty hard for it to not be MUI.

Why is everyone so concerned about using dummy units as effect? I think my effect looks cool and since I'm nulling the units it shouldn't make any bigger difference.

If it's working fine for you then feel free to keep doing it. The whole nonsense about being 0.000001% more efficient isn't worth caring about. It's just that it's kind of a bad habit to get into given that it doesn't work properly for the vast majority of effects--any of them that use particles, that is--and it requires you to make a separate dummy unit for each effect model. That said, in this particular case there isn't really anything wrong with it.

For the timers, I didn't know how else to call for the applyDamage function. But now I guess I can change it to

First, you didn't even need the function in the first place. Second, you can always just call your custom functions the same way you call any other function: say, call this.applyDamage(). Third, the ForGroup is a way to loop over a group of units (similar to your loop set u = firstofgroup etc etc) rather than a way to generally call functions.

 

[muzzel]

and since I'm nulling the units it shouldn't make any bigger difference.

You are too focused on leaks. The nulling has a muuuuuuch smaller impact on the performance than the difference between unit and specialeffect. Nulling only prevents a reference leak.. not even a handle leak, dont overestimate that (oh and you have to null both units and specialeffect so there is really no difference).

 

[TheGoldenGoblin]

I'm updating my old kamehameha spell that got rejected and wanted to make it GUI and Vjass friendly. But I got some questions because both of them got problems..
Why doesn't this work?

call CreateUnit( .owner, DUMMY, .x, .y, .angle)
call UnitApplyTimedLife(GetLastCreatedUnit(), 'BTLF', 0.45)

Here is the whole trigger:

scope Kamehame

    globals
    private constant integer AID = 'A000'
    private constant real aoe    = 200
    private constant real Offset = 50.
    
    private constant integer DUMMY  = 'h000'
    
    private constant attacktype ATTACK_TYPE  = ATTACK_TYPE_NORMAL
    private constant damagetype DAMAGE_TYPE  = DAMAGE_TYPE_NORMAL
    endglobals
    
    private constant function GetDamage takes integer lvl returns real
      return 500.*lvl
    endfunction
    
    private constant function GetRange takes integer lvl returns real
      return 200.*lvl
    endfunction
    
    private constant function GetSpeed takes integer lvl returns real
      return 20. + 0*lvl
    endfunction    
    
    
    native UnitAlive takes unit id returns boolean
    
    private function FilterUnits takes unit u, player p returns boolean
        return (UnitAlive(u)) and (IsUnitEnemy(u, p)) and (not IsUnitType(u, UNIT_TYPE_MAGIC_IMMUNE))
    endfunction
    
    
    
    private struct Spell extends array
       unit caster
       unit dummy
       real damage
       real x
       real y
       location cl
       location sl
       real angle
       player owner
       real range
       real inRange
       real speed
       integer lvl
       
       static integer array rn
       static integer ic = 0
    
    
    private static method line takes nothing returns nothing
    local timer t = GetExpiredTimer()
    local thistype this = GetTimerData(t)
    local location l = GetUnitLoc(.dummy)
    local unit u
    local group g = CreateGroup()
    
    set .x       = GetUnitX(.dummy)
    set .y       = GetUnitY(.dummy)
    
    if (UnitAlive(.caster)) and (.inRange < .range) then
      call GroupEnumUnitsInRange( g, .x, .y, aoe, null)
      loop
        set u = FirstOfGroup(g)
        exitwhen u == null
        call GroupRemoveUnit(g, u)
        if FilterUnits(u, .owner) then
          call UnitDamageTarget(.caster, u, .damage, false, false, ATTACK_TYPE, DAMAGE_TYPE, null)
        endif
      endloop
      call SetUnitPositionLoc( .dummy, PolarProjectionBJ( l, .speed, .angle))
      call CreateUnit( .owner, DUMMY, .x, .y, .angle)
      call UnitApplyTimedLife(GetLastCreatedUnit(), 'BTLF', 0.45)
      set .inRange = .inRange + 5.
    else
      call KillUnit(.dummy)
      set rn[this] = rn[0]
      set rn[0] = this
      set g = null
      set u = null
      set t = null
      set owner = null
      set caster = null
      set dummy = null
      set l = null
      set cl = null
      set sl = null
    
    endif
    
    
   endmethod
    
    private static method run takes nothing returns nothing
    local thistype this
    
    
    set this = rn[0]
    if this == 0 then
        set ic = ic + 1
        set this = ic
    else
        set rn[0] = rn[this]
    endif
    
    
    set .owner   = GetTriggerPlayer()
    set .caster  = GetTriggerUnit()
    set .lvl     = GetUnitAbilityLevel(.caster, AID)
    set .damage  = GetDamage(lvl)
    set .range   = GetRange(lvl)
    set .cl      = GetUnitLoc(.caster)
    set .sl      = GetSpellTargetLoc()
    set .angle   = AngleBetweenPoints(.cl, .sl)
    set .dummy   = CreateUnitAtLoc( .owner, DUMMY, .cl, .angle)
    set .inRange = 0
    set .speed   = GetSpeed(lvl)
    
    call TimerStart(NewTimerEx(this), 0.03, true, function thistype.line)
    
    endmethod
    
    private static method onInit takes nothing returns nothing
    call RegisterSpellEffectEvent(AID, function thistype.run)
    endmethod
    
    
    endstruct
endscope

I'm making the spell in GUI aswell but the indexing doesn't work properly anyone knows why?

GUI Kamehameha:

• Kamehameha Cast
  • Events
    • Unit - A unit Starts the effect of an ability
  • Conditions
    • (Ability being cast) Equal to (==) Ka_Ability
  • Actions
    • -------- -------------------------------------------- --------
    • -------- Set the index of the spell --------
    • -------- -------------------------------------------- --------
    • Set Ka_Index = (Ka_Index + 1)
    • -------- -------------------------------------------- --------
    • -------- Set the caster --------
    • -------- -------------------------------------------- --------
    • Set Ka_Caster[Ka_Index] = (Triggering unit)
    • -------- -------------------------------------------- --------
    • -------- Set point 1 and 2 to get the angle of the spell --------
    • -------- -------------------------------------------- --------
    • Set Tempoint1 = (Position of Ka_Caster[Ka_Index])
    • Set Tempoint2 = (Target point of ability being cast)
    • Set Ka_Angle[Ka_Index] = (Angle from Tempoint1 to Tempoint2)
    • -------- -------------------------------------------- --------
    • -------- Sets the start point of the spell --------
    • -------- -------------------------------------------- --------
    • Set Tempoint3 = (Tempoint1 offset by Ka_Offset towards Ka_Angle[Ka_Index] degrees)
    • -------- -------------------------------------------- --------
    • -------- Get the level of the ability from the caster --------
    • -------- -------------------------------------------- --------
    • Set Ka_AbilityLvl[Ka_Index] = (Level of Ka_Ability for Ka_Caster[Ka_Index])
    • -------- -------------------------------------------- --------
    • -------- Get the range/ level of the spell --------
    • -------- -------------------------------------------- --------
    • Set Ka_Range[Ka_Index] = Ka_Range[Ka_AbilityLvl[Ka_Index]]
    • -------- -------------------------------------------- --------
    • -------- Set the damage --------
    • -------- -------------------------------------------- --------
    • Set Ka_Damage[Ka_Index] = Ka_Damage[Ka_AbilityLvl[Ka_Index]]
    • -------- -------------------------------------------- --------
    • -------- Create the ''leader'' dummy --------
    • -------- -------------------------------------------- --------
    • Unit - Create 1 Kamehameha Dummy for (Owner of Ka_Caster[Ka_Index]) at Tempoint3 facing Ka_Angle[Ka_Index] degrees
    • Set Ka_Dummy[Ka_Index] = (Last created unit)
    • -------- -------------------------------------------- --------
    • -------- Remove the leaks --------
    • -------- -------------------------------------------- --------
    • Custom script: call RemoveLocation (udg_Tempoint1)
    • Custom script: call RemoveLocation (udg_Tempoint2)
    • Custom script: call RemoveLocation (udg_Tempoint3)
      • Multiple FunctionsIf (All Conditions are True) then do (Then Actions) else do (Else Actions)
        • If - Conditions
          • Ka_Index Equal to (==) 1
        • Then - Actions
          • Trigger - Turn on Kamehameha Loop <gen>
        • Else - Actions

• Kamehameha Loop
  • Events
    • Time - Every 0.03 seconds of game time
  • Conditions
  • Actions
    • Do Multiple ActionsFor each (Integer Ka_Loop) from 1 to Ka_Index, do (Actions)
      • Loop - Actions
        • Set Tempoint1 = (Position of Ka_Dummy[Ka_Loop])
        • Set Tempoint2 = (Tempoint1 offset by Ka_Speed[Ka_AbilityLvl[Ka_Loop]] towards Ka_Angle[Ka_Loop] degrees)
          • Multiple FunctionsIf (All Conditions are True) then do (Then Actions) else do (Else Actions)
            • If - Conditions
              • (Ka_Range[Ka_Loop] Equal to (==) 200.00) or ((Terrain pathing at Tempoint2 of type Walkability is off) Equal to (==) True)
            • Then - Actions
              • Game - Display to (All players) the text: then
              • Unit - Kill Ka_Dummy[Ka_Loop]
              • Unit - Remove Ka_Dummy[Ka_Loop] from the game
              • Set Ka_Caster[Ka_Loop] = Ka_Caster[Ka_Index]
              • Set Ka_Caster[Ka_Index] = No unit
              • Set Ka_AbilityLvl[Ka_Loop] = Ka_AbilityLvl[Ka_Index]
              • Set Ka_Damage[Ka_Loop] = Ka_Damage[Ka_Index]
              • Set Ka_Angle[Ka_Loop] = Ka_Angle[Ka_Index]
              • Set Ka_Index = (Ka_Index - 1)
              • Set Ka_Loop = (Ka_Loop - 1)
            • Else - Actions
              • Set Ka_Range[Ka_Loop] = (Ka_Range[Ka_Loop] + 1.00)
              • Unit - Move Ka_Dummy[Ka_Loop] instantly to Tempoint2
              • Unit - Create 1 Kamehameha Dummy for (Owner of Ka_Caster[Ka_Loop]) at Tempoint1 facing (Facing of Ka_Dummy[Ka_Loop]) degrees
              • Unit - Add a 0.45 second Generic expiration timer to (Last created unit)
              • Custom script: set bj_wantDestroyGroup=true
              • Unit Group - Pick every unit in (Units within 100.00 of Tempoint1 matching ((((Matching unit) is A structure) Equal to (==) False) and ((((Matching unit) is alive) Equal to (==) True) and (((Picked unit) belongs to an enemy of (Owner of Ka_Caster[Ka_Loop])) Equal to (==) True)))) and do (Actions)
                • Loop - Actions
                  • Unit - Cause Ka_Caster[Ka_Loop] to damage (Picked unit), dealing Ka_Damage[Ka_AbilityLvl[Ka_Loop]] damage of attack type Spells and damage type Unknown
        • Custom script: call RemoveLocation (udg_Tempoint1)
        • Custom script: call RemoveLocation (udg_Tempoint2)
    • Multiple FunctionsIf (All Conditions are True) then do (Then Actions) else do (Else Actions)
      • If - Conditions
        • Ka_Index Equal to (==) 0
      • Then - Actions
        • Trigger - Turn off (This trigger)
      • Else - Actions

 

[deathismyfriend]

native CreateUnit takes player id, integer unitid, real x, real y, real face returns unit

// vs the one used for GUI

function CreateNUnitsAtLoc takes integer count, integer unitId, player whichPlayer, location loc, real face returns group
    call GroupClear(bj_lastCreatedGroup)
    loop
        set count = count - 1
        exitwhen count < 0
        call CreateUnitAtLocSaveLast(whichPlayer, unitId, loc, face)
        call GroupAddUnit(bj_lastCreatedGroup, bj_lastCreatedUnit)
    endloop
    return bj_lastCreatedGroup
endfunction

Which means you have to do this.

local unit u = CreateUnit( .owner, DUMMY, .x, .y, .angle)
call UnitApplyTimedLife(u, 'BTLF', 0.45)
set u = null // Do this when done.

Since you are using vJass try to use the function button. It helps solve a lot of small problems like this.