knockback simple v.1.03

[Chaosy]

I find the image confusing. Perhaps it's just me.

 

[ZiBitheWand3r3r]

oh, that's not good;]
I added some text formations

unfortunatelly I have no idea how to paste jass code here with indentations, like I have in WE..

 

[KILLCIDE]

unfortunatelly I have no idea how to paste jass code here with indentations

Paste the code in the Plain Text Editor (or BB Code Editor) instead of the Rich Text one. You can do so by clicking on the paper + wrench icon.

 

[Tank-Commander]

You have a lot of needless extra IF statements - a large amount of them can be combined.
if your code follows the structure

if (condition) then
  if (condition) then
    //actions
  endif
endif

then you can just make it

if (condition) and (condition) then
  //actions
endif

Though I'm aware you've obviously done this you should do this all the time when possible

I'd also suggest commonly used constant values like 2 * PI and PI / 2 should be stored in constant variables
Attachment Points should be configurable (or should be selectable when adding a knockback)
In addition using integers is faster than using enum (ForGroup)
via the structure:

local unit u = FirstOfGroup(group)

loop
  exitwhen u == null

  //if filtering
  if (condition) then
    //actions
  endif

  set u = FirstOfGroup(group)
endloop

(obviously only use the priming if this is all the function is going to do, otherwise you'd intialise it as null and move set u = FirstOfGroup(group) to above the exitwhen) Again this is something you do sometimes but not others, do it all the time when you can

 

[ZiBitheWand3r3r]

Ok. Tank, thanks for your tips.
I hope I cut off all of needless "if" statements.

I'd also suggest commonly used constant values like 2 * PI and PI / 2 should be stored in constant variables

done

Attachment Points should be configurable

done

In addition using integers is faster than using enum

I based this concept on ForGroup and group where I keep kb'ed units. If you think it is acceptable please don't force me to change this
I don't get your last suggeston, FirstOfGroup is used once in function KB_CheckUnitsAroundAndBounce, part of the code:
call GroupEnumUnitsInRange(kb_ug, targetX, targetY, 150.00, kb_filtr[filtrNr]) // alive+ground+not structure loop set obst_u = FirstOfGroup(kb_ug) exitwhen obst_u == null

btw is image readable ? I tryed to put there all info about knockbacked unit's behavior.
@KILLCIDE thanks for help;]

 

[FUJI]

I thought it has an optional feature of bouncing like a ball xD. ( would be awesome and better )

 

[Tank-Commander]

I noticed extra functions that aren't needed, for example this line:
"call GroupEnumUnitsInRange(kb_ug, targetX, targetY, 150.00, kb_filtr[filtrNr])"
calls kb_filtr[filtrNr] which is 1 line long
"return KBFiltr_1(GetFilterUnit())"
you can cut out that function entirely and skip straight to it by using if (KB_Filtr_1(u)) then inside you loop like this:

    set kb_player = GetOwningPlayer(kb_u)
    call GroupEnumUnitsInRange(kb_ug, targetX, targetY, 150.00, null)
    loop
        set obst_u = FirstOfGroup(kb_ug)
        exitwhen obst_u == null
        if KBFiltr_1(u) then // alive+ground+not structure
           //actions
        endif
    endloop

doing that will cut the amount of function calls to filter units in half
Aside from that why is it called KBFiltr_1 when you don't have a KBFiltr_2? and what are the other functions for exactly (in the sense that it appears KBFiltr_1 already has the ability to tell which filter is being used and respond accordingly, wouldn't that make the other filters useless as they're only used on that one line I reference above and never otherwise called) Correct me if I'm wrong of course but that's how it seems to me

That said also that 150.00 hardcoded value could be configurable - pretty much all constants can
I'll need to set some time aside for a more detailed review of the code considering the need to compare this with other similar knockback systems so I apologize that I'm just dropping the odd note or two you can expect a full review in about half a week (I'm out for the next few days)

 

[ZiBitheWand3r3r]

I think you were misled of my bad description: // alive+ground+not structure
sorry for that. function KB_CheckUnitsAroundAndBounce among others takes integer filtrNr. This integer refers to 4 avaiable boolexpr array kb_filtr, they are set up on init function at bottom of the library:

 set kb_filtr[KB_FILTR_BOUNCE_NONE] =  Condition(function KBFiltr_groundNotStruc)   
 set kb_filtr[KB_FILTR_BOUNCE_ALL] =  Condition(function KBFiltr_groundNotStruc)
 set kb_filtr[KB_FILTR_BOUNCE_ALLIED_ONLY] = Condition(function KBFiltr_groundNotStrucAlliedOnly)
 set kb_filtr[KB_FILTR_BOUNCE_ENEMY_ONLY] = Condition(function KBFiltr_groundNotStrucEnemyOnly)

condition functions :

function KBFiltr_groundNotStruc takes nothing returns boolean
   return KBFiltr_1(GetFilterUnit())
endfunction
// set kb_player before confirm this 2 checks!
function KBFiltr_groundNotStrucAlliedOnly takes nothing returns boolean
  return KBFiltr_1(GetFilterUnit()) and IsUnitAlly(GetFilterUnit(), kb_player)
endfunction
function KBFiltr_groundNotStrucEnemyOnly takes nothing returns boolean
  return KBFiltr_1(GetFilterUnit()) and IsUnitEnemy(GetFilterUnit(), kb_player)   
endfunction

you are right that func KBFiltr_1 can be deleted, and its content moved to inside KBFiltr_groundNotStruc
but in this case I have to add few more lines (declare local unit u = GetFilterUnit(), and null it 3 times before "return"), so it would look like this:

function KBFiltr_groundNotStruc takes nothing returns boolean
    local unit u = GetFilterUnit()
    local integer a=1
    local integer id = GetUnitUserData(u)
    local boolean isKb = IsUnitInGroup(u, kb_group) // is knockbacked?    
    if IsUnitAlive(u) and (not IsUnitType(u, UNIT_TYPE_FLYING)) and (not IsUnitType(u, UNIT_TYPE_STRUCTURE)) and (IsUnitVisible(u, kb_player)) then
        if not (isKb and ((kb_knockbackType[id]==KB_TYPE_SLIDE) or (kb_knockbackType[id]==KB_TYPE_NO_BOUNCE and kb_bounceFiltrType[id]==KB_FILTR_BOUNCE_NONE))) then
            if not (isKb and ((IsUnitAlly(u, kb_player) and kb_bounceFiltrType[id]==KB_FILTR_BOUNCE_ENEMY_ONLY) or (IsUnitEnemy(u, kb_player) and kb_bounceFiltrType[id]==KB_FILTR_BOUNCE_ALLIED_ONLY))) then
                loop
                    exitwhen a>kb_filterOutAbiInteger
                    if GetUnitAbilityLevel(u, kb_filterOutAbility[a]) > 0 then
                        set u=null
                        return false
                    endif
                    set a=a+1
                endloop
                set u=null
                return true        
            endif
        endif
    endif
    set u=null
    return false
endfunction

I dont know if we get some noticeable advantage with change above?
For "150.00 hardcoded value" to be honest : for what reason?(Bribe has also hardcoded such a value in his gui kb)
I'll wait with future updates for deeper look at the code. Thanks for all your current notes.

 

[FUJI]

For "150.00 hardcoded value" to be honest : for what reason?(Bribe has also hardcoded such a value in his gui kb)

Hardcoded values are now criticized these days, I also disagree in this matter somehow my old spells have these hardcoded haven't been configurable because they are not prefer to be touch by the user. But still, its better to do it for extensive configuration also enabling the user to reach the beyond limitation of the system or spell modifying.

 

[ZiBitheWand3r3r]

because they are not prefer to be touch by the user

yeah, the same feeling here, jake
but of course no problem for me to add constant variable instead of 150.00
In next update I'll change calculations for new unit who was hit by kb'ed unit. Starting speed for new unit will be the same as kb'ed unit's speed at the moment of collision. Small fix but importand for more natural look.

 

[Tank-Commander]

This is more or less approvable aside from the change you're working on above there (I noticed it myself when I was testing), the screenshot used for the system is pretty unclear though without testing the system but that's fine though I'd still recommend using an ingame screenshot and including that one in the description while we have a number of knockback systems this offers a few additional features most others don't have

Notify me when you've updated so I can approve this

 

[ZiBitheWand3r3r]

Tank, I have a question about code.
I'm using dynamicaly created triggers with trigger action as a callback code.
When knockback expires (or new kb applied to a unit already kb'ed) I destroy trigger like this:

  if kb_trg_onHit[id] != null then // id is unitUserData
    call DestroyTrigger(kb_trg_onHit[id])
    set kb_trg_onHit[id] = null
  endif

should I first disable trigger and then destroy, or current code if fine?

 

[Tank-Commander]

I believe the current code is fine so long as you null the trigger variable after destroying it which you do

 

[ZiBitheWand3r3r]

updated to ver 1.03:

• changes in function KB_CheckUnitsAroundAndBounce, replaced real value 150.00 by constant variable KB_DETECT_OBSTACLE_UNIT_RADIUS, and
  
• changed new knockbacked unit distance calculations so starting speed for new unit will be the same as kb'ed unit's speed at the moment of collision
• uploaded pictures

 

[KILLCIDE]

Sorry for the update. Just updating the tags.

 

[HerlySQR]

Hello.