Player Resource Monitoring v1.01

[Moderator]

17:30, 11th March 2016
Tank-Commander: V1.01 - The indentation of one line in the initTrig function is wrong, but the system is in an approvable state - a very simple small piece of code; has some useful application (also just for future reference you don't need to name the trigger variable gg_trig(blah) can just use t as you had before)

Old Reviews

19:30, 10th March 2016
Tank-Commander: There's a few issues remaining with the system, please refer to my post and address them accordingly

00:20, 28th Feb 2016
Tank-Commander: Flux's post covers everything that needs to be said about the system

 

[Flux]

Took a quick look without testing,
- Why create function PRM_Init? Why not just put its content above where you called "PRM_Init"? If I'm not mistaken, Init_TrigPRM will be generated at the bottom of the script so you'll do fine.
- If a player loses gold and lumber at the same time, will trigger PRM run twice? If not, then you won't detect PRM lumber since you're using "elseif".
- You called GetTriggerPlayer twice in PRM_Monitor.
- You're using 1 trigger for all players that means players that didn't change gold/lumber will get registered again. I think it's more efficient to have 1 trigger per player to avoid the loop. Imagine a 12 player melee map, assume each player changes gold/lumber every sec, then your trigger will be 'refreshed' 12 times every second and in those 12 times, you have to loop at all players where 11 of them didn't change their gold/lumber at that point in time.
- Why not make this in JASS since more than 80% is in custom script?

 

[Chaosy]

it should not be else if, it should just be if.

Also, I second Flux's suggestion regarding the init function.

 

[Tank-Commander]

There's basically no reason for this to be in a GUI trigger format - you use so much custom script it should just be in JASS, other than that Flux covers pretty much everything else - it's pretty false to be claiming this is GUI/Triggers at this point (though it is GUI-Friendly)

 

[Chaosy]

There's basically no reason for this to be in a GUI trigger format - you use so much custom script it should just be in JASS, other than that Flux covers pretty much everything else - it's pretty false to be claiming this is GUI/Triggers at this point (though it is GUI-Friendly)

udg, not that debate again. Had it with the mods in the comments of my conversation system.

I'll just say I disagree and leave it at that.

 

[ZiBitheWand3r3r]

thanks guys for feedback

Init_TrigPRM will be generated at the bottom of the script

no, at the top, so it should be like it is ;]

If a player loses gold and lumber at the same time, will trigger PRM run twice?

no, once, but then next recreated trigger runs second time for lumber change. Try build barrack (cost gold + lumber), it works fine. It recreates trigger after any change detected.

You called GetTriggerPlayer twice in PRM_Monitor

ok, added local player. Got silly question: should I null local player or not? Im not sure

You're using 1 trigger for all players

ok I'll change: 1 trigger for each player. Every trigger with 2 events.

Why not make this in JASS since more than 80% is in custom script?

It is in vJass, but I copied to gui-trigger for one reason. Make it as easy for GUI users as possible. No worries for creating variables, just CnP one trigger.

it should not be else if, it should just be if.

Its fine in my opinion.. Trigger runs one time only for one event only, then is destroyed and new one is created. So if you build barracks (costs gld+lumber) then 1st event (gold spend) triggers --> trigger runs, trigger destroyed, new one created --> triggers 2nd event (lumber spend) this new created trigger runs. Thats the way I understand jass, correct me if I wrong.

I will update this today. zibi

ok, updated;]

 

[Flux]

no, at the top, so it should be like it is ;]

You're right, the Actions part are generated at the top, my mistake. But if it is written in JASS, then function PRM_Init is not needed so I suggest just write it in JASS.

no, once, but then next recreated trigger runs second time for lumber change. Try build barrack (cost gold + lumber), it works fine. It recreates trigger after any change detected.

I see, therefore 'elseif' is correct.

ok I'll change: 1 trigger for each player. Every trigger with 2 events.

You could add (if you want) a feature where users will be able to define who to monitor, something like PlayerResourceMonitor(player) and PlayerResourceUnmonitor(player). That way, it doesn't monitor neutral players' resources by default and gives better control.

It is in vJass, but I copied to gui-trigger for one reason. Make it as easy for GUI users as possible. No worries for creating variables, just CnP one trigger.

How does this make it "easy for GUI users" since almost all is in custom script? I think it would be better if it is in JASS (not vJASS) format since it still doesn't require JNGP but gives better readability.

Overall, it's a nice addition to the Spells Section, this is without a doubt a useful resource, well done.

 

[BPower]

Maybe you need to build in a recursion safety.
I can see users adding resource on resource event.
In that case it's a con that it's impossible to disable the custom event trigger.

You should use local player p instead of Player(id).

udg, not that debate again. Had it with the mods in the comments of my conversation system.

I'll just say I disagree and leave it at that.

That's just your opinion. Custom script in GUI triggers hurts inarguably read-ability.

should I null local player or not? Im not sure

Player handles don't leak, nulling them is optional.

It is in vJass, but I copied to gui-trigger for one reason. Make it as easy for GUI users as possible. No worries for creating variables, just CnP one trigger.

You don't use vJass keywords. It's pure JASS expressed in custom script.

 

[ZiBitheWand3r3r]

Yeah, ok let it be in Jass. Added recursion safety, thanks BPower for suggestion. Also added PRM_PlayerMonitor[] variable, thanks Flux!
You know, today I changed idea: recreating trigger is not necessary, I added 2 events for gold/lumber: "equal to 0", and "not equal to 0" ;] it's cover all our needs to fire trigger for any gold/lumber changes;]

EDIT:
forgot to remove few I2R, R2I conversion since now all they can be integers
i'll wait when you look at this new code,
zibi
edit:
updated those I2R, R2I

 

[WereElf]

I'd really love it if this system has an indicator - if the resource is 'gained' trough harvesting - have a variable, that points towards the event response worker (the worker that has returned the resource)
I do realize, that this is going to be a pain in the ass to do, but it will be a nice addition, and this system will have a wider use

 

[Tank-Commander]

Add a version number to the system, other critique:

Major:
- All the code within Trig_PRM_Actions could be placed within initTrig_PRM (the code currently in initTrig_PRM is uneccessary
- udg_PRM_FireEvent should be either a constant function (removing the need for the variable) or should be set in a config function (along with other properties like it) or set in the initTrig function. As it stands the system lacks a configuration
- You only need to create one trigger for the resource monitoring (don't need a new trigger for each set of event listeners)

Minor:
- When setting up the event listeners you could use bj_MAX_PLAYERS instead of the hardcoded 15 (I'd suggest checking if this interferes with monitoring of neutral players)
- For clarity the event variable could be capitalised (fairly standard)

 

[ZiBitheWand3r3r]

thanks for suggestions

may I leave this

         exitwhen id > 15

cause you suggest bj_MAX_PLAYERS (12), you probably mean bj_MAX_PLAYER_SLOTS (16) but then it will look strange:

        exitwhen id > bj_MAX_PLAYER_SLOTS - 1

?

 

[Tank-Commander]

you're quite right that is what I meant - though you could just have it as

id == bj_MAX_PLAYER_SLOTS

and that would do the same thing, either way you can leave it; it's just more clear with the constant (since it describes purpose) hence why I listed it under minor

 

[ZiBitheWand3r3r]

All the code within Trig_PRM_Actions could be placed within initTrig_PRM
done

udg_PRM_FireEvent should be .. or set in the initTrig function
done

You only need to create one trigger for the resource monitoring
yes, it's now only PRM trigger

When setting up the event listeners you could use bj_MAX_PLAYER_SLOTS
done

event variable could be capitalised
changed

@WereElf: I'd really love it if this system has an indicator - if the resource is 'gained' trough harvesting
hmm;] it's suppose to be simple snippet (I personaly like small additions)
..maybe in the future ;]

 

[Daffa]

Might want to update documentation to address changes from 1.29 onward.