Kill Streak / Player Kills Counting System v 1.1.0

[Moderator]

17:39, 8th Mar 2014
PurgeandFire: Review:
https://www.hiveworkshop.com/forums/2496930-post27.html

 

[deathismyfriend]

Picture one and two.

 

[Anachron]

Takes wayyyy to many space.
You already have "Kills Counter" in the multiboard title.
Change the title to "Player Kills" and rechange the column names to:
Hero, Unit, Total

 

[deathismyfriend]

Takes wayyyy to many space.
You already have "Kills Counter" in the multiboard title.
Change the title to "Player Kills" and rechange the column names to:
Hero, Unit, Total

Will do. I always said i suck at naming things.

Any other suggestions are appreciated.

Edit: Much better naming with that lol. Thanks for the suggestion.

 

[BunnyAng]

This would be a very useful resource in my project ^_^. You already know it death.

 

[deathismyfriend]

This would be a very useful resource in my project ^_^. You already know it death.

That's good.

 

[Magtheridon96]

https://www.hiveworkshop.com/forums/spells-569/advanced-streak-system-v5-1-1-0-a-203324/

Oh me oh my~
I don't know how that link got there, my bad ;-;

~~~~
This seems lite-weight. Are you sure there's nothing configurable by the user however? c;

 

[deathismyfriend]

This seems lite-weight. Are you sure there's nothing configurable by the user however? c;

What do you mean by that ?

Also i don't look for other systems on here. My system is not simply a streak system.

Edit: Yours is also vJass mine is for GUIers / Jassers

The customizing is fully in GUI. With only a few needed custom functions which are in the jass script.

 

[Kazeon]

https://www.hiveworkshop.com/forums/spells-569/advanced-streak-system-v5-1-1-0-a-203324/

Oh me oh my~
I don't know how that link got there, my bad ;-;

~~~~
This seems lite-weight. Are you sure there's nothing configurable by the user however? c;

trying to compare your resource with the others is really not cool man, beside, you are moderator, you should know that..

 

[deathismyfriend]

trying to compare your resource with the others is really not cool man, beside, you are moderator, you should know that..

The only problem i have with comparing is comparing something that is vJass to jass or GUI. I don't see how they should be compared.

 

[Kazeon]

I'm sorry, maybe he isn't trying to compare but,, em. indirectly, he is promoting his resource which is not allowed ^^

 

[deathismyfriend]

I'm sorry, maybe he isn't trying to compare but,, em. indirectly, he is promoting his resource which is not allowed ^^

I wouldn't call that promoting i would call it comparing.

 

[Magtheridon96]

beside, you are moderator, you should know that..

Moderators are clearly infallible Gods, ey?

I'm a user.
With privileges and responsibilities.

I was posting that system to show him the amount of configuration this kind of system could use because I missed the GUI trigger.

Teach me more about your ideal Modus Operandi please~

edit
I'm not trying to attack anyone, I'm just not very pleasant this time of year and I'm going through pure bullshit IRL, so apologies to you if you felt that.

 

[deathismyfriend]

edit
I'm not trying to attack anyone, I'm just not very pleasant this time of year and I'm going through pure bullshit IRL, so apologies to you if you felt that.

Don't worry mag i know you weren't. I was just lost on why you posted a link to your system.

If you missed the GUI trigger it is a simple mistake no big deal.

 

[Kazeon]

@mag: you sometimes look funny
I'm very sorry if my words disturbed you

 

[IcemanBo]

Hello, I had one bug when testing it.

When I killed the knight(player2) my kill was not count.
I tryed it more often, with always same result.

Could not test with my own conditions, because I don't use JNGP.

 

[deathismyfriend]

Hello, I had one bug when testing it.

When I killed the knight(player2) my kill was not count.
I tryed it more often, with always same result.

Could not test with my own conditions, because I don't use JNGP.

You don't need JNGP for this.

Also if you look in the setup trigger it shows that when you kill the knight it will not count.

• (Unit-type of RCSDeadUnit) Not equal to Knight

 

[IcemanBo]

ok then.. I havent read it all.

I do need JNGP, if I want change/save it. Error appears, and WE deactivates most of the trigger.

 

[deathismyfriend]

ok then.. I havent read it all.

I do need JNGP, if I want change/save it. Error appears, and WE deactivates most of the trigger.

Ill have to check that out thanks for the update.

Edit: I forgot to remove the unneeded libraries. ( i use a template map for all my systems so i don't have to add everything i need all the time. )

Will update in 2 seconds.

Edit 2: Updated the dl map

 

[IcemanBo]

Ok, thx you changed it.. now it works for me to save the map.

When units by neutral hostile killed my own units, your system also registered it.

The kill amount of neutral hostile was shown in multiboard in very first row & field.

 

[deathismyfriend]

Can you post test map please.

 

[IcemanBo]

Yes.

 

[deathismyfriend]

Yes.

Updated thanks for the bug reporting. It now works as intended.

 

[Kazeon]

this is not GUI friendly, you kill GUI users by that setup trigger instead (hard to understand)

then, I wonder why you need to use that local timer

    function RCSGameTimerSetup takes nothing returns nothing
        local timer t = GetExpiredTimer()
        call TimerStart( udg_RCSGameTimer, udg_RCSGameTimeAccuracy, true, function RCSGameTimerEventEnd)
        set udg_RCSKillsUnitGroup = CreateGroup()
        call DestroyTimer(t)
        set t = null
    endfunction

for what?

also I highly recommend you to shorten thos variables and functions names. It's really hard to review it bcs of those shity names..

 

[deathismyfriend]

this is not GUI friendly, you kill GUI users by that setup trigger instead (hard to understand)

then, I wonder why you need to use that local timer

    function RCSGameTimerSetup takes nothing returns nothing
        local timer t = GetExpiredTimer()
        call TimerStart( udg_RCSGameTimer, udg_RCSGameTimeAccuracy, true, function RCSGameTimerEventEnd)
        set udg_RCSKillsUnitGroup = CreateGroup()
        call DestroyTimer(t)
        set t = null
    endfunction

for what?

also I highly recommend you to shorten thos variables and functions names. It's really hard to review it bcs of those shity names..

How is this not GUI friendly ? Everything they need to know is in GUI.
The local timer is to clean up the timer that expired. Otherwise it would leak a timer.
The variables are longer to not interfere with other systems. That is part of coding in GUI.

 

[Kazeon]

I'm Jasser, and I still can't understand, as example, this part

• -------- This is where you register The players Non Hero Deaths to a Multiboard. --------
• -------- --------
• Custom script: function RCSNonHeroDeathsCounterActions takes nothing returns nothing
• If (All Conditions are True) then do (Then Actions) else do (Else Actions)
• If - Conditions
• RCSPlayerInteger Less than 4
• Then - Actions
• Multiboard - Set the text for myMultiboard item in column MbIntegerColumnArray[RCSPlayerInteger], row MbIntegerRowArray[(((RCSPlayerInteger - 1) x 6) + 4)] to (String(RCSInteger))
• Else - Actions
• Custom script: endfunction
• -------- --------

still I recommend you to shorten thos variable and function names..

thnks for the info!

 

[PurgeandFire]

Review:

• Most of your comments say:
  
  • -------- Filter out all units in the ITE that you do not want to count the kills for. In the example peasants will not be counted for kills. --------
  But most of the conditions don't have anything in them. The one that does filter out a unit just filters out a knight. It is not a big deal, but IMO you should change the comments to prevent confusion (you probably just updated the trigger and forgot to update the comments).
  .
• In RCSGameTimerSetup, you can just directly use:
  call DestroyTimer(GetExpiredTimer())
  Rather than declaring a local, destroying it, and then nulling the local.
  .
• In RCSRegisterIndividualKills, you can just directly declare the local as such:
  

  local integer i = LoadInteger( udg_RCSHashtable, id, childKey) + 1

  Rather than declaring it, and then incrementing it by 1 in the next line.
  .
• There is a lot of repeated code. Take for example, RCSForceGroupEnumIndKills:
  
  Hidden

  Original:
  

      function RCSForceGroupEnumIndKills takes nothing returns nothing
          local unit u = GetEnumUnit()
          local integer id = GetHandleId(u)
          local boolean b = IsUnitType( u, UNIT_TYPE_HERO)
          if not b and udg_RCSIndividualKillsBoolean[1] and RCSNonHeroKillsIndividualFilter() then // This filter out the non hero kills
              if udg_RCSGameTimeReset[1] < ( udg_RCSGameTime - LoadReal( udg_RCSHashtable, id, 0)) then
                  call SaveInteger( udg_RCSHashtable, id, 1, 0)
                  set udg_RCSKillerUnit = u
                  set udg_RCSInteger = 0
                  call RCSKillStreakCheckKills()
                  set udg_RCSKillerUnit = null
              endif
          elseif b and udg_RCSIndividualKillsBoolean[2] and RCSHeroKillsIndividualFilter() then // This filter out the hero kills
              if udg_RCSGameTimeReset[1] < ( udg_RCSGameTime - LoadReal( udg_RCSHashtable, id, 0)) then
                  call SaveInteger( udg_RCSHashtable, id, 2, 0)
                  set udg_RCSKillerUnit = u
                  set udg_RCSInteger = 0
                  call RCSKillStreakCheckKills()
                  set udg_RCSKillerUnit = null
              endif
          endif
          set u = null
      endfunction

  
  How can we improve this? Notice that the two if blocks do essentially the same
  thing, except that one uses a different key. As such, here is one possible implementation:
  

      function RCSForceGroupEnumIndKills takes nothing returns nothing 
          local unit    u  = GetEnumUnit()
          local integer id = GetHandleId(u)
          local boolean b  = IsUnitType(u, UNIT_TYPE_HERO)
          local integer hkey
  
          if not b and udg_RCSIndividualKillsBoolean[1] and RCSNonHeroKillsIndividualFilter() then
              set hkey = 1
          elseif b and udg_RCSIndividualKillsBoolean[2] and RCSHeroKillsIndividualFilter() then
              set hkey = 2
          else
              set u = null
              return
          endif
  
          if udg_RCSGameTimeReset[1] < (udg_RCSGameTime - LoadReal(udg_RCSHashtable, id, 0)) then
              call SaveInteger(udg_RCSHashtable, id, hkey, 0)
              set udg_RCSKillerUnit = u
              set udg_RCSInteger    = 0
              call RCSKillStreakCheckKills()
              set udg_RCSKillerUnit = null
              set u = null
          endif
      endfunction

  That looks pretty neat. And it makes the differences between the two if-conditions more visible. In the original, you'll think that the if-conditions evaluate to two completely different things. However, in the example above, you'll know that only the keys change.
  
  You can also delegate them to a function, which may be neater:
  

      // Try to come up with a better name than I did :P
      function RCSForceGroupEnumIndKillsEvaluate takes unit enum, integer id, integer hkey returns nothing
          if udg_RCSGameTimeReset[1] < (udg_RCSGameTime - LoadReal(udg_RCSHashtable, id, 0)) then
              call SaveInteger(udg_RCSHashtable, id, hkey, 0)
              set udg_RCSKillerUnit = enum
              set udg_RCSInteger    = 0
              call RCSKillStreakCheckKills()
              set udg_RCSKillerUnit = null
          endif
      endfunction
  
      function RCSForceGroupEnumIndKills takes nothing returns nothing 
          local unit    u  = GetEnumUnit()
          local integer id = GetHandleId(u)
          local boolean b  = IsUnitType(u, UNIT_TYPE_HERO)
  
          if not b and udg_RCSIndividualKillsBoolean[1] and RCSNonHeroKillsIndividualFilter() then
              call RCSForGroupEnumIndKillsEvaluate(u, id, 1)
          elseif b and udg_RCSIndividualKillsBoolean[2] and RCSHeroKillsIndividualFilter() then
              call RCSForGroupEnumIndKillsEvaluate(u, id, 2)
          endif
  
          set u = null
      endfunction

  Of course, this has no real efficiency boost, but rather a fundamental logic/readability boost. That is important, too. Remember the DRY principle.
  
  
  .
• ^A similar dilemma as the above note with RCSGetIndivKillsFromUnit. Also, you can remove the return 0 within the if-blocks since it'll return 0 outside the block.
  .
• (Optional) The test map could be better. You should include some general information and a way to respawn units (especially for a kill system--once you kill all the units on the map, you can't test it anymore). But this is just a recommendation, not a requirement.

tl;dr Eh, I'm going to go ahead and approve this. It works just fine and has some nice customizability. The code is pretty difficult to read and has a few problems mentioned above. The test map could be better. I haven't really considered alternate implementations, but your code itself is fine. Most of my notes are just incredibly minor, and I know this has been sitting unreviewed for a while, so I apologize for that.

Approved.

 

[BunnyAng]

When i have time i'll add this into my project but not really sure if i can make it work lol

 

[deathismyfriend]

Review:

• Most of your comments say:
  
  • -------- Filter out all units in the ITE that you do not want to count the kills for. In the example peasants will not be counted for kills. --------
  But most of the conditions don't have anything in them. The one that does filter out a unit just filters out a knight. It is not a big deal, but IMO you should change the comments to prevent confusion (you probably just updated the trigger and forgot to update the comments).
  .
• In RCSGameTimerSetup, you can just directly use:
  call DestroyTimer(GetExpiredTimer())
  Rather than declaring a local, destroying it, and then nulling the local.
  .
• In RCSRegisterIndividualKills, you can just directly declare the local as such:
  

  local integer i = LoadInteger( udg_RCSHashtable, id, childKey) + 1

  Rather than declaring it, and then incrementing it by 1 in the next line.
  .
• There is a lot of repeated code. Take for example, RCSForceGroupEnumIndKills:
  
  Hidden

  Original:
  

      function RCSForceGroupEnumIndKills takes nothing returns nothing
          local unit u = GetEnumUnit()
          local integer id = GetHandleId(u)
          local boolean b = IsUnitType( u, UNIT_TYPE_HERO)
          if not b and udg_RCSIndividualKillsBoolean[1] and RCSNonHeroKillsIndividualFilter() then // This filter out the non hero kills
              if udg_RCSGameTimeReset[1] < ( udg_RCSGameTime - LoadReal( udg_RCSHashtable, id, 0)) then
                  call SaveInteger( udg_RCSHashtable, id, 1, 0)
                  set udg_RCSKillerUnit = u
                  set udg_RCSInteger = 0
                  call RCSKillStreakCheckKills()
                  set udg_RCSKillerUnit = null
              endif
          elseif b and udg_RCSIndividualKillsBoolean[2] and RCSHeroKillsIndividualFilter() then // This filter out the hero kills
              if udg_RCSGameTimeReset[1] < ( udg_RCSGameTime - LoadReal( udg_RCSHashtable, id, 0)) then
                  call SaveInteger( udg_RCSHashtable, id, 2, 0)
                  set udg_RCSKillerUnit = u
                  set udg_RCSInteger = 0
                  call RCSKillStreakCheckKills()
                  set udg_RCSKillerUnit = null
              endif
          endif
          set u = null
      endfunction

  
  How can we improve this? Notice that the two if blocks do essentially the same
  thing, except that one uses a different key. As such, here is one possible implementation:
  

      function RCSForceGroupEnumIndKills takes nothing returns nothing 
          local unit    u  = GetEnumUnit()
          local integer id = GetHandleId(u)
          local boolean b  = IsUnitType(u, UNIT_TYPE_HERO)
          local integer hkey
  
          if not b and udg_RCSIndividualKillsBoolean[1] and RCSNonHeroKillsIndividualFilter() then
              set hkey = 1
          elseif b and udg_RCSIndividualKillsBoolean[2] and RCSHeroKillsIndividualFilter() then
              set hkey = 2
          else
              set u = null
              return
          endif
  
          if udg_RCSGameTimeReset[1] < (udg_RCSGameTime - LoadReal(udg_RCSHashtable, id, 0)) then
              call SaveInteger(udg_RCSHashtable, id, hkey, 0)
              set udg_RCSKillerUnit = u
              set udg_RCSInteger    = 0
              call RCSKillStreakCheckKills()
              set udg_RCSKillerUnit = null
              set u = null
          endif
      endfunction

  That looks pretty neat. And it makes the differences between the two if-conditions more visible. In the original, you'll think that the if-conditions evaluate to two completely different things. However, in the example above, you'll know that only the keys change.
  
  You can also delegate them to a function, which may be neater:
  

      // Try to come up with a better name than I did :P
      function RCSForceGroupEnumIndKillsEvaluate takes unit enum, integer id, integer hkey returns nothing
          if udg_RCSGameTimeReset[1] < (udg_RCSGameTime - LoadReal(udg_RCSHashtable, id, 0)) then
              call SaveInteger(udg_RCSHashtable, id, hkey, 0)
              set udg_RCSKillerUnit = enum
              set udg_RCSInteger    = 0
              call RCSKillStreakCheckKills()
              set udg_RCSKillerUnit = null
          endif
      endfunction
  
      function RCSForceGroupEnumIndKills takes nothing returns nothing 
          local unit    u  = GetEnumUnit()
          local integer id = GetHandleId(u)
          local boolean b  = IsUnitType(u, UNIT_TYPE_HERO)
  
          if not b and udg_RCSIndividualKillsBoolean[1] and RCSNonHeroKillsIndividualFilter() then
              call RCSForGroupEnumIndKillsEvaluate(u, id, 1)
          elseif b and udg_RCSIndividualKillsBoolean[2] and RCSHeroKillsIndividualFilter() then
              call RCSForGroupEnumIndKillsEvaluate(u, id, 2)
          endif
  
          set u = null
      endfunction

  Of course, this has no real efficiency boost, but rather a fundamental logic/readability boost. That is important, too. Remember the DRY principle.
  
  
  .
• ^A similar dilemma as the above note with RCSGetIndivKillsFromUnit. Also, you can remove the return 0 within the if-blocks since it'll return 0 outside the block.
  .
• (Optional) The test map could be better. You should include some general information and a way to respawn units (especially for a kill system--once you kill all the units on the map, you can't test it anymore). But this is just a recommendation, not a requirement.

tl;dr Eh, I'm going to go ahead and approve this. It works just fine and has some nice customizability. The code is pretty difficult to read and has a few problems mentioned above. The test map could be better. I haven't really considered alternate implementations, but your code itself is fine. Most of my notes are just incredibly minor, and I know this has been sitting unreviewed for a while, so I apologize for that.

Approved.

I'll try to update when I can as the fixes you mentioned should be done.
Thanks for the approval and the comments.
I think I was drunk when I coded this so I'm surprised it didn't need more fixing.

@ BunnyAng
If you need help implementing it just ask.

 

[Wrda]

Imo, this system is really easy to do. Oh well, very useful for people who are lazy to do it
Looks very well. +rep