Kill Streak / Player Kills Counting System v 1.1.0

Moderator · Dec 6, 2013

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

deathismyfriend · Dec 6, 2013

Picture one and two.

Anachron · Dec 6, 2013

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 · Dec 6, 2013

Anachron said:
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 · Dec 6, 2013

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

deathismyfriend · Dec 6, 2013

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

That's good.

Magtheridon96 · Dec 6, 2013

http://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 · Dec 6, 2013

Magtheridon96 said:
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 · Dec 8, 2013

Magtheridon96 said:
http://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 · Dec 8, 2013

SecondHand said:
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 · Dec 9, 2013

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

deathismyfriend · Dec 9, 2013

SecondHand said:
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 · Dec 9, 2013

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 · Dec 9, 2013

Magtheridon96 said:
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 · Dec 10, 2013

@mag: you sometimes look funny

I'm very sorry if my words disturbed you

IcemanBo · Dec 14, 2013

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 · Dec 14, 2013

IcemanBo said:
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 · Dec 14, 2013

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 · Dec 14, 2013

IcemanBo said:
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 · Dec 14, 2013

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 · Dec 14, 2013

Can you post test map please.

IcemanBo · Dec 14, 2013

deathismyfriend · Dec 14, 2013

IcemanBo said:
Yes.

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

Kazeon · Mar 5, 2014

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

JASS:

    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 · Mar 5, 2014

Dalvengyr said:
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

JASS:

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 · Mar 6, 2014

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 · Mar 9, 2014

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:
JASS:
```
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:

Original:

JASS:

    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:

JASS:

    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:

JASS:

    // 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 · Mar 9, 2014

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

deathismyfriend · Mar 9, 2014

PurgeandFire said:
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:

JASS:

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:

Original:

JASS:

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:

JASS:

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:

JASS:

// 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 · Mar 9, 2014

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

Looks very well. +rep

Kill Streak / Player Kills Counting System v 1.1.0

Just another Warcraft III map (Map)

Moderator

Moderator

Attachments

Attachments

Similar threads