knockback simple v.1.03

Chaosy · Aug 7, 2016

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

ZiBitheWand3r3r · Aug 7, 2016

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 · Aug 8, 2016

ZiBitheWand3r3r said:
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 · Aug 8, 2016

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

JASS:

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

then you can just make it

JASS:

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:

JASS:

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 · Aug 10, 2016

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

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

done

Tank-Commander said:
Attachment Points should be configurable

done

Tank-Commander said:
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 · Aug 10, 2016

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

Tank-Commander · Aug 15, 2016

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:

JASS:

    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 · Aug 15, 2016

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:

JASS:

 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 :

JASS:

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:

JASS:

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 · Aug 15, 2016

ZiBitheWand3r3r said:
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 · Aug 15, 2016

jakeZinc said:
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 · Aug 20, 2016

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 · Aug 20, 2016

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:

JASS:

  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 · Aug 20, 2016

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

ZiBitheWand3r3r · Aug 20, 2016

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 · Nov 15, 2016

Sorry for the update. Just updating the tags.

HerlySQR · Jul 1, 2020

Hello.

knockback simple v.1.03

library knockback simple v.1.03 (Map)

Similar threads