Wc3C.net

Wc3C.net (http://www.wc3c.net/forums.php)
-   Scripts (http://www.wc3c.net/forumdisplay.php?f=737)
-   -   SpellEvent (http://www.wc3c.net/showthread.php?t=105374)

Anitarf 04-07-2009 11:24 PM

SpellEvent
 
I feel the documentation in the library explains it well enough, so there's not much I can add here.
It requires Table, by the way.
Collapse JASS:
library SpellEvent initializer Init requires Table

//*****************************************************************
//*  SPELL EVENT LIBRARY 1.3
//*
//*  written by: Anitarf
//*  requires: -Table
//*
//*  Maps with many triggered spells require many triggers that run
//*  on spell events. Whenever a spell is cast, all those triggers
//*  need to be evaluated by the game even though only one actually
//*  needs to run. This library has been written to reduce the
//*  number of triggers in such maps; instead of having a trigger
//*  per spell, this library contains a single trigger which then
//*  runs only the code associated with the spell that's actually
//*  being cast.
//*
//*  Perhaps more significant than the marginal speed gain is the
//*  feature that allows you to access all the spell event
//*  responses from all spell events, something that the native
//*  functions senselessly do not support. With this system you can
//*  for example easily get the target unit of the spell on the
//*  casting finish event.
//*
//*  All functions following the Response function interface that
//*  is defined at the start of this library can be used to respond
//*  to spell events. You can register a response with one of the
//*  following functions, each for a different spell event:
//*
//*    function RegisterSpellChannelResponse takes integer spellId, Response r returns nothing
//*    function RegisterSpellCastResponse takes integer spellId, Response r returns nothing
//*    function RegisterSpellEffectResponse takes integer spellId, Response r returns nothing
//*    function RegisterSpellFinishResponse takes integer spellId, Response r returns nothing
//*    function RegisterSpellEndCastResponse takes integer spellId, Response r returns nothing
//*
//*  The first event occurs at the very start of the spell, when
//*  the spell's casting time begins; most spells have 0 casting
//*  time, so in most cases this first event occurs at the same
//*  time as the second one, which runs when the unit actually
//*  begins casting a spell by starting its spell animation. The
//*  third event occurs when the spell effect actually takes place,
//*  which happens sometime into the unit's spell animation
//*  depending on the unit's "Animation - Cast Point" property.
//*  The fourth event runs if the unit finishes casting the spell
//*  uninterrupted, which might be important for channeling spells.
//*  The last event runs when the unit stops casting the spell,
//*  regardless of whether it finished casting or was interrupted.
//*
//*  If you specify a spell id when registering a response then
//*  that response will only run when that ability is cast; only
//*  one function per ability per event is supported, if you
//*  register more responses then only the last one registered will
//*  be called. If, however, you pass 0 as the ability id parameter
//*  then the registered function will run for all spells. Up to
//*  8190 functions can be registered this way for each event.
//*  These functions will be called before the ability's specific
//*  function in the order they were registered.
//*
//*  This library provides its own event responses that work
//*  better than the Blizzard's bugged native cast event responses.
//*  They still won't work after a wait, but unlike Blizzard's
//*  natives they will work on all spell events.
//*
//*  Here are usage examples for all event responses:
//*
//*    local integer a = SpellEvent.AbilityId
//*    local unit u = SpellEvent.CastingUnit
//*    local unit t = SpellEvent.TargetUnit
//*    local item i = SpellEvent.TargetItem
//*    local destructable d = SpellEvent.TargetDestructable
//*    local location l = SpellEvent.TargetLoc
//*    local real x = SpellEvent.TargetX
//*    local real y = SpellEvent.TargetY
//*    local boolean b = SpellEvent.CastFinished
//*
//*  SpellEvent.TargetLoc is provided for odd people who insist on
//*  using locations, note that if you use it you have to cleanup
//*  the returned location yourself.
//*
//*  SpellEvent.CastFinished boolean is intended only for the
//*  EndCast event as it tells you whether the spell finished or
//*  was interrupted.
//*
//*
//*  Note that a few spells such as Berserk and Wind Walk behave
//*  somewhat differently from regular spells: they are cast
//*  instantly without regard for cast animation times, they do not
//*  interrupt the unit's current order, as well as any spell it
//*  may be casting. SpellEvent 1.1 now handles such spells without
//*  errors provided they are truly instant (without casting time).
//*
//*  It also turned out that a few rare abilities like Charge Gold
//*  & Lumber trigger a spell effect event, but not any other.
//*  SpellEvent 1.2 no longer ignores these lone effect events.
//*
//*  It also turned out that removing the spell ability would run
//*  the endcast event, so if the ability was removed from one of
//*  the effect callbacks the spell event data would get cleaned
//*  up prematurely, SpellEvent 1.3 fixes this issue.
//*****************************************************************

    // use the RegisterSpell*Response functions to add spell event responses to the library
    public function interface Response takes nothing returns nothing

// ================================================================

    private keyword effectDone
    private keyword init
    private keyword get
    private keyword destroy

    private struct spellEvent
        private static HandleTable casterTable
        boolean effectDone=false

        integer AbilityId
        unit CastingUnit
        unit TargetUnit
        item TargetItem=null
        destructable TargetDestructable=null
        real TargetX=0.0
        real TargetY=0.0
        boolean CastFinished=false

        readonly boolean destroyWhenDone=false
        
        // Some abilities like Berserk can be cast instantly without interrupting
        // the caster's current order, which includes any spells the caster may
        // already be casting. The following member allows the system to recover
        // the original spellEvent when such an instant spell overwrites it.
        private spellEvent interrupt

        method operator TargetLoc takes nothing returns location
            return Location(.TargetX, .TargetY)
        endmethod
        
        private static method create takes nothing returns spellEvent
            return spellEvent.allocate()
        endmethod
        static method init takes nothing returns spellEvent
            local spellEvent s=spellEvent.allocate()
            set s.AbilityId = GetSpellAbilityId()
            set s.CastingUnit = GetTriggerUnit()
            set s.TargetUnit = GetSpellTargetUnit()
            if s.TargetUnit != null then
                set s.TargetX = GetUnitX(s.TargetUnit)
                set s.TargetY = GetUnitY(s.TargetUnit)
            else
                set s.TargetDestructable = GetSpellTargetDestructable()
                if s.TargetDestructable != null then
                    set s.TargetX = GetDestructableX(s.TargetDestructable)
                    set s.TargetY = GetDestructableY(s.TargetDestructable)
                else
                    set s.TargetItem = GetSpellTargetItem()
                    if s.TargetItem != null then
                        set s.TargetX = GetItemX(s.TargetItem)
                        set s.TargetY = GetItemY(s.TargetItem)
                    else
                        set s.TargetX = GetSpellTargetX()
                        set s.TargetY = GetSpellTargetY()
                    endif
                endif
            endif
            set s.interrupt = spellEvent.casterTable[s.CastingUnit]
            set spellEvent.casterTable[s.CastingUnit]=integer(s)
            return s
        endmethod
        method destroy takes nothing returns nothing
            if SpellEvent!=0 then
                // the library is in the middle of running callbacks, this can happen if
                // the spell ability gets removed from the unit in one of the callbacks
                set .destroyWhenDone=true
                return
            endif
            if .interrupt!=0 then
                // this spell interrupted another spell, some instant spells can do this
                set spellEvent.casterTable[.CastingUnit]=.interrupt
            else
                call spellEvent.casterTable.flush(.CastingUnit)
            endif
            set .CastingUnit=null
            call .deallocate()
        endmethod

        static method get takes unit caster returns spellEvent
            return spellEvent(spellEvent.casterTable[caster])
        endmethod
        static method onInit takes nothing returns nothing
            set .casterTable=HandleTable.create()
        endmethod
    endstruct
    
    globals
        spellEvent SpellEvent=0
    endglobals
    
// ================================================================

    //! textmacro spellEvent_make takes name
    globals
        private Response array $name$CallList
        private integer $name$CallCount=0
        private Table $name$Table
    endglobals

    private function $name$Calls takes integer id returns nothing
        local integer i=0
        local spellEvent previous=SpellEvent
        set SpellEvent=spellEvent.get(GetTriggerUnit())
        loop
            exitwhen i>=$name$CallCount
            call $name$CallList[i].evaluate()
            set i=i+1
        endloop
        if $name$Table.exists(id) then
            call Response($name$Table[id]).evaluate()
        endif
        if SpellEvent.destroyWhenDone then
            set SpellEvent=0
            call spellEvent.get(GetTriggerUnit()).destroy()
        endif
        set SpellEvent=previous
    endfunction

    function RegisterSpell$name$Response takes integer spellId, Response r returns nothing
        if spellId==0 then
            set $name$CallList[$name$CallCount]=r
            set $name$CallCount=$name$CallCount+1
        else
            set $name$Table[spellId]=integer(r)
        endif
    endfunction
    //! endtextmacro

    //! runtextmacro spellEvent_make("Channel")
    //! runtextmacro spellEvent_make("Cast")
    //! runtextmacro spellEvent_make("Effect")
    //! runtextmacro spellEvent_make("Finish")
    //! runtextmacro spellEvent_make("EndCast")

// ================================================================

    globals
        // Morph abilities like Metamorphosis will cause an additional spell effect
        // event to run when the caster morphs back to its original form. To avoid
        // such duplicates, SpellEvent is designed to ignore any effect event that
        // does not have a matching channel event preceding it.
        // However, there are also rare abilities, like Charge Gold&Lumber, which
        // only cause an effect event to run, so these events must not be ignored
        // even though they occur without a matching channel event. This Table
        // tracks ability IDs of spells that did cause a channel event so that when
        // a spell is cast that doesn't cause one, its effect event is not ignored.
        private Table CastAfterChannel
    endglobals

    private function Channel takes nothing returns nothing
        call spellEvent.init()
        call ChannelCalls(GetSpellAbilityId())
    endfunction

    private function Cast takes nothing returns nothing
        call CastCalls(GetSpellAbilityId())
    endfunction

    private function Effect takes nothing returns nothing
        local spellEvent s=spellEvent.get(GetTriggerUnit())
        local integer id=GetSpellAbilityId()
        if s!=0 and not s.effectDone then
            set s.effectDone=true
            call EffectCalls(id)
            if not CastAfterChannel.exists(id) then
                set CastAfterChannel[id]=1
            endif
        elseif not CastAfterChannel.exists(id) then
            set s = spellEvent.init()
            call EffectCalls(id)
            call s.destroy()
        endif
    endfunction

    private function Finish takes nothing returns nothing
        set spellEvent.get(GetTriggerUnit()).CastFinished=true
        call FinishCalls(GetSpellAbilityId())
    endfunction

    private function EndCast takes nothing returns nothing
        call EndCastCalls(GetSpellAbilityId())
        call spellEvent.get(GetTriggerUnit()).destroy()
    endfunction

// ================================================================

    private function InitTrigger takes playerunitevent e, code c returns nothing
        local trigger t=CreateTrigger()
        call TriggerRegisterAnyUnitEventBJ( t, e )
        call TriggerAddAction(t, c)
        set t=null
    endfunction
    private function Init takes nothing returns nothing
        set ChannelTable=Table.create()
        set CastTable=Table.create()
        set EffectTable=Table.create()
        set FinishTable=Table.create()
        set EndCastTable=Table.create()
        call InitTrigger(EVENT_PLAYER_UNIT_SPELL_CHANNEL, function Channel)
        call InitTrigger(EVENT_PLAYER_UNIT_SPELL_CAST, function Cast)
        call InitTrigger(EVENT_PLAYER_UNIT_SPELL_EFFECT, function Effect)
        call InitTrigger(EVENT_PLAYER_UNIT_SPELL_FINISH, function Finish)
        call InitTrigger(EVENT_PLAYER_UNIT_SPELL_ENDCAST, function EndCast)
        set CastAfterChannel=Table.create()
    endfunction

endlibrary
Expand Usage example:
Expand Yes, if all your spells are made with triggers and use this library then you can even do this:

Anitarf 04-08-2009 01:10 AM

I agree, getting all cast event responses to work for Finish and EndCast events is the main advantage here, not the speed. Regarding that, this system will definitely be faster at a certain number of spells, but it might take a lot of spells and even then the advantage will be marginal. I guess my point regarding speed is that it won't be slower (except in maps with too few spells to matter) than an individual trigger per spell, so the features the script brings don't cost you anything in terms of map performance.

Edit: Added a usage example to the first post.

Rising_Dusk 04-08-2009 02:08 AM

I really dislike the naming of "SpellEvent_GetTargetUnit()" and stuff, but I'm awful glad it flows so well with standard WC3 naming for functions. I think it might be better if instead of RegisterFinishResponse(ABILITY_ID, function OnCast), it had something like RegisterSpellResponse(ABILITY_ID, RESPONSE_TYPE_FINISHED, function OnCast). I realize it's a bit more typing, but I think it's more appropriate to change the integer in there than to change the function call you're using.

Outside of that, this is awesome. Approved.

EDIT:
You mispelled the keyword 'library' in the first post. Please fix that. (I could do it for you, but I would prefer you to)

Bobo_The_Kodo 04-08-2009 02:37 AM

Quote:

RegisterSpellResponse(ABILITY_ID, RESPONSE_TYPE_FINISHED, function OnCast)

I agree with this

The overly long function names associated with SpellEvent_blahblahblah is a pain

Also, with the constant as ALL_CAPS_LIKE_THIS fits more with the warcraft event natives, which is probably a good thing to keep :)

Anitarf 04-08-2009 02:47 PM

Quote:

Originally Posted by Rising_Dusk
I think it might be better if instead of RegisterFinishResponse(ABILITY_ID, function OnCast), it had something like RegisterSpellResponse(ABILITY_ID, RESPONSE_TYPE_FINISHED, function OnCast).

Don't approve the resource, then, since once it's approved I need to maintain backwards compatibility and there's no way I can ever change that, and I'm perfectly willing to do that if enough people think that makes for a better user interface, after all it would be a pretty lame public resource if people didn't use it because they didn't like the API. So, I'm moving this back to submissions to get more opinions.

Sinnergy 04-08-2009 02:58 PM

nice, does this works like vexorian's caster system spell events? which i stopped using because it seems like there is a blizzard bug which causes my spell to malfunction? or is this better?

Vexorian 04-08-2009 03:05 PM

Quote:

I wondered when someone would submit something like this. It definitely has its uses, but I don't think you should make "marginal speed gain" a selling point (and considering the presence of cache and .evaluate, I'm actually skeptical of that claim in the first place, not that it matters either way in this kind of system).
As multiple triggers mean at least multiple Evaluate calls, I wouldn't be so skeptical.

I don't like the blizz event response-like madness or that it has to save all event responses everytime, even though most of them are not needed or are empty.

If the syntax for using event responses will be the ugly blizz-like one, I'd prefer to just use blizz' event responses rather than have to use those new hard to remember event responses that also consume extra time during events, I mean really, at least it would make it easier to port old spells to this.

Not a fan of making it SpellEvent_RegisterEventResponse instead of just RegisterEventResponse, it is a little large, and public stuff are already non-scoped, but oh well.

emjlr3 04-08-2009 03:19 PM

why the need for the SpellEvent prefix ?

Rising_Dusk 04-08-2009 04:17 PM

Quote:

Originally Posted by Anitarf
Don't approve the resource, then, since once it's approved I need to maintain backwards compatibility and there's no way I can ever change that, and I'm perfectly willing to do that if enough people think that makes for a better user interface, after all it would be a pretty lame public resource if people didn't use it because they didn't like the API. So, I'm moving this back to submissions to get more opinions.

You wouldn't lose backwards compatibility by adding a function call with different parameters, though. There'd be nothing to remove. I guess if you removed the public prefix for those functions, then it might be different.

You also still haven't fixed the spelling of library in the beginning of the script.

Anitarf 04-10-2009 12:57 PM

Quote:

Originally Posted by Vexorian
I don't like the blizz event response-like madness or that it has to save all event responses everytime, even though most of them are not needed or are empty.

Well, unless I require users to specify for every single spell what event responses it is expected to use (which I don't think is very practical), there's no way to avoid this. I could also make global switches for which event responses are used but in that case, as soon as one spell requires the targeted destructable, the system would have to call that event response on all spell events. In either of these two cases, you'd need a bunch of if statements checking whether an event response should be used, so it wouldn't be considerably faster than calling all event response functions indiscriminately every time.

There's also the option of only storing the event responses that don't work on the Finish and EndCast events and for the rest letting the user call the natives as needed, but the problem is that the only event responses that work on those events are GetSpellAbilityId (which the script needs to call anyway) and GetTriggerUnit (which pretty much every spell needs to call), so it's both faster to store those too as well as it standardizes all event responses to the same new format.

As for this format, I think wrapper functions which resemble the original event responses and get inlined anyway are better than direct struct access; I could do something with methods/method operators, where the user functions would take the struct as a parameter and then users would call them, something to the effect of set u = GetSpell.TargetUnit()//optional@ , but that doesn't look particularly different from wrapper functions to me.

Quote:

If the syntax for using event responses will be the ugly blizz-like one, I'd prefer to just use blizz' event responses rather than have to use those new hard to remember event responses that also consume extra time during events, I mean really, at least it would make it easier to port old spells to this.
Well, first of all, do event responses still work when you call a function via .evaluate/.execute? Second, how would reading a global array consume more time than calling an event response? Third, in neither of the previous two points is an issue, what's stopping you from using the original natives anyway?

Well, in any case, I'm open for syntax suggestions, that's why this is here.

Quote:

Not a fan of making it SpellEvent_RegisterEventResponse instead of just RegisterEventResponse, it is a little large, and public stuff are already non-scoped, but oh well.
Well, if the function names are specific enough to be unlikely to cause conflicts then the public keyword isn't needed, but on the other hand if you decide to use the public keyword you can shorten the function name. SpellEvent_RegisterEventResponse can easily be shortened to SpellEvent_RegisterResponse, while RegisterEventResponse alone is kinda too general and would need to be lengthened to RegisterSpellEventResponse anyway, which in the end brings us to about the same length. Now the only question is whether the underscore is too much of an aesthetic problem.


Quote:

Originally Posted by Rising_Dusk
You wouldn't lose backwards compatibility by adding a function call with different parameters, though. There'd be nothing to remove. I guess if you removed the public prefix for those functions, then it might be different.

I'm not really a fan of having two sets of functions that do the same thing. If there were some functional difference that'd be one thing but to have two functions that only differ in style is too much.

Quote:

You also still haven't fixed the spelling of library in the beginning of the script.
Oh, right, fixed now.

Vexorian 04-10-2009 02:13 PM

Quote:

set u = GetSpell.TargetUnit()
That's still blizz like stuff. I would wish this used arguments, you know, if you specified what responses you'd like, then arguments would be possible, maybe too hard.

Quote:

Well, first of all, do event responses still work when you call a function via .evaluate/.execute?
All but GetTriggeringTrigger.

Quote:

Second, how would reading a global array consume more time than calling an event response?
I don't care about speed, it is more about having to remember brand new event response names.

Quote:

Well, unless I require users to specify for every single spell what event responses it is expected to use (which I don't think is very practical)
I actually think it is practical, could use bitflags, well bitflags could be a little slow.

What annoys me the most is thelocation call, the others are in theory not that bad. Also, why store it as location? It could at least be helpful if it converted it to x,y already. It would be nice if you could tell it not to call the location thing.

Quote:

local spellEvent s=spellEvent.allocate()
set s.spellId=GetSpellAbilityId()
set s.caster=GetTriggerUnit()
set s.target=GetSpellTargetUnit()
set s.targetItem=GetSpellTargetItem()
set s.targetDest=GetSpellTargetDestructable()
set s.targetLoc=GetSpellTargetLoc()
set s.finished=false

Well, most of the times, you don't need any of them and could use the responses directly (SPELL_EFFECT/CAST) , the other times, you don't need all of them.

Guess spellId and caster are constants, but there are only 4 other things, it is not that bad:

Collapse JASS:
struct targetType extends array
    static constant integer Unit         = 1
    static constant integer Item         = 2
    static constant integer Destructable = 4
    static constant integer Widget       = 7
    static constant integer Point        = 8
endstruct

//...
call SpellEvent_RegisterFinishResponse(ABILITY_ID,  targetType.Unit +targetType.Item) //maybe it hits both things?

//...
            local spellEvent s=spellEvent.allocate()
            set s.spellId=GetSpellAbilityId()
            set s.caster=GetTriggerUnit()
            if ( flag >= 8) then
                  set s.targetLoc=GetSpellTargetLoc()
                  set flag=flag-8
            endif
            if ( flag >= 4) then
                  set s.targetDest=GetSpellTargetDestructable()
                  set flag=flag-4
            endif
            if ( flag >= 2) then
                  set s.targetItem=GetSpellTargetItem()
                  set flag=flag-2
            endif
            if ( flag >= 1) then
                  set s.target=GetSpellTargetUnit()
            endif

            set s.finished=false
            set spellEvent.casterTable[s.caster]=integer(s)
            return s


Just suggesting here.

Anitarf 04-10-2009 03:04 PM

Quote:

Originally Posted by Vexorian
That's still blizz like stuff. I would wish this used arguments, you know, if you specified what responses you'd like, then arguments would be possible, maybe too hard.

I'm not sure I understand. You mean the functions following the function interface would take the event responses as arguments? That'd be either a very long argument list or you'd need different functions for different types of spells (and a point-or-unit target spell like shockwave would still have a long argument list). I thought you hated long argument lists.

Quote:

I don't care about speed, it is more about having to remember brand new event response names.
Then how's it being Blizz like a problem? That last example was a Blizzard native with a "." in the middle. It was essentially the same thing, you can even use auto-completion and then add the dot after GetSpell (well, except for the GetSpell.TargetX/Y which wouldn't have a native counterpart). It's as intuitive as it can get.

Quote:

I actually think it is practical, could use bitflags, well bitflags could be a little slow.

What annoys me the most is thelocation call, the others are in theory not that bad. Also, why store it as location? It could at least be helpful if it converted it to x,y already. It would be nice if you could tell it not to call the location thing.
I don't see why would it annoy you. To get rid of it you need an additional GC call (to get the flag from the spellID in your example) and depending on the map there might be plenty of cases where after that you still need to call GetSpellTargetLoc. So the speed gain from doing it like this is marginal at best and you just said you don't care about speed.

What about something like this:
Collapse JASS:
        set s.targetUnit = GetSpellTargetUnit()
        if s.targetUnit == null then
            set s.targetDest = GetSpellTargetDestructable()
            if s.targetDest == null then
                set s.targetItem = GetSpellTargetItem()
                if s.targetItem == null then
                    set l = GetSpellTargetLoc()
                    set s.targetX = GetLocationX()
                    set s.targetY = GetLocationY()
                    call RemoveLocation(l)
                    set l = null
                endif
            endif
        else //optional, would also be there for items and destructables but I don't feel like typing it
            set s.targetX = GetUnitX()
            set s.targetY = GetUnitY()
        endif

Vexorian 04-10-2009 03:11 PM

it is annoying, it does handle allocation and deallocation, that's annoying.

It is vJass, you only need one gc call.

Anitarf 04-10-2009 03:28 PM

Quote:

Originally Posted by Vexorian
it is annoying, it does handle allocation and deallocation, that's annoying.

Why?

Quote:

It is vJass, you only need one gc call.
True, but it complicates the library somewhat.

Anyway, what about my last example? Would a GetSpellTargetLoc() call for point-target spells (where it's needed anyway) and no-target spells (where it isn't), but not for widget-target spells be acceptable?

Vexorian 05-07-2009 03:24 PM

Ok, do the Loc stuff.

Anitarf 05-08-2009 01:40 PM

Updated. The script now doesn't create a location on widget targetting spell casts.

I haven't quite decided on the user syntax yet, the current script includes two versions, GetAbilityId() and GetSpell.AbilityId(), another possibility would be SpellEvent.AbilityId. I'd like to decide on one of them and delete the rest before this gets approved. Opinions?

grim001 05-08-2009 01:58 PM

Last one.

Rising_Dusk 05-08-2009 02:15 PM

Yeah, scrap the public prefixes. I'd never use this just because of that.

Anitarf 05-11-2009 10:58 AM

Quote:

Originally Posted by Litany
GetAbilityId, GetAbilityTargetUnit, IsAbilityFinished, etc. Blizzard is inconsistent with Spell and SpellAbility, but that doesn't mean you have to be.

Inconsistent? It seems to me that "Ability" is used when dealing with any ability, including passives, while "Spell" is used when dealing with spell events. Hence UnitAddAbility (can add any ability, not just spells), GetSpellTargetUnit (only works on spell cast) and GetSpellAbilityId (gets a generic ability id on a spell event). That seems perfectly consistent to me.

Quote:

Consistent use of Ability in the function names also removes the need for the public prefixes, which are incredibly ugly.
Quote:

Originally Posted by Rising_Dusk
Yeah, scrap the public prefixes. I'd never use this just because of that.

Okay.

So, I've decided to make the UI work either like this: local unit c = SpellEvent.CastingUnit or like this: public function interface Response takes integer abilityId, unit castingUnit, unit targetUnit, destructable targetDest, item targetItem, real targetX, real targetY returns nothing I don't think anyone will like the latter, so I'll just write the former.

wraithseeker 05-11-2009 11:07 AM

Doesn't this do the job although you guys banned that user.

Anitarf 05-11-2009 12:56 PM

Quote:

Originally Posted by wraithseeker
Doesn't this do the job

No.

Well, based on my testing there is no way for a unit to start casting a new spell without first stopping the casting of the previous spell, so the safety check in the EndCast function is not needed and will be removed in the next version.

Edit:
Quote:

Originally Posted by Litany
I find combining spell and ability in general to be inconsistent. They're treated as virtual synonyms in WC3, so the distinction here makes no sense. And of course, they actually aren't synonyms, which makes it even worse. Ensnare is not a spell, so why should the word "spell" figure in the event responses?

It may not be consistent with our understanding of the word (which would mean specifically magical abilities) but the word's use within WC3 natives is internally consistant as it is used to refer to activated abilities, as such it is also not synonymous to the word "Ability" which can refer to any ability, not just an activated one.

Edit 2: Well, I just realised I can't make static method operators so I guess I'll need to come up with a different UI.

Anitarf 05-14-2009 02:30 PM

Updated, I managed to get the syntax I wanted although it required some tweaking to maintain proper encapsulation. The user could in theory still call .destroy on the struct since I can't make that method private but the system can survive that, the user would eventually get an error message about trying to destroy a null struct.

I realize now I forgot to add the method operator for TargetLoc, gotta run now so I'll fix that later.

Anitarf 05-16-2009 12:06 AM

Quote:

Originally Posted by Anitarf
I realize now I forgot to add the method operator for TargetLoc, gotta run now so I'll fix that later.

Okay, I fixed this now, also the usage example was still using the old syntax in one place, fixed that too. This should be it, I think.

Tyrande_ma3x 05-16-2009 05:38 AM

> SpellEvent.CastingUnit
Is there any way this could become TriggerUnit? I think most people don't usually use GetCastingUnit() and it's going to be a bit hard with all those switches between names.

Anitarf 05-18-2009 12:16 PM

Casting unit is more descriptive, though. And what switches between names, all event responses are copies of natives with "GetSpell" replaced with "SpellEvent." (minus those responses that are new), so there's only one switch.

rain9441 05-18-2009 03:35 PM

I use pretty much the same concept for registering for events for abilities. Just want to quickly add that you also have access to the OrderID of the casting unit at some stages (not all) which is very handy in some circumstances.

I dunno, it allows me to say when a unit casts a spell and some conditions exist, create a dummy unit, add the ability to the dummy unit, set the level of the ability to that of the casting unit, and order the dummy unit to (orderid) the target again, or the x/y coords again, or the item again, or "immediately", or at a random unit within 300 of target, etc, etc, etc.

Ability Level may also be useful, as the level when casted may not be the level 1 second later.

Also, any reason the EVENT_PLAYER_UNIT_SPELL_CHANNEL event is unsupported?

Thirdly, why set a global variable to the current spell event when you can pass the spell event to the Response function?

Anitarf 05-18-2009 03:59 PM

Spell level and orderid are useful information, I'll look into including them in the event responses. The level itself might not be as useful since if it's a channeling spell (which is the only case I can think of where you could increase the level before the spell finishes) you will likely create a struct when the channeling starts anyway and you can store the level there, but since it's the kind of event response function that will likely get called anyway there's no loss of performance if it's included.

In my experience, EVENT_PLAYER_UNIT_SPELL_CHANNEL doesn't do anything special, it just seems to run after the cast event for all spells. If there is some significant difference, I might include it.

I use a global variable instead of passing it as a parameter because it simplifies the function interface somewhat ("takes nothing returns nothing" is easier to remember) and you don't have to pass the struct to any functions you call.

Vexorian 05-21-2009 11:51 PM

channel IS useless. Effect is better there.

Quote:

I use a global variable instead of passing it as a parameter because it simplifies the function interface somewhat ("takes nothing returns nothing" is easier to remember) and you don't have to pass the struct to any functions you call.
ah the sound of losing abstraction over worthless code length saving.

but well, it has been in the submit forum for enough time approved...

Anitarf 05-24-2009 07:35 PM

Alright, I fixed a minor bug that could cause a unit-target spell to keep the TargetDestructable value from a previously cast destructable-target spell.

Also, I did a small tweak to facilitate spell transmutation, check out the second usage example for more information.

Viikuna- 05-24-2009 09:05 PM

Doesnt _CHANNEL trigger before casting time? I have used it to change units casting animation in some cases.

( Dont know if _CAST does the same thing, but I was under impression that _CHANNEL triggers before _CAST )

Anitarf 05-24-2009 09:12 PM

Channel and cast are pretty much the same thing as far as I know.

Bobo_The_Kodo 07-07-2009 04:51 AM

_CHANNEL -> cast time -> _CAST -> cast point -> _EFFECT -> cast backswing -> _FINISH

Anitarf 07-07-2009 10:38 AM

Curiously, FlameStrike doesn't work this way as it appears to be hardcoded to treat casting time differently than any other spell for some reason, but I have been able to confirm what you said by giving a casting time to a normal spell. Since SPELL_CHANNEL does not appear to be useless afterall, I added it to the library. Thanks for the report.

Bobo_The_Kodo 07-10-2009 08:54 AM

How does blink work? It doesn't use the normal cast point for heroes either

Themerion 07-16-2009 08:00 PM

What I really don't like about your system, Anitarf, is that it tend to do to much in the majority of cases. Add some optimization possibilities, and I shall bow before you.

Some suggestions (which would be compatible with your current syntax):

First suggestion is to make a struct-like function from two arrays. Storing boolean wantStruct and function pointer Response. Then we link table to the "struct" instead of the response ability (enabling us to bypass the struct population if it isn't needed).

Collapse JASS:
set SpellEvent_wantStruct=false
call RegisterSpellEffectResponse('PHAT',Lewt)

Collapse JASS:
globals
    private integer array $name$N
    private wantStruct array $name$wantStruct
    private Response array $name$Response
endglobals

function RegisterSpell$name$Response takes integer spellId, Response r returns nothing
        if spellId==0 then
            set $name$CallList[$name$CallCount]=r
            set $name$CallCount=$name$CallCount+1
        else
          set $name$N=$name$N+1
      // from global variable
          set $name$wantStruct[$name$N]=wantStruct
          set wantStruct=false
          set $name$Response[$name$N]=r
            set $name$Table[spellId]=$nameN$
        endif
    endfunction

Second suggestion is to be able to turn off the global events. All maps don't have use for them ( <=> there is at least one map which doesn't :)

Collapse JASS:
// And what about the global...

private constant boolean USE_ALL_SPELL_EVENTS

// for avoiding the zero-looping. It will get inlined away (I hope!?)

Collapse On Run:
function $NAME$WhateverSpellEffectIsTriggering takes nothing returns nothing
local integer N = $NAME$table[GetSpellAbilityId()]
if $NAME$wantStruct[N] then
    // do init structs
endif
if USE_ALL_SPELL_EVENTS then
   // Do the loop in the 'zero-list'.
endif
call $NAME$responses[N].execute()
 // Perhaps execute instead? :)
 // Just a suggestion.
endfunction

Anitarf 07-16-2009 10:40 PM

The benefit of having all spell event responses available on all spell events is significant, the speed gained by removing it is not.

I could write another paragraph on why the feature shouldn't be optional, but since the speed gain is not significant in the first place, I don't see a reason to spend more time explaining this.

Besides, the bj_wantDestroyGroup-esque syntax is ugly.

If your particular map doesn't require this feature then you can always delete the event responses code from the library yourself, nobody is forcing you to only use the code as provided.

The second optimization you suggested is meaningless, an if statement isn't faster than an exitwhen statement in any meaningful way.

Themerion 07-17-2009 09:15 AM

Quote:

Originally Posted by Anitarf
The benefit of having all spell event responses available on all spell events is significant, the speed gained by removing it is not.


I'm not quite sure if you mean variables or callback-functions. If you mean that I benefit from having the target unit on spell finish [for spell x], when [spell x] only use spell cast, then you are simply wrong. If you thought that I wanted you to remove a callback-function (such as spell finish), then you are wrong as well :)

Quote:

Originally Posted by Anitarf
I could write another paragraph on why the feature shouldn't be optional, but since the speed gain is not significant in the first place, I don't see a reason to spend more time explaining this.


Well. The speed gain is not significant, and it might never matter, BUT IT FEELS BAD WITH ALL THIS STUFF RUNNING WHEN I DON'T NEED IT TO! :emote_wink:

Quote:

Originally Posted by Anitarf
If your particular map doesn't require this feature then you can always delete the event responses code from the library yourself, nobody is forcing you to only use the code as provided.


Some of the spells in map might need to have event response variables at spell finish, for them I want struct population. But certainly most of my spells won't (which is why I want to be able to bypass the struct in those cases).

Quote:

Originally Posted by Anitarf
Besides, the bj_wantDestroyGroup-esque syntax is ugly.


Yeah, but I wanted to make a suggestion that allows you to keep your current syntax (I hate when people tell me: Do that, and that! But not how it's actually supposed to happen).

Quote:

Originally Posted by Anitarf
The second optimization you suggested is meaningless, an if statement isn't faster than an exitwhen statement in any meaningful way.


I even wrote that it should be inlined/optimized away/whatever you will call it. I just assumed that if VexMapOptimizer finds if false then, it will remove that statement.

Anitarf 07-17-2009 10:08 AM

Quote:

Originally Posted by Themerion
Well. The speed gain is not significant, and it might never matter, BUT IT FEELS BAD WITH ALL THIS STUFF RUNNING WHEN I DON'T NEED IT TO! :emote_wink:

Your bad feelings have no relevance to the objective worth of this proposed optimization. It remains worthless. Even if it were significant, I have good reasons to insist that all code using this library uses it's event responses. Again, if you disagree, feel free to add the optimization to your own map yourself.

Quote:

I'm not quite sure if you mean variables or callback-functions. If you mean that I benefit from having the target unit on spell finish, when I only use spell cast, then you are simply wrong. If you thought that I wanted you to remove a callback-function (such as spell finish), then you are wrong as well :)
Quote:

Some of the spells in map might need to have event response variables at spell finish, for them I want struct population. But certainly most of my spells won't (which is why I want to be able to bypass the struct in those cases).
Emphasis mine. Your statements contradict each other. Please revise your message so it is internally consistent.

Quote:

I even wrote that it should be inlined/optimized away/whatever you will call it. I just assumed that if VexMapOptimizer finds if false then, it will remove that statement.
One if statement vs no if statement is still insignificant. This isn't a physics system, please stop asking that it be optimized as one.

Themerion 07-17-2009 10:16 AM

Quote:

Originally Posted by Anitarf
Emphasis mine. Your statements contradict each other. Please revise your message so it is internally consistent.


If you mean that I benefit from having the target unit on spell finish for spell X, when spell X only use spell cast, then you are simply wrong.

Some of the spells in map might need to have event response variables at spell finish , for them I want struct population. But certainly most of my spells won't (which is why I want to be able to bypass the struct in those cases).

Quote:

Originally Posted by Anitarf
One if statement vs no if statement is still insignificant. This isn't a physics system, please stop asking that it be optimized as one.


I see opportunity for less code to be run, and I think it would be a good thing. You have good reasons for not sharing my opinion, so I shall just drop this. Sorry to have been bothering you.

Quote:

Originally Posted by Anitarf
Again, if you disagree, feel free to add the optimization to your own map yourself.


Good point.

This might be the wrong place to post this, but what the hell :P Feel free to remove CastEvent from the submissions.

Anitarf 07-17-2009 12:57 PM

Quote:

Originally Posted by Litany
One suggestion Ani: you really should initialize some of the struct variables to null or 0. If I target, say, an item or destructable with one ability, then that event instance gets recycled by another ability that targets a unit, the unit-targeted ability will still return the previous target item or destructable. This probably won't matter for the ability's registered event--after all, why would someone check the target item of an ability that targets units?--but events registered to id 0, which run for all abilities, might make such checks.

Ouch, that's what I get for maintaining two copies of the script during early development. I definitely fixed this bug before (I even posted about it) but somehow the fix got lost between updates. Will correct this immediately.

Quote:

It might also be worth initializing TargetX/Y to the caster for abilities that take no target, although in the case of summons they should really be initialized to the summoned unit.
I noticed during my testing that some no-target abilities (like thunder clap) actually do give the caster's coordinates as the target point, while others don't (they return a null location which then translates into coordinates 0,0; I wonder how the new natives in 1.24 work in such situations; I should eventually port this to use them), so maybe this distinction would represent meaningful information to events registered to id 0. That and the problem with summons are the reasons why I didn't bother implementing something like this.


All times are GMT. The time now is 05:44 PM.

Powered by vBulletin (Copyright ©2000 - 2019, Jelsoft Enterprises Ltd).
Hosted by www.OICcam.com
IT Support and Services provided by Executive IT Services