[Snippet] RegisterPlayerUnitEvent

[Magtheridon96]

Fine D:
I'll put them back

edit
In my defense:
You don't read the code, the code reads YOU

 

[BBQ]

EDIT: I must be blind.

Feel free to delete this post.

 

[Magtheridon96]

I didn't see the original post
What exactly was in it? ^^

 

[BBQ]

I missed the previous page where you had discussed And() and Or(). I mentioned that this library will fail if the first boolean passed to Or() returns true, due to the fact that And() and Or() use short-circuit evaluation.

For example, this code

//! zinc
library Test requires RegisterPlayerUnitEvent
{
    function onInit()
    {
        RegisterPlayerUnitEvent(EVENT_PLAYER_UNIT_DEATH, function() -> boolean
        {
            BJDebugMsg("A " + GetUnitName(GetTriggerUnit()) + " has died!\nThe first response is being evaluated!");
            return true;
        });
        RegisterPlayerUnitEvent(EVENT_PLAYER_UNIT_DEATH, function() -> boolean
        {
            BJDebugMsg("A " + GetUnitName(GetTriggerUnit()) + " has died!\nThe second response is being evaluated!");
            return false;
        });
        KillUnit(CreateUnit(Player(0xF), 'hfoo', 0., 0., 0.));
    }
}
//! endzinc

will result in only the first response being evaluated, with the second one being completely ignored (because it is enough for the first response to be evaluated in order for the whole expression to be considered true).

But not only that I "found" the previous page, I also noticed that the documentation mentions that it is mandatory for the dummy conditions to return false (or nothing).

 

[Troll-Brain]

If i remember correctly Or() , And() creates a new boolexpr each time they are called, no matter that the boolexpr arguments have already be used/created or not.
So actually for a speedfreak reason (without any valid benchmark providen), you introduce a leak, awesome !

 

[Bribe]

As they are never destroyed, how is it a leak?

Unless they work like RegionAddRect, where you can remove the rect but the region still remembers it, you will need to not destroy the old as you add to it with a new "And".

 

[Magtheridon96]

Disclaimer:
I recommend using SpellEffectEvent for spells instead of using this function because SpellEffectEvent is faster (when it comes to spells )

 

[Troll-Brain]

You are missing something Bribe.
Each time Or(), And() is called, a new boolexpr is created, that doesn't work the same as Filter(), Condition(), so you can safely destroy the previous one, since in this script it will be used only one time. (But in fact you just can't destroy it here, since boolexpr variables are passed by reference)
So yes an unused left object with no reference to it is a leak.

library RegisterPlayerUnitEvent // Special Thanks to Bribe
    globals
        private trigger array t
        private boolexpr array b
    endglobals
    function RegisterPlayerUnitEvent takes playerunitevent p, code c returns nothing
        local integer i = GetHandleId(p)
        local integer j = 16
        if null == t[i] then
            set t[i] = CreateTrigger()
            loop
                set i = i - 1
                call TriggerRegisterPlayerUnitEvent(t[i], Player(j), p, null)
                exitwhen 0 == i
            endloop
            set b[i] = Filter(c)
        else
            call TriggerClearConditions(t[i])
            set b[i] = Or(b[i], Filter(c)) // will leak once it will be the third time and more 
        endif
        call TriggerAddCondition(t[i], b[i])
    endfunction
endlibrary

Also again, nothing proves that is faster than a triggercondition.
What about limit op, is it reset when the second boolexpr is evaluated ?

Plus, like BBQ said it's error prone, even if i'm agree that the boolean returned in a such boolexpr should be false, if the user returns true it will work on a triggercondition, since all triggerconditions are always evaluated, no matter if one or more previous one returns false.
Triggerconditions don't use short-circuit evaluation.

 

[Magtheridon96]

That's why I added this:

You must return false or nothing

I'm not sure about the presence of that leak though.
Maybe if you call this function 1000 times a second, you could monitor how much RAM wc3 is using, and if the amount is increasing greatly, a leak is in fact present.

 

[Bribe]

The benchmarks were probably done during the time of Reinventing the Craft
so we could use some more current ones.

But I imagine the efficiency has to do with because And/Or are more low-level
than triggerconditions which have more features like TriggerRemoveCondition, TriggerAddCondition and TriggerClearConditions, not to mention IsTriggerEnabled
and DestroyTrigger.

 

[Troll-Brain]

@Magtheridon96 : A warning doesn't change the fact that is bug prone, but nevermind.

@Bribe : While it makes sense and all, it still should be confirmed.

But that doesn't change the fact that is leak prone (even if it doesn't matter that much).

A limit op test should also be done.

Also i've just realized that even if it's reset (need to be tested) on the second boolexpr evaluated, it won't allow several huges codes together.

If for example you have 3 codes which all by themselves nearly reach the limit op.
Then with a merged Or(), it will obviously reach the limit op before the end of the third boolexpr.

You can argue that is a bad pratice though, i'm just trying to catch every limitations.

 

[Magtheridon96]

Good point Troll-Brain

After some tests are done (Not me xD), I'll decide exactly what to do.

 

[Bribe]

Every parameter that takes "code" as an argument reaches that code with
the same general pattern:

Find code,
Reset op limit,
Start new thread so if this thread crashes the previous one can continue,
???
PROFIT!

I'll dew some benchmarks tonight.

 

[Troll-Brain]

Again, you are missing something (even if it's a general rule which can be applied for And(), Or() ) :

function F1 ...
// limit op nearly reached

function F2 ...
// limit op nearly reached

function F3 ...
// limit op nearly reached

TriggerAddCondition(trig, And ( And(F1,F2) , F3 ) // hopefully you get the idea, add function and filter yourself

F2 will likely never be finished since the huges boolexpr F1 and F2 will be merged.

Or you are right, but a test is still needed.

 

[Magtheridon96]

Find code,
Reset op limit,
Start new thread so if this thread crashes the previous one can continue,
???
PROFIT!

Clever!

I'll dew some benchmarks tonight.

Thanks for investing time into this resource
(Benchmarks aren't really my thing )

 

[Bribe]

They aren't really mine either but I can manage it I hope.

The op limit is easy to test though.

function TestOp takes nothing returns boolean
    local integer i = 10000
    loop
        set i = i - 1
        exitwhen 0 == i
    endloop
    call BJDebugMsg(I2S(i))
    return true
endfunction
function InitTrig_Test takes nothing returns nothing
    call ForceEnumPlayersCounted(CreateForce(), And(Filter(function TestOp), And(Filter(function TestOp), And(Filter(function TestOp), And(Filter(function TestOp), Filter(function TestOp))))), 1)
endfunction

All you have to do is copy and paste that into an empty trigger and see if it displays 0 for everything.

 

[Magtheridon96]

Amazing!
I tested it with 65 boolexprs that loop 10000 times and it didn't hit the op limit

I used this:

library Test initializer Init
    globals
        private integer AA = 0
    endglobals
    
    private function TestOp takes nothing returns boolean
        local integer i = 10000
        loop
            set i = i - 1
            exitwhen 0 == i
        endloop
        set AA=AA+1
        return false
    endfunction
    
    private function Init takes nothing returns nothing
        local boolexpr b = Filter(function TestOp)
        local integer i = 104
        loop
            exitwhen 0==i
            set b = Or(b,Filter(function TestOp))
            set i=i-1
        endloop
        call ForceEnumPlayersCounted(CreateForce(),b,1)
        call DisplayTimedTextToPlayer(GetLocalPlayer(),0,0,60,I2S(AA))
    endfunction
endlibrary

 

[Troll-Brain]

Good to know.

About the leak check yourself :

globals
    timer Tim = CreateTimer()
    integer I = 10000 // ms
endglobals

function F1 takes nothing returns boolean

    call DoNothing()
    return false
endfunction

function F2 takes nothing returns boolean

    call DoNothing()
    call DoNothing()
    return false
endfunction

function Test takes nothing returns nothing
    set I = I-1
    if I <= 0 then
        call PauseTimer(GetExpiredTimer())
        return
    endif
    call Or(null,null)
endfunction

function StartTest takes nothing returns boolean
    call TimerStart(Tim,0.001,true,function Test)
    return false
endfunction

//===========================================================================
function InitTrig_test takes nothing returns nothing
    local trigger trig = CreateTrigger()
    call TriggerRegisterPlayerEventEndCinematic(trig,GetLocalPlayer())
    call TriggerAddCondition(trig,function StartTest)
    call BJDebugMsg(I2S(GetHandleId(Or(null,null))))
    call BJDebugMsg(I2S(GetHandleId(Or(null,null))))
    call BJDebugMsg(I2S(GetHandleId(Or(function F1,function F2))))
    call BJDebugMsg(I2S(GetHandleId(Or(function F1,function F2))))
endfunction

I got 2768 k of memory used, and for windows task manager i suppose 1 k = 10³ octets

So that would mean one boolexpr created with Or() takes about 276.8 octets.

Note that is an empty boolexpr, a real one could take more (or less ?!).

Yes i know this is an urealistic case. And ofc there is also the handle id leak, which should be worse than the "simple" memory one because the virtual jass machine slown down as many handles id are used.

Also what about an unregister ?

 

[Bribe]

Unregister doesn't need to be supported by this. A seperate library could
have its own stack which use the unregister/disable features.

 

[Troll-Brain]

I quite disagree since there is the old way destroy trigger with the native way.
Even if i'm agree that such events don't really need to be dynamic.

 

[Bribe]

A seperate library running on dynamic event registry would handle something like this for the user, or Magtheridon could add built-in support (an extra function, for example) for dynamic registry. It doesn't really matter though too much, though sometimes if you want to enable an event for only a frame and then disable it right after, it makes sense to be able to have such an option.

 

[Troll-Brain]

I don't know what i had in mind but the leak can be fixed :

library RegisterPlayerUnitEvent // Special Thanks to Bribe
    globals
        private trigger array t
        private boolexpr array b
    endglobals
    function RegisterPlayerUnitEvent takes playerunitevent p, code c returns nothing
        local integer i = GetHandleId(p)
        local integer j = 16
        local boolexpr b
        if null == t[i] then
            set t[i] = CreateTrigger()
            loop
                set i = i - 1
                call TriggerRegisterPlayerUnitEvent(t[i], Player(j), p, null)
                exitwhen 0 == i
            endloop
            set b[i] = Filter(c)
        else
            call TriggerClearConditions(t[i])
            set b = Or(b[i], Filter(c))
            call DestroyBoolExpr(b[i]) // you can also check if b[i] != null before destroy it
            set b[i] = b
            set b = null
        endif
        call TriggerAddCondition(t[i], b[i])
    endfunction
endlibrary

EDIT : Or not ...

It needs to be checked as destroying the previous boolexpr could break the new one.
At least i've found what i had in mind ...

 

[Bribe]

I was thinking of doing the same thing, but it not working correctly if you destroy it just means that it's a one-way street, once you register it you are stuck with it.

In which case it's still fine, because it still uses the same handle count as triggerconditions would. Provided there is a speed advantage.

 

[Troll-Brain]

Speed advantage must still to be proven (yes i'm annoying).
In fact more i see new scripts, more i'm sicked.

First TriggerConditions instead of triggerActions, then short name, then struct allocators inlined, now this, what's next ?

Don't take me wrong i know the advantage for triggerconditions, beyond speed, they don't need to be destroyed when you destroy a trigger.
But coding in (v)jass becomes a nightmare in general with all this new speedfreak standards.

I will wait your benchmark, but i would'nt be suprised if that doesn't matter that much , we are talking about events anyway, not a huge loop, fast periodic timer or whatever else ...

Also if you use this, then there is no more good way to unregister/disable since it's all merged together.
Ofc you could use a (double) linked list, remove all and build again the super Or() with all but not the removed boolexpr, but that is lame ...

But ok you can argue about the usefulness of a such thing.

Time for me to stop comments about resources i think.

 

[Magtheridon96]

then struct allocators inlined

That's actually optional
We do it for an extra speed boost.. and because we're cool


And for the 'unregister' thing to work, I'd have to move back to the old method (Adding trigger conditions)

 

[Troll-Brain]

Cool != freak/crazy

My main complain is that you always rush to the omg fastest solution to win few nanoseconds, while it doesn't matter most of time.
Worse, it makes the code more spaghetti, and sometimes, such as here, it even removes some possibilities.

Not to mention that the speed advantage has still to be proven and in which scale it improves the speed or not ...

 

[Magtheridon96]

Even if I had the chance, I wouldn't make an unregister method because this wasn't made to be used for Dynamic events.
If you want dynamics, you could use a boolean inside the desired function.

edit

Just for the lolz:

library ImplementThis requires FatalError

    static if LIBRARY_GTrigger then
        private struct AwesomeStruct extends array
            private static method onInit takes nothing returns nothing
                call ExitWarcraft()
            endmethod
        endstruct
    endif

endlibrary

 

[BBQ]

Could you compare this version (I'm unable to test anything, sorry) against yours, as far as leakage is concerned:

//! zinc
library RegisterPlayerUnitEvent
{
	private
	{
		trigger          eventTriggers    [8190];
		boolexpr         eventResponses   [8190];
		triggercondition currentCondition [8190];
	}
	
	public function RegisterPlayerUnitEvent(playerunitevent whichEvent, code whichResponse)
	{
		integer  eventId = GetHandleId(whichEvent), i;
		boolexpr previousResponse;
		
		if (eventTriggers[eventId] == null)
		{
			eventTriggers [eventId] = CreateTrigger();
			eventResponses[eventId] = Filter(whichResponse);
			
			for (i = 15; i >= 0; i -= 1)
				TriggerRegisterPlayerUnitEvent(eventTriggers[eventId], Player(i), whichEvent, null);
		}
		else
		{
			TriggerRemoveCondition(eventTriggers[eventId], currentCondition[eventId]);
			
			previousResponse = eventResponses[eventId];
			eventResponses[eventId] = Or(previousResponse, Filter(whichResponse));
			
			DestroyBoolExpr(previousResponse);
			previousResponse = null;
		}
		
		currentCondition[eventId] = TriggerAddCondition(eventTriggers[eventId], eventResponses[eventId]);
	}
}
//! endzinc

Note that it may not even compile (although it should) and may be greatly flawed, 'cos I wrote it just like that, out of boredom (and to test my vJass & Zinc editing modes for jEdit), a few minutes ago.

 

[Bribe]

I agree that the boolexprs should be destroyed. Creating a boolexpr with "Or" works the same as adding a rect to a region - you can change the rect or remove it but the region stays the same.

This prints 3, as it should:

function Blah takes nothing returns boolean
    set bj_forLoopAIndex = bj_forLoopAIndex + 1
    return false
endfunction

function InitTrig_Untitled_Trigger_003 takes nothing returns nothing
    local boolexpr b = Filter(function Blah)
    local boolexpr be = Or(b, b)
    local boolexpr be2 = Or(be, b)
    call DestroyBoolExpr(be)
    set bj_forLoopAIndex = 0
    call ForceEnumPlayers(bj_FORCE_ALL_PLAYERS, be2)
    call BJDebugMsg(I2S(bj_forLoopAIndex))
endfunction

So it seems destroying the old boolexpr is worth it. However, then you get into a lot of complexity like "when should I destroy the boolexpr" - do you want to destroy the first boolexpr passed in that you created via Filter()? Probably not.

I will look into this a little bit more to see how much speed difference we get between trigger conditions and this Or() thing.

 

[BBQ]

The version I posted doesn't even work. So it is completely useless (even though it is leakless). The original version leaks a bit, but gets the job done.

 

[Bribe]

Why doesn't it work? What I posted works.

 

[BBQ]

I've no idea... that's the whole problem. If I evaluate the boolexpr the way you did (or evaluate the trigger itself), everything works the way it should. However, when I try to evaluate it by triggering the event, only the last one is evaluated (fucking weird).

Your version doesn't work either as long as the boolexpr is attached on a trigger and is not evaluated manually - but by an event (which is, well, the whole point of this snippet).

 

[Bribe]

Interesting that an event would work differently. I will try use it after a "wait", maybe that is why.

Yep - new results are in.

function Blah takes nothing returns boolean
    set bj_forLoopAIndex = bj_forLoopAIndex + 1
    return false
endfunction

function InitWait takes nothing returns nothing
    local boolexpr b = Filter(function Blah)
    local boolexpr be = Or(b, b)
    local boolexpr be2 = Or(be, b)
    call DestroyBoolExpr(be)
    call TriggerSleepAction(0)
    set bj_forLoopAIndex = 0
    call ForceEnumPlayers(bj_FORCE_ALL_PLAYERS, be2)
    call BJDebugMsg(I2S(bj_forLoopAIndex))
endfunction

function InitTrig_Untitled_Trigger_003 takes nothing returns nothing
    call ExecuteFunc("InitWait")
endfunction

Only displays "1". So destroying the boolexpr is dangerous and should never be done.

 

[BBQ]

Okay, that is definitely fucking weird.

I have this code:

//! zinc
library Test requires RegisterPlayerUnitEvent
{
    function onInit()
    {
        unit u = CreateUnit(Player(0xF), 'hfoo', 0., 0., 0.);
        RegisterPlayerUnitEvent(EVENT_PLAYER_UNIT_ISSUED_ORDER, function() { BJDebugMsg("a"); });
        RegisterPlayerUnitEvent(EVENT_PLAYER_UNIT_ISSUED_ORDER, function() { BJDebugMsg("b"); });
        RegisterPlayerUnitEvent(EVENT_PLAYER_UNIT_ISSUED_ORDER, function() { BJDebugMsg("c"); });
        RegisterPlayerUnitEvent(EVENT_PLAYER_UNIT_ISSUED_ORDER, function() { BJDebugMsg("d"); });
        RegisterPlayerUnitEvent(EVENT_PLAYER_UNIT_ISSUED_ORDER, function() { BJDebugMsg("f"); });
        IssueImmediateOrder(u, "stop");
    }
}
//! endzinc

and everything works properly.

However, if I pre-place a unit on the map by myself, and manually issue a "stop" order to it, only the last message is displayed. WHAT IN THE BLOODY HELL?

Alright, it's the wait indeed. Fuck Blizzard.

So now, if the boolexpr is evaluated during map initialization (rofl), everything goes properly, whereas if it's evaluated somewhere mid-game, only the last "piece of it" is evaluated... it doesn't make any sense.

 

[Bribe]

Yeah, I guess the boolexpr doesn't actually destroy until after a 0-second timer.

 

[Dirac]

Do realize that those anonymous functions you're creating through zinc do not return boolean false, that might be causing some issues.

 

[BBQ]

Do realize that those anonymous functions you're creating through zinc do not return boolean false, that might be causing some issues.

Nope, as long as the function doesn't return true, there are no problems. The error is definitely not on my behalf.

 

[Magtheridon96]

That's quite wierd...

I'm pretty sure it has to do with how they initialize their 'functionList' class...
F- Blizzard indeed.

 

[Bribe]

Please keep the language G-rated (for those not familiar, that's a USA movie rating that doesn't allow any bad language or such).

 

[Magtheridon96]

Please keep the language G-rated (for those not familiar, that's a USA movie rating that doesn't allow any bad language or such).

Sorry about that Bribe :3

 

[Dirac]

I've heard that Filtering functions that doesn't return boolean causes errors on Mac

 

[Bribe]

I have also heard this. Though the only Mac I have access to is the iMac is at work and I can't install stuff on it.

 

[BBQ]

I've heard that Filtering functions that doesn't return boolean causes errors on Mac

According to a friend of mine, Blizzard did a ninja-fix on that (quite a long time ago), along with some other Mac-related issues.

 

[BBQ]

Okay, I ran a few very quick tests to compare the whole Or() thingy to just adding plain conditions. As it turns out, multiple conditions end up being faster (or at least that's what my, again, very quick tests showed).

I had registered the same event 100 times on a trigger, first using the technique this snippet does, and then did the same but without merging the boolexprs.

The trigger was evaluated 1100 times per second (11 times per 0.01 second), which in the first case (this snippet) yielded 11-14 FPS, as opposed to the second case, which yielded 14-16 FPS.

With that being said, and the fact that Or() is prone to leaking, I suggest that you revert everything back to "normal" (it wouldn't be bad if you ran some more tests though).

 

[Bribe]

Or() returns a new boolexpr the same as TriggerAddCondition returns a new triggercondition. They both leak the same.

However I think it's generally a more smooth process to just add conditions on top, and it's safer as well because a bad return doesn't screw up the whole thing.

 

[Magtheridon96]

Ok, I guess changing this snippet back to it's original form shouldn't be a problem :3

edit
Updated.

 

[BBQ]

Or() returns a new boolexpr the same as TriggerAddCondition returns a new triggercondition. They both leak the same.

The thing is, TriggerClearConditions() does not free up the memory that the trigger conditions use up, it only detaches them from the trigger. So both "versions" leak trigger conditions (if you could call that leaking...).

 

[Bribe]

Yeah, then it makes sense to not use this "or" thing any further.

 

[Troll-Brain]

Last time i checked, use TriggerClearConditions free up the memory.
More : when you destroy a trigger you don't have to use it, trigger conditions never leak.

But that's exactly the opposite with trigger actions and also TriggerClearActions.

Or my tests were really wrong (or my memory fail).

If no one is willing to make a test, i will remake it, probably in the next days.

 

[Magtheridon96]

I'm going to make a vanilla Jass version of this so that GUIers and vanilla Jassers can enjoy the optimization too ^_^