Revive Event v2.2

[Moderator]

12th Dec 2015
IcemanBo: For long time as NeedsFix. Rejected.

17:28, 1st April 2015
IcemanBo: Read post in thread.

Old

14:22, 23th Jan 2015
IcemanBo: http://www.hiveworkshop.com/forums/spells-569/revive-event-v2-1-a-253758/index3.html#post2643711

09:30, 8th Jul 2014
BPower:
http://www.hiveworkshop.com/forums/spells-569/heroalivesystem-v1-0-a-253758/#post2552917

 

[GywGod133]

post the jass code to easy review

 

[Trigger.edge]

post the jass code to easy review

done!

 

[IcemanBo]

• You should remove the restriction of these two teams. And btw, PlayerId doesn't necessarily define to which team you belong.
• You don't have to null struct members at declaration.
• Local players dont have to be nulled. Also you don't have to null the trigger unless you don't destroy it.
• Thinks like "gg_rct_Hero_respaw_1" should not be used in code. You should make a config for it and then use a variable in your revive function.
• In your condition, GetOwningPlayer(GetDyingUnit()) --> GetTriggerPlayer() and IsUnitType(u, UNIT_TYPE_HERO) == true --> IsUnitType(u, UNIT_TYPE_HERO)
• You could merge your action & condition functions to only one condition function.

Could you make it optional to show the timerdialog for allies, too? Else the system seems useful to me.

 

[Trigger.edge]

• You should remove the restriction of these two teams. And btw, PlayerId doesn't necessarily define to which team you belong.
• You don't have to null struct members at declaration.
• Local players dont have to be nulled. Also you don't have to null the trigger unless you don't destroy it.
• Thinks like "gg_rct_Hero_respaw_1" should not be used in code. You should make a config for it and then use a variable in your revive function.
• In your condition, GetOwningPlayer(GetDyingUnit()) --> GetTriggerPlayer() and IsUnitType(u, UNIT_TYPE_HERO) == true --> IsUnitType(u, UNIT_TYPE_HERO)
• You could merge your action & condition functions to only one condition function.

Could you make it optional to show the timerdialog for allies, too? Else the system seems useful to me.

thanks for the suggestion...! right now I'm working on a new version

 

[BPower]

First of all you don't need CSData, because TimerUtils can also attach integers to timers.
In this case you have to use NewTimerEx(d) instead of NewTimer(). Btw NewTimer is actually NewTimerEx(0).

TriggerActions are normally replaced by TriggerConditions.

You should figure out a way to support this for more than 2 teams. Consider a custom map like Footmen Frenzy.

    local timerdialog td    = CreateTimerDialog(t)
    set d.td = td

--> set d.td = CreateTimerDialog(t)
.
call TimerDialogSetTitleColor(td,255,200,120,255) could be part of the configuration.

gg_rct_Hero_respaw_1 does not really work for me. Use coordinates and make it work for multiple teams.

GetDyingUnit() --> GetTriggerUnit()

Please take into consideration what IcemanBo said.

 

[A Void]

I don't see any use for this system , since there are many ways to revive a hero in Warcraft 3. Some of the well known: Hero Revival Timer, Grave Death Revival, Normal Building Revival and the Auto Revival (Floating or a simple Text without the Timer).

 

[IcemanBo]

The name maybe should be change from HeroAlive to HeroRevive or something similar. Just a detail. ^^

 

[Trigger.edge]

I change everything in the code, in this time I've used T32 instead of TimerUtils.

and I am convinced that the system can work better using table?

I hope some suggestions...

 

[Bannar]

TimerUtils + T32? I bet you don't need both of them. Also, you might consider adding support for CTL if you want to stick with T32.

Library name is too generic - make it full lenght, instead of a shortcut.
Exceptions/Costs should be wrapped within vector or deque structure (if you want to stick with array model) or list container. With such approach, user would be able to push/pop dynamicaly limitations and/or other data instead of them being static.

Your library is constructed in pretty wierd way. Since you written most of it with help of functions, why not stick with it? Or meaby you prefer static struct approach? Here, you divide different parts of system between struct and functions without clear reason. Whatmore, even the inits are repeated - just use one init instead.

You don't need 2 trigger locals here:

private function Init takes nothing returns nothing
    local trigger t  = CreateTrigger()
    local trigger t2 = CreateTrigger()

Just reinitialize trigger t once used.

    call TriggerRegisterTimerEvent(t2,0.01,false)
    call TriggerAddAction(t2,function initHAS)

Use timer instead of perioding event. Just call TimerStart with CreateTimer() and in expression code, destroy it. Switch actions into conditions, and you should already know why.

Get rid of BJs, e.g LeaderboardResizeBJ is declared as:

function LeaderboardResizeBJ takes leaderboard lb returns nothing
    local integer size = LeaderboardGetItemCount(lb)

    if (LeaderboardGetLabelText(lb) == "") then
        set size = size - 1
    endif
    call LeaderboardSetSizeByItemCount(lb, size)
endfunction

if IsUnitType(GetTriggerUnit(),UNIT_TYPE_HERO) and IsPlayerInGroup(GetTriggerPlayer()) and not IsExeptionUnit(GetTriggerUnit()) thenWithin method onDeath, please store triggering unit - this will improve both, readability and efficiency.

if .timeloop == .time or STOP_TIMER[.pn] == true or GetWidgetLife(.u) > 0.405 thenIf you want to check if unit is alive, checking just it's life state is not enough. Either use UnitAlive native or IsUnitDead function with type, typeid and life state checks.

Since you are using here a lot of timerdialogs and such, you should consider storing and recycling these handles. Why it that? - you might ask. Firstly, there is limited amount of leaderboards that could possibly be displayed. Secondly, I bet the amount of heroes displayed at any given time is also limited. You end up with fixed/small amount of required handles. When timerdialog is used, see if your recycle stack is empty - if so, create new timerdialog - if not, use the recycler, set it up and unhide. When your handle expires, just hide it instead of destroying.

Function initHAS - you don't need ForGroup here, (if I'm wrong, let me know) since I don't think you need to keep group data after enumeration. If so, replace ForGroup with GroupEnum + FirstOfGroup loop. You might even consider using global group instead of local one.

Do you really need bj_MAX_PLAYERS here? Does Neutral victim, passive etc have to be considered when processing your operations? I bet they don't.

Improve the overall readability - enter spaces between function arguments - line breaks between variable declaration and actual code. Consider giving some space between loops and if statements. Right now it's more of a code bloat. As an example, look at "my" IsPlayerInGroup and then look again at yours

private function IsPlayerInGroup takes player p returns boolean
    local integer i = 0

    loop
		exitwhen i == bj_MAX_PLAYERS

        if IsPlayerInForce(p, TEAM_FORCE[i]) then
            return true
        endif

        set i = i + 1
    endloop

    return false
endfunction

See the difference?

Fix the comments i.e comments should not fill the line breaks between code - it makes your script even more unreadable. Also, //================================================================================= spam is unnecessary. In regard to indentation, which also has room for improvements, just read JPAG and follow it's convention.

Thats probably all right now, we shall continue once you finish those tickets.

 

[Trigger.edge]

oh thank you, I fixing the system now!

 

[GywGod133]

Your codes is looks good keep it up

 

[Trigger.edge]

This is the second udapte to this system.... I just hope the moderators can have time to rating this.

Pd: I have taken any of your tips Bannar.... Thanks for this

 

[Trigger.edge]

Your codes is looks good keep it up

Thanks

 

[edo494]

it seems you actually force the leaderboard, what if I already have leaderboard for some other score going on, and I dont want yours to show up? You could make a private constant boolean in there that will mark whether or not display the leaderboard, or maybe even some API with nonconstant(Constant for static if, nonconstant for runtime change)

 

[Trigger.edge]

For every unit/hero you can show the leaderboard/timerdialog or not...

Only set:

set Revive[whichUnit].showTimerdialog = false

or

set Revive[whichUnit].showLeaderboard = false

Here is the list of all configuration parameters for all units:

/*
       - method showTimerdialog takes boolean show returns nothing
       - method showLeaderboard takes boolean show returns nothing
       - method simpleLeaderBoard takes boolean show returns nothing
           - When is enabled only show the timer of your hero/unit in the leaderboard 
             and not the timer of the other players units.
       - method selectInstant takes boolean show returns nothing
           - Select the unit instant or not on revive.
       - method removeInstant takes boolean instant returns nothing
           - When revive a not hero unit is removed, then another is 
             created to replace this. Use this to remove with delay or not.
       - method awakenFx takes boolean show returns nothing
           - Set your custom awaken fx.
       - method onX takes boolean show returns nothing
           - x point of the unit.
       - method onY takes boolean show returns nothing
           - y point of the unit.*/

 

[edo494]

well I didnt notice :/ but good you already thought of that and made it available

 

[deathismyfriend]

Remove the BJs.

You need to null the local trigger variable.

You loop through all the players so you should combine the loop and the bj action below.

loop
                exitwhen i == bj_MAX_PLAYERS
                set p = Player(i)
                if GetPlayerController(p) == MAP_CONTROL_USER and GetPlayerSlotState(p) == PLAYER_SLOT_STATE_PLAYING then
                    set ta.leaderboard[i] = CreateLeaderboard()
                    call PlayerSetLeaderboard(p,ta.leaderboard[i])
                    call LeaderboardSetLabel(ta.leaderboard[i],TIMER_TITLE)
                    call LeaderboardDisplay(ta.leaderboard[i],false)
                endif
                set i.boardGroup = CreateGroup()
                set i = i+1
            endloop
            call TriggerRegisterAnyUnitEventBJ(t,EVENT_PLAYER_UNIT_DEATH)

You need to null the timer in this function onDialogEnd and in this one configDialog.

 

[IcemanBo]

You need to null the local trigger variable.

You need to null the timer in this function onDialogEnd and in this one configDialog.

No need to null them, because neither the trigger, nor the timers will be destroyed.

 

[Trigger.edge]

Remove the BJs.

You need to null the local trigger variable.

You loop through all the players so you should combine the loop and the bj action below.

You need to null the timer in this function onDialogEnd and in this one configDialog.

private static method onDialogEnd takes nothing returns nothing
            local timer t = GetExpiredTimer()
            local thistype this = GetTimerData(t)
            call DestroyTimerDialog(.td)
            call ReleaseTimer(t)
        endmethod

        private method configDialog takes nothing returns nothing
            local timer t = NewTimerEx(this)
    
            if .showDialog then
                set .td = CreateTimerDialog(t)
                if (GetLocalPlayer() == .p) then
                    call TimerDialogDisplay(.td,true)
                endif
                call TimerDialogSetTitle(.td,TIMER_TITLE)
                call TimerDialogSetTitleColor(.td,TD_RED,TD_GREEN,TD_BLUE,0)
                call TimerStart(t,.countDown,false,function thistype.onDialogEnd)
            endif
        endmethod

Thanks, had not noticed it.

 

[deathismyfriend]

No need to null them, because neither the trigger, nor the timers will be destroyed.

Timers still need to be nulled. Triggers are said to be nulled or not but I don't think anyone actually tested them.

 

[edo494]

timers also dont need to be nulled in reality(it is good practice tho), since he allocates them from pre-existing pool and returns them to the pool, the timers are never destroyed in that pool

 

[Trigger.edge]

well ... now I'm confused I have seen many jass codes, and in all of these all local variables (triggers, timers) are nulled.

 

[deathismyfriend]

timers also dont need to be nulled in reality(it is good practice tho), since he allocates them from pre-existing pool and returns them to the pool, the timers are never destroyed in that pool

This is true except the local handle is never released unless it is nulled. The timers are reused with that system but the local itself can't be reused.

 

[edo494]

new claims araising every day. I can only remember the handleid not being freed if you dont null the variable, but this shouldnt be problem here

 

[deathismyfriend]

new claims araising every day. I can only remember the handleid not being freed if you dont null the variable, but this shouldnt be problem here

Ya that is what I was talking about when nulling. Thanks.

 

[Kazeon]

Review emerges!

Codes:

•         method getCountDown takes nothing returns integer
              return R2I(.countDown)
          endmethod

  It should returns real
  

•         private static method delay takes nothing returns nothing
              local timer t = GetExpiredTimer()
              call RemoveUnit(GetUnitById(GetTimerData(t)))
              set t = null
          endmethod

  You never release the timer.
  

•         //.execute method ("revive" not hero units).
          method execute takes nothing returns nothing

  I recommend you to utilize [Snippet] ReviveUnit instead of keep re-creating normal units on revive. And you can save so many lines:
  (You can probably eliminate them all)
  

                  //Replacing unit.
                  set u = CreateUnit(p,GetUnitTypeId(this.unit),0.,0.,0.)
                  set re_lastRevivedUnit  = u
                  @set newId               = GetUnitId(u)@
                  @set newId.u             = u@
                  @set newId.p             = p@
                  @set newId.fx            = this.fx@
                  @set newId.x             = this.x@
                  @set newId.y             = this.y@
                  @set newId.timeBase      = this.timeBase@
                  @set newId.upgradeTime   = this.upgradeTime@
                  @set newId.instantSelect = this.instantSelect@
                  
                  @if IsUnitInGroup(this.unit,REVIVE_GROUP) then@
                      @call GroupRemoveUnit(REVIVE_GROUP,this.unit)@
                      @call GroupAddUnit(REVIVE_GROUP,u)@
                  @endif@
                  
                  call SetUnitPosition(u,newId.x,newId.y)
                  
                  if newId.fx != null or newId.fx != "" then
                      call DestroyEffect(AddSpecialEffectTarget(newId.fx,u,"origin"))
                  endif
                  
                  @if .instantSelect then@
                      @call newId.selectUnit()@
                  @endif@
          
                  set u = null
                  @set newId.u = null@
                  @set newId.p = null@

  And you don't need local unit u You can use re_lastRevivedUnit directly.
  

•             else
                  debug call DisplayTimedTextToPlayer(GetLocalPlayer(),0.,0.,60.,"[ReviveEvent]|c00FF0000Error|r: Invalid unit type, use ReviveHero instead.")

  =>
  

              debug else
                  debug call DisplayTimedTextToPlayer(GetLocalPlayer(),0.,0.,60.,"[ReviveEvent]|c00FF0000Error|r: Invalid unit type, use ReviveHero instead.")

•         private method @udapdeBoard@ takes nothing returns nothing

  ??
  
  I have found some similar mistakes. Please watch your typing.
  

•     function GetLasRevivedUnit takes nothing returns unit
          return Revive.getLast()
      endfunction

  Kinda like a lousy BJ function
  =>
  

      function GetLasRevivedUnit takes nothing returns unit
          return Revive.re_lastRevivedUnit
      endfunction

  .getLast method should be removed.
  

• method setAutomatic takes real timeBase, @boolean upgradeTime@, real x, real y, boolean instantSelect returns nothing

  Basing on its name, how come it's a boolean?
  
  And why do you reset .fx everytime you call that function?
  

              set .fx = DEFAULT_AWAKEN_FX

•                 set t = NewTimerEx(this)
                  call TimerStart(t,TIMER_PERIOD,true,function thistype.onDeathLoop)
                  set t = null

  =>
  

                  call TimerStart(NewTimerEx(this),TIMER_PERIOD,true,function thistype.onDeathLoop)

•         private method onRevive takes nothing returns nothing
              if IsUnitType(.u,UNIT_TYPE_HERO) then
                  @call ReviveHero(.u,.x,.y,true)@
                  call DestroyEffect(AddSpecialEffectTarget(.fx,.u,"origin"))
                  if .instantSelect then
                      call .selectUnit()
                  endif

  =>
  

          private method onRevive takes nothing returns nothing
              if IsUnitType(.u,UNIT_TYPE_HERO) then
                  @call ReviveHero(.u,.x,.y,false)@
                  call DestroyEffect(AddSpecialEffectTarget(.fx,.u,"origin"))
                  if .instantSelect then
                      call .selectUnit()
                  endif

• I didn't take a look at your leaderboard since personally, I want it to be removed from the system, I don't like it. Imho, any system that is using leader(multi)board will become very map specific which is no good.

Object data & Miscs:

• 
  How come I have 3 different timers?
  
• Object data is fine
• All imported files should be removed. It's really hurt to download 200KB file
  Spoiler

  for me
  .

Overall:

Well, the idea is just normal and the system is very map specific. The coding is not that good. I have nothing to say about object data. So

Rating of 2/5 and vote for Needs Fix.

(P.S. I may raise the rating after you fix those issues.)​

 

[Trigger.edge]

O_O Oh! very well... I fix your suggested points. Where we not agree it's in the use of leadeboard, as this can be enabled/disabled for any unit, same for the timerdialog.

Thanks for your review!

 

[Magtheridon96]

Your documentation style is oddly familiar.

I like it.

 

[Trigger.edge]

Your documentation style is oddly familiar.

I like it.

Thanks.... i like your avatar.... xD

 

[IcemanBo]

Could you stick to normal GUI functions? I can't load the map because of non existimg functions for me.

Trigger function doesn't exist in database: CreateSpecialEffectLoc ... but maybe there are more.

These are probably special UMSWE functions?

 

[Trigger.edge]

Could you stick to normal GUI functions? I can't load the map because of non existimg functions for me.

Trigger function doesn't exist in database: CreateSpecialEffectLoc ... but maybe there are more.

These are probably special UMSWE functions?

updated!

 

[IcemanBo]

*   You can:
*   ________
*
*       - Revive units.
*       - Use custom fx on revived units.
*       - See the cooldown in timerdialog or leaderboard.
*       - Detect when hero/unit is revived.
*       - Configure all parameters for all units.
*

You also can autoselect the reviving unit.
Also your select implies you also move the cam to reviving unit. I just mean it can be noted.

set re_lastRevivedUnit=CreateUnit(p,GetUnitTypeId(.unit),0.,0.,0.)

^This is actually the correct method to re-create a unit, but a problem can occur when the unit is removed already.
UnitTypeIds for removed units will equal 0. This may occur if unit gets removed via user functions any time, but also if the revive duration is just very long, so units will decay.
It may work in most cases, but it's safer immediatly to save the UnitTypeId , encapsylate it, and then just to read out the value.

It's good you use 03125 as periodical time, but it would not hurt to put it in a variable on top.

ReviveUnitReset resets the revival data, but also stops the current revivial, if unit is reviving, if I'm correct? It's probably worth to note it.

private constant real COOLDOWN_SECONDS_PER_LVL = 4.00

^For level 1, revive time will be 4 seconds, for level 2 will be 8 seconds, ... and so on?
Maybe a constant function could so some better instead the constant to allow own formula.

Good, that optional boards are provided to show revive time. You should note that the board is MPI, so only registers one hero per player.

Some short, but explanatory comments here and there would not hurt. In main system, but also in your optional boards.

It's well written, easy to understand. It should be approveable soon. Ah, and onInit you just could use the BJ for death event in this case.

 

[Trigger.edge]

@IcemanBo: Updated and fixed ^^

Edit: Thanks again for the suggestions

 

[IcemanBo]

Doesn't compile for me when I deactivate optional resources.

Now I think operations like pan cam/clear seletion/select unit are not needed to be implemented in system.
User has the event for revive, right? He should put named operations there if needed.
You don't need to bother with these extras in your code.

function IsUnitDead takes unit whichUnit returns boolean
    return IsUnitType(whichUnit,UNIT_TYPE_DEAD)
endfunction

Hm... ^^ I'm sceptical if this wrapper is useful. Not really in my opinion.

private trigger r=CreateTrigger()
private trigger t=CreateTrigger()
private trigger o=CreateTrigger()
private trigger p=CreateTrigger()

^Bad names for example.. and there are some more non-descrptive names.

Can't hook be avoided with periodical check for UnitTypeId?