Hero tavern

[Moderator]

12th Dec 2015
IcemanBo: Too long as NeedsFix. Rejected.

16:44, 7th Jan 2013
Magtheridon96: Thank you for your submission to Hiveworkshop.

Code evaluation:

• Currently, the "All Random" trigger is out of place. I think it's best that you create multiple categories and only put this trigger as an example. The problem is that it's integrated with the system itself since it relies on the "Random" trigger. This disallows the system from being flexible.
• The system has several leaks. Here, we mean memory leaks. A memory leak is created when you allocate memory in RAM, and leave it there. This eventually slows down a computer. When the handle count gets high in Warcraft III, the handle Id count also tends to be high, making Warcraft III slower and slower. This can be fixed however.
  Refer to this thread to learn more about how to clear memory leaks.
• You need to allow users to configure the system in any way they like quickly and easily. You're providing them with an array to store whatever heroes they want, which is cool. It would be cooler if you could do that for the locations to which the heroes are moved to upon creation. It will not always be the start location, so the user would have to modify the system quite a lot in order to get it working the way he wants it to. By providing configuration, you would be making a super cool system that's easy to use.
• The gold cost for heroes should be configurable.
• An initial gold amount should only be given from a map specific trigger for system testing. The user would be responsible for giving the players gold. This system shouldn't do that.
• I will point out the leaks to make it easier for you:
  
  Leaks

  • • Camera - Pan camera for (Picked player) to (Center of Taverna) over 0 seconds
    ^Location leak.
  • • Unit - Move (Sold unit) instantly to ((Owner of (Sold unit)) start location)
    ^Location leak.
  • • (Number of units in (Units owned by (Triggering player) matching (((Matching unit) is A Hero) Equal to true))) Equal to 0
    ^Group leak.
    This one is a condition in the "Random" trigger. You have to move it to an if-statement instead. This would allow you to store the group into a variable and destroy it when you're done.
  • • Unit - Create 1 TV_HeroArray[TV_Data[TV_Random]] for (Triggering player) at ((Owner of (Triggering unit)) start location) facing Default building facing degrees
    ^Location leak.
  • • Camera - Pan camera for (Triggering player) to (Center of Taverna) over 0 seconds
    ^Location leak.
  • • Unit Group - Pick every unit in (Units owned by (Triggering player) matching (((Matching unit) is A Hero) Equal to true)) and do (Actions)
    ^Group leak.
  • • Unit - Create 1 (Unit-type of (Sold unit)) for (Triggering player) at ((Owner of (Sold unit)) start location) facing Default building facing degrees
    ^Location leak.
  • • Unit - Create 1 TV_HeroArray[TV_Data[TV_Count]] for (Player((Integer A))) at ((Owner of (Triggering unit)) start location) facing Default building facing degrees
    ^Location leak.
• "Do nothing" should never, ever be used.
• You should cache things into variables so that you don't constantly repeat them over and over again. For example, at the beginning of a trigger in which you repeat (Triggering unit) more than once, store (Triggering unit) into a unit variable called TempUnit, and use that variable. It makes your code cleaner and more efficient.
• You need to allow the user to configure a player count. I forgot to mention this before, but all configuration should be put alone into a trigger that runs on map initialization. The triggers should also be properly named. For example, a trigger used for configuration would be called "Tavern Configuration", and it would contain only actions in which you set the heroes, the player count, the gold cost per hero, the max hero count per player (makes it more flexible), the locations that the heroes should be moved to for each player, etc... Anything that is hardcoded should be put into the configuration trigger. The system should be as flexible as possible to be considered a good system. Always keep that in mind. Always.
• You should not be selecting all heroes owned by a player in the repick trigger. You should keep track of the selected heroes. Sometimes, a user might want players to have an extra dummy hero, like a backpack or something. The best way to do a repick trigger is to only remove the picked hero(s).
  I'd recommend treating the repick trigger as if it were a plugin and making one central trigger for your system that manages picked heroes and all that.
  This makes it much easier for users to implement your system. They would be able to take the system core and copy-and-paste whatever plugins they want.
  This can be pulled off by adding a few comments that mention this or simply adding Trigger Comments that separate the triggers, allowing users to distinguish between core system triggers and optional system plugins.
• (Owner of (Triggering unit)) is almost always (Triggering player). The only case I know in which this is false is when you have a trigger with a Region Enter/Leave event.
• You should not initialize the system's variables at 5 seconds. You should do this on map initialization. Camera panning should not be done at the start. That's up to the user. Same goes for visibility modifiers, and gold as I said earlier. All of that should be done in a trigger made only for this testmap and it should be put into a different category. This means that the "Units" trigger needs to be split into 2 triggers: One trigger for the test map and another trigger for allowing users to configure your system. Above, I described an ideal configuration trigger.
• Finally, in the field of documentation, this needs to be improved quite a lot. You need to make your description better. Describe what the system does, post the triggers inside [trigger][/trigger] tags within a [hidden=Triggers][/hidden] tag, add a changelog, give credits to World-Editor-Tutorials for the outline, anything would be better than the current description. Importing instructions are a must. It's currently very hard for a new user to implement your system. You need to help him out with that. Walk him through it with step by step instructions.

This needs a ton of work to be approved.
I hope this helped.

~Magtheridon96

 

[Maker]

Remove the Do nothing actions.

This leaks:

• (Number of units in (Units owned by (Triggering player) matching..

 

[Tank-Commander]

For the common public, it tends to help when you have the triggers/script in the description so that it can be reviewed -without- having to download the map, just to make the process a bit faster.

Additionally, mind pointing out the functions of this which make it different from any or better than any of the previous editions of such system when they're very common?

 

[zv27]

post fuckin' code man

 

[SpasMaster]

Magtheridon just wasted his time trying to help that guy. I bet he didn't even come back to check it...