[System] Melee/Ranged Attack Switch v1.1.1.1

[Moderator]

10th Nov 2011
Bribe: The system is cool, useful and Highly Recommended.

Thanks for making this resource.

 

[Bribe]

There are some syntax errors of LastOrder is not found.

I recommend making it a requirement than optional, because otherwise this thing is going to get even more extreme with static if's.

 

[Maker]

I added one more static if, now it won't cause syntax errors.

Making Last Order a requirement might be a good idea.

 

[Bribe]

Are you using JassHelper 0.A.2.B? You can comment out the whole "Delay3" function if you have that version.

It also might not compile properly even with the LastOrder library found since you have no "requires optional LastOrder" after the "library" keyword.

There is also the LIBRARY_LastOrder constant provided if the library is found, which you can use instead of the LAST_ORDER constant you currently have.

But I think it's better to just have LastOrder as a total requirement. The benefit it gives to this library is strong.

 

[Maker]

Are you using JassHelper 0.A.2.B? You can comment out the whole "Delay3" function if you have that version.

Yeah, I have that version. Why can I commet that function out?

The reason making Last Order optional is that if the mode change is set to auto, then Last Order isn't used for anyhting currently.

 

[Bribe]

Well, currently you comment out the whole loop, making the entire function quite pointless if LastOrder is not found.

 

[BlackRangerXIII]

Nice system maker ^^
Not rating until you finish the bug fixing

 

[Bannar]

No love for RegisterPlayerUnitEvent? ;/

        if SquareRoot((GetUnitX(u1)-GetUnitX(u2))*(GetUnitX(u1)-GetUnitX(u2))+(GetUnitY(u1)-GetUnitY(u2))*(GetUnitY(u1)-GetUnitY(u2))) > LoadReal(HT, GetUnitTypeId(u1), 0) then
            if not LoadBoolean(HT, GetHandleId(u1), 0) then
                call ToggleMeleeRanged(u1, u2, true)
            endif
        elseif LoadBoolean(HT, GetHandleId(u1), 0) then
            call ToggleMeleeRanged(u1, u2, false)
        endif

->>

        if SquareRoot((GetUnitX(u1)-GetUnitX(u2))*(GetUnitX(u1)-GetUnitX(u2))+(GetUnitY(u1)-GetUnitY(u2))*(GetUnitY(u1)-GetUnitY(u2))) > LoadReal(HT, GetUnitTypeId(u1), 0) and not LoadBoolean(HT, GetHandleId(u1), 0) then
            call ToggleMeleeRanged(u1, u2, true)
        elseif LoadBoolean(HT, GetHandleId(u1), 0) then
            call ToggleMeleeRanged(u1, u2, false)
        endif

>>>>>>>>>>>>>>>>>>>>>

        local unit u1 = GetAttacker()
        local unit u2
        if u1 == null then
            set u1 = GetTriggerUnit()
            if HaveSavedInteger(HT, GetUnitTypeId(u1), 1) then  // Unit type is initialized for the system
                if GetHandleId(GetTriggerEventId()) == 40 then  // Target unit order
                    set u2 = GetOrderTargetUnit()
                    if GetIssuedOrderId() == 851971  then   // Order is smart
                        if not IsUnitEnemy(u2, GetOwningPlayer(u1)) or /*
                        */ GetOrderTargetItem() != null then
                            set u1 = null
                            return
                        endif
                    elseif GetIssuedOrderId() != 851983 then // Order is not attack
                        set u1 = null
                        return  // Spells won't trigger the system
                    endif
                else
                    set u2 = GetEventTargetUnit()
                endif
            else
                set u1 = null
                return
            endif
        // Event was unit attacked
        elseif HaveSavedInteger(HT, GetUnitTypeId(u1), 1) then // Unit type is initialized
            set u2 = GetTriggerUnit()
        else
            set u1 = null
            return
        endif

->>

        local unit u1 = GetAttacker()
        local unit u2
        local integer i = GetHandleId(GetTriggerEventId()
        if u1 == null then
            set u1 = GetTriggerUnit()
            if HaveSavedInteger(HT, GetUnitTypeId(u1), 1) and i == 40 then
                set i = GetIssuedOrderId()
                set u2 = GetOrderTargetUnit()
                if (i == 851971 and (not IsUnitEnemy(u2, GetOwningPlayer(u1)) or GetOrderTargetItem() != null)) or i != 851983 then
                    set u1 = null
                    return
                endif
            elseif i != 40 then
                set u2 = GetEventTargetUnit()
            else
                set u1 = null
                return
            endif
        elseif HaveSavedInteger(HT, GetUnitTypeId(u1), 1) then // Unit type is initialized
            set u2 = GetTriggerUnit()
        else
            set u1 = null
            return
        endif

^I'll review that part again in sec; since your 'if' spam can be cut a lot. You should think more about: since I do 'set u1 = null & return' in many cases, why shouldn't that be a main statement in that function?

 

[Maker]

No love for RegisterPlayerUnitEvent? ;/

At the moment I have no plans to add that.

Code

        if SquareRoot((GetUnitX(u1)-GetUnitX(u2))*(GetUnitX(u1)-GetUnitX(u2))+(GetUnitY(u1)-GetUnitY(u2))*(GetUnitY(u1)-GetUnitY(u2))) > LoadReal(HT, GetUnitTypeId(u1), 0) then
            if not LoadBoolean(HT, GetHandleId(u1), 0) then
                call ToggleMeleeRanged(u1, u2, true)
            endif
        elseif LoadBoolean(HT, GetHandleId(u1), 0) then
            call ToggleMeleeRanged(u1, u2, false)
        endif

->>

        if SquareRoot((GetUnitX(u1)-GetUnitX(u2))*(GetUnitX(u1)-GetUnitX(u2))+(GetUnitY(u1)-GetUnitY(u2))*(GetUnitY(u1)-GetUnitY(u2))) > LoadReal(HT, GetUnitTypeId(u1), 0) and not LoadBoolean(HT, GetHandleId(u1), 0) then
            call ToggleMeleeRanged(u1, u2, true)
        elseif LoadBoolean(HT, GetHandleId(u1), 0) then
            call ToggleMeleeRanged(u1, u2, false)
        endif

My way: 2 comparisons for change to ranged, 2 comparisons to change to melee
Your way: 2 comparisons to change to ranged, 3 comparisons to change to melee

Your second suggestion will fail. Please test before suggesting changes. But I can try to improve the logic if possible.

 

[Bannar]

My way: 2 comparisons for change to ranged, 2 comparisons to change to melee
Your way: 2 comparisons to change to ranged, 3 comparisons to change to melee

What the hek?
Nested 'if' is pointless, since it does nothing if not LoadBoolean(HT, GetHandleId(u1), 0) returns false.

Omg I hate seeing 'your suggestion will fail' I had to go a walk with my dog and I've said 'I'll review that in second again'. But surely my statement is true:

your 'if' spam can and should be prevented.
My script will be fixed in sec.

EDIT: Fixed. There are two options: either the one, I've posted above, or check all the conditions that lead to: 'return' statement; and in 'else' implement 'u2 =' selection. I have not posted the 2nd case, because calling so many statements in 'single/double if' may fuck up readability and may be slower than 1st method.

EDIT2: I see you have changed Action function in meanwhile.. Anyways, this one can be improved too:

trig

        local unit u1
        local unit u2
        local integer id = GetHandleId(GetTriggerEventId())
        
        if id == 18 then // A Unit is Attacked
            set u1 = GetAttacker()
            if HaveSavedInteger(HT, GetUnitTypeId(u1), 1) then
                set u2 = GetTriggerUnit()
            else
                set u1 = null
                return
            endif
        elseif id == 60 then // A Unit Acquires a Target
            set u1 = GetTriggerUnit()
            if HaveSavedInteger(HT, GetUnitTypeId(u1), 1) then
                set u2 = GetEventTargetUnit()
            else
                set u1 = null
                return
            endif
        else // A Unit Is Issued an Order Targeting an Object
            set u1 = GetTriggerUnit()
            if HaveSavedInteger(HT, GetUnitTypeId(u1), 1) then
                set u2 = GetOrderTargetUnit()
                if GetIssuedOrderId() == 851971  then   // Order is smart
                    if not IsUnitEnemy(u2, GetOwningPlayer(u1)) or GetOrderTargetItem() != null then
                        set u1 = null
                        set u2 = null
                        return
                    endif
                elseif GetIssuedOrderId() != 851983 then // Order is not attack
                    set u1 = null
                    set u2 = null
                    return  // Spells won't trigger the system
                endif
            else
                set u1 = null
                return
            endif
        endif

        local unit u1
        local unit u2
        local integer id = GetHandleId(GetTriggerEventId())

        if id == 18 then // A Unit is Attacked
            set u1 = GetAttacker()
            if HaveSavedInteger(HT, GetUnitTypeId(u1), 1) then
                set u2 = GetTriggerUnit()
            else
                set u1 = null
                return
            endif
        else
            set u1 = GetTriggerUnit()
            if HaveSavedInteger(HT, GetUnitTypeId(u1), 1) then
                if id == 60 then
                    set u2 = GetEventTargetUnit()
                else
                    set u2 = GetOrderTargetUnit()
                    set id = GetIssuedOrderId()
                    if (id == 851971 and (not IsUnitEnemy(u2, GetOwningPlayer(u1)) or GetOrderTargetItem() != null)) or id != 851983 then
                        set u1 = null
                        set u2 = null
                        return
                    endif
                endif
            else
                set u1 = null
                return
            endif
        endif

 

[Maker]

Trig

if SquareRoot((GetUnitX(u1)-GetUnitX(u2))*(GetUnitX(u1)-GetUnitX(u2))+(GetUnitY(u1)-GetUnitY(u2))*(GetUnitY(u1)-GetUnitY(u2))) > LoadReal(HT, GetUnitTypeId(u1), 0) and not LoadBoolean(HT, GetHandleId(u1), 0) then
            call ToggleMeleeRanged(u1, u2, true)
        elseif LoadBoolean(HT, GetHandleId(u1), 0) then
            call ToggleMeleeRanged(u1, u2, false)
        endif

Translates into:
If in ranged distance and in melee mode
----change to ranged
else if in ranged mode
----change to melee

Spot the fail.

Answer

Results the unit to change to melee if it is in ranged distance and in ranged mode

The other suggestion causes syntex error (i instead of id) and won't make the unit change the type when smart is ordered.

I appreciate you trying to help, I really do. I'm very thankful for taking the time to post and taking a look at my spell. But please test before posting. I have wasted much time trying your solutions.

I will update the code soon.

EDIT:Updated to 1.0.0.4

 

[Magtheridon96]

    private function Delay3 takes nothing returns nothing
        local unit u = FirstOfGroup(G1)
        call DisableTrigger(TRG)
        loop
            static if LIBRARY_LastOrder then
                if GetPastOrder(u, 3) != null then
                    call IssuePastOrder(u, 3)
                endif
                call GroupRemoveUnit(G1, u)
                set u = FirstOfGroup(G1)
                exitwhen u == null
            endif
        endloop
        call EnableTrigger(TRG)
    endfunction

This hits the op limit without LastOrder and crashes the calling thread.

->

    private function Delay3 takes nothing returns nothing
        local unit u = FirstOfGroup(G1)
        call DisableTrigger(TRG)
        loop
            static if LIBRARY_LastOrder then
                if GetPastOrder(u, 3) != null then
                    call IssuePastOrder(u, 3)
                endif
                call GroupRemoveUnit(G1, u)
                set u = FirstOfGroup(G1)
                exitwhen u == null
            else
                exitwhen true
            endif
        endloop
        call EnableTrigger(TRG)
    endfunction

Or, better yet, comment out the entire function if LastOrder isn't present.

if SquareRoot((GetUnitX(u1)-GetUnitX(u2))*(GetUnitX(u1)-GetUnitX(u2))+(GetUnitY(u1)-GetUnitY(u2))*(GetUnitY(u1)-GetUnitY(u2))) > LoadReal(HT, GetUnitTypeId(u1), 0) then
            if not LoadBoolean(HT, GetHandleId(u1), 0) then
                call ToggleMeleeRanged(u1, u2, true)
            endif
        elseif LoadBoolean(HT, GetHandleId(u1), 0) then
            call ToggleMeleeRanged(u1, u2, false)
        endif

Here, you should be caching the coordinates in local variables for increased performance.

There are lots of other possible optimizations too, but I'm sure Bribe and Spinnaker stated them above (lol didn't read xD)

 

[Maker]

^
There is

static if LIBRARY_LastOrder then
    call GroupAddUnit(G1, u)
    call TimerStart(T1, 0.00, false, function Delay3)
endif

to prevent it from going to the loop

I'll propably do something with the reals.

EDIT: v1.0.0.5
-Got rid of SquareRoot in the distance check

 

[Magtheridon96]

Oh and I just updated RegisterPlayerUnitEvent for your sake
I added a vanilla Jass version that compiles in the normal editor.
Thinking of using it now? (It will optimize large maps with lots of triggers by literally several thousands of times on event firing )

This is actually a really good library ^_^

 

[Maker]

Yeah, it is a good library

-Added local reals for distance calculation

 

[Bannar]

Maker, I wasn't near WE so I couldn't test solutions in game. I was giving ideas since there was a space for improvements.

Since you've changed your Action function during I was writing one of post it was 100% sure that was useless then.
However, as I can see you've used my idea about removing one of 'if' statement and replaced that with

        if id == 60 then
            set u2 = GetEventTargetUnit()
        else

GetIssuedOrderId() could be replaced with 'id' integer; two calls < one call + set local
I'll run some tests tomorrow.

 

[Maker]

GetIssuedOrderId() could be replaced with 'id' integer; two calls < one call + set local

Yeah, it could. But there won't be two calls made every time, so in some cases that would lead to worse performance.

 

[RakEnRoL]

Nice timing. I need this. Great job, Maker. I will be using it after it gets approved.

 

[Marsal]

Well since I can understand shit of this code I guess it works like the battle system in Warhammer 40k so it's awesome.

 

[zv27]

private function DisableAbil takes integer a returns nothing
local integer i = 0
loop
call SetPlayerAbilityAvailable(Player(0), a, false)

should be
"call SetPlayerAbilityAvailable(Player(i), a, false)"
exitwhen i == 15
set i = i + 1
endloop
endfunction

 

[Maker]

Well since I can understand shit of this code I guess it works like the battle system in Warhammer 40k so it's awesome.

No need to guess if you try it

call SetPlayerAbilityAvailable(Player(0), a, false)

should be
call SetPlayerAbilityAvailable(Player(i), a, false)

Fixed

 

[Bribe]

I don't see any instances of "DestroyTrigger", we could be looking at lots of event leaks as the game progresses.

I recommend using the approach I did in DamageEvent (Nestharus used it in his vJass damage event library). Basically every 15 or so removed units, you destroy the trigger and re-add all the events of the still-existing units.

 

[Maker]

v1.0.1.6
-Added trigger recycling, events for removed units are cleared
-Uses u2 == null instead of GetManipulatedItem != null
-Static if now covers all actions in Delay3 function

 

[Bannar]

        local trigger t1 = CreateTrigger()
        local trigger t2 = CreateTrigger()
        local region r = CreateRegion()
        
        call RegionAddRect(r, bj_mapInitialPlayableArea)
        call TriggerAddCondition(t1, function Enter)
        call TriggerRegisterEnterRegion(t1, r, null)
        call TriggerAddCondition(t2, function Die)
        call TriggerRegisterAnyUnitEventBJ(t2, EVENT_PLAYER_UNIT_DEATH)
        set t1 = null
        set t2 = null

->>

        local trigger t = CreateTrigger()
        local region r = CreateRegion()
        
        call RegionAddRect(r, bj_mapInitialPlayableArea)
        call TriggerAddCondition(t, function Enter)
        call TriggerRegisterEnterRegion(t, r, null)
        set t = CreateTrigger()
        call TriggerAddCondition(t, function Die)
        call TriggerRegisterAnyUnitEventBJ(t, EVENT_PLAYER_UNIT_DEATH)
        set t = null

Marginal issue: 0.0 can be set to just 0.

 

[Bribe]

Instead of using a condition it could use the filter function, and instead of GetTriggerUnit() it would then use GetFilterUnit().

The only thing you can't use that filter for is for GetTriggeringTrigger() or GetTriggeringRegion().

 

[Bribe]

I almost thought this library could make use of the GroupTargetOrderById function but sadly not

Would have been cool though.

You can use "elseif" in static if's so you don't need a seperate "static if" block for that.

The dynamic trigger "TRG" should not have the TriggerRegisterAnyUnitEventBJ events attached to it. Those should be in a seperate trigger.

You must use trigger conditions for dynamic triggers, because trigger actions are not recycled properly when the trigger is destroyed (conditions are).

 

[Maker]

I'll update it. I have made it use a unit indexer, so it handles units that are revived correctly among other things.

 

[Maker]

Updated to v1.1.0.0
-Combined Enter and UnitFilt functions
-Removed two integer globals
-Uses unit indexer to correctly handle revived units
-Removed unneeded variables from setup function
-Small static if change
-Two more static ifs, in Delay2 and Toggle functions
-Added a trigger to which unit acquires target events are added
-Uses filter function instead of condition for units that enter the map

 

[Maker]

Updated to v1.1.1.1, check change log.

 

[Atideva]

Can you add "Animation tags" change.
Some models have different anim tags for melee and range attacks.
Need to change them, using your system, please.

 

[Mr.Foxy]

How to use it?

 

[-Kobas-]

NOTE: Try to replace "metamorphosis" with "bear form", there are bugs with first one because of animations, if you use unit with few animation categories they can mix. I lost 4 hours on fixing simple system, I hope that this will help you guys so you don't bother.
I managed to fix this shit, I'm still confused was this above real reason why it failed so many times.

 

[defskull]

Maker, can you do like change the unit from Melee to Range (vice-versa) which only requires 1 unit ?
It would be more awesome
I hate to copy each unit...

 

[Sunchips]

I tried to implement this system in my map but for some reason it messed up my units' max health. Suddenly all units on the map has like 100879 max health. Why is this? I use another unit indexer for Zwiebels threat system if that has any relevance.

 

[Maker]

The reason is most likely that I (and you) have used Metamorphosis as the base ability, it can cause such behaviour. It would be better to use Bear Form as the base ability.

 

[Duke]

Hey, so I tried to use this system in my campaign, but it didn't work (got black screen after selecting mission). So I created a new empty campaign, imported the system map and tested it again. This time the mission loads fine, but the system does not work.
Any idea how to make this work in a campaign? Would highly appreciate it!