GetSpellTargetUnit() & GetSpellAbilityId() malfunction within itself

[DracoL1ch]

Whenever you're using GetSpellTargetUnit() & GetSpellAbilityId() , you have to pre-save them into some other variable everytime you're going to cast another ability which can be detected by this trigger. Basically, if your trigger catches every spell casted, it will fuck up if you cast another spell in response to the event.

Code:

function Earthshaker_Aftershock_Effect2 takes unit u, integer lvl returns nothing
    local unit d
    call BJDebugMsg("#"+I2S(lvl)+": starting dummy spell: target="+GetUnitName(GetSpellTargetUnit())+" id="+I2S(GetSpellAbilityId()))
    set d=CreateUnit(GetOwningPlayer(u),'ewsp',GetUnitX(u),GetUnitY(u),0)//e00E -> Spellcaster
    call UnitAddAbility(d,'AOws')
    call IssueImmediateOrder(d,"stomp")
    call BJDebugMsg("#"+I2S(lvl)+": finishing dummy spell: target="+GetUnitName(GetSpellTargetUnit())+" id="+I2S(GetSpellAbilityId()))
    set d=null
endfunction

function GenericOnCastEvent takes nothing returns nothing
    local integer i=GetSpellAbilityId()
    local unit u=GetTriggerUnit()
    local unit target=GetSpellTargetUnit()
    local integer c=GetTriggerEvalCount(GetTriggeringTrigger())
    if IsUnitType(u,UNIT_TYPE_HERO)then
        call BJDebugMsg("    ")
        call BJDebugMsg("    ")
        call BJDebugMsg("------   NEW HERO CAST -----")
    endif
    call BJDebugMsg("#"+I2S(c)+": casting spell: target="+GetUnitName(GetSpellTargetUnit())+" id="+I2S(GetSpellAbilityId()))
    if IsUnitType(u,UNIT_TYPE_HERO)then
        call Earthshaker_Aftershock_Effect2(u,c)
    endif
    call BJDebugMsg("#"+I2S(c)+": after casting: target="+GetUnitName(GetSpellTargetUnit())+" id="+I2S(GetSpellAbilityId()))
    set u=null
    set target=null
endfunction


function InitTrig_Untitled_Trigger_002 takes nothing returns nothing
    local trigger t=CreateTrigger()
    call TriggerRegisterAnyUnitEventBJ(t,EVENT_PLAYER_UNIT_SPELL_EFFECT)
    call TriggerAddCondition(t,Condition(function GenericOnCastEvent))
    call BJDebugMsg("Throw a bolt onto enemy and look")
endfunction

Example:

Dummy cast in red, broken natives in yellow. It's over as soon as dummy cast get registered by the same trigger, no recovery.

Reason of that broken behavior - those natives were added in hurry, unlike most other getters they do not directly use a storage of GetTriggerThings, but follow absolutely different algoritm. Therefore they cannot be "stacked" as GetTriggerUnit() does.

This behavior is broken and should be fixed.

 

[Wareditor]

This is old news.
It should be fixed for GUI users but it shouldn't be an issue for any jass coder as it's very bad practice to have code like that.

 

[DracoL1ch]

This is old news.
It should be fixed for GUI users but it shouldn't be an issue for any jass coder as it's very bad practice to have code like that.

old news, still not fixed even tho patches went through. And no, its not a bad practice, I wanna my spells being reflected faster than it takes effect, witohut rewriting everything. In fact you're trying to defend a thing which is broken from the start.

 

[Wareditor]

"I wanna my spells being reflected faster than it takes effect"
What do you mean by that ?

Making two functions calls, while you could only make one and store the value returned by the function into a local variable, is indeed bad practice. Any response getter should only be called once per response, calling it more than one time is a waste. As GUI users don't have direct access to local variables and must call the getter each time then want to reference that unit, this should be fixed for them.

 

[DracoL1ch]

You can have tons of different triggers, each of them would like to obtain data, building up everything around a single trigger is only good when you're starting from scratch, not when you coming into any live project which had years of development and started with GUI.

 

[Wareditor]

I don't understand what you are talking about. Never I mentioned building things around one trigger.
Here is a version that fix your issue, using local variables.

 

[DracoL1ch]

Sigh. If there would be 2 triggers which detects PLAYER_UNIT_SPELL_EFFECT event, the one located below would also receive null and 0.
Why its bad? Because other getters work just like expected, staking data into a pile.

 

[Wareditor]

I would love you to show me that issue taking place because when I added a few triggers to the test map, everything was working perfectly.

 

[DracoL1ch]

So, it become broken withing single trigger thread then? Good to know, still bugged, but at least workaroundable.

 

[actboy168]

Thanks for informing. I‘m thinking about fixing it in my editor.

 

[GhostWolf]

Not sure what the surprise is, most of the getters accessible also via GUI are not per trigger-invocation. You don't need to cast another ability recursively to see this, just add a wait and cast two times. This is the standard "why you use locals in GUI" case. In pure Jass you'd generally set it to a local either way, just because caching results is standard.

 

[DracoL1ch]

Not sure what the surprise is, most of the getters accessible also via GUI are not per trigger-invocation. You don't need to cast another ability recursively to see this, just add a wait and cast two times. This is the standard "why you use locals in GUI" case. In pure Jass you'd generally set it to a local either way, just because caching results is standard.

wrong, there are set of "default" getters which are designed to work recursively w/o any limitations. You have GetTriggerPlayer()/Unit(), GetEnumUnit() and other similar things wherever you want with no issues. While these 2 is broken by design, somehow

 

[Kam]

Thank you for posting this and providing a test map.

 

[GhostWolf]

Yes, most of the getters are global, didn't say all of them are. But this has been old news when I wrote my first custom script :thinking:

 

[DracoL1ch]

Yes, most of the getters are global, didn't say all of them are. But this has been old news when I wrote my first custom script :thinking:

Sigh. As I told, that dedicated to blizzard.