[Solved] Any issues with this Revive Hero Death Timer?

[plumsy460]

Hey I just wanted some feedback on this trigger below. It appears to work perfect ingame but since I'm new to making triggers purely of custom scripts I would be very happy to get some feedback. I am concerned about leaks.

Do local variables LEAK? Do you have to delete them or do they delete themselves like they do in SC2?

Does the GetRectCenter(gg_rct_Shop), true ) cause a leak? (2nd bottom function)

Thank you for looking into this

• TimerDie Expires
  • Events
  • Conditions
  • Actions
    • Custom script: local timerdialog WINDOW
    • Custom script: local integer HEROWAIT
    • Custom script: local timer OURTIMER
    • Custom script: local unit OURHERO
    • Custom script: set OURHERO = GetDyingUnit()
    • Custom script: set HEROWAIT = (20)
    • Custom script: set OURTIMER = CreateTimer()
    • Custom script: call StartTimerBJ( OURTIMER, false, ( I2R(HEROWAIT) ))
    • Custom script: call CreateTimerDialogBJ( OURTIMER, GetPlayerName(GetOwningPlayer(OURHERO)) )
    • Custom script: set WINDOW = GetLastCreatedTimerDialogBJ()
    • Custom script: call TimerDialogDisplayForPlayerBJ( true, WINDOW, GetOwningPlayer(OURHERO) )
    • Custom script: call PolledWait( HEROWAIT )
    • Custom script: call ReviveHeroLoc(OURHERO, GetRectCenter(gg_rct_Shop), true )
    • Custom script: call DestroyTimerDialog(WINDOW)

 

[IcemanBo]

Created objects like GetRectCenter(gg_rct_Shop) do indeed leak. And also some locals have to be nulled. Have a look Memory Leaks.

 

[JC Helas]

Created objects like GetRectCenter(gg_rct_Shop) do indeed leak. And also some locals have to be nulled. Have a look Memory Leaks.

sir how about those unable to null variables , like booleans?

 

[IcemanBo]

Only objects that you can create and destroy have to be nulled. booleans don't have to be nulled, or else what.

 

[plumsy460]

Thank you for the link IcemanBo +rep. From what I can tell I simply need to do "set localvar = null" on the different local variables I made.

I tried making a "local location LOC" variable but I can't seem to make it work with the function "call ReviveHeroLoc(OURHERO, LOC), true". Does that function only work with rect?

 

[Pyrogasm]

The "true" argument needs to be inside the call: call ReviveHeroLoc(OURHERO, LOC, true)

 

[plumsy460]

The "true" argument needs to be inside the call: call ReviveHeroLoc(OURHERO, LOC, true)

Thank you Pyrogasm.

[EDIT] I hope I've made it right this time. I added a rect and a point and managed to put it together with the call ReviveHeroLoc. I also nulled all the leaks I could see. Below you will see what I have so far. It works ingame. But is it approvable or does it still have leaks?

• TimerDie Expires
  • Events
  • Conditions
  • Actions
    • -------- Local Var --------
    • Custom script: local timerdialog WINDOW
    • Custom script: local integer WAIT
    • Custom script: local timer TIMER
    • Custom script: local unit HERO
    • Custom script: local rect REC
    • Custom script: local location LOC
    • -------- Local Var setup --------
    • Custom script: set HERO = GetDyingUnit()
    • Custom script: set WAIT = (20)
    • Custom script: set TIMER = CreateTimer()
    • Custom script: set REC = (gg_rct_Shop)
    • Custom script: set LOC = GetRectCenter(REC)
    • -------- Functions --------
    • Custom script: call StartTimerBJ( TIMER, false, ( I2R(WAIT) ))
    • Custom script: call CreateTimerDialogBJ( TIMER, GetPlayerName(GetOwningPlayer(HERO)) )
    • Custom script: set WINDOW = GetLastCreatedTimerDialogBJ()
    • Custom script: call TimerDialogDisplayForPlayerBJ( true, WINDOW, GetOwningPlayer(HERO) )
    • Custom script: call PolledWait( WAIT )
    • Custom script: call ReviveHeroLoc(HERO, LOC, true )
    • Custom script: call DestroyTimerDialog(WINDOW)
    • -------- Nulls --------
    • Custom script: set WINDOW = null
    • Custom script: set HERO = null
    • Custom script: set TIMER = null
    • Custom script: set REC = null
    • Custom script: set LOC = null

 

[IcemanBo]

• Timer itself is not destroyed, which nullifies the nulling's purpose. (Only the timer windows is destroyed)
• Same for the location.

Else, no leaks, but just some other notes if you're interested.

Instead of using location, you might use:

native GetRectCenterX takes rect whichRect returns real
native GetRectCenterY takes rect whichRect returns real

.. for example GetRectCenterX(gg_rct_Shop) is the center x-coordinate of the shop rect.
and then native ReviveHero takes unit whichHero, real x, real y, boolean doEyecandy returns boolean has to be used for revive, which works with cooridnates.

Do you know about timer usage in JASS? It could help to remove the PolledWait(), which is not as clean as timers, if one has a look at PolledWait()'s implementation.
Crash Course 10 might be a starter excercise for timers.

EDIT:

Is there a reason you don't convert to JASS? It will be much more readable, and easier to enhance.

 

[plumsy460]

Is there a reason you don't convert to JASS? It will be much more readable, and easier to enhance.

First off, thank you for your insightful help!

By all means, I would love to use JASS. But it is a first timer for me.

I'm definatly curios to try and make this trigger with JASS so I'll give it a shot. I tried implementing your code but I must have done it wrong... The hero didn't come back. I'm gonna keep trying to see what I did wrong and make it work. I understand PulledWait is bad to use but I lack insight on other alternatives.

[EDIT] If I understand correctly, when you destroy a localvar there's no point in nullifying it? This is what I have made so far, I hope all the leaks are gone now:

// Local Var
    local timerdialog WINDOW
    local integer WAIT
    local timer TIMER
    local unit HERO
    local rect REC
    local location LOC
    // Local Var setup
    set HERO = GetDyingUnit()
    set WAIT = (20)
    set TIMER = CreateTimer()
    set REC = (gg_rct_Shop)
    set LOC = GetRectCenter(REC)
    // Functions
    call StartTimerBJ( TIMER, false, ( I2R(WAIT) ))
    call CreateTimerDialogBJ( TIMER, GetPlayerName(GetOwningPlayer(HERO)) )
    set WINDOW = GetLastCreatedTimerDialogBJ()
    call TimerDialogDisplayForPlayerBJ( true, WINDOW, GetOwningPlayer(HERO) )
    call PolledWait( WAIT )
    call ReviveHeroLoc(HERO, LOC, true )
    // Nulls
    call DestroyTimerDialog(WINDOW)
    call DestroyTimer(TIMER)
    call RemoveLocation (LOC)
    set WINDOW = null
    set HERO = null
    set TIMER = null
    set REC = null
    set LOC = null

 

[IcemanBo]

nulling is in Tequnique of removal at reference leaks.

And just keep trying! With enough examples and lookups in other one's it will definitely help. : )

 

[plumsy460]

nulling is in Tequnique of removal at reference leaks.

And just keep trying! With enough examples and lookups in other one's it will definitely help. : )

Thank you for the encouragement This is going to be engaging for me.

Just to be sure, I wanna go back to the trigger I previously posted (I did some minor changes with the null/destroy order). I hope it's leak free at this point? Any suggestions on improvements I'd be happy to take. But better post the whole example for your suggestion, otherwise I won't necessarily understand how to implement it. I really appreciate it! Thank you.

 

[Pyrogasm]

No, you do have to keep the set TIMER = null in that function, but the DestroyTimer() is going to happen in your timer callback function. Which you don't have one of yet because you're using StartTimerBJ() which doesn't let you choose one. Instead you want:

native TimerStart           takes timer whichTimer, real timeout, boolean periodic, code handlerFunc returns nothing

//like this
call TimerStart(TIMER, WAIT, false, function YourCallback)
//then
function YourCallback takes nothing returns nothing
  set TIMER = GetExpiredTimer()
  call DestroyTimer(TIMER)
  set TIMER = null
endfunction

 

[IcemanBo]

I hope it's leak free at this point?

In the link above, there's an example for when to null something. And your shown code is not same as the "good example" there. ;P

 

[plumsy460]

No, you do have to keep the set TIMER = null in that function, but the DestroyTimer() is going to happen in your timer callback function. Which you don't have one of yet because you're using StartTimerBJ() which doesn't let you choose one. Instead you want:

native TimerStart           takes timer whichTimer, real timeout, boolean periodic, code handlerFunc returns nothing

//like this
call TimerStart(TIMER, WAIT, false, function YourCallback)
//then
function YourCallback takes nothing returns nothing
  set TIMER = GetExpiredTimer()
  call DestroyTimer(TIMER)
  set TIMER = null
endfunction

Thank you for writing it down for me. Is "YourCallback" the reference name of the trigger I'm using? I tried using this code as if meant to be in a trigger by itself refering to the one I already have. I also tried implementing it ontop of it but I can see I lack sufficient insight to set it up by myself. I couldn't make it work.

In the link above, there's an example for when to null something. And your shown code is not same as the "good example" there. ;P

Thank you for the reference. I did read through it. I'm afraid all I understood was to put "null" after "destroy", when you "null" something the reference is gone for the "destroy" to work. I did edit that change in the previewed trigger above.

I still have a way to go with JASS. Since this is my first attempt to use it I'll be very thankful if you can show me specificly what changes goes where. I remain unsuccesful in implementing the examples you show me.

 

[Pyrogasm]

JASS (without vJASS additions) is really just a bunch of functions and global variables. Triggers (global variables) are made by adding different events (global variables... kinda) that check the conditions (functions) and actions (functions). Everything in all scripts is also ultimately a bunch of functions, so get used to 'em. They are declared as I wrote above with other arguments instead of nothing for more complex functions. TimerStart takes a function as an argument when you call it. A code handlerFunc which translates to function TheNameYouGaveToYourFunction.

A function you wrote can call or otherwise 'use' any other function you wrote provided one of two things is true:

1. The function you're calling to is literally above the code you are calling from in the script.
   

   function a takes nothing returns nothing
     call b() //NOPE it's below this
   enfunction
   
   function b takes nothing returns nothing
     call a() //YEP it's above this
   endfunction

   
   
2. The function you're calling to is in the "Custom script" section of your map at the top left of the trigger window. And the function you're calling from is anywhere else (or is in the CS section too but is below the func).

You basically: you also have to null local handles in functions. Handles usually need to be manually cleaned up (groups, locations, timers...) which involves some Remove/Destroy-named function so they tend to coincide but even when you just refer to something with a local variable like local unit u = GetTriggerUnit() you'll have to set u = null at the end of the function.

 

[plumsy460]

I appreciate the amount of explanation you're giving me. I hate to say I feel like more help makes me more confused. I can tell there's a lot of research and technical terms to get around before hoping to understand just the basics of JASS. So maybe I shouldn't hold back on asking for the most simplistic things. This is going to make me look stupid.

You're talking about handles are leaking, but I'm not sure what handles are. I assume it's any reference to an object, like a point or a unit?
The script you showed me for a better/non-leaking way of creating a Timer is confusing to me because I don't understand where to put it. I'm still not clear if I'm supposed to replace/remove any of the functions I already have in my trigger or if I'm supposed to put the code in a seperate trigger. If I were to guess I'd say it should look something like this:

function Trig_HeroRevive_Actions takes nothing returns nothing
 // Local Var
    local timerdialog WINDOW
    local integer WAIT
    local timer TIMER
    local unit HERO
    local rect REC
    local location LOC
 // Local Var setup
    set HERO = GetDyingUnit()
    set WAIT = (20)
    set TIMER = CreateTimer()
    set REC = (gg_rct_Shop)
    set LOC = GetRectCenter(REC)
 // Functions
    call StartTimerBJ( TIMER, false, ( I2R(WAIT) ))
//Is the function above supposed to be replaced by the function below?:
//"call TimerStart(TIMER, WAIT, false, function YourCallback)"
//And also, what exactly do I write in YourCallback? "HeroRevive"?
    call CreateTimerDialogBJ( TIMER, GetPlayerName(GetOwningPlayer(HERO)) )
    set WINDOW = GetLastCreatedTimerDialogBJ()
    call TimerDialogDisplayForPlayerBJ( true, WINDOW, GetOwningPlayer(HERO) )
    call PolledWait( WAIT )
    call ReviveHeroLoc(HERO, LOC, true )
 // Nulls
    call DestroyTimerDialog(WINDOW)
    call DestroyTimer(TIMER)
    call RemoveLocation (LOC)
    set WINDOW = null
    set HERO = null
    set TIMER = null
    set REC = null
    set LOC = null
//If I understand the guide correctly this is the proper order to null/remove... handles?
endfunction
//===========================================================================
//Is this where I'm supposed to insert the rest of the code you suggested? (green code below)
function InitTrig_HeroRevive takes nothing returns nothing
// set TIMER = GetExpiredTimer()
 // call DestroyTimer(TIMER)
  // set TIMER = null
    set gg_trg_HeroRevive = CreateTrigger(  )
    call TriggerAddAction( gg_trg_HeroRevive, function Trig_HeroRevive_Actions )
endfunction

If you can copy the script I posted above and implement your changes I could see first hand how it's supposed to work

As always thank you so much for taking the time to help me

 

[IcemanBo]

It looks better already. But if you keep the rect usage, then it should be destroyed, too. Nulling here alone is not enough, too.

Here would be current code without leaks, and without rect and location usage.

function ReviveDyingHero takes nothing returns nothing
    local real duration = 20
    local unit hero     = GetDyingUnit()
    local real x        = GetRectCenterX(gg_rct_Shop)
    local real y        = GetRectCenterY(gg_rct_Shop)
    local timer clock   = CreateTimer()
    local timerdialog timer_window = CreateTimerDialogBJ( clock, GetPlayerName(GetOwningPlayer(hero)) )
  
    call TimerDialogDisplayForPlayerBJ( true, timer_window, GetOwningPlayer(hero) )
    call TimerStart(clock, duration, false, null)
  
    call PolledWait(duration)
  
    call ReviveHero(hero, x, y, true )
  
    call DestroyTimerDialog(timer_window)
    call DestroyTimer(clock)
    set timer_window = null
    set clock = null
    set hero = null
endfunction

function InitTrig_HeroRevive takes nothing returns nothing
    set gg_trg_HeroRevive = CreateTrigger(  )
    call TriggerAddAction( gg_trg_HeroRevive, function ReviveDyingHero )
endfunction

.. purple function means => native function. A native function is the is the lowest function level you can access.
.. red function means => some sort of function that internally again calls or an other red function, or a native function (or multiple ones).

So, going from your code, some red function ( bj/blizzard jass - functions) were replaced by native functions that do just the same -- as it often makes no sense in calling the bj functions.
One can look it up what the bj's internally do by looking into the blizzard.j, and then just use the used native.

Just tell if there are problems with code-understanding.

The timer thing is actually ok, too, in terms of leak, but Pyro wanted explain a bit improved way of using the tmer, over PolledWait(). But now, maybe shifting this task to a later time would be better, idk, how you want. It would require some more knowledge, or for example in dynamic indexing, or general hashtable usage, vjass structs.

 

[plumsy460]

The timer thing is actually ok, too, in terms of leak, but Pyro wanted explain a bit improved way of using the tmer, over PolledWait(). But now, maybe shifting this task to a later time would be better, idk, how you want. It would require some more knowledge, or for example in dynamic indexing, or general hashtable usage, vjass structs.

I sincerely apprecaite getting to know the best methods of coding. But like you said maybe it's better I settle for more primitive ways for the time being for the sake of learning.

Just tell if there are problems with code-understanding.

I'm extremely happy that I have people I can rely on

I testet the code and it works perfectly. Thanks again to the both of you for helping me finish the trigger. Also thank you both a lot for the explanation and the insight.
My problem is solved.

 

[Pyrogasm]

Bo’s link doesn’t seem to be working for me so here’s another link to a Blizzard.j (wrapper functions you probably don’t want to use) and common.j (natives you probably do want to use).

JASS Manual: API Browser - Blizzard.j

JASS Manual: API Browser - common.j