RPG Threat System v3.2c

Moderator · Mar 1, 2014

IcemanBo:
- date: 3th March 2015
- submission: Special Creep Engine v3.2b

The system allows to define unique unit-type behaviours
for creeps to improve the enemy's AI creep controle.

All in all the code is efficient, leakless and works fine.
I have not found nor bugs nor major flaws in the system.

2*r <= 360 -> r <= 180.
It might be a bit confusing for user that the creep sleeping is not the same as the normal wc3 unit sleeping.
It might be helpful if it was clarified.
Arithmetic like "SECONDS_IN_A_DAY/86400" could be calculated once onInit.
The term "generator unit" that is used is not really explained. (?)

Good job. Approved.

IcemanBo: http://www.hiveworkshop.com/forums/...p-engine-v3-2b-248858/index5.html#post2640438

General

Coding

Concept

Design

Misc

Docs

Required Changes

Purgeandfire :: 29 October 2014

Criteria (Systems):

Coding (20): efficiency and potential improvement.
Concept (10): originality and features.
Design (20): modularity, importability, ease of use, and method of attack.
Misc (10): test map, image, hive submission, misc.
Docs (10): documentation

Score:

Coding: 10 / 20
Concept: 7 / 10
Design: 14 / 20
Misc: 8 / 10
Docs: 5 / 10
Total Score: 44 / 70

Needs Fix :: see Required Changes for approval.

Current Rating

3 / 5

65 - 70 becomes 5 / 5
55 - 65 becomes 4 / 5
40 - 55 becomes 3 / 5
30 - 40 becomes 2 / 5
0 - 30 becmes 1 / 5

http://www.hiveworkshop.com/forums/2589556-post54.html

GetWorldBounds() leaks. You need to store it in a variable and then
remove it manually (2 occurrences). [-3]
Just a note, adding [13] does not do anything for the following line:
private trigger array Handler[13]
It does not change anything unless you use a size > 8192.
filter2 should have a better name. [-1]
In pickHelper, if I'm not mistaken, it seems like you are trying to
choose the closest units and order them to help. In this case, it may
be better to take advantage of Spinnaker's system and use:
GetClosestNUnitsInRange
It is designed to do what you need to do, and it will result in a
significantly lighter computation. Note: You will have to change your
entire loop to adapt to this change. [-2]
This:
set Real[R_TEMP_FACE] = GetRandomReal(0, 359.9) * bj_DEGTORAD
Is equivalent to:
sett Real[R_TEMP_FACE] = GetRandomReal(0, 2*bj_PI)
Or you can use 6.28319 instead of 2*bj_PI.
In onHit, you can move the assignment of 'dex' outside the blocks.
(saves 1 line)
In onDeath, you need to set u to null. [-1]
If you really wanted to be extreme, you could redo this entire
system using structs instead of hashtables. > [-2]
Why use a trigger action for initUnit? (use a condition) [-1]

Score: 10 / 20

Cool concept! I love the diverse features! It really shows effort.
Creep engines are quite common though.
It could be cool to have creep-specific modularity, e.g. creep
specific sleep durations, so forth.

Score: 8 / 10

The module design is not very good. Generally, the best design is where
a user does not have to touch the system at all, and only needs to know
its functions. Generally, it is better to separate example code from the
library itself (usually in a different trigger).
A second design flaw is that the data is all expected upon initialization.
The setup functions have too generic names, e.g. ResetTimer or LowBound

To fix issues 1-3, I recommend translating your setup over to a struct
and adding a .register() method so the user can invoke registering a creep.

JASS:

        struct SpecialCreep extends array
            private static group   cg  = CreateGroup()
            private static integer uid = 0

            method operator tamed= takes boolean tame returns nothing
                call SaveBoolean(Hashtable, P_TAMED_BOOL, this, tame)
            endmethod

            method operator sleepDuration= takes real duration returns nothing
                call SaveReal(Hashtable, P_SLEEP_DUR, this, duration)
            endmethod

            // etc...

            private static method registerCreeps takes nothing returns boolean
                local unit f = GetFilterUnit()
                if GetUnitTypeId(f) == thistype.uid then
                    // ... actions to register creeps ... 

                    // uid -> rawcode of creep being registered
                    // f -> creep being registered
                endif  
                set f = null 
                return false
            endmethod

            method update takes nothing returns nothing
                local rect r = GetWorldBounds()
                set uid = this
                call GroupEnumUnitsInRect(cg, r, Filter(function thistype.registerCreeps))
                call RemoveRect(r)
                set r = null
            endmethod

            static method get takes integer raw returns thistype
                return raw
            endmethod
        endstruct

This may be a little advanced. In this situation, we are treating
the raw ID as the struct itself. With this, we do not even need to
map the rawcodes to integers 1, 2, 3... etc. We can just use the
rawcode for everything! In case this was confusing, maybe this will
help:

JASS:

            local SpecialCreep murloc = SpecialCreep.get('nmrl')
            set murloc.tamed = false
            set murloc.sleepDuration = 7.25
            set murloc.sleepTime = 21.0 
            // etc. 
            call murloc.update()

Note: my code above would change a lot of the system. I recommend
making these changes only if you understand the code 100%. But the
advantage is it would allow you to update unit stats at any time by
using .update()! That is awesome modularity.

When you order a unit to patrol (via the MOVE order), won't it ignore
enemy targets?

Score: 14 / 20

The test map should have instructions on what to do, or some
form of debriefing. It can either be in a message, a quest, etc. [-2]

Score: 8 / 10

Needs documentation. It is preferrable to have it at the top of
the code, and have a list of the functions that should be used.
Ideally, a user should not have to look through your system at all
in order to use your system.

Add some descriptions to functions and such. See the JASS section
for examples. It will make your code a lot more user-friendly. [-5]

Score: 5 / 10

Coding: #1, #4 (or explain if I read your code wrong), #7, #9
Design: #5 (need a response)
Misc: #1

Kazeon · Mar 1, 2014

forgot to mention
this system is not friendly for GUI user because you need a little basic JASS knowledge about filtering unit.

Daffa · Mar 1, 2014

Dalvengyr said:
forgot to mention
this system is not friendly for GUI user because you need a little basic JASS knowledge about filtering unit.

Found some bugs:
- dead units still ordered to attack
- unit is attacked event is buggy, I recommend everybody to change the event with any DDS. I can do nothing about this.

Bug Patching Help :
- check if unit is alive, if it's not, removed it from list.
- I recommend create several versions that suits all renown DDS (LFH and Bribe GUI DDS' are top priority since they're used a lot).

Kazeon · Mar 1, 2014

Daffa the Mage said:
Bug Patching Help :
- check if unit is alive, if it's not, removed it from list.
- I recommend create several versions that suits all renown DDS (LFH and Bribe GUI DDS' are top priority since they're used a lot).

I will just use lfh's..

bcose it's super lovely and easy to use!

Daffa · Mar 1, 2014

It's recommended that CBS_Loop Value shouldn't go below integer 0.3125 or so, that's the base value of Jass spells loop.

TheGoldenGoblin · Mar 1, 2014

You should give examples on how to use dds. Also if you use spells on the units they won't attack you.

BPower · Mar 1, 2014

The big con is the tremendous ForGroup loop, which runs every 0.03125 seconds.

I would appreciate it, if you would outline your code with comments.
Makes it much easier for me and others to read.

Kazeon · Mar 1, 2014

TheGoldenGoblin said:
You should give examples on how to use dds. Also if you use spells on the units they won't attack you.

you are right..

but there are many choices of DDS, that's why I got a feeling to left it optional (using basic unit is attacked event)

BPower said:
The big con is the tremendous ForGroup loop, which runs every 0.03125 seconds.

I would appreciate it, if you would outline your code with comments.
Makes it much easier for me and others to read.

yeah, actualy using high accuracy doesn't really affects the performance lol, event using 0.5 is not noticeable..

300th post!!!

EDIT:

Updated to v1.2 with the following:
- Accuracy changed to a reasonable value
- Added some outlines
- Added with DDS version

credit's going to looking_for_help

TriggerHappy · Mar 6, 2014

The documentation should be included in the code, so when people copy it to their maps or to another website, it's there.

Also, inside CBS_InitLibrary I think there should be a wrapper forSaveIntegerto improve readability and not require people to specify a parent key (custom allocation).
This will also prevent users from using a different hashtable.

By default this system should probably require a DDS, because of how inaccurate the attack event is.

Malhorne · Mar 7, 2014

JASS:

function DistanceBetweenCoords takes real x,real y,real xt,real yt returns real
        local real dx = xt - x
        local real dy = yt - y
        
        return SquareRoot(dx * dx + dy * dy)
    endfunction

->

JASS:

function DistanceBetweenCoords takes real x,real y,real xt,real yt returns real
        return SquareRoot((xt-x)*(xt-x) + (yt-y)*(yt-y))
    endfunction

JASS:

if angle > 360 then
            loop
                set angle = angle - 360
                exitwhen (angle <= 360)
            endloop
        elseif angle < 0 then
            loop
                set angle = angle + 360
                exitwhen angle >= 0
            endloop
        endif

->

JASS:

if angle > 360. then
            set angle = ModuloReal(angle, 360)
        elseif angle < 0. then
            set angle = ModuloReal(-angle, 360)
        endif

JASS:

function CBS_Filter2 takes nothing returns boolean
        local unit u = GetFilterUnit()
        local unit c = LoadUnitHandle(udg_CBS_Hash, 0, 0)
        local real a1
        local real a2
        local real rad
        local integer dex
        local player pa = Player(CBS_PassivePlayer())
        local player ag = Player(CBS_AggressivePlayer())
        local player p = GetOwningPlayer(u)
        
        set dex = LoadInteger(udg_CBS_Hash, 1911, GetUnitTypeId(c))
        set a1 = GetUnitFacing(c) + 360.0
        set a2 = AngleBetweenCoords(GetUnitX(c),GetUnitY(c),GetUnitX(u),GetUnitY(u)) + 360.0
        // Load sight radius
        set rad = LoadReal(udg_CBS_Hash, 5511, dex) * 0.5
        // Filter out target not in sight radius
        if (a2 <= a1 - rad or a2 >= a1 + rad) and (a2 <= a1 - rad - 360.0 or a2 >= a1 + rad - 360.0) then
            return false
        endif
        if p == pa or p == ag then
            return false
        endif
        return IsUnitVisible(u, pa) and IsUnitVisible(u, ag) and not IsUnitType(u, UNIT_TYPE_DEAD)
    endfunction

This function leak ^^'
I suggest storing the result into a boolean, null every local handle and at least return the boolean.

JASS:

function CBS_HelpFilter takes nothing returns boolean
        local unit u = GetFilterUnit()
        local integer ut = GetUnitTypeId(u)
        local integer dex = LoadInteger(udg_CBS_Hash, 1911, ut)
        local integer ht = LoadInteger(udg_CBS_Hash, 7550, dex)
        
        if ht == 1 then
            if ut != udg_CBS_DuTy then
                return false
            endif
        else
            if ht < 1 then
                return false
            endif
        endif
        return GetOwningPlayer(u) == Player(CBS_PassivePlayer()) and not LoadBoolean(udg_CBS_Hash, 2343, dex) 
    endfunction

Same there.

if GetWidgetLife(u) > .405 then isn't enough to check if a unit is dead :
if GetWidgetLife(u) > .405 and not IsUnitType(u, UNIT_TYPE_DEAD)

JASS:

local player pa = Player(CBS_PassivePlayer())
        
        if GetOwningPlayer(a) == Player(CBS_AggressivePlayer()) then

->

JASS:

local player pa = Player(CBS_PassivePlayer())
        
        if GetOwningPlayer(a) == pa then

JASS:

else
                if not CBS_ColorBool() then
                    call SetUnitColor(u, GetPlayerColor(ag))
                endif

->

JASS:

elseif not CBS_ColorBool() then
    ....
endif

JASS:

call SetPlayerAllianceStateBJ(ag, p, bj_ALLIANCE_UNALLIED)
                // Passive player threats other players as allies
                call SetPlayerAllianceStateBJ(pa, p, bj_ALLIANCE_ALLIED)

Bjs needed ?

call TriggerRegisterEnterRectSimple(t, GetWorldBounds())
This one can be avoid and it leaks :/

IcemanBo · Mar 7, 2014

JASS:

private function AngleBetweenCoords takes real x, real y, real xt, real yt returns real
        local real angle = Atan2((yt - y), (xt - x)) * bj_RADTODEG
       
        if angle > 360 then
            loop
                set angle = angle - 360
                exitwhen (angle <= 360)
            endloop
        elseif angle < 0 then
            loop
                set angle = angle + 360
                exitwhen angle >= 0
            endloop
        endif
        return angle
    endfunction

This is ok, but I would just do:

JASS:

private function AngleBetweenCoords takes real x, real y, real xt, real yt returns real
    return Atan2((yt - y), (xt - x)) * bj_RADTODEG
endfunction

If you keep yours (not wrong) change the first check "if angle > 360 then" to if equal or bigger. (because if you have 360 you want to get it 0)

JASS:

private function filter2 takes nothing returns boolean
        local unit u = GetFilterUnit()
        local unit c = LoadUnitHandle(HASH, 0, 0)
        local real a1
        local real a2
        local real rad
        local integer dex
        local player pa = Player(PASSIVE)
        local player ag = Player(AGGRESSIVE)
        local player p = GetOwningPlayer(u)
        local boolean b = true
       
        set dex = LoadInteger(HASH, 1911, GetUnitTypeId(c))
        set a1 = GetUnitFacing(c) + 360.0
        set a2 = AngleBetweenCoords(GetUnitX(c),GetUnitY(c),GetUnitX(u),GetUnitY(u)) + 360.0
        set rad = LoadReal(HASH, 5511, dex) * 0.5
        if (a2 <= a1 - rad or a2 >= a1 + rad) and (a2 <= a1 - rad - 360.0 or a2 >= a1 + rad - 360.0) then
            set b = false
        endif
        if (p == pa or p == ag) or IsUnitType(u, UNIT_TYPE_DEAD) then
            set b = false
        endif
        if not IsUnitVisible(u, pa) and not IsUnitVisible(u, ag) then
            set b = false
        endif
        set u = null
        set c = null
        return b
    endfunction

Use locols for UnitX/Y since you make 2 function calls.
Atm you do 3 checks, even b would be FALSE already after first one. You could put further checks in 'else' of first 'if' to prevent useless checks.

Same with move further 'if's' in 'else' of first 'if' to prevent needless checks for

JASS:

 private function helpfilter

Then soon stopped reading code carefully because Im too lazy to read this all :d

But in "private function ini takes nothing returns nothing" you forgot to null local region.

And does your system work if I want to have more than 1 AGGRESIVE and 1 PASSIVE player?

And for me idc, but you have to change variable names. Only capital letters --> only constants. Start normal variables with lowcase and each new word with capital letter.

Kazeon · Mar 7, 2014

And does your system work if I want to have more than 1 AGGRESIVE and 1 PASSIVE player?

you want it for what? C:

Use locols for UnitX/Y since you make 2 function calls.

I use it only once

If you keep yours (not wrong) change the first check "if angle > 360 then" to if equal or bigger. (because if you have 360 you want to get it 0)
But in "private function ini takes nothing returns nothing" you forgot to null local region.

okay, thnks!

And for me idc, but you have to change variable names. Only capital letters --> only constants. Start normal variables with lowcase and each new word with capital letter.

can you give me example? I don't understand that part

thnks for the review..

IcemanBo · Mar 7, 2014

you want it for what? C:

I dont need it, just wanted to know

I use it only once

Oh yo, one time 'c' and other one is 'u'. You're right

can you give me example? I don't understand that part
thnks for the review..

private constant string NAME : good
private constant string name : not good

private string MAPNAME : not good
private string mapName : good

IcemanBo · Mar 7, 2014

In my system BPower told me not to use _
~~So if it's constant then maybe NAMENAME else nameName.~~
http://www.hiveworkshop.com/forums/2494473-post3.html

Edit:

CONSTANT_NAMES are all capital letters and each new word
is indicated by being seperated from the rest by an underscore.

MAP_NAME was correct for constant ^^

Malhorne · Mar 7, 2014

Remember that if you use only one time the value in the function there is no need to use a local

I didn't check the code on this criteria but seemed good.

JASS:

set dex = LoadInteger(HASH, 1911, GetUnitTypeId(c))
        set a1 = GetUnitFacing(c) + 360.0
        set a2 = AngleBetweenCoords(GetUnitX(c),GetUnitY(c),GetUnitX(u),GetUnitY(u)) + 360.0
        set rad = LoadReal(HASH, 5511, dex) * 0.5

Could be done on local declaration

JASS:

local unit u = gettarget()
        local unit a = getsource()

Set the values of those local inside the if block and put the nulling of those local in the if block too

JASS:

local player pa = Player(PASSIVE)
        
        if GetOwningPlayer(a) == Player(AGGRESSIVE) then

->

JASS:

local player pa = Player(PASSIVE)
        
        if GetOwningPlayer(a) == pa then

You forgot to use call RemoveRect(r) in the init function

JASS:

if GetUnitCurrentOrder(u) != OrderId("move") and GetUnitX(u) == LoadReal(HASH, hand, 9) and GetUnitY(u) == LoadReal(HASH, hand, 10) then
            return false
        endif
        return true

->

return not(GetUnitCurrentOrder(u) != OrderId("move") and GetUnitX(u) == LoadReal(HASH, hand, 9) and GetUnitY(u) == LoadReal(HASH, hand, 10))

Kazeon · Mar 7, 2014

Roger that!

local player pa = Player(PASSIVE)

if GetOwningPlayer(a) == Player(AGGRESSIVE) then

there is PASSIVE and AGGRESIVE, they are not the same

JASS:

        local unit u = gettarget()
        local unit a = getsource()

I think that part just stay at local declaration is okay

Malhorne · Mar 7, 2014

Holy xD !
My eagle eye didn't see this ^^

Kazeon · Mar 7, 2014

call RemoveRect(r) result in complie error? :/ or maybe you meant call RemoveRegion(r) ? because r is region

Malhorne · Mar 7, 2014

Oh yes you're right ^^

Xonok · Mar 7, 2014

I recommend not using ForGroup at all. Use a FirstOfGroup loop or put units into an indexed array.
This is because ForGroup gets crazily expensive with lots of units. Tests show that it's about on par with FirstOfGroup with around 3-4 units. After that the performance has dropped drastically.

Malhorne · Mar 7, 2014

Yep you're right...
If you don't mind destroying the group I suggest using a FoG loop

Kazeon · Mar 7, 2014

I dont understand what are you gays talking about x.X

what's FoG? I never use it before

Malhorne · Mar 7, 2014

JASS:

local unit u
loop
    set u = FirstOfGroup(g, u)
    exitwhen u == null
    call GroupRemoveUnit(g, u)
    //Do actions with the unit
endloop

Xonok · Mar 7, 2014

Dalvengyr said:
I dont understand what are you gays talking about x.X

what's FoG? I never use it before

Basically FirstOfGroup (FoG) is just a function. It gives you 1 unit from a unit group.
If you wish to use groups, then do it like this:

JASS:

function stuff takes group g returns nothing
    local group backup = CreateGroup()
    local unit u
    loop
        set u = FirstOfGroup(g)
        exitwhen u == null
        call GroupAddUnit(backup,u)
        //Do something with unit
        call GroupRemoveUnit(g,u)
    endloop
    call DestroyGroup(g)
    set g = backup
    set backup = null
    set u = null
endfunction

This piece of code assumes that you want to keep the group intact.

Kazeon · Mar 7, 2014

will that improve the performance? seems like it's just the same if we have many units :/

Xonok · Mar 7, 2014

Dalvengyr said:
will that improve the performance? seems like it's just the same if we have many units :/

It will. For some reason the ForGroup function spends excessive amounts of processing power with lots of units.
Due to this, a FoG loop is much faster. It's stupid, but that's how it is.

Kazeon · Mar 7, 2014

thanks, I will change it then

wait, are you sure this works
set g = backup
?

EDIT:
it works!
but strange, when I destroy the backup it stops working
call DestroyGroup(bu)
if I dont destroy it, will that causes leak?

Updated to v1.9
- Added 2 new APIs
- Improved loops
- Fixed another sleep bug
- Many improvements (credit's going to Malhorne and xonok)

Xonok · Mar 7, 2014

Dalvengyr said:
thanks, I will change it then
wait, are you sure this works
set g = backup
?

EDIT:
it works!
but strange, when I destroy the backup it stops working
call DestroyGroup(bu)
if I dont destroy it, will that causes leak?

Updated to v1.9
- Added 2 new APIs
- Improved loops
- Fixed another sleep bug
- Many improvements (credit's going to Malhorne and xonok)

Well, you have to keep in mind that you're not destroying a variable, but an object.
A unit group is an object, so if you do something like:
local group a = CreateGroup
local group b = a
local group c = b

Then you have 1 group and 3 variables.
Thus, to clear them you have to destroy the group once (through any of them, but only 1) and null all 3 of them.

Malhorne · Mar 7, 2014

It is a reference to the object to be precise.

Malhorne · Mar 8, 2014

You're totally welcome !

EDIT :

looking_for_help for GUI damage detection (actually it's Jass, he was drunk)

<3-ed this

Adiktuz · Mar 20, 2014

It seems to look useful... Though sadly I think most systems like this gets not much exposure from the users it was actually intended for...

Heck, my super now useless, inferior, maybe not so optimized missile system has more than 2x downloads than my really useful systems...

and maybe because less and less people mod nowadays...

Kazeon · Mar 20, 2014

that's all I want to know, is this system actually useful or not? :3 thnks adikutz!

Adiktuz · Mar 21, 2014

It is useful...

As for usability, IDK... haven't look closely at the code so...

For example, you can make a really useful script, but then it's not easy for people to implement, so while it is useful, it's not that much usable

Kazeon · Mar 21, 2014

it's not that hard to implement

Imo
just copy all the codes and give 2 empty player slot, then configure everything (yes, this part need little more understanding actually

)

Adiktuz · Mar 22, 2014

It was just an example of usable vs useful... Never said your system was hard to implement

Kazeon · Mar 22, 2014

Adiktuz said:
It is useful...

As for usability, IDK... haven't look closely at the code so...

For example, you can make a really useful script, but then it's not easy for people to implement, so while it is useful, it's not that much usable

there you said it..

Adiktuz · Mar 22, 2014

If you read the first part of the greened text, it says, FOR EXAMPLE... IDK what part of the word EXAMPLE you don't understand...

and the next one too, YOU CAN

So you see, I said EXAMPLE and CAN... I never said YOU DID

and in case you missed my second line: "As for usability, IDK... haven't look closely at the code so..."
Logically, how can I say that your system is hard to implement when I haven't looked closely at it?

Malhorne · Mar 22, 2014

Please guys xD !

Adiktuz · Mar 23, 2014

but really, maybe the mods are just busy...

A Void · Mar 31, 2014

Shame that this is not in GUI. I am pro-GUI. This would be very useful in GUI maps. Since most maps out there are GUI.

Malhorne · Mar 31, 2014

I don't see the point of doing maps without JNGP....
Please guys stop this narrow minded way ><

A Void · Mar 31, 2014

I am using Jass New Gen Pack. But I'm not into vJass nor Jass.

Malhorne · Mar 31, 2014

And you say you're pro-GUI ?
You can't without JASS knowledge...

And btw vJASS is easier to configure than GUI :/

A Void · Mar 31, 2014

I don't like writing codes. I like doing things with interface.

Malhorne · Mar 31, 2014

Oh that's sure that when you code nowadays interface is like : https://www.youtube.com/watch?v=nUreKfeBGWk
The title was supposed to be a part of the sentence but oh anyway, arguing is close

msongyboi · Apr 1, 2014

sir this is excellent, maybe you can add some spells ai funcs.

spells
targetable (disable, nukes, ally buff, aoe disable, aoe buff, aoe disable, aoe damage)
immediate (self buff, aoe damage, aoe buff, aoe disable)
point (etc..)

like
function addTargetableSpell takes integer unitID, integer abilityID, integer castType, real time, integer chance returns nothing

let the time proc and when it hits 0, there's a chance for a unit to cast a spell with different controls per castType or something like

function addRetaliationSpell takes blah blah balh

every time the unit is damaged, there's a chance to proc this spell blah blah blah.

once again lovely script +rep

Malhorne · Apr 1, 2014

Infact it is the definition of AI XD

A Void · Apr 7, 2014

So any plans on this engine to actually be in GUI? I suggest on doing that as I mentioned. More people will then use this.

Kazeon · Apr 7, 2014

there is no point in recoding it to GUI, unless user wish to modify the core, which is impossible in the most cases, mostly user just want to use not modify.. so I refuse

and it's currently easy for GUI ers just to use it..

RPG Threat System v3.2c

RPG Threat System (Map)

Moderator

Moderator

General

Coding

Concept

Design

Misc

Docs

Required Changes