[Snippet] RegisterPlayerUnitEvent

[Nestharus]

By the way, something that has always annoyed me... you have your arguments backwards.....

function RegisterPlayerUnitEvent takes playerunitevent p, code c returns nothing

vs

native TriggerRegisterPlayerUnitEvent takes trigger whichTrigger, player whichPlayer, playerunitevent whichPlayerUnitEvent, boolexpr filter returns event

Code first, then event >.>.


Because of this, your thing is extremely difficult to work with. Every time I need to change an event from a native or one of mine, I have to flip everything around just for your script... changing back means I have to flip back around. Furthermore, everything (from natives to my stuff) takes bool expressions. Just for your special thing, I have to remove the Condition around it. To go back, I have to add it back in. You have no idea how frustrating it is to work with a script that does not follow the standards set out by JASS natives... I'm thinking about not using your script anymore and just making my own that does follow standards because I am really getting tired of it.


Furthermore, I have to continue to keep reminding myself that your script does not follow the standards and is backwards + takes arguments that are not the norm. A script should make life easier, not give the user headaches like yours does >.>.

In fact, in my maps, I am now making yours a wrapper for my own standard version >.>.


This here makes infinitely more sense
function RegisterPlayerUnitEvent takes boolexpr c, playerunitevent p returns nothing

#1, it is in the correct order, so if someone needs to go between natives and yours, they don't have to rewrite everything.
#2, it takes the correct argument types, so if someone needs to go between natives and yours, they don't have to redo the code argument.


Why this was approved in its current state I have no idea, but the API is written poorly.

 

[watermelon_1234]

I don't understand why you're complaining when the native version has the event listed first and then the boolexpr. In my opinion, it makes more sense to have the event first since you're registering the code to that event; the important thing comes first.
Honestly, I don't even see how hard it is to simply change some words around and if you made your own wrapper, that's good for you.

 

[Nestharus]

The thing being registered in the natives (the trigger) goes before the event being registered ;o.

To give you another idea, instead of TriggerRegisterEvent, it'd be CodeRegisterEvent, or BooleanExpressionRegisterEvent.

 

[Bribe]

Taking boolexpr's as arguments was never a good practice in the first place. It just adds verbosity on the user's side.

function RegisterPlayerUnitEvent takes @playerunitevent@ p, @code@ c returns nothing 

vs

native TriggerRegisterPlayerUnitEvent ... @playerunitevent@ whichPlayerUnitEvent, @boolexpr@ filter returns event

I see nothing wrong with the order above, in fact tying to use it as example API disproves your own API.

 

[Nestharus]

The code being registered on PlayerUnitEvent isn't a filter, it is the thing being registered, just like the trigger on the trigger registration isn't the filter, it is the thing being registered ;\.

 

[Bribe]

Hmm, looking at it that way, then there should be two functions that have to be called, too.

With seriousness, at this point him trying to change the API is almost as bad as me trying to change the API of NewTable.

And if he were to do it, the function named "RegisterPlayerUnitEventForPlayer" should be called "RegisterPlayerUnitEvent", and the existing function should be called "RegisterAnyPlayerUnitEvent".

Your Event library is also too late to take "code" as arguments as well, even though that would really eliminate a lot of verbosity.

 

[Nestharus]

And if he were to do it, the function named "RegisterPlayerUnitEventForPlayer" should be called "RegisterPlayerUnitEvent", and the existing function should be called "RegisterAnyPlayerUnitEvent".

/agree. I'd go even farther to say that it should be CodeRegisterPlayerUnitEvent and CodeRegisterAnyPlayerUnitEvent ; P.

 

[Magtheridon96]

*The awkward moment when I change the API*
Mwahahahaha!

I wish I could change the API :/
I would've gone with RegisterPlayerUnitEvent and RegisterAnyPlayerUnitEvent because those are way better D:
RegisterPlayerUnitEventForPlayer is only for mapmakers who are registering 10-20 events in the same function =p
It would make sense to make 10-20 iterations instead of 160-320 xD

 

[Nestharus]

Uh, add to the API then so as not to break backwards compatibility ; P.

 

[Magtheridon96]

Updated with a new GetPlayerUnitEventTrigger function.
I needed it for RecipeEngine, else, an AFTER_COMPILE event would be impossible because of my recursive design.

 

[Ruke]

It would be nice if the functions returns the trigger. Also, you should add a filter on that, to avoid unnecessary registers.

Greetings, nice snippet .

 

[Magtheridon96]

You wouldn't need the trigger at all.
Each event has one trigger which is retrievable using GetPlayerUnitEventTrigger.
I only made that function because some people may really need it.

I'm using it in this Recipe system I'm writing so I can make the System's Event run before any of the game's Events.

The only things you could do with a returned trigger involve enabling and disabling or adding conditions.
If you wanted to enable/disable the trigger, you would be enabling or disabling all systems using the Event.
If you wanted to add anything, you would be using RegisterPlayerUnitEvent

The only time you would need the trigger is when you want to enable and disable to change the order of execution
of events, or to stop recursion, and in that case, you could use GetTriggeringTrigger to retrieve the trigger running
the function that is currently executing.

 

[Ruke]

Ah, you're right, I'm looking the code and you're right xD.
But, what's up with the filter?.

I mean, if you only have 1 player, it will still calling 15 times those functions?.

Greetings.

 

[Magtheridon96]

It won't be calling the function 15 times. To register a playerunitevent for a player is to make it possible for a player to trigger the event, running the code once for all players.

 

[Ruke]

It won't be calling the function 15 times. To register a playerunitevent for a player is to make it possible for a player to trigger the event, running the code once for all players.

If the player isn't there, why would I want to call:

call TriggerRegisterPlayerUnitEvent(t[i], Player(k), p, null)

?

 

[Magtheridon96]

There might be other units owned by that player even though he might not actually be playing.

Plus, if your map is single player and uses Neutrals, you can modify this so that it calls the function 5 times (1 for the player and 4 for the neutrals).

 

[Ruke]

There might be other units owned by that player even though he might not actually be playing.

Plus, if your map is single player and uses Neutrals, you can modify this so that it calls the function 5 times (1 for the player and 4 for the neutrals).

Well, in that case you can group all of the units of "k" Player. If they are more than 0, then call the function.

 

[Magtheridon96]

That wouldn't work, what if there are no units on the map for a certain player until after like 3 seconds of game time? D:

Plus, it's way faster to just edit the function to suit your needs

I might write a library to address this problem, so you probably won't have to edit /anything/

 

[Ruke]

That wouldn't work, what if there are no units on the map for a certain player until after like 3 seconds of game time? D:

Plus, it's way faster to just edit the function to suit your needs

I might write a library to address this problem, so you probably won't have to edit /anything/

When I came here to check answers, I was thinking on that xD.

 

[Magtheridon96]

MAJOR SPEED BOOST.
Got rid of one addition!
Now your maps are going to load MICROSECONDS faster!!

 

[Luorax]

MAJOR SPEED BOOST.
Got rid of one addition!
Now your maps are going to load MICROSECONDS faster!!

Aw yea! It's just great dude! Now my map will become popular, because the only reason why people didn't play it is because it took 8.00001 seconds to load, instead of 8 seconds.

 

[moyackx]

MAJOR SPEED BOOST.
Got rid of one addition!
Now your maps are going to load MICROSECONDS faster!!

Any microsecond counts, any microsecond, that's the reason I love you, you code bastard

 

[Adiktuz]

[PokemonStyle]
Magtheridon Hyperspeed!!!
[/PokemonStyle]

 

[edo494]

Mag, isnt

            call TriggerRegisterPlayerUnitEvent(t[i], GetLocalPlayer(), p, null)

better then

            loop
                call TriggerRegisterPlayerUnitEvent(t[i], Player(k), p, null)
                exitwhen k == 0
                set k = k - 1
            endloop

?

 

[chobibo]

Wouldn't that desync as the trigger will be attached with a different event per machine?

 

[edo494]

well it shouldnt, because the event passed is the same for everyone, just the player changes, and its even faster, because it should run in time that it takes 1 call to TriggerRegisterPlayerUnitEvent

I tried that in my map, and it didnt seem to desync and it was running well(for all players)

 

[chobibo]

Then wouldn't a spell cast by Player 1's unit not be detected on Player 2's machine because that trigger isn't registered to detect Player 1's unit casts?

 

[Magtheridon96]

Yes, this would desync~

 

[TriggerHappy]

Yes, this would desync~

Are you sure?

I can see how it may actually work.

EDIT: just tested and yea it could cause desyncs.

 

[edo494]

it desyncs, tested:

scope s initializer init
    
    private function p takes nothing returns boolean
        call DisplayTimedTextToPlayer(GetLocalPlayer(), 0, 0, 10, "Died")
        return false
    endfunction
    
    private function init takes nothing returns nothing
        local trigger t = CreateTrigger()
        call TriggerAddCondition(t, Filter(function p))
        call TriggerRegisterPlayerUnitEvent(t, GetLocalPlayer(), EVENT_PLAYER_UNIT_DEATH, null)
        set t = null
    endfunction
    
endscope

host got message, others insta kick

 

[Adiktuz]

I think GetLocalPlayer doesn't just return a player, but alters how things are run too internally...

btw did you try to do it like this?

scope s initializer init
    
    private function p takes nothing returns boolean
        local player x = GetTriggeringPlayer //Not sure of the exact function
        if GetLocalPlayer() == x
            call DisplayTimedTextToPlayer(x,0, 0, 10, "Died")
        endif
        return false
    endfunction
    
    private function init takes nothing returns nothing
        local trigger t = CreateTrigger()
        call TriggerAddCondition(t, Filter(function p))
        call TriggerRegisterPlayerUnitEvent(t, GetLocalPlayer(), EVENT_PLAYER_UNIT_DEATH, null)
        set t = null
    endfunction
    
endscope

 

[TriggerHappy]

I think GetLocalPlayer doesn't just return a player, but alters how things are run too internally...

Wrong, It returns the local player.

 

[Adiktuz]

and let's you run things local to that player

 

[TriggerHappy]

and let's you run things local to that player

I think you really are misunderstanding what GetLocalPlayer does.

All it does it return a player handle, it doesn't cause anything to be handled different internally.

if (GetLocalPlayer() == Player(0) then
endif

This is how that piece of code translates for Players 1 & 2.

// player 1
if (Player(0) == Player(0)) then
endif
// player 2
if (Player(1) == Player(0)) then
endif

So as you can see the conditional will simply be false when when the person playing isn't Player 1 (red).

 

[Adiktuz]

oh wait... I forgot that the codes run individually in each comp...

 

[Magtheridon96]

There could be cases where it doesn't desync, but I haven't tested thoroughly enough. I just know for sure that when the string tables and handle tables are not _exactly_ identical on all ends, a desync will occur somewhere.

 

[edo494]

string tables most of the time are not in sync when you do things as silly as this:

local string s = ""
if IsPlayerInGroup(GetLocalPlayer(), GROUP_I_WANT_EFFECT_TO_SEE) then
    set s = "SomeEffectString\\NeverForgetDoubleBackSlash"
endif
call DestroyEffect(CreateEffect...(..., s, ...))

and I see no way this could potentionally desync

and yes I know you can check the opposite way, so you have

local string s = "SomeEffectString\\NeverForgetDoubleBackSlash"
if not IsPlayerInGroup(GetLocalPlayer(), GROUP_I_WANT_EFFECT_TO_SEE) then
    set s = ""
endif
call DestroyEffect(CreateEffect...(..., s, ...))

 

[Magtheridon96]

It has caused desyncs before for a user on here.

Actually, his problems were arising from the presence of a SubString call in a local code block.

 

[edo494]

leasson learned, Kids, never substring your strings in local code

 

[Zeatherann]

I don't think your array index is synced between adding an event for all players, and adding one for only one player. Why not use GetHandleId(playerevent) only?

 

[Bannar]

^ above +1

You can never actually retrieve "forPlayer" trigger with such approach. Honestly, I dont think separating individual and generic registering triggers should be done, when event fires you can always filter triggering player and get the job done. Unneccessary trigger handles anyone?

Btw, sucks that functions arent named RegisterPlayerUnitEvent and RegisterAnyPlayerUnitEvent. What was Maggy thinking about (or smoking) when he was uploading api with switched names?

 

[Cokemonkey11]

Well, this is awkward. After nearly 3 years I've started using this snippet.

* What range of values does GetHandleId(p) live on?

* You can cache Player(k) but that's minor

* Who requested RegisterPlayerUnitEventForPlayer() and why?

* Who the hell requested GetPlayerUnitEventTrigger() and why?

* Have you compared the performance of this vs the naive method for 10,100,1000 instances?

 

[Ceday]

* What range of values does GetHandleId(p) live on?

(Last time I tested)The native conversion functions, such as ConvertPlayerColor, ConvertPlayerUnitEvent.. etc, returns the handle id according to their parameter.For example GetHandleId(ConvertPlayerColor(90)) is equal to 90.So playerunitevent handle id range is minimum 18 maximum 277.

* Have you compared the performance of this vs the naive method for 10,100,1000 instances?

This doesn't change trigger's run performance, yes it slowers registration process a little but the point of snippet is reducing handle count anyway.

 

[Cokemonkey11]

(Last time I tested)The native conversion functions, such as ConvertPlayerColor, ConvertPlayerUnitEvent.. etc, returns the handle id according to their parameter.For example GetHandleId(ConvertPlayerColor(90)) is equal to 90.So playerunitevent handle id range is minimum 18 maximum 277.

Cool, thanks.

This doesn't change trigger's run performance, yes it slowers registration process a little but the point of snippet is reducing handle count anyway.

Are you sure? evaluating many conditions of one trigger may be marginally faster than evaluating one condition of many triggers

 

[PurgeandFire]

* You can cache Player(k) but that's minor

Player(k) changes within the loop (since it is iterating over k), so it can't be cached.

 

[Cokemonkey11]

Player(k) changes within the loop (since it is iterating over k), so it can't be cached.

You misunderstand.

See for example:

library Players initializer i requires optional AI
	globals
		public player locl
		public boolean array playing[12]
		public boolean array aiControlled[12]
		public boolean array alive[12]
		public player array id[12]
		public player west = Player(PLAYER_NEUTRAL_AGGRESSIVE)
		public player east = Player(PLAYER_NEUTRAL_PASSIVE)
		public player extra = Player(bj_PLAYER_NEUTRAL_EXTRA)
		public integer aliveCount = 0
		public integer aiCount = 0
	endglobals
	
	public function count takes nothing returns integer
		return aliveCount
	endfunction
	
	private function i takes nothing returns nothing
		local integer index=0
		set locl = GetLocalPlayer()
		loop
			exitwhen index>11
			set id[index] = Player(index)
			set playing[index] = (GetPlayerSlotState(id[index])==PLAYER_SLOT_STATE_PLAYING)
			set alive[index] = playing[index]
			set aiControlled[index] = (GetPlayerController(id[index])==MAP_CONTROL_COMPUTER)
			if aiControlled[index] then
				set aiCount = aiCount + 1
				static if LIBRARY_AI then
					call AI_addPlayer(id[index])
				endif
			endif
			if playing[index] then
				set aliveCount = aliveCount + 1
			endif
			set index=index+1
		endloop
		call AI_addPlayer(west)
		call AI_addPlayer(east)
	endfunction
endlibrary

Players_id[0] for example is a static player reference

 

[Ceday]

Are you sure? evaluating many conditions of one trigger may be marginally faster than evaluating one condition of many triggers

Hmm yeah you are right I didnt notice that.For example naive way fires 10 events and checks 10 conditions whereas this way fires 1 event and checks 10 conditions?

 

[PurgeandFire]

@Cokemonkey: Oh I see what you mean. Like GetPlayer. Same could be said for plenty of systems, but indeed it is minor.

 

[Magtheridon96]

* Who the hell requested GetPlayerUnitEventTrigger() and why?

Global event disables might be needed sometimes. Consider a recipe system where some drop events need to be done behind the scenes while remaining undetected during some sub-routine.

I wanted to use it at one point, and so I updated this resource to have it.

Someone else might need to manipulate the events globally for silent events at one point, so I was like "Why not?"

 

[IcemanBo]

Submission: [Snippet] RegisterPlayerUnitEvent v5.1.0.1 Date: 18 October 16 Status: Graveyard Note: The resource was previously approved and therefore should work fine. If you use this script already in your codes, it's okay to keep it there. But now we have this approved [Snippet] RegisterEvent pack, and so it's time to graveyard these specializations.