Zephyr Contest #15 - Results

[IcemanBo]

---

FinalScore = (30*PollVotes/POLL_VOTES_TOTAL) + (70*JudgeScore/JUDGE_SCORE_TOTAL)
Max Judgement Score: 40

First some words.
I tried to give conrete point deductions for the critique. Sometimes it's not easy to decide how many points a mistake is worth, but then if something seemed too much at one thing, I tried to compensate it at an other one.
I also applied following:

Breaking any of these rules may lead to punishment in the judgement score, up to disqualification from the contest, depending on the weight of the issue.

^this means that I gave certain malus for doing or not doing something that was asked for. For example, not providing any WorkInProgress (WIP) has been leading to a malus. That, and also some other points.
That simply because I think it's pretty unfair, if we set up rules, and the ones who always do things correctly get absolutly nothing for it. So breaking in certain rules has lead to malus this time, which means it simply gets substracted from the final score.

---

Fruit Forest - Overlord Slam

Result Judgement Concept Code Visuals Penalty Poll-votes: 2/21 Final Judgement Score: 30/40 Penalty: -3 -------------- ------ Final Score: 52.3/100 Concept: 8/10 Code: 13.5/20 Visuals: 8.5/10 ---------- ------ Score: 30/40 8/10 Critique Concept Garrosh Hellscream makes a leap towards the targeted location. When reaching, Garrosh makes a powerful strike towards the ground which damages enemies and pushes them away. While being pushed, the enemy unit will receive extra damage once colliding with an obstacle. Furthermore Garrosh regenerates his health points for each initial damage, done by his strike. The strike-jump idea is fitting the warrior theme pretty well. Knockback is always neat and nice to look at, at such strikes. Extra effect onCollide speaks for good interest in details. The chosen self-healing part does not fit good in this strike conept, and instead, I would focus on something bloody or damage making. (-1) The origin of this idea, with damaging and pushing enememy units, was seen several times already (-1), but it anyways does fit the model pretty good, and the extra onCollision affect does bring some uniqueness with it. 13.5/20 Critique Code ==== Configuration===== The configuration trigger could run already on "Map Initialiozation", or at least at "Elapse Game Time - 0.00" - as there is no need to wait some duration, might it be short, or long. Though, this is a very minor point. The naming conventions could be orientated more to [GUI] - GPAG - GUI Proper Application Guide. (every variable IS_WRITTEN_LIKE_A_CONSTANT) Furthermore "ANIMATION_INDEX" misses the system prefix. Normal GUI "group" variables do not to be initialized by default, so technicaly this like does leak in the config: Custom script: set udg_OVSLAM_GROUP = CreateGroup() It is also not required to clean the group, by the way, right after it is created, as it will be always be empty by default. (-0.5) points extra are splitted on the points above, at which there is no explicit score deduction. ===== Cast Trigger ===== At casting start there should be some pathing check to ensure that the unit won't stack into for example deep water, or inside a structure - which results in breaking the unit movement. (-0.5) onCast trigger the trigger condition can be used, so the extra if-statement, the very first one, becomes useless. Hashtables are powerful, but not required to achieve multi instanceability. It's even negative to use it in such cases, like here. The reason is simple that is makes code harder to read, and so also to maintain and to debug. You don't have such issues when using other tequniques, like for example Visualize: Dynamic Indexing. Just have a look at the first four lines for example: Hashtable - Save (Angle from OVSLAM_POINT[0] to OVSLAM_POINT[1]) as 1 of OVSLAM_INTEGER in OVSLAM_HASH Hashtable - Save (Distance between OVSLAM_POINT[0] and OVSLAM_POINT[1]) as 7 of OVSLAM_INTEGER in OVSLAM_HASH Hashtable - Save 0.00 as 8 of OVSLAM_INTEGER in OVSLAM_HASH Hashtable - Save 1.00 as 2 of OVSLAM_INTEGER in OVSLAM_HASH - 1st line: angle is saved in key "1" - 2nd line: distance is saved in key "7" - 3rd + 4th line: I have absolutly no idea what is does And this is done with "23" keys, which is just a mess to read, and with no benefits at all, in comparison with dynamic indexing for example. (-0.5) When using custom script you might use the native GetHandleId instead of the bj GetHandleIdBJ - as the BJ does nothing else as calling the native inside, so it leads to a slower way with the exactly same result. (like many other BJs, too) The POINT[0] gets destroyed and then re-set to the same value, which is redundant Responses like (Last Created Unit) are a function call each time they are used. So, when used more often, the value should be stored into a variable for further usage instead of calling the same function over and over again. The hero replicate procedure should be a thing of the periodic interval trigger, as it directly depends on the movement procedure - it's more intuitive. And furthermore, in case the hero gets removed, or anything else happens, then the already created replicates would be wrongly created / created without purpose. (-0.5) (-0.5) points extra are splitted on the points above, at which there is no explicit score deduction. ===== Periodic Trigger ===== The priodic trigger should not run if the group is empty. I know you make a integer comparison which checks the number of units in the group, but this action is already redundant - the trigger should not run at all, and thus be turned off once not needed anymore. onCast trigger, there should be then a check if it's required to turn on the periodic trigger again. Also, having a custom counter instead of using the "Count Units in Group" function is some faster, as it's only getting and setting a variable. (-0.5) The spell could take usage of a knockback system. Such logics is a good thing to use, as otherwise we would replicate same things over and over again, in each of our spells. A pathing checker library should be used to check the pathing, instead of lengthy workaround + keep changing the unit's collision. (-0.5) At the moment the both logics, Jump-movement & Knockback-movement, share the same periodic trigger, which is not the perfect structure. They can co-exist, but should exist modular, which means each of them should be written as stand alone structure, because there might exist units that are knochbacked, or also jumping units just seperatly without requiering the other party to exist. (-0.5) To check when to reveal a new replicate, the distance check is technicaly a bit vague. More accurate would be to check for already traveled distance, and rely on this value, so you can 100% ensure to correctly spawn a new replicate. The used "Crow Form" tequnique is not perfect, as it always removed the ability from the unit, and also makes the same procedure on each periodic call. Once you applied a unit with this method, it doesn't need an other run to work properly with adapting flying height. How to Give a unit the ability to fly In case the caster is dead or removed from game, no further jump action must run, none at all, except proper removal from system. That includes all moving, all repilcate, or damage actions and things alike. (-0.5) The jump should not be fixed at the target destination, but should work with traveled time/distance. That is, because it might be possible that the hero is forcely moved elsewhere on map (maybe even with triggers), and then the spell will bug out - but when it just keeps the angle, and checks for travaled distance, for example, then it will act "independantly" from target location. (-0.5) The texttag is something neat in games, but spells should not require to use it. The simple idea behind is that the user is limitted to maximum 100 texttags at all, and he should have the choice if he wants to bind some of them to imported spells, or not. So making it configurable is definitly good. The unit filtering syntax is not readable, have a look at Convenient Unit Group Filtering in GUI. Don't do: Do nothing It does nothing for you, too. : ) In case the hero is removed from game, it won't deal any damage anymore, also not on the knockback parts, onCollision. (-1) points extra are splitted on the points above, at which there is no explicit score deduction. ===== Other ===== The "How To Import" is missing the part to automaticaly create GUI variables / how to enable it. The dummy unit could have the system prefix added in object editor. The used effects at different parts should be defineable by user. The ability could be preloaded. (-0.5) points extra are splitted on the points above, at which there is no explicit score deduction. 8.5/10 Critique Visuals The tooltip is informative, but is not written very well, like for example it contains grammatical errors like "The longer jump is the more damage will deal." It's not very nice to read. (-1) The ability icon is fitting the spell. The texttag would look a bit nicer if it was moving upwads. (-0.5) The dummy hero units while jumping look very good. Reasoning Penalty Jumped the start (1 day) before official contest date -1.5 WIP(s) were not provided -1.5 ---------------------- Final Penalty -3

PurplePoot - Prismatic Spray

Result Judgement Concept Code Visuals Penalty Poll-votes: 3/21 Final Judgement Score: 36.5/40 Penalty: 0 -------------- ------ Final Score: 68.1/100 Concept: 9.5/10 Code: 17/20 Visuals: 10/10 ---------- ------ Score: 36.5/40 9.5/10 Critique Concept The caster is firing multiple rays with random colors in multiple directions. Each color defines represents a unique effect which the ray has. There are various of effects, from speed manipulations, over stunning, damaging, healing, up to instant killing. The effect of the green ray, for instantly killing enemies under 200HP might be too extreme in some scenario, but the rest of effects is good. And as it's configurable one could minimize the "instant kill" effect a bit, which how ever only partialy compensates.(-0.5) The spell fits the chosen theme. Overall it is a solid mage concept. 17/20 Critique Code ==== Configuration===== The ray namings could be set to the namings following the tooltip, like "Dark Ray" -> "Green Ray". The native UnitAlive should be used as check if a unit is dead, or a not IsUnitType(u,UNIT_TYPE_DEAD) and GetUnitTypeId(u) != 0 check. Units might potentialy (prracticaly very unlikely) get their healthpoints manipulated via code, also after death, for example, and then a check for the widget's life might return a false positive. A short warning or statement for the user not to work with ally/enemy filters would be useful, as I also only saw it later in code that such player filters will be applied internaly by the system, orientated on the respectice effect. (-0.5) points extra are splitted on the points above, at which there is no explicit score deduction. ===== Geometry ===== Generaly it's a good thing to take usage from already existing systems, just to outsource work, and not to repeat similar logics over and over again. Example: [vJASS] - [System] Polygon, or Line Segment Enumeration .v2.1a. Your own geometry approach, I must admit, is more performant than using said systems, especially the function to check if a point is inside the trigonometric rect, but you still need to do some "ugly" logics yourself, like enumerating all potential units + filtering out for real rect units, for example. But this is only a minior point, andonly in this very specific case, because you were also right to tailor a modular library right for this purpose, and which is efficient, so in theory such thing could have been submitted as standalone resource. Afterall, that's only a minimal deduction point. The members like real p1x or real x12diff should be readonly. It could make sense to make it public, but then as public method operators. If the user has access to directly change for example the x1 value, then it must be ensured that the corresponding x1-delta values change with it respectivly to keep the system working. I'm pretty sure that actually "<=" should be "<" in your "containsPoint" function, but even if I'm right it doesn't change something practicaly. It's just a mentioning. Also the function isn't really readable because of the very long line, it could become a bit more readable like this: method containsPoint takes real px, real py returns boolean return (py - this.p1y)*this.x12diff <= (px - this.p1x)*this.y12diff and /* */(py - this.p2y)*this.x23diff <= (px - this.p2x)*this.y23diff and /* */(py - this.p3y)*this.x34diff <= (px - this.p3x)*this.y34diff and /* */(py - this.p4y)*this.x41diff <= (px - this.p4x)*this.y41diff endmethod - No deduction here. (-0.5) points extra are splitted on the points above, at which there is no explicit score deduction. ===== Spray Rays ===== rays = InitHashtable() It's actually not needed, it may be stored into an integer[array] instead of a hashtable. A group does not need to be explicitly cleared when it's about getting destroyed. the onDestroy method should not be used - the only use might be in a extending struct. destroy() -> normal function call onDestroy() -> generate trigger, triggercondition, and will run through a trigger evaluation each time called (-0.5) points extra are splitted on the points above, at which there is no explicit score deduction. ===== Prismatic Spray ===== It's not required to create one dummy unit per order. The system might take usage of global dummy/ie(s). See example: Single Target Spell for Multiple Units attacktype and damagetype could be defineable by user. onDestroy() -> destroy() No extra deduction here, as it's the same point as it was already before. Condition and Actions might be combined to only conditions, and the boolexpr might be wrapped into a if-statement. The reason behind is, that each time a trigger condition or trigger action runs, a new operation thread is setup behind the scenes, so we can avoid this overhead. And we choose conditions because the jass machine might still try to evaluate the conditions, might some exist or not - but the actions are only executed if the condition returns true. The "EndActions" function does run twice under normal conidtions, once for "Finish Cast" event, and once for "End Cast" event. The result is wrong attempt to destroy and to deallocate structs, and the not required clear of hashtable entries. There must be a safety check to only run the removal operations if the (Triggering Unit) is ensured to be still caster of a prismatical spray. (-0.5) In function ShootRays should be a tiny safety check in case user defines "1" as amount of rays , because we should not devide through "0" here in: PrismaticSprayConfig_RAY_EFFECT_ARC/I2R(numRays - 1) The result would be a thread crash. (-1) points extra are splitted on the points above, at which there is no explicit score deduction. ===== Other ===== The abilities could be preloaded. (-0) A minimum amount of deduction was added above. At first I wished there was a bit more documentation in the main codes, but it was written very clear and structured, and I didn't have any problems to read and understand your code. 10/10 Critique Visuals Good tooltip. The ability icon is fitting. Rays look colorful and neat. Animation does fit the spell procedure. Correct usage of buffs. Actually nothing bad I could spot. The only sad thing I have in mind that the map itself still had black mask/fog, which annoyed me a bit at testing, like I could not visualy see everything what happened at any time. But I really won't deduct something for this single point. Reasoning Penalty ---------------------- Final Penalty -0

NEL - Prison

Result Judgement Concept Code Visuals Penalty Poll-votes: 2/21 Final Judgement Score: 29.5/40 Penalty: 2 -------------- ------ Final Score: 52.4/100 Concept: 8/10 Code: 13.5/20 Visuals: 8/10 ---------- ------ Score: 29.5/40 8/10 Critique Concept Shame creates a circular wall around himself to trap nearby enemies. The enemies periodicly receive damage, armor reduction, are slowed, and not able to pass throrugh the builded walls. The concept is like torturing the enemy units and it fits the chosen theme well. The spell itself fits more the "channeling" category, and should not be a one-time cast. Like for example the Shame should not be able to leave the area by himself. I think it makes no sense if the Wall doesn't directly relate to the current action of the shame and his position. Channeling as requirement would make more sense.(-1) The tree destruction somehow confuses me a bit, it doesn't seem on point to the theme. I'm not sure it related much to the other goals, but it's nothing really critical. (-0.5) The apprupt teleporting backwards of enemy units seems a unsmooth methodic.(-0.5) 13.5/20 Critique Code ==== Configuration===== What starts here, and goes through all code is the lack of saftey when it comes to name collisions from functions. We must consider the spell being imported into an other map, so everything that is public must have some prefix, or a very specific name, to ensure it would collide. Example: function DPS -> function PRISON_DPS And this must really match for everything. (-0.5) attrib_Base could directly return the integer hero stats, example "bj_HEROSTAT_INT", etc. But it must be done carefuly as "_STR" is "0".And then no string operations with casesare required at all, but simply an integer comparison. Instead of using GetHeroStatBJ the natives GetHeroStr, etc, might be used. There are very much more BJs that could be replaced with natives in the code. When you have a look in the function list you can always see what is behind the BJ (red marked function). In most cases in here it makes the code really GUI-ish, and is the opposite of what we try to produce in JASS - clean, efficient, and readable coding. Sorry, but this is an important point to get away from uneeded BJs. There are some good BJs, like TriggerRegisterAnyUnitEventBJ, as it can hide ugly code logics, but most of them is not worth, like check out for example the UnitCreate BJs. I deduct here for all of such hightly inefficient points at once, which should not be part of good JASS. (-2) The used way to achieve "dynamic" values, taking the current level into consideration is not really dynamic. It's a list of static values, which is a overhead in this case. Instead, one formula can be used to achieve a real dynamic pattern - here is an example of the "DOT" function, but it's the same in some other functions, too: function DPS takes integer lvl, unit u returns real return lvl*10 + attrib_Percentage(lvl, u) endfunction (-0.5) Explicit comparisons with true or false are like never needed, and can be just be cut off. if attrib_Bonuses() == true -> if attrib_Bonuses() and in case you need to negate it, so if you would use "false", then simply put a not before the statement, so it gets negated, example: if not attrib_Bonuses() And in your specific case you could even shorten it completly and wrap the returned boolean value directly in GetStat function, which also requires (this) boolean value, like: return GetHeroStr(u, attrib_Bonuses() ) * Percentage[lvl] Shouldn't "aura size" always equal the radius? Not sure why both exists. Not very sure what user can exactly expect from this: constant function transparent_Spd takes nothing returns real return 2.0 endfunction ^At such points some longer description might become useful. The teleport/blink mechanics were not really mentioned before at the concept or so, but then there are configs for it. With "unable to escape" one does not imply porting close to caster, necessarily. (-0.5) points extra are splitted on the points above, at which there is no explicit score deduction. ===== Cast Trigger ===== Here, and everywhere else - the used locations are not needed. reals should be used, in combination with natives. Locations aren't useful in most JASS cases, and are not needed overhead. reals is leight weight, and more flexible. In most cases when location become useful in JASS is when you need to call GetLocationZ native. (-0.5) It's enough if you null the variable only once, at the end of a function. When changing a unit's scale, only the first scale parameter matters, the y/z can be ignored. Before you index a new group for the instance, there could be a comparison that checks if the group does already exist, or not. For GUI array variables, for example, the index "0" and "1" are automaticaly initialisized. A KillTree system might be used, as it's not so good to replicate logics in each spell seperatly. (example [Snippet] IsDestructableTree) (-0.5) points extra are splitted on the points above, at which there is no explicit score deduction. ===== Periodic Trigger(s) ===== Periodic triggers is something for GUI. In JASS, we take usage of timers, which we use for one-time runs, but also to run functions periodicly. And instead of enabdling/disabling triggers, one can just pause/start timers respetivly. Why need overhead trigger structure, when it's not needed right? : ) Naming / structure and sometimes docu is really useful for the reader, but not when done not correctlly. What I mean is that I have problems intuitivly to understand the code flow, or the exact purpose of something, example: //=========================================================================== // // [trigger] Time - Every periodicTime() seconds of game time // function trigger_transparentLoop takes nothing returns nothing The documentation does not really help me at all. The "trigger_" implies it's binded to a trigger, which doesn't really bother me, too, in the name. "transparentLoop" is positive though, it does tell me something about the actual purpose of the function. Now when there was 1 short statement why this function exists, it would be totaly fine already, and I wouldn't have to jump here or there to find out what the goal is. For example: "Regulates appearance of the aura-dummy unit." In all trigger loops - the check to turn off the periodic trigger should only happen onDeindex, instead of each loop run. All periodic actions should definitly stop once the caster died, and especially if he gets removed. In such case everything should be instantly deindexed and everything instance related must be removed. (-0.5) It's not required to create one dummy unit per order. The system might take usage of global dummy/ie(s). See example: Single Target Spell for Multiple Units UnitIndexer or hashtable might be used to bind data to a unit, such as a lightning index. Then no loop would be needed to find the correct index, when the buffs is removed. And by the way, once the correct unit was found in the loop, then it can directly exit, using exitwhen true. Instead of creating and destroying local groups for temporary enumerations, a global group can be used, which never destroyed. Two duration counters are not required, even there is this extra "per second" part. One could use only one, with for example using modulo operation. But it's a minor point. (-1) points extra are splitted on the points above, at which there is no explicit score deduction. ===== Other ===== It's usually not recommended to use abilities with more than maximum 4 levels, because of the drasticaly increasing loading time in case the map gets/gets not widgetizes (a optimizing tool). The widgetizer tries to put the data into a SLK table, but fails after level 4, and the rest stays at the .w3a file. It's more of a note here, and no critical point. The alternative way is to create multiple abilities. Always when same values are needed, even if multiple times, then you don't need to call the same function over and over again, but just one variable can be used. Example: local location loc = GetRectCenter(GetPlayableMapRect()) local location locA = GetRectCenter(GetPlayableMapRect()) local location locB = GetRectCenter(GetPlayableMapRect()) Overall to documentation - it's not often helpful. The code is honestly not very nice to read at the moment, and the existing docu also doesn't really help. I give an other example, because I think it's very important: // Check if udg_PRISON_lightningMax reach the 0, then turn off this // trigger if(udg_PRISON_lightningMax == 0) then call DisableTrigger(udg_PRISON_trigger_lightning) endif ^Such style is used more often - but it doesn't tell me anything I can't immediatly see myself. A comparison with "0" and then a trigger is turned off. I don't need to have it explained. I need to have explained why things happen, in other places where I don't intuitivly see what's happening. Like here: // nearby enemy units set nearbyGroup_loc = GetUnitLoc(udg_PRISON_aura[udg_PRISON_Index]) set nearbyGroup = GetUnitsInRangeOfLocAll(AoE(), nearbyGroup_loc) call ForGroupBJ( nearbyGroup, function nearbyGroup_Func) call DestroyGroup(nearbyGroup) call RemoveLocation(nearbyGroup_loc) set nearbyGroup = null set nearbyGroup_loc = null "nearby enemy units" is the comment, but hell, what happens with them? I have absolutly no idea. It's pretty much useless. When I read the code with "AoE" then only I can think of that they maybe get damaged, or so. There already was partialy deduction for documentation, so it doesn't weight too much in here. The ability could be preloaded. (-0.5) points extra are splitted on the points above, at which there is no explicit score deduction. 8/10 Critique Visuals The tooltip looks pretty neat, though the description is written a bit weird - some words are missing to make a good and clean sentence. The buff description is missing the damage factor. (-0.5) I'm not sure there is no better model for the wall, but it looks actually fine, in this circular creation. The circle model, though, looks somehow off. It's looking doesn't fit so much to the happening actions. I would definitly remove or replace it. (-1) The green effects on units + the grenish effects on wall destruction doesn't fit somehow. (-0.5) The purple (color) lightning I think is much better. No deduction for it, but I think it would look even some nicer if there was an effect at the endpoints of the lightning, as it usually doesn't look very good, in case you look closely. Overall it looks pretty good / impressive, as the whole thing is pretty big, too. Somehow I think though that "Prison" is not a perfect name for it, I would try to give it a more epic name. Maybe "Dark Imprisonment", or so - I'm personaly bad at finding names. ;D Reasoning Penalty WIP(s) were not provided -1.5 Missing import instructions -0.5 ---------------------- Final Penalty -2

Tank-Commander - APex Blizzard

Result Judgement Concept Code Visuals Penalty Poll-votes: 2/21 Final Judgement Score: 36/40 Penalty: 0 -------------- ------ Final Score: 65.8/100 Concept: 7.5/10 Code: 19/20 Visuals: 9.5/10 ---------- ------ Score: 36/40 7.5/10 Critique Concept The caster fires a torrent of ice towards the target location, damaging the first enemy unit it hits. Furthermore it deals AoE damage to nearby enemies, and also freezes every unit in radius, which makes them unable to act. Concept fits the chosen theme. I think freezling really all enemies in the radius is pretty strong, combined with the damaging, as they can't move or aattack at all. (-0.5) But the fact that the radius and duration is configurable makes it some more adjustable to compensate it. The concept, with the AoE freezing and damaging reminds me strongly of Frost Nova, but as an improved version. (-2) 19/20 Critique Code ===== Cast ===== set angleIncrement = 360. / projCountCircle -> set angleIncrement = bj_PI*2 / projCountCircle as then, this won't be needed each loop run: set radAngle = bj_DEGTORAD * angle (-0) points extra are splitted on the points above, at which there is no explicit score deduction. ===== Periodic ===== I believe SquareRoot should be sligthly faster than Pow, mainly, because it has less arguments! Joke aside, I just wondered why you used it, it's no negative point at all. ;D Attaching data to a unit is more neat than making O(n) search loops. See like APEX_GetStunNode. The spell could take usage of a stun system. But the used stun logic in system is pretty small, so it's not a big deal. Missiles are autonomous damage maker in the code, but as it's the caster's spell, it was good if he was also the unit that deals the damage out. Though, the positive thing in used logics is that in case caster is removed, the damage will still be there. I guess a combination of both would be the best, even it's a not a same fast/straight forward coding solution. Maybe an extra function that make checks and deals out the damage would become useful for such issues. The check for turning off the timer could just happen in the recylce process/function. But I think I understand your intention to do it in the loop logic --, that you might have thought to filter out the redundant overhead, in case the parent node is the one being recylced. But also with this minimum risk of redundancy, it would still be a bit more efficient style. Now the 'problem' with my suggestion is, that I wouldn't recommend to apply this tequnique in node creation, for you, in this spell -- as the "win/lose" would result much worse here. There is only 1 possible turn on, anyways, and that is at parent creation, and all other child creation would not need to check for a turn on. Ability could be preloaded. (-1) points extra are splitted on the points above, at which there is no explicit score deduction. Proper code standard, you have there, and good sharing of node data. 9.5/10 Critique Visuals Good tooltip. Good looking spell effects, together with flying missiles. Good buff usage, with descriptions, for spell- and stun effect. Icon fits good. The only thing that comes in mind, is that the very last effect, when enemies get frozen is too big. It seems a bit too enormous, and covers a too wide area. (-0.5) Reasoning Penalty ---------------------- Final Penalty -0

Anachron - The Dark Plague

Result Judgement Concept Code Visuals Penalty Poll-votes: 1/21 Final Judgement Score: 28/40 Penalty: 0.5 -------------- ------ Final Score: 49.9/100 Concept: 9/10 Code: 13/20 Visuals: 6/10 ---------- ------ Score: 28/40 9/10 Critique Concept The caster infects enemy units in a targeted area. Infected units will suffer from a plague, which decreases their attack- and movement speed, and also limits their sight radius. Furthermore, as long as a unit is infected, it has a periodical chance to get 'confusion', which will increase the chance for a sudden death. In case of a sudden death, the dying unit will again infect nearby enemies with the plague. The plague concept fits the model very well. The sickness fits the lowering of units attributes. Lowering values like damage, or armor would fit, too, but I guess it could become an endless game, so it's fine as it is. Though, the sudden death is too powerful, and a periodical, very low damage + chance for extra damage each few seconds, or something similar, would be more smooth in my eyes. (-1) 13/20 Critique Code ==== Configuration===== private constant integer HERO_SPELL_CODE='Adkp' private constant integer MAIN_SPELL_CODE='Adpb' intuitivly not very clear that "MAIN_SPELL_CODE" is the one for buff. Extra comment could help. Rawcode 'dumy' should be part of the config part. No deduction for it, but there could be a small sign when the config part ended. The "period" is defined as "2 seconds" in description, but is actually "2.5" seconds. It should be also a constant variale, and furthermore it could be just be configurable. Spell related things, like created effects, should be configurable via code. (-0.5) (-0.5) points extra are splitted on the points above, at which there is no explicit score deduction. ===== Cast Trigger ===== After the dummy has casted, it should get the initial ownership back. ===== Periodic Trigger ===== The struct utils has no definition, or api description. And when I looked for it, I saw it was graveyarded. It isn't optimal to use it "out of nowhere", and using other libs, or constructing the periodic logic yourself would've been better. It's not really straight forward having to declare a parent struct for importing the module with stub method onPeriod, for overwriting it with with the wanted onPeriod method inside the main spell struct. We go this way pretty much for a simple call each "x" seconds. Starting one timer per instance would be a neat solution instead. (-1) Furthermore there is a double deallocation problem, which is I believe related to the used library/applied method. But I'm not sure what to precily suggest to change, because firtsly it would need to be rewritten. (-0.5) The dealt damage for "sudden death" should be ATTACK_TYPE_NORMAL. The damage type is correct. The reason is explained here: WC3's Damage System You're dealing x amount of damage, where x is the exact life of the unit -- so you must ensure that no damage is blocked at all, and never reduced. For most cases your method would still work with attack type "null" if a a very big damage number, like "999999" was used, but using correct damage- and attacktype on top of that is still proper. You may test it with giving bracers item to a unit for example, then it will reduce said damage by 33%. Not sure if this check is intentional, or something forgotten: if GetHeroLevel(theUnit) > 0 then return endif I guess you really wanted not to apply the sudden death for hero units. In this case they would not even need a table / counter for the 'confusions/shocks'. Also, nothing critical, but there exists also this: constant native IsHeroUnitId takes integer unitId returns boolean which directly checks if a unit is a hero. The duration if off by one "period". An easy, but required fix would be du reduce the timeout before the "> 0" comparison inside the onPeriod function. The inner structure of handling instances is not very elegant at the moment. Actually, no allocating is required, in case no enemy units are infected. The instance will be registered for nothing. (-0.5) Also, the current code structure does not elegantly allow (by tooltip) explained "infecting nearby enemies on sudden death" because the instance will just expire after duration, independatly from responses from infected units. For doing it, it would be probably good if a new struct existed which manages infected units. I won't deduct points for it, though, and will judge it as 'wrong tooltip'. (-1) points extra are splitted on the points above, at which there is no explicit score deduction. ===== Other ===== Big purpose of written lines is to properly handle dummy units, to cast and to remove spell/buffs from units. In general to handle object editor data. Coded features/effects (not necessarily meant "visuals") are not the majority of this implementation. That is nothing bad per se, but Zephyr is meant to rely mostly on coding, an thus deduction for code complexity seems fair to me. (-1.5) : call FogEnable(false) call FogMaskEnable(false) ^isn't spell related -> should be moved outside. Two init functions are used, one is initialisized by the struct, and one by the library; only one is needed. member caster should be nulled in destroy function. The check if a unit is dead should not solely happen with a unit life check. Units might potentialy (prracticaly very unlikely) get their healthpoints manipulated via code, also after death, for example, and then a check for the widget's life might return a false positive. The native UnitAlive should be used as check if a unit is dead, or a not IsUnitType(u,UNIT_TYPE_DEAD) and GetUnitTypeId(u) != 0. When a plague-affected unit dies it should be removed from unitHitGroup. When the last infected unit dies, the instance might be destroyed directly. (-0.5) when duration ends for one infected unit, or for the plague itself, the respective unit(s) must get the buffs removed, as: 1) the buff duration is defined in object editor 2) the object editor can not define dynamic durations, such as the user can with the custom function in configuration So the only good way is manualy to remove the buff after duration. (-0.5) Ability could be preloaded. Documentation inside code could be a bit more. (-0.5) points extra are splitted on the points above, at which there is no explicit score deduction. 6/10 Critique Visuals The tooltip speaks of nearby infection on sudden death, which is not implemented. I guess it was initialy thought of, but then forgotten/ or implemented because of time. (-0.5) 'The Dark Plague' is the name of the spell, and can be mentioned as it inside the tooltip - quoting it with and in combination with small letters seems not very correct, like "dark plague". Not sure, but nothing of the effects to really remind me of a plague intuitivly. Not the initial effect (from object editor data) (-1), nor the target effect around the enemy. It doesn't give me impression of 'sickness' or aggresive bacteria. (-1). Having them configurable would give the user the chance to change it. Some other tooltip aspects: "fwe" -> "few". "and/or" is not really wrong, but it's technicaly not very formally. (what a tooltip should strive for) Tooltip seems a bit too long. After learning the spell, the per level description, might maybe be a bit shorter, and not explain every general info again. (-0.5) No effect on sudden death. (-1) Casting time is a bit too long in spell object data. Reasoning Penalty Missing import instructions -0.5 ---------------------- Final Penalty -0.5

Loner-Magixxar - Touch of Nature

Result Judgement Concept Code Visuals Penalty Poll-votes: 2/21 Final Judgement Score: 31/40 Penalty: 0 -------------- ------ Final Score: 57.1/100 Concept: 10/10 Code: 13.5/20 Visuals: 7.5/10 ---------- ------ Score: 31/40 10/10 Critique Concept The Dark Ancient uses the power of nature to work with trees. There planty of aspects, and the effects are categorized basicly by ally/enemy + unit/tree/strucutre + ground + tree. The many variations really tried to cover all different behaviour, but still to follow the theme topic in a way. I would even closely call it overkill for a spell to have so many aspects, but for the contest it's fine. The only thing I would have probably let out is healing 'structures', as it's something not living, and when trees gave some life to my barracks it felt a bit wrong. But the fact the user may config the possible target aspects, like enabling/disabling targeting on strucutures is so awesomet that it compensates it. (on a side note; I would have love to see such spell in 'Diversity' contest theme : )) 13.5/20 Critique Code ==== Configuration===== Instead of storing tree types into array, an other method should be used. Basicly a harvester dummy exists, and can be oredered to harvest a destructable - and orders in JASS return a true/false boolean in order functions, which means that the "harvest" order returns "true", then it is a tree, and if "false" then it's a different type of destructable. (-0.5) The regrown tree types could be stored simply into an array, similar like the first treetype config part, like: Set SummonedTreeType[1] = Summer Tree Wall ... And actually, it would be even better if the user could respawn the exactly same tree type, that originaly existed. (-0.5) Instead of using multiple SFX units, one dummy unit type could be used, and the SFX attached to it's origin. Then the dummy is still dynamicly scaleable via trigger, and some object data is reduced. (-0.5) A dummy unit and ability is used to generate tree-to-enemy damage, which is pretty... object editor focused. 0 dmg, and a DDS is used to make the damage still dynamicly, though, which is good. The AoE is still only defineable in object editor (and not mentioned somewhere). Customizing the missilies completly, or just using a missile system is good way to go for such things (-0.5) ===== Cast Trigger ===== Responses like '(Triggering Unit)' or '(Custom value of < unit >)' should be stored into variable to reduce the amount of calls of the same functions over and over again. That also counts for any other trigger (loop). (-0.5) Instead of all the namings Real_A, Real_B, Real_C, etc, the name should be specified, as the purpose is specified, as well. [GUI] - GPAG - GUI Proper Application Guide It makes it harder to follow and to understand what is actually done, with only generic names. (-0.5) The mana return / stop order does not work. Besides the attempt not working I would recommend to apply an other approach -- to catch the whole thing onOrder, instead of onCast event. Then, you have possibility to filter out all required stuff, and to stop mana taking / cooldown usage. Here is some info of where such aspects like mana usage takes place: Casting Events Guide There might still exist the onCast trigger, which will then properly index everything after all checks. (-0.5) Instead of using dummy casters there could be used the lightning type to show effect, it's more more efficient, and reduces object data. (-0.5) Instead of creating units to check for available paths, you might need take usage of pathing check system. I'm not very sure why 'Pause Unit' is used on enemy buildings, as such thing isn't even mentioned in the description/tooltip. When 'Skip Remaining Actions' is used, no further action in the function will run, that means that also no location will be destroyed after it. (-0.5) This is pretty redundant: Set ToN_TempLoc_B = ToN_TempLoc_A[(Custom value of (Triggering unit))] Custom script: call RemoveLocation(udg_ToN_TempLoc_B) ^and could just be: Custom script: call RemoveLocation(udg_ToN_TempLoc_A) (-0.5) points extra are splitted on the points above, at which there is no explicit score deduction. ===== Periodic Trigger ===== The periodic trigger may be turned off when the group is empty, and be turned on again when it's not. Even there is the condition not to execute the actions in case the group is empty,, the trigger will still fire and will be evaluated each "0.03" seconds One periodic trigger for each group would be more clean, and slightly more efficient. Forget about speed, but having clean structure is good. (-0.5) points extra are splitted on the points above, at which there is no explicit score deduction. ===== Other ===== There's a overall massive usage of object editor data for things that could be coded. A good example is the combination of a dummy unit, dummy ability, in combination with a DDS, and dummy unit is requiring 2 data structures to handle attacks + waiting for removal. Then abilities are used simply for SFX, dummy units mixed with casts are used for mimic lightnings. It's just very hard to read and to understand, I say honestly. Would there have been no comments, I would have died for sure - thanks for making good comments. They're really helpful. (-0.5) The 'debug' trigger should be configured in case of removal? There's already a unit indexer imported, it has a "deindex" event, which fires when a unit is being deindexed from system /removed. This event should be used to catch unit removal, instead of custom registration of operations inside debug trigger. (-0.5) The casting range is much too big, probably it is/was for test map. The ability could be preloaded. 7.5/10 Critique Visuals The icon seems to have a dark or demonic touch for me, maybe just becase of the crawls. I think something else might fit better for nature. (-0.5) The affected units could have buffs, especially buildings, which are affected under +/- armor for example. (-1) The used special effect for enemy structured (with negative effects) is the very same as for allied structures. A visual difference would be good. (-0.5) The tooltip is bit of a info dump; numbers, numbers, numbers. I know it strongly depends because you used so many different aspects in your conept, but it still results in an long and not very nice to read overall description. (-0.5) Reasoning Penalty ---------------------- Final Penalty -0

KILLCIDE - Flame Burst

Result Judgement Concept Code Visuals Penalty Poll-votes: 9/21 Final Judgement Score: 38/40 Penalty: 0 -------------- ------ Final Score: 79.3/100 Concept: 10/10 Code: 18.5/20 Visuals: 9.5/10 ---------- ------ Score: 38/40 10/10 Critique Concept Hyperion uses Flame Burst to ignite an enemy unit, reducing it's movement speed, dealing damage over time, and sending out fire waves which can also ignite nearby enemies. One, by Flame Burst killed unit explode and deal some extra splash damage. -- Good one. Very solid and pretty balanced concept. It's very good as it is, and I have nothing to recommend. 18.5/20 Critique Code ===== Cast / Order Trigger ===== I'm not a great of SimError, as it's not really compatible with other shown messages (forced alignment of "x"). I would personaly love to see it optional / defineable via config. But I know, it's the standard way of showing errors, and so only a tiny point. ===== Periodic Trigger ===== It's right that there might still fire the last wave at time "0", but in case the target is already dead, probably not. Maybe you can check of "Less Than 0" and then just change the two steps, creating wave, and death checking. In case caster is removed from game for what ever reason, the targets won't receive DoT no more. Maybe it's a good solution to change ownership of dummy in this case, and let him make the damage, so at least the target gets correct amount of damage -- we might think of a good standard solution in such a case for future, hm.. It feels a bit weird that fired waves can't cross something unwalkable. The pathing check is for ground units, but in case there's water, or so, I don't think such a wave should be blocked. It maybe makes sense in some way, else you would not have done it, and you can tell me, but for now it makes more sense only check for map bounds for myself. (-0.5) ===== Other ===== Some default cooldown would be good. The air units might die onExplosion, but actually can't be targeted. They should be immune. The dummy onInit can be used to also preload the ability next to effects. (-1) points extra are splitted on all points above, at which there is no explicit score deduction. 9.5/10 Critique Visuals Good tooltip. Good buffs. Good icon. Effects are nice, except the explosion looks bad for me having so much height. I would definitly choose a lower attachment point as default one. (-0.5) Reasoning Penalty ---------------------- Final Penalty -0

Marjosh38 - Elementum Spectrum

Result Judgement Concept Code Visuals Penalty Poll-votes: 0/21 Final Judgement Score: 19/40 Penalty: 3.5 -------------- ------ Final Score: 29.7/100 Concept: 4/10 Code: 8/20 Visuals: 7/10 ---------- ------ Score: 19/40 4/10 Critique Concept The caster summons water waves which circulate around the targeted point, and get closer until they reach, and explode. Afer each casts there's possibility to use an an other ability to switch to an element form; either water, or fire. Each element form lasts only for some seconds, and provides a special ability that can be used exclusivly by the respective form. The concept is a bit off the theme to have one (main) ability, with one potential sub ability. It's actually off pretty much. There is a main ability, and then two abilities two switch forms. The forms in clude some bonus damage, and alternative model, which I definitly already see as sub ability. On top of that, each element form has one special ability that might be used, one fire, and one water ability. Furthermore, I don't accept the both element ability, the water crush ability, and the fire bolt shooting, as sub abilities. They are like normal, independant abilities, which are just used by the elemental form, and have nothing much to do with the main ability... it's quite random, and more a sort of elemental spell collection. (-3) The added water spell, was simple, but ok. The fire spell felt just random, and felt like you just wanted to put some other content to match the fire theme. It doesn't really add something interesting. (-0.5) It has definitly something interesting, that the caster has access to an other elemenal form. Though, the implementation feels very rushed, and not complete. It's very hard to test, and especialy to enjoy the given content. The jumpy add/remove ability feels uncomfortable to use, plus you only have a few seconds in your form, which does end very fast, too. I would definitly work on the time management. (-1.5) The circular movement is pretty nice. Though, the spell is practicaly pretty overpowered, and with level 3 I could just kill all enemies without even reaching the explosion and tossing part. Some values needs to be changed, and user can not do that. (-1) 8/20 Critique Code ==== Configuration===== Values that define measurable effects, like damage, range, duration, speed, etc... and visualy created effects that the user might wanna change, and mostly as much as possible should be put into the configuration part. Big configuration, and setup, are often a good sign for internal code, too, as it will have dynamicly to apply all settings correctly. Very few config, even it's short, is often a bad sign, and si also bad for the user in the end. I deduct here, for all config triggers and aspects at once. It's a pretty heavy aspect, as there's actually nothing 'officialy' configurable, which is a no-no. (-2.5) ===== Cast Trigger ===== Manipulating 'Custom Value' of a unit is a forbidden operation for every system or spell, that is not a Unit Indexer. A Unit Indexer's job is to give a unique index, this custom value, to each unit. Your spell import would break the mechanics of all unit indexers, and so isn't allowed to be publicly shared. It's stated in the rules, too. (-1) The turn-on check, and the extra burn registration check hasn't to happen each loop run. The concept with elemental abilites is to work with enable/disable abilities for the player. This leads to MPI behaviour, and not to MUI, which is no-no. (-2) (Triggering Player) might be stored as well into hashtable, instead of repeating (Owner Of (Unit)). Using (Integer A) inside loops is not very efficient, because you end up calling functions internaly. Integer A/B loop should only be used if it's not used inside the loop body - else custom variable loops should be prefered. ===== Periodic Trigger ===== The filtering method is not efficient, and unreadable. Such method should be used: Convenient Unit Group Filtering in GUI (-0.5) Responses like (Last Created Unit) or (Picked Unit) are a function call each time they are used. So, when used more often, the value should be stored into a variable for further usage instead of calling the same function over and over again. There are really many of them. (-0.5) The turn-trigger-off check should only happen once a unit is removed from group. A stun should be used to temporary disable a unit. Here are some downsides explained from the native 'Pause Unit' function: [vJASS] - [Snippet] SuspendUnit The stored group in hashtable is never destroyed, and so does leak. (-0.5) The caster won't deal damage in case he's removed from game. (-0.5) The caster should primaly deal the damage, not a dummy unit. Instead of using a default fire bolt ability a stun system might be used, to also ensure configurability. And what's redundant at the moment is also that for each cast a new dummy is created. (-0.5) ===== Other ===== The namings don't met conventions set by GPAG, and thus also not our spell rules. There are too big flaws in this regard, like not having a common suffix. Please carfuly read the GPAG thread. (-1) The used "RA" is hard to find, I'm not sure where it is. Where do you have it from? It works not perfectly - it uses wrong deindex methodic, and might cause wrong results. It's not necessarily your fault that it might bug, but it's your choice to choose the help products, and it's negligent to pick a not approved one. (-0.5) 3 hashtables are used. That not a no no per se, but it's not something really good, as hashtables are expensive. We only have 256 hashtables at max, and should not use 3 of them only for a spell concept, when one could be used easily. Alternativly you could use dynamic indexing / with indexer. 0.03 seconds intervals is usually good for movement behaviour. 0.01/0.02 are in most situations not required. Removing crow form is not a very good idea in each case. Imagine the unit by default has already the crow form ability, then it won't have no more after it was applied to the system, which is unwanted. There's a safe method for applying the crow form ability, have a look here: ability When you pause units, you don't need to update the group on each iteration at hashtable, you might do this action only once, after the group loop: Hashtable - Save Handle OfTempGroup as (Key BURNtempgroup) of (Key Handle) in ES_Hastable The duration for casting the elemental spells should be "0", at the moment it's not so easy to properly use it, because the abilities are so quickly removed again. (-0.5) There was documentation, but it can and should be improved. Have a look at [GUI] - GPAG - GUI Proper Application Guide. (-0.5) Fire and Water spell should be coded seperatly. Cast and periodic trigger. (-0.5) It can be considered usind a dummy recylcer system, as planty of dummies are used. It's a very tiny point. The abilities could be preloaded. (-1) points extra are splitted on the points above, at which there is no explicit score deduction. 7/10 Critique Visuals The icon doesn't really remind me of a elemental spectrum, crushing waves, ice/fire spell. It seems a bit random. (-1) The circular movment and the final crushing looks good. The visiual indicators of ice/water spell looks very neat above the casters head. The fire spell looks a pretty boring, and feels not required. (-0.5) There are no buffs for the enemy ;when the enemy is affected with pause. (-0.5) The tooltip isn't written very well, for example, "have" -> "has" , or he/she/it without "s". And the extra tooltips for the fire/water ability don't even share the same pettern, and are written without similar highlithng. (-1) Reasoning Penalty Entry was submitted too late -1.5 WIP(s) were not provided -1.5 Missing import instructions -0.5 ---------------------- Final Penalty -3.5

---

I congratulate the winners, and I'm really glad that in the end some more people joined the contest! It was nice to see the different spells submitted.

Contest Thread | Poll

 

[IcemanBo]

By the way, I almost forgot!

The maximum rep award was "30" before, because the rep power limit was "1" since Hive 2. But now, as the rep power is back to "4", we also increase the rep awards again, so the new awards are 45/30/20 , and still 5 for each other entry + 5 for judge. : )

 

[Flux]

Damn, if I had known the result would be this fast I would have joined.
Congrats to the winners.

 

[Loner-Magixxar]

Thanks for the review, and congrats to the winners.

As I read my mistakes in the code section, I respected more and more my choice to not take part in a coding contest again, because I SUCK at it And no way I'll have enough time to learn those or correct them. So that was my last one. Thanks everyone. These contests were so much educational (even the cinematic contest, last year), but definitely this much education is too much for my brain

I was kind of toxic, in previous contests, towards some of you, Tank-Commander, KILLCIDE and others that I don't remember them, and I apologize for that. Loners have a habit of turning into lost souls sometimes.

Here, I earned a good amount of experience and confidence towards my own abilities, some negative, some positive. The raven is crowing nevermore as always, and the time for modding maps is drawing to an end for me. There is only time to finish up whatever is in progress. I still have one last thing to do here before moving on. So you still have to put up with me a little longer.

And as for my spell in this contest: If it doesn't get approved because of code deficiency, just reject it, because I don't have time to fix it and in most cases I don't even know how. Man I wish I knew how to code like some of you do. I definitely took a wrong turn somewhere and ended up in a place that I don't belong to.

Ah I rambled again, sorry!

 

[KILLCIDE]

Awesome job, IcemanBo! I'm super impressed with how fast you came out with these results while still holding up to detailed reviews. Here are just some comments I have on your review of mine!

I'm not a great of SimError, as it's not really compatible with other shown messages (forced alignment of "x").
I would personaly love to see it optional / defineable via config. But I know, it's the standard way of showing errors, and so only a tiny point.

Yeah I know what you mean. I'm always conflicted on whether or not an error message should be shown.
By optional / defineable values, are you referring to the X/Y position of the text? I will definitely consider that next time. I should have also added a boolean of the message showing up or not.

Maybe you can check of "Less Than 0" and then just change the two steps, creating wave, and death checking.

If I'm understanding you correctly, are you saying I should first check if the unit is dead, then check for wave creation, and then the deindex conditions? That sounds ugly xD

In case caster is removed from game for what ever reason, the targets won't receive DoT no more. Maybe it's a good solution to change ownership of dummy in this case, and let him make the damage, so at least the target gets correct amount of damage -- we might think of a good standard solution in such a case for future, hm..

Never actually thought about that before. For the record, the dummy unit only casts the abilities that act as just simply showing the buff / debuff status of the units. I think if a caster gets removed from the game, all the instances they created should be removed as well.

It feels a bit weird that fired waves can't cross something unwalkable. The pathing check is for ground units, but in case there's water, or so, I don't think such a wave should be blocked. It maybe makes sense in some way, else you would not have done it, and you can tell me, but for now it makes more sense only check for map bounds for myself.

So my thought process at first was to make them go through terrain and such. However, I personally think it would be weird for the flame wave to go through a building, and Check Walkablity has no way of determining what kind of "unpathable" terrain you're on. It would be great if you can point me to a resource that does just that.

Some default cooldown would be good.

If you are referring to the cooldown of the spell, I made it like that on purpose for the test map haha.

The air units might die onExplosion, but actually can't be targeted. They should be immune.

I originally wanted to allow the spell to affect air units, but it required some extra coding on my part for the movement of effects. Unfortunately, I didn't have time for it, so an easy fix was to not allow it to target air units! haha I know it's lazy, but it's all I had. For the explosion, I figured it would be weird if a big explosion such as the one you saw when a unit died did not hit an air unit, so I allowed air units to pass through the filter for that reason.

The dummy onInit can be used to also preload the ability next to effects.

I preload two different units onInit: the global dummy caster and just a regular unit for the effects. I need the global dummy caster to stay alive, so if I decided to preload the effects with it, the effects will temporarily show up. I believe I left a comment stating why I use two dummy units to preload them.

 

[Anachron]

Congratz to the winners! And thanks to @IcemanBo for his judging!

Even I ended up not taking a high place I enjoyed it and won experience.

Next time I will have more time to finish I guess.

 

[StoPCampinGn00b]

Congrats! I love the visuals on some of these.

Bo, I think you forgot to move this to the main Arena forum. Doing it for ya right now.

 

[Tank-Commander]

• set angleIncrement = 360. / projCountCircle
  ->
  set angleIncrement = bj_PI*2 / projCountCircle
  
  as then, this won't be needed each loop run:
  set radAngle = bj_DEGTORAD * angle

Both angle and radAngle need to be passed to the projectile creator in order to have the right facing and momentum: if I swap it to bj_PI*2 I would then have to do bj_RADTODEG * angle (and would use radAngle = radAngle + angleIncrement each time), I went with 360. first since it saves an operation

• I believe SquareRoot should be sligthly faster than Pow, mainly, because it has less arguments!
  Joke aside, I just wondered why you used it, it's no negative point at all. ;D

force of habit, no real other reason - will change it XD

• Attaching data to a unit is more neat than making O(n) search loops. See like APEX_GetStunNode.

How would I go about doing that given I'm indexing via linked list, without using custom value of the unit or introducing a hashtable?

• Missiles are autonomous damage maker in the code, but as it's the caster's spell, it was good if he was also the unit that deals the damage out. Though, the positive thing in used logics is that in case caster is removed, the damage will still be there. I guess a combination of both would be the best, even it's a not a same fast/straight forward coding solution. Maybe an extra function that make checks and deals out the damage would become useful for such issues.

The caster is dealing the damage - though yes I could check if the caster has been removed, would be somewhat silly since we don't normally account for our spellcasters disappearing all together mid-spell

call UnitDamageTarget(udg_APEX_Unit[udg_APEX_Parent[node]], udg_APEX_Target[udg_APEX_Parent[node]], udg_APEX_ProjectileDamageHealth[udg_APEX_Parent[node]], false, false, APEX_AttackType(), APEX_DamageType(), APEX_WeaponType())

udg_APEX_Unit[udg_APEX_Parent[node]] references the caster

• The check for turning off the timer could just happen in the recylce process/function. But I think I understand your intention to do it in the loop logic --, that you might have thought to filter out the redundant overhead, in case the parent node is the one being recylced. But also with this minimum risk of redundancy, it would still be a bit more efficient style.

Yeah that's what I've done - projectiles are recycled the most often but by the logic of the code can never be the last thing to be recycled - so when they are recycled I don't run the check (at level 3 it saves about 30 IF checks), I do however run the check on the core recycle and stunned unit recycle since either of them could be the last thing

 

[IcemanBo]

I should have also added a boolean of the message showing up or not.

Iirc I meant such a boolean, too.

first check if the unit is dead, then check for wave creation, and then the deindex conditions?

Do I have the code wrong in mind or can you directly check for deindex condition if you make the time comparison to "<0", I think I meant that.

I think if a caster gets removed from the game, all the instances they created should be removed as well.

I'm not very sure, but it's very up to you I guess!

Damn, if I had known the result would be this fast I would have joined.

The results wouldn't have been such very fast if one more guy joined. Joke aside, you should have joined, yes.

However, I personally think it would be weird for the flame wave to go through a building, and Check Walkablity has no way of determining what kind of "unpathable" terrain you're on.

I'm not very sure there exists such a native way from a library. I know Quilnez made a lib for detecting all pathing types, but it doesn't check (by alone) if it's explicitly ba a structure, though maybe you can check it yourself if needed. Maybe a periodic check just if a building is in range would help?- I'm not very sure.

modding maps is drawing to an end for me.

Even you stop creating new maps, you still can just hang around and helping and learning on hive. Why the urge to be inactive? : )

I don't have time to fix it and in most cases I don't even know how.

Do you know how much time I've sent for the content? : ) At least take a look, and if there's really something you wanna fix you can ask for example always in the help forums- there are very nice and skilled helpers hanging.^)^

If you are referring to the cooldown of the spell, I made it like that on purpose for the test map haha.

There's a GUI to reset cooldowns, so I would prefer such things in the demo code personaly, I guess. But it was just a tiny note. ^^

... so I allowed air units to pass through the filter for that reason.

Ah, ok.^^ Didn't know it was on purpose.

Next time I will have more time to finish I guess.

Hope we can stay on the same side next time, too. : (

will change it

It was just a joke note, anyways. ^^ Sometimes I had very weird thoughts to note in the reviews. Luckily I mostly did not finaly. ;D

without using custom value of the unit or introducing a hashtable?

Not sure it's possible without indexer/hashtable, but it was only a minimal note iirc. Sometimes I didn't even know how "much" points to deduct for some notes, that's why I hope such sum in the end of several points is better. ;D

Yeah that's what I've done - projectiles are recycled the most often but by the logic of the code can never be the last thing to be recycled - so when they are recycled I don't run the check (at level 3 it saves about 30 IF checks), I do however run the check on the core recycle and stunned unit recycle since either of them could be the last thing

Yep, and I think you're for sure right/better performance wise. It was just a idea in my head, but you don't should bother to change it, if you're good with current method, I'm good with it, too, I guess.

@Tank-Commander , you had a nice winning streak! Though I guess now it's killcide's throne. And well-deserved, I would say!
@Loner-Magixxar, your concept was really good, maybe my favorite one. Also nice you put so much effort into the test map.


One last thing that counts to really more than one demo map. In a demo map:
I usually don't want huge cooldowns -> trigger it
I don't want that my caster dies, and I have to restart the map. -> trigger it
I don't want to trigger myself that I can see everything around me
I don't want to test the spell in a rushy/ always figthing scenario, I want a chilled atmosphere

It's just sometimes annoying, if you need to edit multiple maps yourself, to test the spells properly. Maybe we really can make a common demo template next time even, hm.

As I'm writing I see KILLCIDE is rushing through the spells section already, handling the entries, so your threads are probably all unlocked in some minutes already. so thanks, it was fun!

 

[KILLCIDE]

As I'm writing I see KILLCIDE is rushing through the spells section, so your threads are probably all unlocked in some minutes already. so thanks, it was fun!

Yes I'm also leaving reviews for them based on yours and setting them to their status as needed.

OT: Would you be able to unlock my thread for me?

 

[Loner-Magixxar]

Btw I just realized something I guess, my spell being too long betrayed me! Had I chosen a simpler and shorter one, maybe I would get around 17 or 18 in the code section. The more I coded, the more I sank There is a phenomenon called "perverse effect" which I think I fell victim for that. But no problem.

 

[Exarch]

Gratz winners!
I like the judging man, very impressive.

 

[PurplePoot]

• The ray namings could be set to the namings following the tooltip, like "Dark Ray" -> "Green Ray".

They were named to be flavourful (after the spell it's based on) but this would probably be better.

• The native UnitAlive should be used as check if a unit is dead, or a not IsUnitType(u,UNIT_TYPE_DEAD) and GetUnitTypeId(u) != 0 check. Units might potentialy (prracticaly very unlikely) get their healthpoints manipulated via code, also after death, for example, and then a check for the widget's life might return a false positive.

Setting dead unit life above 0 already breaks most existing code (including all GUI) so I don't think we should do anything to encourage people to make bad decisions.

• Generaly it's a good thing to take usage from already existing systems, just to outsource work, and not to repeat similar logics over and over again. Example: [vJASS] - [System] Polygon, or Line Segment Enumeration .v2.1a.
  Your own geometry approach, I must admit, is more performant than using said systems, especially the function to check if a point is inside the trigonometric rect, but you still need to do some "ugly" logics yourself, like enumerating all potential units + filtering out for real rect units, for example. But this is only a minior point, andonly in this very specific case, because you were also right to tailor a modular library right for this purpose, and which is efficient, so in theory such thing could have been submitted as standalone resource. Afterall, that's only a minimal deduction point.

This feels kind of backwards here. You criticize me for not making microoptimizations where it doesn't matter and criticize me for optimizing code where it does.

• The members like real p1x or real x12diff should be readonly. It could make sense to make it public, but then as public method operators. If the user has access to directly change for example the x1 value, then it must be ensured that the corresponding x1-delta values change with it respectivly to keep the system working.

Meh, could be changed I guess but it's an internal struct and not intended to be available to anyone who doesn't know what they're doing. I only reluctantly use methods, and think of structs as being C-like.

• I'm pretty sure that actually "<=" should be "<" in your "containsPoint" function, but even if I'm right it doesn't change something practicaly. It's just a mentioning.
  Also the function isn't really readable because of the very long line, it could become a bit more readable like this:
  

  method containsPoint takes real px, real py returns boolean
        return (py - this.p1y)*this.x12diff <= (px - this.p1x)*this.y12diff and /*
        */(py - this.p2y)*this.x23diff <= (px - this.p2x)*this.y23diff and /*
        */(py - this.p3y)*this.x34diff <= (px - this.p3x)*this.y34diff and /*
        */(py - this.p4y)*this.x41diff <= (px - this.p4x)*this.y41diff
      endmethod

The <= doesn't really matter. The line break thing is a neat trick though, haven't seen that.

• rays = InitHashtable() It's actually not needed, it may be stored into an integer[array] instead of a hashtable.

This was done to prevent any artificial and unnecessary restriction on the number of rays (although I suppose in practice things probably would start lagging long before this point).

• A group does not need to be explicitly cleared when it's about getting destroyed.

Source? Having to do this was common wisdom back in the day. I'm not sure if it was patched or just plain incorrect though.

• the onDestroy method should not be used - the only use might be in a extending struct.
  destroy() -> normal function call
  onDestroy() -> generate trigger, triggercondition, and will run through a trigger evaluation each time called

Wasn't aware vJass now let you write your own implementation of destroy(), and the documentation says nothing about it. Can you link some specific documentation which explains how it works?

• Condition and Actions might be combined to only conditions, and the boolexpr might be wrapped into a if-statement.
• The reason behind is, that each time a trigger condition or trigger action runs, a new operation thread is setup behind the scenes, so we can avoid this overhead.
  And we choose conditions because the jass machine might still try to evaluate the conditions, might some exist or not - but the actions are only executed if the condition returns true.

Makes code less readable and introduces bugs for no good reason.

• The "EndActions" function does run twice under normal conidtions, once for "Finish Cast" event, and once for "End Cast" event. The result is wrong attempt to destroy and to deallocate structs, and the not required clear of hashtable entries.
  There must be a safety check to only run the removal operations if the (Triggering Unit) is ensured to be still caster of a prismatical spray. (-0.5)

This is a good point if it happens (haven't tested to confirm, and surprised this wouldn't cause any bugs): double frees are bad. I didn't notice anything when testing and am used to an environment where a double free pretty much instantly takes down your system.

---

Thanks for the review and the contest!

 

[IcemanBo]

Thanks, too, PurplePoot.
Yes, you're also right that some points I mentioned were, or partialy personal, or not always really relevant. I tried not to deduct, though, or only very minimal at such points. So I guess some 'points' were kind of combination between deduction point and personal comment.

Source? Having to do this was common wisdom back in the day. I'm not sure if it was patched or just plain incorrect though.

I think that one major source for statements about leak-amounts was "gekko's big leaktest" on a german site: gekkos großer Leaktest! - inWarcraft Forum!
Sorry, it's written in german.
But he states (from his test) that a group leaks like 0.62kb + 0.025kb-0.040kb per unit inside, when not destroyed. But when destroyed, there's no difference in size of group.

What is an interesting point, or where we should be sceptical is that he states that global group variables do have always a mnimum leak of 0.289kb,
and that at other side, he didn't meassure any leak with local group variables (after destroying and nulled), which seems weird behaviour. But one must say that
his tests were not 100% correct, and only estimations, as for example he each time applied 2MB leak tolerance after one test. (one test is usually 100'000 repetitions of one code block)
Maybe we should definitly do tests again, not lastly for compare local/global groups.

Just for sharing, here are translated results:

original

-Grösse der Leaks:
Leak einer leeren UG: 0,62 kb
Leak pro Unit in der UG: 0,025-0,040 kb
Verbleibendes leak wenn man eine Gruppe löscht: 0,289kb (unabhängig von Grösse der UG)(mindestens 54% reduzierung)
Leak einer Location: 0,361kb
Verbleibendes leak nach löschen einer Location: 0,032kb(91,1% reduzierung)
Leak pro nicht zerstörtem Effekt: 11,631kb(!)
Leak pro zerstörtem Effekt: 0,09kb (99,2% reduzierung!)
Leak einer Unit, die dann mit fadetimer removed wird: 0,173kb
Leak einer Unit, die sofort gehided wird und mit fadetimer removed: 0,074

which means following: (I revise it, because he did not consider his nulling result always)

Leakage	Memory Occupation
Empty unit group	0.62 kb
Unit inside unit group	0.025kb-0.040 kb
Remaining leak of a unit group after destroyed (global)	0.289kb (indepenent from group size)
Location	0.361kb
Remaining leak of a group after destroy (local)	0kb (??) -- not sure if global would really always leak that much more in comparison Location after removed	-
Effect	11.631kb
Effect after destroyed	0.09kb (but he did not null it in this test, so we should not worry)
Unit Removal	From testing scenario I can't properly read out how the conditions were. No clear statement here for me.

Wasn't aware vJass now let you write your own implementation of destroy(), and the documentation says nothing about it. Can you link some specific documentation which explains how it works?

I'm a younger forum coder and vjass user than you are, but I'm personaly not aware of that it wasn't applyable at one time for me. I always simply declare like:

method destroy takes nothing returns nothing
    call .deallocate()
    // maybe some other member nulling, etc
endmethod

// at some other code
call .destroy()

^and if destroy() is placed above the caller function, then no trigger evaluation will happen, but just normal function call -- but with onDestroy there will always a eval happen, this is because it has to be ensured onDestroy always runs in case it's a child struct, because after parent destroy() it will try to execute the child's onDestroy() with help of trigger.