Pyrogasm
There are some who would use any excuse to ban me.
- Reaction score
- 134
Breaking news: I'm finished.
The raw scores for coding:
Please note that I only gave 5/5 for perfection in that specific category; if there was 1 leak or 1 inefficient thing, then I gave an automatic 4/5.
Download the RTF file with the gradings formatted nicely
Gradings:
The raw scores for coding:
- Doom-Angel: 6/10
- kc102: 6/10
- Thanatos: 8/10 (Should've been 9 )
- jonadrian619: 8/10
- Überplayer: 5/10
- dhk_undead_lord: 6/10
Please note that I only gave 5/5 for perfection in that specific category; if there was 1 leak or 1 inefficient thing, then I gave an automatic 4/5.
Download the RTF file with the gradings formatted nicely
Gradings:
Doom-Angel — Elite Spark - 3/5 - Efficiency
3/5 - Leakage
- Let me first start by saying this: Because you made an omnislash, I'm going to be a complete ass about grading it to make sure that you did everything perfectly; an omnislash is neither a difficult nor original spell.
- "(GetUnitState(GetFilterUnit(),UNIT_STATE_LIFE) > 0)" should be "(GetWidgetLife(GetFilterUnit()) > 0.405); A unit "dies" when its life reaches 0.405 or less, and as such, the comparison should be to this number as to not get any false positives. Additionally, GetWidgetLife() is better than GetUnitState().
- Why the backwards comparison in this?: "(IsUnitEnemy(GetTriggerUnit(),GetOwningPlayer(GetFilterUnit())))". Wouldn't it be smarter to compare the Filtered unit to the owner of the triggering unit...? (Not that your way doesn't work)
- Do you really need the "Region" function? The entire function is 1 line of code...
- Shouldn't "call SetUnitTimeScale(u,4-level)" be "call SetUnitTimeScale(u,level+1)"? Why would the unit move slower at "monstrous speed" than at "elite speed"?
- "call PolledWait(0.20)" == bad. Polledwaits are bad.
- UGH. The way you go about looping is extremely stupid. If you've ALREADY started a timer callback, why not just run all the periodic group stuff inside the loop? Doing it by starting a timer with no callback and then checking its time remaining... That's abominably stupid because: A) It leaks a timer B) Is not configurable C) It leaves everything up to the polledwaits (Which, by-the-way, cause the caster to only slash ~0.09 seconds faster, whereas in the level 1-2 jump, the speed increase is 0.35 seconds), and D) Is an inefficient way to make an Omnislash!
- In "call SetUnitFacingTimed(u,AngleBetweenPoints(loc,loc2),0.01)", why are you using locations?! It wouldn't have been too hard to find a function that's widely used called "AngleBetweenPointsXY".
- "call UnitDamageTarget(u,target,100, true, false, ATTACK_TYPE_HERO,DAMAGE_TYPE_ENHANCED,WEAPON_TYPE_METAL_MEDIUM_SLICE)" should be "ATTACK_TYPE_CHAOS" and "DAMAGE_TYPE_UNIVERSAL", else the unit will not deal full damage.
- "call AddSpecialEffectTarget("Objects\\Spawnmodels\\Undead\\UndeadBlood\\UndeadBloodNecromancer.mdl",target,"chest")" and "call AddSpecialEffectTarget("Abilities\\Spells\\Orc\\MirrorImage\\MirrorImageCaster.mdl",u,"origin")" both leak special effects
- As I said earlier, the line "call PolledWait(1.06-(0.35*level))" doesn't cause the effective speed increase from levels 2-3. As the minimum wait time is roughly 0.27 seconds, the difference between 0.36 and 0.01 is not 0.35 seconds, but 0.09.
- WHY ARE YOU USING A RECT?! Additionally, the whole need for globals udg_X and udg_Y could be eliminated using the following format instead of the ForGroup() call:
loop
set U = FirstOfGroup(G) //U being a unit variable not in use and G being the group you need to loop through
exitwhen U == null
//Do your stuff with U; note that this means all variables present in the parent function are still useable, this does not create a new thread
call GroupRemoveUnit(G, U)
endloop
- There's a double-free of bj_groupAddGroupDest because you have this line twice: "call DestroyGroup(bj_groupAddGroupDest)"
- The use of that global group also makes this entire spell non-MUI, which sucks. Once again, this could've been avoided by using the above group loop format.
- "set bj_groupAddGroupDest = null" is not needed; it's a global group.
- Your function names are abominable. "filter" and "conditions"; come on.
kc102 — Blink Throw - 2/5 - Efficiency
4/5 - Leakage
- "IsUnitAliveBJ(GetFilterUnit()) == true" is bad! You should always use a life comparison: "(GetWidgetLife(GetFilterUnit()) > 0.405); A unit "dies" when its life reaches 0.405 or less, and as such, the comparison should be to this number as to not get any false positives. Additionally, GetWidgetLife() is better than GetUnitState().
- Wouldn't it be better to remove the "*4" on this line to avoid confusion: "local integer level = GetUnitAbilityLevel(GetTriggerUnit(), 'A000') * 4"? You could simply multiply 'level' by 4 when needed, or rename the variable to something like "levelfactor".
- In this line: "local group randus = GetRandomSubGroup(level, GetUnitsInRangeOfLocMatching(400.00 + (100 * level), caspos, Condition(function isally)))", there is a group leaked from the "GetUnitsInRangeOfLocMatching" part and a boolexpr leaked from the "Condition(function isally)". Would it really have been too hard to do this:
local boolexpr b = Condition(function isally)
local group g = GetUnitsInRangeOfLocMatching(400.00 + (400 * level), caspos, b)
local group randus = CreateGroup()
local integer i = 0
local unit u
loop
set i = i+1
exitwhen i > level*4
set u = GroupPickRandomUnit(g)
call GroupAddUnit(randus)
call GroupRemoveUnit(g)
endloop
call DestroyGroup(g)
call DestroyBoolExpr(b)
- Additionally, USE X AND Y COORDINATES! They're USEFUL!
- " if num == 0 then
return
else
endif " The formatting burns! Also, the "else" is not needed; only the "if" and "endif" are important.
- On a small map, "set rand = GetRandomLocInRect(GetPlayableMapRect())" seems OK... but what about on a big map? That, and you're not checking to see if it's pathable or not.
- "call DestroyEffect(AddSpecialEffectLoc("Abilities\\Spells\\NightElf\\Blink\\BlinkCaster.mdl", rand))" and "call SetUnitPositionLoc(move, rand)" should both be replaced with X/Y value-related equivalents.
- "call UnitDamageTargetBJ( caster, move, 50.00 * (level / 4), ATTACK_TYPE_CHAOS, DAMAGE_TYPE_UNIVERSAL )" could easily be re-written without the BJ; it's only one extra argument that never changes!
- Instead of "exitwhen loopa >= level", I would do "exitwhen loopa > level" and then declare "loopa" as 1 instead of 0.
Thanatos — Hurricane - 4/5 - Efficiency
4/5 - Leakage (Hooray for KaTTaNa's leaks!)
- First thing off is that I like the fact that everything is concise and JESP (as opposed to everything else), but I really hate the way you lay out your coding. You should never separate function names from their arguments, like so:
"local player P = Player (i)"
Even if it makes it go with the code's formatting.
- AUGH! NO! BAD! THIS IS WHY WE DON'T USE KATTANA'S SYSTEM ANYMORE:
"function LocalVars takes nothing returns gamecache
local gamecache a = InitGameCache ("jasslocalvars")
local integer i = HV_H2I (a)
set a = null
return i
return null
endfunction"
The above function LEAKS GAMECACHES! If you have to use something that uses GameCache, use CSCache instead!
- In the "Preloads" section, "local player b = Player (PLAYER_NEUTRAL_PASSIVE) " is pointless; players don't leak, and you only use the player once in the function.
- When you create a special effect like so: "AddSpecialEffectTarget (Hurricane_SpecialEffect(),c,"origin") ", and then destroy it right away, once can simply write this: "call DestroyEffect(AddSpecialEffectTarget (Hurricane_SpecialEffect(),c,"origin"))"
- Local triggers are bad. Period. Vexorian and IceFrog have shown that destroying them causes major Handle Stack screwups... but if you don't destroy them, they leak. Additionally, "call TriggerRegisterUnitEvent (castEnd,caster,EVENT_UNIT_SPELL_ENDCAST)" only registers when the unit stops casting involuntarily; you should also add this line in: "call TriggerRegisterUnitEvent (castEnd,caster,EVENT_UNIT_SPELL_FINISH)". Oh wait... now that I've read the rest of the code, you probably don't need that, do you? >_<
- "call UnitDamageTarget (caster,target,damage,false,false,ATTACK_TYPE_NORMAL,DAMAGE_TYPE_MAGIC,null)" should be "ATTACK_TYPE_CHAOS" and "DAMAGE_TYPE_UNIVERSAL".
- Holy crap. Nice coding.
jonadrian619 — Phase Swap - 3/5 - Efficiency
5/5 - Leakage
- The event "Unit - A unit is attacked" FIRES WHEN A UNIT INITIATES THE ATTACK; NOT WHEN THE ATTACK/DAMAGE ACTUALLY HAPPENS! This means that a player can constantly trigger this event by right-clicking a unit, or by ordering a unit to attack another unit. Even excluding ranged attackers does not help.
- On your Custom Script lines, you should be using the native functions, not the BJ ones. I suggest you just learn JASS if you're going to use it in Custom Script form in GUI. Really.
- Instead of "Set PointAroundattacker = (Region centered at Point with size (100.00, 100.00))" and "Set TempPoint = (Random point in PointAroundattacker)", you should learn to use the polar offset function. Using this, you would write this instead: "Set TempPoint = (Point offset by (Random real between 0.00 and 100.00) towards (Random Angle) degrees)"
- "Unit - Cause (Attacked unit) to damage (Attacking unit), dealing 50.00 damage of attack type Normal and damage type Normal" should be "attack type Chaos and damage type Universal".
- All instances of "Attacked Unit" should be replaced with "Triggering Unit".
- Why is "Custom script: call AddLightningLoc( "CHIM", udg_Point, udg_TeleportPoint )" a custom script call? One can simply use the GUI function and then set the local variable in the next line in the same way.
- Simply adding 100% evasion to the attacked unit for 1.00 seconds does work, but it has the drawback of getting screwed up if the unit is attacked twice or more during that 1.00 second window. If, for instance, the unit was attacked by 1 footman, and then 0.85 seconds later was attacked by another footman. When the first instance of the trigger ended, the "evasion" ability would be removed from the hero, which would mean that there is now NO evasion ability on it like there should be for the next 0.85 seconds (the second instance of the trigger).
- When you do this: "Unit - Move (Attacked unit) instantly to TempPoint", shouldn't you also make the hero face the target?
Überplayer — Rapid Draw - 2/5 - Efficiency
3/5 - Leakage
- AUGH AGAIN! This function leaks gamecaches!:
function LocalVars takes nothing returns gamecache
return InitGameCache("jasslocalvars.w3v")
endfunction
- Shouldn't "call Preload("Abilities\\Weapons\\ZigguratFrostMissile\\ZigguratFrostMissile.mdl")" just use the constant function for SFX?
- You didn't null "t" in the init function
- You overuse "GetTriggerUnit()" in the function "Rapid_Draw_Activate_Actions"; set it to a variable for christ's sake!
- Why the "GetUnitFacing(GetTriggerUnit())" in the dummy create function? If it's a dummy, it doesn't matter which way it's facing...
- In "call SetHandleHandle(t, "u", GetTriggerUnit())", "u" is NOT a good name; it could easily be overwritten by something else.
- Why this: "call PolledWait(8.00)"? If you've got the timer set up, do some simple math (8.00/0.11) to find out how many times the timer will loop before it's reached 8.00 seconds. Then, attach an integer counter that decreases every callback of the timer; finally, destroy the timer when its counter reaches 0. Not that hard, is it?
- GetAttacker() is overused in Rapid_Draw_Unleash_Actions.
- What the hell is with storing the unit's handle index to its custom value? Why don't you simply check to see if the attacking unit has the ability?!: "return GetUnitAbilityLevel(GetTriggerUnit(), DummySpellId()) > 0"
- Your constant functions could use prefixes.
- Instead of "call PolledWait(2.00); call RemoveUnit(d)", you can simply add a 2.00 second expiration timer to the unit: "call UnitApplyTimedLIfe(d, 'BTLF', 2.00)".
- ...any particular reason you didn't simply modify an orb of slow/frost ability and add that to the hero for the duration of the spell instead of doing complicated, unreliable attack (not damage) detection and dummy spell triggering?
-
dhk_undead_lord — Troll Speed Spells - 2/5 - Efficiency
4/5 - Leakage
- Making 2 spells for a contest and trying to pass it off as "ok" is really dumb. Point and case.
- All instances of "Unit - A unit Begins casting an ability" in all triggers should be changed to "Unit - A starts the effect of an ability"
- Why do all the triggers have "(Unit-type of (<Whatever> unit)) Equal to Troll" in them? Shouldn't the fact that they have the ability be just as good?
- The event "Unit - A unit Is attacked" registers when the unit initiates the attack, not when the damage happens; refer to above gradings for more information as to why that's bad.
- All instances of "Attacked Unit" in all triggers should be replaced with "Triggering Unit".
- "Unit - Cause (Attacking unit) to damage (Attacked unit), dealing (5.00 x (Real((Level of Troll Axe Frenzy for (Attacking unit))))) damage of attack type Hero and damage type Normal" should be "of attack type Chaos and damage type Universal".
- You should set the integer variable to a new random value before each If/Then/Else statement; the way you have it set up now makes it impossible to get bonus damage without the other two effects (for a number to be less than 5, it must also be less than 7 and 10).
- "Wait until (((Triggering unit) has buff Troll Frenzy ) Equal to False), checking every 1.00 seconds" is a bad way to do things, but in GUI you've got no option. However, since you've already knocked this down to MPI, why not just create 12 timer countdown triggers?
- "Set Temp_Point2 = (Position of (Random unit from (Units within 250.00 of Temp_Point matching ((((Matching unit) is alive) Equal to True) and (((Owner of (Matching unit)) is an enemy of (Owner of (Attacking unit))) Equal to True)))))" leaks a unit-group.
- What purpose does the "Troll Frenzy Slow Debug" trigger serve? After any sort of a wait, "Target Unit of Ability Being Cast" returns no unit, effectively making the trigger do nothing, 100% of the time.
- The sheer number of triggers is, well, daunting.
3/5 - Leakage
- Let me first start by saying this: Because you made an omnislash, I'm going to be a complete ass about grading it to make sure that you did everything perfectly; an omnislash is neither a difficult nor original spell.
- "(GetUnitState(GetFilterUnit(),UNIT_STATE_LIFE) > 0)" should be "(GetWidgetLife(GetFilterUnit()) > 0.405); A unit "dies" when its life reaches 0.405 or less, and as such, the comparison should be to this number as to not get any false positives. Additionally, GetWidgetLife() is better than GetUnitState().
- Why the backwards comparison in this?: "(IsUnitEnemy(GetTriggerUnit(),GetOwningPlayer(GetFilterUnit())))". Wouldn't it be smarter to compare the Filtered unit to the owner of the triggering unit...? (Not that your way doesn't work)
- Do you really need the "Region" function? The entire function is 1 line of code...
- Shouldn't "call SetUnitTimeScale(u,4-level)" be "call SetUnitTimeScale(u,level+1)"? Why would the unit move slower at "monstrous speed" than at "elite speed"?
- "call PolledWait(0.20)" == bad. Polledwaits are bad.
- UGH. The way you go about looping is extremely stupid. If you've ALREADY started a timer callback, why not just run all the periodic group stuff inside the loop? Doing it by starting a timer with no callback and then checking its time remaining... That's abominably stupid because: A) It leaks a timer B) Is not configurable C) It leaves everything up to the polledwaits (Which, by-the-way, cause the caster to only slash ~0.09 seconds faster, whereas in the level 1-2 jump, the speed increase is 0.35 seconds), and D) Is an inefficient way to make an Omnislash!
- In "call SetUnitFacingTimed(u,AngleBetweenPoints(loc,loc2),0.01)", why are you using locations?! It wouldn't have been too hard to find a function that's widely used called "AngleBetweenPointsXY".
- "call UnitDamageTarget(u,target,100, true, false, ATTACK_TYPE_HERO,DAMAGE_TYPE_ENHANCED,WEAPON_TYPE_METAL_MEDIUM_SLICE)" should be "ATTACK_TYPE_CHAOS" and "DAMAGE_TYPE_UNIVERSAL", else the unit will not deal full damage.
- "call AddSpecialEffectTarget("Objects\\Spawnmodels\\Undead\\UndeadBlood\\UndeadBloodNecromancer.mdl",target,"chest")" and "call AddSpecialEffectTarget("Abilities\\Spells\\Orc\\MirrorImage\\MirrorImageCaster.mdl",u,"origin")" both leak special effects
- As I said earlier, the line "call PolledWait(1.06-(0.35*level))" doesn't cause the effective speed increase from levels 2-3. As the minimum wait time is roughly 0.27 seconds, the difference between 0.36 and 0.01 is not 0.35 seconds, but 0.09.
- WHY ARE YOU USING A RECT?! Additionally, the whole need for globals udg_X and udg_Y could be eliminated using the following format instead of the ForGroup() call:
loop
set U = FirstOfGroup(G) //U being a unit variable not in use and G being the group you need to loop through
exitwhen U == null
//Do your stuff with U; note that this means all variables present in the parent function are still useable, this does not create a new thread
call GroupRemoveUnit(G, U)
endloop
- There's a double-free of bj_groupAddGroupDest because you have this line twice: "call DestroyGroup(bj_groupAddGroupDest)"
- The use of that global group also makes this entire spell non-MUI, which sucks. Once again, this could've been avoided by using the above group loop format.
- "set bj_groupAddGroupDest = null" is not needed; it's a global group.
- Your function names are abominable. "filter" and "conditions"; come on.
kc102 — Blink Throw - 2/5 - Efficiency
4/5 - Leakage
- "IsUnitAliveBJ(GetFilterUnit()) == true" is bad! You should always use a life comparison: "(GetWidgetLife(GetFilterUnit()) > 0.405); A unit "dies" when its life reaches 0.405 or less, and as such, the comparison should be to this number as to not get any false positives. Additionally, GetWidgetLife() is better than GetUnitState().
- Wouldn't it be better to remove the "*4" on this line to avoid confusion: "local integer level = GetUnitAbilityLevel(GetTriggerUnit(), 'A000') * 4"? You could simply multiply 'level' by 4 when needed, or rename the variable to something like "levelfactor".
- In this line: "local group randus = GetRandomSubGroup(level, GetUnitsInRangeOfLocMatching(400.00 + (100 * level), caspos, Condition(function isally)))", there is a group leaked from the "GetUnitsInRangeOfLocMatching" part and a boolexpr leaked from the "Condition(function isally)". Would it really have been too hard to do this:
local boolexpr b = Condition(function isally)
local group g = GetUnitsInRangeOfLocMatching(400.00 + (400 * level), caspos, b)
local group randus = CreateGroup()
local integer i = 0
local unit u
loop
set i = i+1
exitwhen i > level*4
set u = GroupPickRandomUnit(g)
call GroupAddUnit(randus)
call GroupRemoveUnit(g)
endloop
call DestroyGroup(g)
call DestroyBoolExpr(b)
- Additionally, USE X AND Y COORDINATES! They're USEFUL!
- " if num == 0 then
return
else
endif " The formatting burns! Also, the "else" is not needed; only the "if" and "endif" are important.
- On a small map, "set rand = GetRandomLocInRect(GetPlayableMapRect())" seems OK... but what about on a big map? That, and you're not checking to see if it's pathable or not.
- "call DestroyEffect(AddSpecialEffectLoc("Abilities\\Spells\\NightElf\\Blink\\BlinkCaster.mdl", rand))" and "call SetUnitPositionLoc(move, rand)" should both be replaced with X/Y value-related equivalents.
- "call UnitDamageTargetBJ( caster, move, 50.00 * (level / 4), ATTACK_TYPE_CHAOS, DAMAGE_TYPE_UNIVERSAL )" could easily be re-written without the BJ; it's only one extra argument that never changes!
- Instead of "exitwhen loopa >= level", I would do "exitwhen loopa > level" and then declare "loopa" as 1 instead of 0.
Thanatos — Hurricane - 4/5 - Efficiency
4/5 - Leakage (Hooray for KaTTaNa's leaks!)
- First thing off is that I like the fact that everything is concise and JESP (as opposed to everything else), but I really hate the way you lay out your coding. You should never separate function names from their arguments, like so:
"local player P = Player (i)"
Even if it makes it go with the code's formatting.
- AUGH! NO! BAD! THIS IS WHY WE DON'T USE KATTANA'S SYSTEM ANYMORE:
"function LocalVars takes nothing returns gamecache
local gamecache a = InitGameCache ("jasslocalvars")
local integer i = HV_H2I (a)
set a = null
return i
return null
endfunction"
The above function LEAKS GAMECACHES! If you have to use something that uses GameCache, use CSCache instead!
- In the "Preloads" section, "local player b = Player (PLAYER_NEUTRAL_PASSIVE) " is pointless; players don't leak, and you only use the player once in the function.
- When you create a special effect like so: "AddSpecialEffectTarget (Hurricane_SpecialEffect(),c,"origin") ", and then destroy it right away, once can simply write this: "call DestroyEffect(AddSpecialEffectTarget (Hurricane_SpecialEffect(),c,"origin"))"
- Local triggers are bad. Period. Vexorian and IceFrog have shown that destroying them causes major Handle Stack screwups... but if you don't destroy them, they leak. Additionally, "call TriggerRegisterUnitEvent (castEnd,caster,EVENT_UNIT_SPELL_ENDCAST)" only registers when the unit stops casting involuntarily; you should also add this line in: "call TriggerRegisterUnitEvent (castEnd,caster,EVENT_UNIT_SPELL_FINISH)". Oh wait... now that I've read the rest of the code, you probably don't need that, do you? >_<
- "call UnitDamageTarget (caster,target,damage,false,false,ATTACK_TYPE_NORMAL,DAMAGE_TYPE_MAGIC,null)" should be "ATTACK_TYPE_CHAOS" and "DAMAGE_TYPE_UNIVERSAL".
- Holy crap. Nice coding.
jonadrian619 — Phase Swap - 3/5 - Efficiency
5/5 - Leakage
- The event "Unit - A unit is attacked" FIRES WHEN A UNIT INITIATES THE ATTACK; NOT WHEN THE ATTACK/DAMAGE ACTUALLY HAPPENS! This means that a player can constantly trigger this event by right-clicking a unit, or by ordering a unit to attack another unit. Even excluding ranged attackers does not help.
- On your Custom Script lines, you should be using the native functions, not the BJ ones. I suggest you just learn JASS if you're going to use it in Custom Script form in GUI. Really.
- Instead of "Set PointAroundattacker = (Region centered at Point with size (100.00, 100.00))" and "Set TempPoint = (Random point in PointAroundattacker)", you should learn to use the polar offset function. Using this, you would write this instead: "Set TempPoint = (Point offset by (Random real between 0.00 and 100.00) towards (Random Angle) degrees)"
- "Unit - Cause (Attacked unit) to damage (Attacking unit), dealing 50.00 damage of attack type Normal and damage type Normal" should be "attack type Chaos and damage type Universal".
- All instances of "Attacked Unit" should be replaced with "Triggering Unit".
- Why is "Custom script: call AddLightningLoc( "CHIM", udg_Point, udg_TeleportPoint )" a custom script call? One can simply use the GUI function and then set the local variable in the next line in the same way.
- Simply adding 100% evasion to the attacked unit for 1.00 seconds does work, but it has the drawback of getting screwed up if the unit is attacked twice or more during that 1.00 second window. If, for instance, the unit was attacked by 1 footman, and then 0.85 seconds later was attacked by another footman. When the first instance of the trigger ended, the "evasion" ability would be removed from the hero, which would mean that there is now NO evasion ability on it like there should be for the next 0.85 seconds (the second instance of the trigger).
- When you do this: "Unit - Move (Attacked unit) instantly to TempPoint", shouldn't you also make the hero face the target?
Überplayer — Rapid Draw - 2/5 - Efficiency
3/5 - Leakage
- AUGH AGAIN! This function leaks gamecaches!:
function LocalVars takes nothing returns gamecache
return InitGameCache("jasslocalvars.w3v")
endfunction
- Shouldn't "call Preload("Abilities\\Weapons\\ZigguratFrostMissile\\ZigguratFrostMissile.mdl")" just use the constant function for SFX?
- You didn't null "t" in the init function
- You overuse "GetTriggerUnit()" in the function "Rapid_Draw_Activate_Actions"; set it to a variable for christ's sake!
- Why the "GetUnitFacing(GetTriggerUnit())" in the dummy create function? If it's a dummy, it doesn't matter which way it's facing...
- In "call SetHandleHandle(t, "u", GetTriggerUnit())", "u" is NOT a good name; it could easily be overwritten by something else.
- Why this: "call PolledWait(8.00)"? If you've got the timer set up, do some simple math (8.00/0.11) to find out how many times the timer will loop before it's reached 8.00 seconds. Then, attach an integer counter that decreases every callback of the timer; finally, destroy the timer when its counter reaches 0. Not that hard, is it?
- GetAttacker() is overused in Rapid_Draw_Unleash_Actions.
- What the hell is with storing the unit's handle index to its custom value? Why don't you simply check to see if the attacking unit has the ability?!: "return GetUnitAbilityLevel(GetTriggerUnit(), DummySpellId()) > 0"
- Your constant functions could use prefixes.
- Instead of "call PolledWait(2.00); call RemoveUnit(d)", you can simply add a 2.00 second expiration timer to the unit: "call UnitApplyTimedLIfe(d, 'BTLF', 2.00)".
- ...any particular reason you didn't simply modify an orb of slow/frost ability and add that to the hero for the duration of the spell instead of doing complicated, unreliable attack (not damage) detection and dummy spell triggering?
-
dhk_undead_lord — Troll Speed Spells - 2/5 - Efficiency
4/5 - Leakage
- Making 2 spells for a contest and trying to pass it off as "ok" is really dumb. Point and case.
- All instances of "Unit - A unit Begins casting an ability" in all triggers should be changed to "Unit - A starts the effect of an ability"
- Why do all the triggers have "(Unit-type of (<Whatever> unit)) Equal to Troll" in them? Shouldn't the fact that they have the ability be just as good?
- The event "Unit - A unit Is attacked" registers when the unit initiates the attack, not when the damage happens; refer to above gradings for more information as to why that's bad.
- All instances of "Attacked Unit" in all triggers should be replaced with "Triggering Unit".
- "Unit - Cause (Attacking unit) to damage (Attacked unit), dealing (5.00 x (Real((Level of Troll Axe Frenzy for (Attacking unit))))) damage of attack type Hero and damage type Normal" should be "of attack type Chaos and damage type Universal".
- You should set the integer variable to a new random value before each If/Then/Else statement; the way you have it set up now makes it impossible to get bonus damage without the other two effects (for a number to be less than 5, it must also be less than 7 and 10).
- "Wait until (((Triggering unit) has buff Troll Frenzy ) Equal to False), checking every 1.00 seconds" is a bad way to do things, but in GUI you've got no option. However, since you've already knocked this down to MPI, why not just create 12 timer countdown triggers?
- "Set Temp_Point2 = (Position of (Random unit from (Units within 250.00 of Temp_Point matching ((((Matching unit) is alive) Equal to True) and (((Owner of (Matching unit)) is an enemy of (Owner of (Attacking unit))) Equal to True)))))" leaks a unit-group.
- What purpose does the "Troll Frenzy Slow Debug" trigger serve? After any sort of a wait, "Target Unit of Ability Being Cast" returns no unit, effectively making the trigger do nothing, 100% of the time.
- The sheer number of triggers is, well, daunting.