are the results gonna be soon ? Uareanoob said 5-9 days..10 days now..i don't want to rush anything just courious..i don't bother anymore until the results
Pyrogasm said:Spell Contest III - Traps
Coding Grades by Pyrogasm
Please note that I gave full marks in any category only if I could find no errors in that category.
Demonic Force - Monsterous
Efficiency - 4/10
Leakage - 1/10 Total: 5/25
Importability - 0/5
Look-over notes:
- "Casting_Unit" is a bad choice for a variable name; it will easily conflict with other spells.
- Both instances of "Casting Unit" should be "Triggering Unit".
- Why use "Position of (No unit)"; that just means 0.00 degrees.
- You leak 2 locations.
- Shit, dude. You leak 274 locations just from your SFX, then another 6 from other lines.
"(Position of (Attacking Unit)) offset by X, Y" leaks 2 locations, and you did it 140 times
- It would be hell to try to change the effect added... why not use a variable?
- The way you trigger the event is much better than adding events.
- The entire thing could be done with a few loops:
For each (Integer A) from 1 to 27, do (Actions)
Loop - Actions
Special Effect - Create a special effect at ((Position of (Attacking unit)) offset by ((-5.00 + ((Real((Integer A))) x 5.00)), (300.00 - ((Real((Integer A))) x 30.00)))) using Abilities\Spells\Orc\TrollBerserk\HeadhunterWEAPONSLeft.mdl
Set Special_Effect[(Integer A)] = (Last created special effect)
And so on for the rest, you just need to do the math (Note that this still leaks locations).
- Another loop to replace all the SFX crap at the end:
For each (Integer A) from 1 to 137, do (Actions)
Loop - Actions
Custom script: call DestroyEffect (udg_Special_Effect[GetForLoopIndexA()])
- Any uses of "Attacked Unit" should be replaced with "Triggering Unit"; the latter is always preferable.
- You should store "Attacking Unit" to a variable so as to have a less of a chance for mishap, and it makes it more customizable.
- "Wait X.XX seconds" has been known to Desync— use "Wait X.XX Game-time seconds" instead.
- The "Damage Circular Area" function is bugged; it DOES NOT WORK. Instead, do this:
Custom script: set bj_wantDestroyGroup = true
Unit Group - Pick every unit in (Units within 350.00 of ((Position of (Attacking unit)) offset by (0.00, -90.00)) matching (((Matching Unit) is an enemy of (Owner of (Casting_Unit))) = true) and do (Actions)
Unit - Cause Casting_Unit to damage (Picked Unit) dealing ((Real((Level of Demonic Force for Casting_Unit))) x 200.00) damage of attack type Chaos and damage type Normal
Use "damage type Universal" instead of "Normal", as the latter will be affected by armor.
Note that this still leaks locations.
- When you do "Unit - Set (Attacked unit) movement speed to ((Current movement speed of (Attacked unit)) / 6.00)", that factors
in all Movespeed modifying bonuses too; you're not just modifying the base
- Overall, pretty poor and simplistic; you didn't even slide the unit into the center.
Chain Trap - Naminator
Efficiency - 9/10
Leakage - 9/10 Total: 22/25
Importability - 4/5
Look-over notes:
- Nice use of prefixed variables; this would be easy to import.
- You could've reused the variable "CT_Point" instead of using "CT_Trapped_Loc".
- You should've used an entirely new ability instead of modifying the Aerial Shackles ability.
- The only problem with the spell is the fact that it leaks events; when you add an event to the second trigger, there is no way to remove it, and
once the trap is dead, its specific event is useless. A better way to do it (the way Monsterous has done) is to give your dummy trap an attack
with a range equal to that of the radius of the spell and a very slow projectile speed; then, detect when a unit is attacked by a trap, order the
trap to stop, and do your effects from there.
- Do you know JASS? Setting the picked unit to a variable is a very JASS-ish thing to do, not really seen in GUI as people don't realize what function calls are.
- Very simple spell, but it is done in a top-notch fashion! Well done!
Water Image - Ozaru
Efficiency - 5/10
Leakage - 5/10 Total: 12/25
Importability - 2/5
Look-over notes:
- Dear god! LEARN TO INDENT YOUR CODE!
- You have constants... but they are very, very minimalistic.
- The way you've done this means that you must have 3 additional units for the ability; if you'd simply used the Wand of Illusion ability/bug trick
you could've cloned the unit. This would have removed the UI icon in the top left, made the unit tinted to the owning player for easy recognition,
and removed all hero abilities automatically.
- You should prefix your functions for importability.
- What does "IT" stand for in the Condition function? Additionally, you should make a compound return like so:
local integer Id = GetUnitTypeId(GetAttackedUnitBJ())
return Id == 'H002' or Id == 'H003' or Id == 'H001'
- In the function "Level", it is possible that you will return an un-initialized integer; to fix this, do "local integer i = 0".
- In both "Level" and "ITCondition", you should change "GetAttackedUnitBJ()" to "GetTriggerUnit()" and store it to a variable instead of calling it each time.
- "local unit Trap=GetAttackedUnitBJ()" should be "local unit Trap=GetTriggerUnit()"
- Use X/Y coordinates! That's one of the things JASS is most useful for! I won't show what to change because I would be basically rewriting the trigger for you.
Figure it out on your own, and please don't use locations.
- AUGH! JASS is useful because you don't have to do dumb things like "GetLastCreatedUnit()"! Set it to a variable!:
local unit Dummy=CreateNUnitsAtLoc(1,DummyID(),TrapOwner,Loc,bj_UNIT_FACING)
call SetUnitAbilityLevel(Dummy,DummyAbility(),Level())
call UnitDamagePointLoc(Dummy, 0, 300.00, Loc, 60.00*Level(), ATTACK_TYPE_MAGIC, DAMAGE_TYPE_COLD )
call IssueTargetOrder(Dummy, "magicleash", Attacker )
- You leak your dummy unit, as you never kill it, remove it, or give it an expiration timer.
- Neither the SFX or their number is configurable.
- Don't use "UnitDamagePointLoc(...)" as it can bug out; instead, do what I advised Monsterous to do. Additionally, use "ATTACK_TYPE_CHAOS" and
"DAMAGE_TYPE_UNIVERSAL", as the other options are subject to armor reduction, etc.
- "PolarProjectionBJ(...)" is a very nasty, slow function; pick it apart and use coordinates:
NewX = OldX+Distance*Cos(Angle*bj_RADTODEG)
NewY = OldY+Distance*Sin(Angle*bj_RADTODEG)
Additionally, you're leaking a location there.
- You forgot to null "Attacker", "TrapOwner", "Loc", and "LocEffect".
- Again, overall below average work. You should really fix up your JASS.
Titan's Containment - Ghan_04
Efficiency - 9/10
Leakage - 10/10 Total: 23/25
Importability - 4/5
Look-over notes:
- At last! Concise implementation directions! Thank god.
- "Unit - Turn collision for (Last created unit) Off" is unnecessary as long as the unit has locust or a collision size of 0, which dummy effect units should.
- "Custom script: call UnitAddAbilityBJ( 'Aloc', GetLastCreatedUnit() )" should be "Custom script: call UnitAddAbility( 'Aloc', GetLastCreatedUnit() )"
And again, why not give the unit the ability in the Object Editor?
- I'm pretty sure "Animation - Play (Last created unit)'s stand animation" is unneeded, as the unit will automatically play it.
- Why do you set EffectUnits[1] so late? It would be better to set it as soon as the unit is created and then reference it instead of "Last Created Unit".
- "Time Equal to 125" should be "Time Greater Than or Equal to 125" in the event that the variable bugs out.
- Your variables could use prefixes.
- I'm not sure if it was intended to be implemented when another person implements your spell into his or her map, but the "Item Info" trigger is missing
Events for all players besides Player 1 and the selection thing won't work.
- I could have sworn that you knew JASS... why is this in GUI?
- Overall, not shabby; if it was MUI, this spell would be killer.
Intellectual Crush - mr-death
Efficiency - 7/10
Leakage - 7/10 Total: 18/25
Importability - 4/5
Look-over notes:
- The trigger explaining the spell was neat and concise, if hard to read in some spots.
- The variable-setting trigger is a good idea; it removes the need to come up with a complex formula.
- The comment notes are very helpful.
- You should prefix your variables.
- The "Spell Cast" trigger should look more like this:
If (All Conditions are True) then do (Then Actions) else do (Else Actions)
If - Conditions
(Number of units in (Units owned by (Owner of (Casting unit)) of type Intellectual Crush Trap)) Greater than 10
Then - Actions
Unit - Kill (Random unit from (Units owned by (Owner of (Casting unit)) of type Intellectual Crush Trap))
Else - Actions
Unit - Create 1 Intellectual Crush Trap for (Owner of (Casting unit)) at (Target point of ability being cast) facing Default building facing degrees
Unit - Set level of Level Identification for (Last created unit) to (Level of Intellectual Crush for (Casting unit))
The way you were doing it before would allow for a max of 9 traps (You had "Less than 10"), not 10. The above way is also more optimized.
Note: this does still leak.
- You leak 2 points and 2 groups.
- Any/all instances of "Casting Unit" should be replaced with "Triggering Unit".
- There is no need for this line: Set damage = 0.00, as you can simply do this:
Set damage = (manaburn[(Level of Level Identification for trap)])
Set damage = (((Max mana of (Attacked unit)) - (Mana of (Attacked unit))) x ((Real((Intelligence of (Attacked unit) (Include bonuses)))) x 0.01))
- Replace all instances of "Attacked Unit" with "Triggering Unit".
- Use "attack type Chaos" instead of "Spells", as the latter will be affected by armor.
- "Wait 0.40 seconds" should be "Wait 0.40 game-time seconds", as the former has been known to desync.
- I understand what you were trying to do with the destroying of the lightning, but you're ending up trying to destroy non-existant ones. To fix this, do this:
For each (Integer A) from 1 to lightinteger, do (Actions)
Loop - Actions
Custom script: if udg_light != null then
Lightning - Destroy light[(Integer A)]
Custom script: endif
This will only destroy the lightning effects that aren't already destroyed.
- The "Do Nothing" actions are pointless; remove them.
- Success! An MUI GUI spell! I'm overjoyed.
- Aside from that, this spell ain't too bad; just a few fixes here and there.
Frost Trap - Pigger
Efficiency - 6/10
Leakage - 2/10 Total: 10/25
Importability - 2/5
Look-over notes:
- The problem with this spell is the fact that it leaks events; when you add an event to the second trigger, there is no way to remove it, and
once the trap is dead, its specific event is useless. A better way to do it (the way Monsterous has done) is to give your dummy trap an attack
with a range equal to that of the radius of the spell and a very slow projectile speed; then, detect when a unit is attacked by a trap, order the
trap to stop, and do your effects from there.
- Because you don't have a condition for checking to see if the Triggering Unit is an enemy of the trap, you can trigger the trap yourself. This is a
problem that I found myself continually running into when testing the map.
- You leak 135 locations, all of which come from this line:
Unit - Create 1 FrozenDummy for (Owner of Trap) at ((Position of TrappedUnit) offset by 300.00 towards Angle degrees) facing (Position of TrappedUnit)
- There's no reason to do "Set Dummy = No unit", when Dummy is a global unit variable.
- You should prefix your variables.
- You leak another point right after the loop.
- "Wait X.XX seconds" has been known to Desync— use "Wait X.XX Game-time seconds" instead.
- Referencing "Last Created Unit" after a wait is a VERY BAD IDEA. It is hightly likely that a unit has been created in the timeframe of that wait.
To fix this, simply store the unit to a variable; your code is not MUI anyway.
- All 3 of these are pointless:
Set Trap = No unit
Set Angle = 0.00
Set TrappedUnit = No unit
- Overall, an average spell; not too great but it could get there if it became MUI and was fixed to work properly.
Earth Trap - Ninja_Sheep
Efficiency - 9/10
Leakage - 10/10 Total: 23/25
Importability - 4/5
Look-over notes:
- Prefix your variables.
- I can't see any coding errors... Well done!
- The only bad thing that I can see is the way you add the abilities to the traps when they enter the map. Instead of setting the level of the ability,
you could simply make it a 1-level ability and reference the level of the unit instead when it casts the spell.
- If you don't change that, then this line: "Unit - Set life of (Last created unit) to (100.00 x (Real((Level of (Ability being cast) for (Triggering unit)))))"
should become this: Unit - Set life of (Last created unit) to (100.00 x (Real((Level of (Activate Earth Trap [Q]) for (Triggering unit)))))
- Nothing complex, but you've fixed it up nicely.
Clockwerk Goblin Trap - Sooda
Efficiency - 7/10
Leakage - 8/10 Total: 20/25
Importability - 5/5
Look-over notes:
- Your use of comments, though sometimes intrusive, are very helpful in understanding this code...
- If you comment out a section of code because you don't need it... please try to remove it in the submission.
- Any constant functions in which you have commented how the formula works should instead take all relevant parts of the formula as arguments. It's much
more flexible that way. Additionally, please prefix your constants and give you spell a header.
- If you're using vJASS and globals blocks... why do you still have constant functions that don't take arguments. Replace all of them with a globals block
that has constant integers/reals
- I understand your use of ConditionFuncs as ActionFuncs, but it's not always needed, especially for spells. When you have a separate ConditionFunc and ActionFunc,
people can easily add their own conditions. Additionally, you save yourself from initializing variables that don't need to be (like "caster" in the function
"CreateClockWerkGoblinTrapUnit").
- In function "CreateClockWerkGoblinTrapUnit", these lines inside the if are pointless; they're already taken care of outside:
set caster = null
return false
- You're leaking a null BoolExpr in this line: call TriggerRegisterPlayerUnitEvent(clockWerkGoblinTrig,Player(playerIndex),EVENT_PLAYER_UNIT_SPELL_CHANNEL,null)
- Why is the trigger AddNewUnitsOnMap global?
- Replace "EVENT_PLAYER_UNIT_SPELL_CHANNEL" with "EVENT_PLAYER_UNIT_SPELL_EFFECT".
- Anywhere there is a bj_DEGTORAD or bj_RADTODEG, replace it with the appropriate real number.
- This entire if statement should be reworked:
if clockWerkAbilityData_total == 0 then
set clockWerkAbilityData_total = clockWerkAbilityData_total + 1
set clockWerkAbilityData_ar[clockWerkAbilityData_total] = abilityCastingInfo
call TimerStart(clockWerkAbility_timer,ReOrderingInterval(),true,function OrderToTargetPointUntilDead)
else
set clockWerkAbilityData_total = clockWerkAbilityData_total + 1
set clockWerkAbilityData_ar[clockWerkAbilityData_total] = abilityCastingInfo
endif
To this:
set clockWerkAbilityData_total = clockWerkAbilityData_total + 1
set clockWerkAbilityData_ar[clockWerkAbilityData_total] = abilityCastingInfo
if clockWerkAbilityData_total == 1 then
call TimerStart(clockWerkAbility_timer,ReOrderingInterval(),true,function OrderToTargetPointUntilDead)
endif
- The function ValidUnit should be rewritten to this:
function ValidUnit takes nothing returns boolean
local unit filterUnit = GetFilterUnit()
local boolean B = GetWidgetLife(filterUnit) > .405 and IsUnitEnemy(filterUnit,GetOwningPlayer(GetTriggerUnit()))
set filterUnit = null
return B
endfunction
- Why do you set the values of the integers/reals in the struct to 0? You only need null the units, as far as I know.
- I don't see a purpose to the "detectWhenUnitComesInRange" trigger, even though you add events and an ActionFunc to it... it seems that even if it did trigger,
it would not do anything. Would you care explaining?
- I'm baffled... you have all these constants and stuff for dealing damage... but no damaging function is ever called, nor are many of the constants. What's going on?!
Additionally, your Clockwerk Goblin units still have the "Kaboom" ability, so that might be fucking with things.
- Overall, I think you put the most work into the code of anyone who submitted, and there aren't any glaring issues. However, I think you could've achieved a similar
effect with a fair amount less work; but that's just my opinion and the fact that I don't write vJASS.
Spirit Trap - --Thanatos--
Efficiency - 8/10
Leakage - 10/10 Total: 23/25
Importability - 5/5
Look-over notes:
- If you are using vJASS, why aren't you using constant globals for things like Spell Ids?
- Nice, perfect, comprehensive constants. My only gripe is that they take "integer a", whereas "integer Level" would've made slightly more sense.
- Preloading is also always very nice.`
- The entire Spirit_Trap_Filter function should be rewritten as follows:
function Spirit_Trap_Filter takes nothing returns boolean
local timer StTimer = GetExpiredTimer ()
local StStrct data = GetData (StTimer)
local unit target = GetFilterUnit ()
local boolean b = GetUnitAbilityLevel(target,DummyAbilityRawcode()) == 0 and GetWidgetLife(target) > 0.405 and IsUnitEnemy(target,GetOwningPlayer(data.caster)) and (IsUnitType (target,UNIT_TYPE_STRUCTURE) == false) and (Spirit_Trap_Ultimate() or (IsUnitType(target,UNIT_TYPE_MAGIC_IMMUNE) == false))
set StTimer = null
set target = null
return b
endfunction
- "call SetUnitUserData(spirit,R2I(degree))" is bad; many systems use a unit Custom Value. You should simply store this to a struct attached to the unit
or just store it with the DataSystem.
- Why do stuff like "set data.duration = 0.00" each time? Could you not simply declare each of those values as the specified value when you declare the struct?
- The timer interval should be in a constant function/constant global; this also: "set duration = duration + 0.04".
- Always compute "(3.14159 / 180.00)" and "(180.00 / 3.14159)".
- In the function "Spirit_Trap_Spins", you should set "Spirit_Trap_SsRawcode()" to a variable, as you reference it multiple times.
- What is the purpose of "call SetUnitVertexColor(spirit,0,0,0,0)"? I thought I saw some funky shadows, but a dummy unit shouldn't have a model file anyway...
- Both Spirit_Trap_DegreeAccelerationA() and Spirit_Trap_DegreeAccelerationB() should be set to variables before their respective loops.
- These two lines are repeated directly below themselves:
call SetUnitPathing (spirit,false)
call SetUnitPosition (spirit,x,y)
And I think the first is pointless; if it's a dummy unit it shouldn't have collision anyway.
- I'm pretty sure that "call UnitApplyTimedLife(spirit,'BTLF',0.00001)" will just apply a 0 second expiration timer; wc3's floating-point goes to 3 places max.
- Whenever possible, you should not initialize variables until you need them, and only null them when you're sure they don't have a value. For example:
function BadFunc takes nothing returns nothing
local group G = CreateGroup()
if udg_SomeBoolean then
call GroupEnumUnitsInRange(...)
call DoSomethingWith(G)
endif
call DestroyGroup(G)
set G = null
endfunction
function GoodFunc takes nothing returns nothing
local group G
if udg_SomeBoolean then
set G = CreateGroup()
call GroupEnumUnitsInRange(...)
call DoSomethingWith(G)
call DestroyGroup(G)
set G = null
endif
endfunction
- Very nice work, once again; griping about the non-use of constant globals is the only configuration problem, but aside from that your code is free from
major errors. Just fix up a few things here and there.
. So now we have to wait for uareanoob gradings?
The contest ended a long time ago.Hydra said:
It's not that they don't win (take my Temporal Fluxuation spell for example); it's that it's a lot easier to screw up with complicated stuff. Your spell would've been a 10/10 for "Difficulty of Coding", whereas Ninja_Sheep's Earth Trap would've gotten maybe a 2/10.Sooda said:
Varine:
The Helper:
Varine:
Varine:
The Helper:
Varine:
The Helper:
The Helper:
The Helper:
The Helper:
The Helper:
The Helper:
The Helper:
The Helper:
The Helper:
The Helper:
The Helper:
The Helper:
The Helper:
The Helper:
