• 🏆 Texturing Contest #33 is OPEN! Contestants must re-texture a SD unit model found in-game (Warcraft 3 Classic), recreating the unit into a peaceful NPC version. 🔗Click here to enter!
  • It's time for the first HD Modeling Contest of 2024. Join the theme discussion for Hive's HD Modeling Contest #6! Click here to post your idea!

[Snippet] SpellEffectEvent

Bribe

Code Moderator
Level 50
Joined
Sep 26, 2009
Messages
9,456
A handy approach to using spell effect responses. You can assign an event to fire for only a specific ability code, or when any ability is cast. API is intuitive and avoids creating tons and tons of handles to get the job done. This kind of thing has similar approaches by other people but this is definitely the most efficient way to do it.


Effectively, this is supposed to turn:

JASS:
private function onCast takes nothing returns boolean
    if (GetSpellAbilityId() == 'A000') then
        // Do actions
    endif
    return false
endfunction
 
private function onInit takes nothing returns nothing
    local trigger t = CreateTrigger()
    call TriggerRegisterAnyUnitEventBJ(t, EVENT_PLAYER_UNIT_SPELL_EFFECT)
    call TriggerAddCondition(t, Condition(function onCast))
    set t = null
endfunction

Into:

JASS:
private function onCast takes nothing returns nothing
    // Do actions
endfunction
 
private function onInit takes nothing returns nothing
    call RegisterSpellEffectEvent('A000', function onCast)
endfunction

As you can see, a lot fewer lines of code. It saves TONS of efficiency on maps with a lot of spells in them. The two main reasons why are:

1. Does not generate 16 events per spell.
2. This uses one trigger evaluation instead of one for each individual spell (some maps have quite a few). This helps keep framerate high and fluid when a spell is cast.


JASS:
//============================================================================
// SpellEffectEvent
// - Version 1.1.0.0
//
// API
// ---
//     RegisterSpellEffectEvent(integer abil, code onCast)
//
// Requires
// --------
//     RegisterPlayerUnitEvent: hiveworkshop.com/forums/showthread.php?t=203338
//
// Optional
// --------
//     Table: hiveworkshop.com/forums/showthread.php?t=188084
//
library SpellEffectEvent requires RegisterPlayerUnitEvent, optional Table
 
//============================================================================
private module M
   
    static if LIBRARY_Table then
        static Table tb
    else
        static hashtable ht = InitHashtable()
    endif
   
    static method onCast takes nothing returns nothing
        static if LIBRARY_Table then
            call TriggerEvaluate(.tb.trigger[GetSpellAbilityId()])
        else
            call TriggerEvaluate(LoadTriggerHandle(.ht, 0, GetSpellAbilityId()))
        endif
    endmethod
 
    private static method onInit takes nothing returns nothing
        static if LIBRARY_Table then
            set .tb = Table.create()
        endif
        call RegisterPlayerUnitEvent(EVENT_PLAYER_UNIT_SPELL_EFFECT, function thistype.onCast)
    endmethod
endmodule
 
//============================================================================
private struct S extends array
    implement M
endstruct
 
//============================================================================
function RegisterSpellEffectEvent takes integer abil, code onCast returns nothing
    static if LIBRARY_Table then
        if not S.tb.handle.has(abil) then
            set S.tb.trigger[abil] = CreateTrigger()
        endif
        call TriggerAddCondition(S.tb.trigger[abil], Filter(onCast))
    else
        if not HaveSavedHandle(S.ht, 0, abil) then
            call SaveTriggerHandle(S.ht, 0, abil, CreateTrigger())
        endif
        call TriggerAddCondition(LoadTriggerHandle(S.ht, 0, abil), Filter(onCast))
    endif
endfunction
 
endlibrary

Lua:
--Lua RegisterSpellEffectEvent
do
    local tb =  {}
    local trig = nil
   
    function RegisterSpellEffectEvent(abil, onCast)
        if not trig then
            trig = CreateTrigger()
            TriggerRegisterPlayerUnitEventBJ(trig, EVENT_PLAYER_UNIT_SPELL_EFFECT)
            TriggerAddCondition(trig, Condition(function()
                local i = tb[GetSpellAbilityId()]
                if i then i() end
            end))
        end
        tb[abil] = onCast
    end
end
   
--syntax is:
--RegisterSpellEffectEvent(FourCC('Abil'), function() print(GetUnitName(GetTriggerUnit())) end)
 
Last edited:
Level 31
Joined
Jul 10, 2007
Messages
6,306
At this point
JASS:
        local player p = casterowner
        local integer c = casterid
        local integer t = targetid
        local integer a = abilcode
        local real cx = casterx
        local real cy = castery
        local real tx = targetx
        local real ty = targety

You want to probably use an array. Incrementing and decrementing an array would probably be a lot faster than declaring all of those locals ; P.

No . needed on these. Also please follow JASS convention (camelcase)
JASS:
    readonly static real casterX = 0
    readonly static real casterY = 0
    readonly static real targetX = 0
    readonly static real targetY = 0

And why are you storing the x,y positions anyways? The user may not need that data at all. You should remove the storage of the coordinates.

private function getEvent takes integer abilcode returns Event

Should be

private function GetEvent takes integer abilcode returns Event

Get rid of this
function OnSpellEffect takes boolexpr c returns nothing

Fire off via an Event instead within your onCast method. You realize that your recursion will fail with how you have it right now right? For any, the direct event can be used. A function wrapper name would probably be

function RegisterAnySpellEffectEvent takes boolexpr c returns nothing


This hurts my eyes to read
JASS:
// RegisterSpellEffectEvent(boolexpr, abilcode)
// TriggerRegisterSpellEffectEvent(trigger, abilcode)
// OnSpellEffect(boolexpr)
// readonly integer SpellEffect.casterid
// readonly integer SpellEffect.targetid
// readonly integer SpellEffect.abilcode
// readonly player SpellEffect.casterowner
// readonly unit SpellEffect.caster
// readonly unit SpellEffect.target
// readonly real SpellEffect.casterx
// readonly real SpellEffect.castery
// readonly real SpellEffect.targetx
// readonly real SpellEffect.targety

Make it prettier ; (, like

JASS:
//Functions:
//---------------------------------------------------
    // function RegisterSpellEffectEvent takes boolexpr, abilityId returns nothing
    // function TriggerRegisterSpellEffectEvent takes trigger, abilityId returns nothing
    // function RegisterAnySpellEffectEvent takes boolexpr returns nothing

//SpellEffect:
//---------------------------------------------------
    // readonly integer casterId
    // readonly integer targetId
    // readonly integer ability
    // readonly player casterOwner
    // readonly unit caster
    // readonly unit target
 

Bribe

Code Moderator
Level 50
Joined
Sep 26, 2009
Messages
9,456
>>You want to probably use an array. Incrementing and decrementing an array would probably be a lot faster than declaring all of those locals ; P.

You're definitely right here.

>>No . needed on these.

Definitely not needed, but it is good programming practice, when you use a real, to use at least a . at the end so someone flipping through a script doesn't have to constantly consult the API to know if it's a real or integer when you omit the .

>>Also please follow JASS convention (camelcase)

It's more annoying to uppercase obviously separated words, but for the case of matching JASS' already-poor syntax, fine.

>>You should remove the storage of the coordinates.

Doing so would make GetSpellTargetX/Y do nothing (remember it's a variable event), so that would be foolish. Although getting rid of caster x/y would be good in case the caster's position changed.

>>This hurts my eyes to read

Hence why I could never be an artist. First 6 or 7 months of my JASS career I was just copying Berb, now I don't know what I'm doing.

Can't fault the recursion as that's not going to screw anything as the spell responses are stored into variables, negating the need for calling the natives. And remember you were the one who told me to use a simple boolexpr evaluation for "any spell event", and now you think I should change it back.

Editing to factor in the changes.
 
Last edited:

Bribe

Code Moderator
Level 50
Joined
Sep 26, 2009
Messages
9,456
Updated. Removed the "casterOwner" variable for the extremely unlikely event that the owner changes during that time, and removed casterX and casterY.

RegisterAnySpellEffectEvent now exists and can actually use the native event responses OR the struct variables. RegisterSpellEffectEvent and the trigger-version can only use the struct variables because they are triggered by a variable event and not by a spell event, but it's more optimal to do it like that anyway.

This is the first release of mine which uses a shorter scope-prefix to privatise names instead of the longer library prefix (because Vex's map optimiser is so broken). There are other libraries planned to be released like this.
 
Level 31
Joined
Jul 10, 2007
Messages
6,306
This now breaks if it's done within the trigger
JASS:
function RegisterAnySpellEffectEvent takes boolexpr c returns nothing
    call TriggerRemoveCondition(trig, func)
    call TriggerAddCondition(trig, c)
    set func = TriggerAddCondition(trig, cond)
endfunction

You do know that removing a triggercondition from within the trigger stops the trigger in its tracks right? : )

Just run it thru an Event man >.>.
 
Level 31
Joined
Jul 10, 2007
Messages
6,306
Now it's actually slower than just firing off an event... >.<

You have an extra trigger evaluation now ; |

2 trigger evals on the firing trigger and 1 trigger evaluation in one of tem

If you did it right, u'd have 1 trigger eval on firing trigger and 1 trigger evaluation on it

Plus you can't register triggers to fire whenever a spell fires.


Just run it through an Event =O, I've said it before and I'll say it as many times as I have to until you get it :p
 
Last edited:

Bribe

Code Moderator
Level 50
Joined
Sep 26, 2009
Messages
9,456
Totally restructured for 0.2.0.0, stripped the library requirements entirely and stripped a lot of the API for raw functionality.

The only public products are two functions available for users: RegisterSpellEffectEvent(integer whichAbility, boolexpr onCast) and RegisterAnySpellEffectEvent(boolexpr onCast).
 
Last edited:

Bribe

Code Moderator
Level 50
Joined
Sep 26, 2009
Messages
9,456
Updated to 0.3.0.0 with re-added functionality with the Event library, but this time without the requirement of Vexorian's Table because it breaks with module initializers.

The speed is almost the same but now you have the TriggerRegisterSpellEffectEvent(integer whichAbility, trigger whichTrigger) functionality back.
 
Level 31
Joined
Jul 10, 2007
Messages
6,306
What's wrong with this JASS : P
JASS:
//boolexpr second? : O
function RegisterSpellEffectEvent takes integer whichAbility, boolexpr c returns nothing
    call GetEvent(whichAbility).register(c)
endfunction

//trigger first
//============================================================================
function TriggerRegisterSpellEffectEvent takes trigger whichTrigger, integer whichAbility returns nothing
    call GetEvent(whichAbility).registerTrigger(whichTrigger)
endfunction

//boolexpr first
//============================================================================
function RegisterAnySpellEffectEvent takes boolexpr c returns nothing
    call TriggerAddCondition(trig, c)
endfunction
 
Bribe, this IS approval worthy :)
Imagine a map with 412 spells (103 heroes with 4 spells each)
That would create 412 * 16 = 6592 event reqisterations and 412 triggers handles.
This system can reduce that number to a static value (16) no matter how many spells
there are in the map.

After you update it, I suggest you approve it immediately ;)
 

BBQ

BBQ

Level 4
Joined
Jun 7, 2011
Messages
97
I don't see a point in using Event for this. Here's what I would do:
JASS:
//! zinc
library SpellEffectEvent requires Table
{
    module initializer
    {
        private static method onInit()
        {
            integer i;
            for (0 <= i <= 15)
            {
                TriggerRegisterPlayerUnitEvent(thistype.mainTrigger, Player(i), EVENT_PLAYER_UNIT_SPELL_EFFECT, null);
            }
            TriggerAddCondition(mainTrigger, static method() -> boolean
            {
                return TriggerEvaluate(thistype.storage.trigger[GetSpellAbilityId()]);
            });
            thistype.storage = Table.create();
        }
    }
    struct eventHandler[]
    {
        private
        {
            static trigger mainTrigger = CreateTrigger();
            static Table storage;
        }
        static method getTrigger(integer abilityId) -> trigger
        {
            if (!thistype.storage.handle.has(abilityId))
            {
                thistype.storage.trigger[abilityId] = CreateTrigger();
            }
            return thistype.storage.trigger[abilityId];
        }
        static method register(boolexpr onEffect, integer abilityId)
        {
            TriggerAddCondition(thistype.getTrigger(abilityId), onEffect);
        }
        static method registerAny(boolexpr onEffect)
        {
            TriggerAddCondition(mainTrigger, onEffect);
        }
        module initializer;
    }
    public function RegisterSpellEffectEvent(boolexpr onEffect, integer abilityId)
    {
        eventHandler.register(onEffect, abilityId);
    }
    public function RegisterAnySpellEffectEvent(boolexpr onEffect)
    {
        eventHandler.registerAny(onEffect);
    }
}
//! endzinc
 
Last edited:
It looks more readable to me this way:

JASS:
//! zinc
library SpellEffectEvent requires Table
{
    module initializer
    {
        private static method onInit()
        {
            integer i;
            for (0 <= i <= 15) { TriggerRegisterPlayerUnitEvent(thistype.mainTrigger, Player(i), EVENT_PLAYER_UNIT_SPELL_EFFECT, null); }
            TriggerAddCondition(mainTrigger, static method() -> boolean { return TriggerEvaluate(thistype.storage.trigger[GetSpellAbilityId()]); });
            thistype.storage = Table.create();
        }
    }
    
    struct eventHandler[]
    {
        private static trigger mainTrigger = CreateTrigger();
        private static Table storage;
        
        static method getTrigger(integer abilityId) -> trigger
        {
            if (!thistype.storage.handle.has(abilityId)) { thistype.storage.trigger[abilityId] = CreateTrigger(); }
            return thistype.storage.trigger[abilityId];
        }
        
        static method register(boolexpr onEffect, integer abilityId)                    { TriggerAddCondition(thistype.getTrigger(abilityId), onEffect); }
        static method registerAny(boolexpr onEffect)                                    { TriggerAddCondition(mainTrigger, onEffect); }
        module initializer;
    }
    
    public function RegisterSpellEffectEvent(boolexpr onEffect, integer abilityId)      { eventHandler.register(onEffect, abilityId); }
    public function RegisterAnySpellEffectEvent(boolexpr onEffect)                      { eventHandler.registerAny(onEffect); }
}
//! endzinc

Also, it's for (0 <= i <= 15) not for (0 <= i <= 16) lol :p
But anyways, if you're going to do that Bribe, you could make Event Optional and use static ifs in the code =D
 

BBQ

BBQ

Level 4
Joined
Jun 7, 2011
Messages
97
TriggerRegisterSpellEffectEvent

Yes, I'm perfectly aware of that (and that's why I didn't even include it in my "version"), but TriggerRegisterSpellEffectEvent() is kind of useless.
 

Bribe

Code Moderator
Level 50
Joined
Sep 26, 2009
Messages
9,456
I have updated this to 1.0.0.0 and backwards compatibility is 100% broken.

JASS:
RegisterSpellEffectEvent(boolexpr onCast, integer abil)
->
RegisterSpellEffectEvent(integer abil, code onCast)

I swapped the code and integer arguments to fall in line with RegisterPlayerUnitEvent's convention of code second, type first. It is more readable anyway because the abil id is the main thing to care about, and the code argument itself is a lesser priority, with these kinds of things. Blizzard often does the same thing with having the code/boolexpr argument come last, except in perhaps one type of case - GroupEnum/ForceEnum...Counted.
 

BBQ

BBQ

Level 4
Joined
Jun 7, 2011
Messages
97
JASS:
    static method onCast takes nothing returns boolean
        call TriggerEvaluate(tb.trigger[GetSpellAbilityId()])
        return false
    endmethod
change this to

JASS:
    static method onCast takes nothing returns boolean
        return TriggerEvaluate(tb.trigger[GetSpellAbilityId()])
    endmethod
 
Level 6
Joined
Jun 20, 2011
Messages
249
funny how out of pure necessity i coded the same resource in my map, has two very small differences:
- function name OnAbilityCast
- I check if the trigger handle exists inside the hashtable to avoid firing null triggers when abilities that have no coding are cast.
 

Bribe

Code Moderator
Level 50
Joined
Sep 26, 2009
Messages
9,456
It's pretty much the same overhead to do that check, Dirac, as would be the setup I have. Evaluating a null trigger is extremely lightweight. And if the spell is actually registered, your way will be slightly slower ;)

OnAbilityCast is also innaccurate. Starts Casting and Starts Effect are two different events in wc3.
 
Level 20
Joined
Aug 13, 2013
Messages
1,696
There's a compile error: trigger is not a member of Table__GTable...
The compile error line in: call TriggerEvaluate(.tb.trigger[GetSpellAbilityId()])
Pls help me I'm a newbie at vJASS and I'm practicing and learning how to use systems

I am using Table 3.1 by Vexorian
 
Top