🏆 Texturing Contest #33 is OPEN! Contestants must re-texture a SD unit model found in-game (Warcraft 3 Classic), recreating the unit into a peaceful NPC version. 🔗Click here to enter!
It's time for the first HD Modeling Contest of 2024. Join the theme discussion for Hive's HD Modeling Contest #6! Click here to post your idea!
Deception
Create a spell which focuses on tricking your opponents. It should mislead the enemies to make bad decisions.
Ardenian
Coding:
Configuration:
Decently sized amount of configurables. You avoided providing very detailed list of configurables which is a plus. However, the scaling of MT_TranceInvDuration is wierd and unnecessary - you should be using direct duration value (seconds) and modify your approach in execution trigger instead.
All triggers should be prefixed with shortcut for your spell's name - Your spell isn't made of single trigger, thus for safety prefixing is required.
Small tip: indent custom scripts e.g loops to make them more readable.
(Unit-type of (Ordered unit)) Equal to MT_TrancerCasterUnitType
Instead, check if (Triggering Unit) has an ability of given type (Trance ability). Abilities should be generic in approach i.e any random unit should be able to perform them once you have corresponding trigger implemented + required object data.
Trigger "Trance Ability Cast": EVENT_PLAYER_UNIT_SPELL_CAST -> EVENT_PLAYER_UNIT_SPELL_EFFECT
(Casting Unit) -> (Triggering Unit)
(Owner of MT_TempUnit) -> (Triggering Player) what should be stored.
Static loop with number of iterations of 3? Each iteration checks all three conditions (regarding which ability has been casted)?
This should be completely redesigned to make spell flexible - I'll reflect that point later in Concept paragraph. Each ability should be a separate trigger instead in order to provide easy way to add / remove new abilities.
Loop handling inventory items is static from 1 to 6 and uses the exact same iterator as outer loop. This is wrong, not only should be there a separate iterator, but inventory size should be calculated before the loop takes place. What if hero has just 1 inventory slot?
Set MT_TempLoc = ((Position of MT_TempUnit) offset by MT_TranceMove_Range towards (Facing of MT_TempUnit) degrees)
First invocation in this script generates a leak: store position of unit and then proceed with offseting it. Don't forget to remove the location as soon as it becomes unneeded.
Trigger "Dummy Removal":
You're familiar with usage of custom systems, since you've been used DDS in your entry. Why not simplify the approach and use UnitIndexer too? Unit - Unit Dies is inaccurate, refer to DEINDEX event instead.
Trigger "InvCounter":
If - Conditions
MT_TranceInvCounter[MT_TempInteger] Equal to MT_TranceInvDuration
Condition operator should be: Greater or equal to - this is important especially when user starts playing with his own values.
If - Conditions
MT_TranceInvCounter[MT_TempInteger] Equal to (MT_TranceInvDuration x MT_InvSoonGoneTime)
Second part of comparison - the result of calculation - should be stored. Right now, you are doing the same calculation over and over again when in fact, the result never changes.
Trigger "Dummy Attacks":
Given the fact that dummy creation should be replaced with usage of Illusion (item ability), this trigger is redundant. Modify the object editor data for illusions instead. This makes implementation of DDS unnecessary too.
15/25
Concept:
Zephyr is about creating ability that matches theme and is fun/innovating in design. Trance, however, is not "an ability", instead it's a concept that fits better Hero Contest rather than Zephyr Contest. Your spell requires several other abilities to function properly. This also makes your concept not flexible when it comes to implementation and altering. Whatmore, you chose dummies instead of simple illusions what adds another level of complexity and whole shifting process is not very intuitive - problematic for both, owner and enemy to distinguish copies from real heroes in chaos of battle.
There is a lot that could be improved, although I know that your intentions were good.
10/15
Visuals:
Heavily reliant on spells that Trancing hero is able to use after and before shifting. Generaly, it's supposed to fix assasin style of hero with limited visuals in order to match such hero's gemaply style.
5/10
DEE-BOO
Coding:
Configuration:
You set data required for level scaling in a wrong fashion. Instead of setting direct value for each level, provide DEFAULT_BASE value and then INCREASE_PER_LVL constant. This way you avoid unnecessary spam when multiple levels are to be considered and make whole scaling process more flexible. Use loop for disabling player abilities rather than writing the same invocation for every player. By following my suggestions you effectively reduce code size and improve its readability which is key when writing longer scripts.
Array of temppoints seems to be overstep. You never require more than 1-2 temp locations in your script.
Trigger "ShapeShift Init":
(Casting Unit) -> (Triggering Unit)
(Owner of (Casting Unit)) -> (Triggering Player)
Store references to player and unit triggering the spell in variables to avoid repeatable calls.
(Number of units in (Units within SS_AoE_Scan_NormalSize of SS_TempPoint[1] matching ((((((Matching unit) is alive) Equal to True) and ((Level of Invulnerable (Neutral) for (Matching unit)) Equal to 0)) and (((Matching unit) is Hero) Equal to False (...)
Set SS_TempTargetUnit = (Random unit from (Units within SS_AoE_Scan of SS_TempPoint[1] matching (((((Owner of (Matching unit)) Equal to Neutral - passive ) or (((Matching unit) belongs to an enemy of (Owner of SS_Caster[SS_Index])) Eqaul to True)) and ((((Matching unit) is (...)
Both scripts leak group, the second one generates a ton of them. Store the groups and destroy them manually or via bj_wantDestroyGroup = true if possible.
Set SS_OriginalOwner[SS_Index] = (Owner of No unit)
Set SS_DestructibleType[SS_Index] = (Destructible-type of No destructible)
It's better to use simple custom script instead, and set the value to "null". You're already familiar with them since you clear some of the leaks with thier help.
Multiple calls to (Last created unit), (Owner of SS_Caster[SS_Index]) - same with above, store value into variable and refer to it.
(Number of units in SS_AnimationGroup) Equal to 1
This is wrong. Each unit is separatively added into SS_AnimationGroup - thus, you should know how many of them actually are there granted that additional integer counter is provided. This should spead up the process a lot.
Set SS_Special_Effect_Height = (SS_Special_Effect_Height + (SS_Config_SFX_TransfHeight / (Real(SS_Config_SFX_TransfThickness))))
Part after '+' operator seems to be not impacted by loop's iterations, thus you should make it a constant, rather than recalculating it over and over again.
Set SS_SpecialEffectGroup[SS_Index] = (Random 10 units from SS_Special_Effect_TempGroup)
Unit Group - Add all units of SS_Special_Effect_TempGroup to SS_SpecialEffectGroup[SS_Index]
Unit Group - Remove all units from SS_Special_Effect_TempGroup
Don't you think that part of this is redundant? Especially the temp group - work with indexed group from the start.
Trigger "Shape Shift Animation Loop":
For loop inside groupenum loop, (n^2) ? You do not need that. Approach can be simplified to loop just once
(SS_Caster[SS_Loop] is in SS_AnimationGroup) Equal to True
This is useless : you're picking unit units inside the group anyway - but i might be wrong
Set SS_TempPoint[4] = (Position of SS_Caster[SS_Loop])
Setting position and removing the leak is currently done within a loop, when in fact you need to set/remove that position just once.
Multiple calls to (Picked unit) -> store the reference and use the variable instead.
Animation - Change (Picked unit) flying height to ((Current flying height of (Picked unit)) + ((SS_Config_SFX_TransfHeight x 0.01) x 2.00)) at 10000.00
Again, part of that statement (after '+' operator) is constant. Don't recalculate something what never changes
(Number of units in SS_AnimationGroup) Equal to 0
As said earlier, you do not need to check for number of units inside the group. It's more efficient to use integer variable to store that number and work with it.
Game - Display to (All players) for 0.01 seconds the text: ANIMATION LOOP OFF
This is completely unnecessary. If you want to add debug messages, use custom scripts for that.
For each (Integer A) from 1 to 6, do (Actions)
Loop - Actions
Hero - Drop the item from slot SS_ItemLoop of SS_Caster[SS_Loop]
Hero - Give (Last dropped item) to SS_BackPack[SS_Loop]
Set SS_ItemLoop = (SS_ItemLoop + 1)
How do you know it's exactly 6? Calculate the unit inventory count and use the return value.
Multiple references to (Last created unit), (Owner of SS_Caster[SS_Loop]) - same as with (Picked unit)
Multiple ifs in regard to "Amrf" -> use script of type AutoFly; This is standard in many maps and there is nothing wrong in using such snippets for zephyr entry.
This is very dangerous thing what you doing here. You should know that dealing with Locust ability is complicated and you have to take more into consideration (even behaviour of custom systems). Consider testing your spell with targets/casters using metamorphosis abilities, load them in zeppelins, check if there are selectable and targetable soon after spell's effect is finished. Make sure to give user heads up about your decision (the decision for using 'Aloc').
JASS:
native UnitSetUsesAltIcon takes unit whichUnit, boolean flag returns nothing
native SetAltMinimapIcon takes string iconPath returns nothing
Those might be helpful if you want your hero minimap icon to disappear, rather than using locust for that.
Selection - Select SS_Command_Unit[SS_Loop] for (Owner of SS_Caster[SS_Loop])
Check if unit had been previously selected. Right now you're forcing the selection.
I don't like the idea of backpack here. If I understood correctly, you just want to "hide" items during the duration of Shape Shift. If so, you can hide them and set their position to coordinates (0,0).
Most maps have loc(0,0) empty anyway and you avoid the whole mess of backpacking etc. Another approach is to use a list to store items of given hero, removing the inventory and creating/readding the items just after effect ends.
Trigger "Shape Shift Loop":
Many mistakes are repeated this from previous triggers;
Multiple calls to unstored (Picked unit)
Loop inside another loop (approach can be simplified)
Set SS_Point[SS_Loop] = (Position of SS_ShapeShiftUnit[SS_Loop])
Set SS_Point[SS_Loop] = (Position of SS_Destructible[SS_Loop])
What if no conditions are met? What happens do previous value of SS_Point? You should make sure that this case is also considered and dealt with approprietly - currently, this could create a leak.
Destructible - Kill SS_Destructible[SS_Loop]
Destructible - Remove SS_Destructible[SS_Loop]
Killing than removing? Seems redundant.
Custom script: call RemoveLocation (udg_SS_TempPoint[3])
Do that just after location with index of [3] becomes unneed. Currently, it might happen that location is not initialized at all, yet custom script will still try to remove it.
Trigger "Shape Shift Command":
multiple calls to: (Owner of SS_ShapeShiftUnit[SS_Loop]) - store the value in variable and refer to it instead.
(ordered unit) -> (Triggering unit)
Trigger Deselection/selection
(Owner of (Triggering unit)) -> (Triggering player)
(Owner of SS_Caster[SS_Loop]) -> As said already, this should be stored in separate variable.
Multiple references to (Triggering unit) without storing it.
Selection - check if unit/hero has been previously selected instead of forcing the selection.
Trigger Secondary Spells
(Casting unit) -> (Triggering unit)
Multiple references to (Ability being cast), (Last created unit) without storing it.
The exact same problems with temp location with index of [3], inventory loop and selection as previously. Again, the way you deal with 'Aloc' ability is dangerous and can cause malfunctions later on in the game.
Trigger Deny Xp and Gold
Multiple references to (Triggering unit) without storing it. That trigger seems to be unrequired and even if needed, you've mentioned yourself that there are systems which deal with such situations better.
14/25
Concept:
Shape shifting is a good examples of trickery - outter shell is different from what hides within. It’s not very unique tho, but I like the overall concept. Main problem found within this entry is lack of restrictions or clear boundaries. After reading Code part, you should know that there are many places where your code might be troublesome.
You could have limited the Shapeshifting to only to units or, if other types seem necessary to be included, provide the actual warning/note for user to notify him about present issues. Concept could be extended by providing additional bonuses if caster becomes unreveled for longer period, or punishment for becoming detected (perhaps this punishment could be directed towards opponent instead?). This would give entry more uniqueness.
11/15
Visuals:
Due to nature of Shapeshifting, there aren’t many visuals connected to this spell. In my opinion, some could be provided that would be visible only to owner/allies of owner of caster. Also, currently there isn’t anything attached to caster that gives user clear information which unit is shifted and which isn’t - this is especially required when multiple instances of such spell are in use.
5/10
Empirean
Coding:
Configuration:
You set data required for level scaling in a wrong fashion. Instead of setting direct value for each level, provide DEFAULT_BASE value and then INCREASE_PER_LVL constant. This way you avoid unnecessary spam when multiple levels are to be considered and make whole scaling process more flexible.
Some configurables are unnecessary. Don't overwhelm user with amount of settings, instead provide those which impact spell the most. Turn rate for example, is not one of them. If one wishes to alter spell in more specific manner, he probably already has enough knowledge about triggers/jass to play with them by himself.
I'd say, the str/agi/int portion of damage should be 0-1 based (or more) since multiplying them by 0.01 constantly after each cast is redundant. If you want to keep such approach, please recalculate (multiply them by 0.01) immdiately after user settings to reduce the operation count.
Trigger Cast
The last previous point is very important here. Statement calculating str/agi/int portion (multiplied by 0.01) is a constant factor. No need to recalculate it over and over again.
Trigger Effect
Again, unnecessary operations. SafeAoE + CrowAoE is a constant for given level. No need to sum them up multiple times within each iteration of an effect trigger. By this time, you can probably natigate all the overheads by yourself, you'r smart
Since these are triggers you are dealing with, meaby you should have splitted Effect one? Currently, each and every effect related to your spell is packed into one bucket. Flags/notify and actionId approach is nice, by notice, that once your Flag changes to FALSE, it will never change to TRUE again; The 'if checks' become redundant.
Consider reflecting units collision radius. Spell seems to be a bit unituitive when it comes to its area of effect.
Set SD_PickedUnit = Brak jednostki
It's a global, no need to constantly null it. Each time you enum new unit, SD_PickedUnit will be assigned to it anyway.
Seems like slow effect doesn't take into consideration Outward/Inward status?
By the way, when checking for status/flag abilities, it might be safer to use 'Greater than 0' comparison rather than strick 'Equal to 1'. Sometimes there is more than just one level.
23/25
Concept:
Bringing crows to the table is brilliant idea, personaly one of my favourites. However, I must say, that the execution hasn’t matched the general idea - this entry had high potential, and seems like you have wasted a lot of it - details matter the most when speaking about execution. Half of your script is delegated to handle movement and visuals of crows. Considering that fact, why would you ever dimish everything you have worked for so much? Crows, all the eye-candy stuff are visible only for a short period of time, and with each level of ability, are even less of a factor.
Next, the end effect seems kind of plain and boring. Crows, crows - why not push the concept even further? Let’s not mark the area exactly, instead, let them wonder and fly around like if they were looking for prey - and opponent doesn’t know where and when they would strike. Current effect is closer to becoming a delayed bomb-alike ability rather then tricky, scary and fun spell which scales with user’s imagination. Dagger part could have been hiden from eyes of an enemy, let’s not show too much
Considering all the facts, you propably realise how much could be improved with just small touches of your hand: improving the delay, spreading crows and such. This would made your entry feel even more unique and fresh. As much as I like the general concept, I’m sad that you somehow waste great portion of its potential.
13/15
Visuals:
Match the contest’s theme and the idea behind is very good. Feels fresh and entertaining, though dagger part seems kind of out of place. The „yellowish” slow effect could also be changed. Overall, this would be an insta contest-crusher, had it had some details improved.
7/10
Flux
Coding:
Configuration:
You give user a vast area of freedom here. However, some variables aren't required to be marked as "configurables" e.g MISSILE_HIT_DISTANCE - such should be placed outside of general configurable block (in this case, anything between 25-50 is acceptable, higher or lower values might be problematic/not intuitive).
Next, you divide some configurables (INHERIT_MANA/LIFE and so on) when in fact, you should leave just one. I know that you want the best for user, however, currently you provide just too much, thus creating overhear which could be avoided. If you really want to stick with that approach, combine some settings into functions to provide better flexibility.
Description is too long, shorten it a bit, same with variable descriptions - don't insert random spaces here and there. Try to find clean way to present everything in good fashion providing enough knowledge for user to understand what lies behind, yet don't overwhelm him with data either.
Spells should be generally written within scopes, not inside libraries.
Unproper division of code to: public / private portions. Spell's "engine" should never be public. Also, extending API is also wierd. Each ability is designed to do its own thing, no need for external sources to interfere.
Function and Struct naming is too generic and might cause complitation errors - many of chosen ones, are popular among maps. Make them spell-specific.
Code organization is a complete chaos. Different types of configurables are spread throughout first 300 lines of code. Why is private constant integer ELEMENTAL_BODIES = 'Aelb' at the begining of block while private constant integer DUMMY_ITEM_ID = 'ches' at the end of it? Group similar configurable types together and split those which are different.
If you have problems understanding what I meant, look at this:
JASS:
private constant integer MAX_NUM_SPELLS = 4 //The maximum number of spells each body can have
//Used to preserve array index
//Configurable Properties of each bodies, look at function InitializeElementalBodies to configure them
private integer array bodyId
private integer array bodySpells
private string array bodyOnHitSfx
private string array bodyMissileSfx
//Initial Body Properties
private constant boolean RANDOM_INITIAL_BODY = true
There is more, since currently code block of configurations follows random path: globals: generic -> system spefic -> function API: generic -> globals: system specific -> function API: generic
Divide it properly, take some time before you put that together. Take a step back and think as if you would be a user looking at your document without any knowledge of its content.
Lack of spaces between certain code statements / structures which make code unreadable. Let's take a look at presented example:
JASS:
function SetMaxProperty takes unit u, real newMaxProperty, unitstate maxState returns nothing
local integer addMaxProperty
local integer digit
local integer index
local integer i
local integer abil
if maxState == UNIT_STATE_MAX_LIFE or maxState == UNIT_STATE_MAX_MANA then
set addMaxProperty = R2I(newMaxProperty - GetUnitState(u, maxState))
if maxState == UNIT_STATE_MAX_LIFE then
set abil = MAX_LIFE_MODIFIER
else
set abil = MAX_MANA_MODIFIER
endif
set index = 5
loop
exitwhen index == 1
set digit = addMaxProperty - (addMaxProperty/digitPlace[index])*digitPlace[index] //non BJ Modulo
set i = (addMaxProperty - digit)/digitPlace[index] //how many times it will loop
set addMaxProperty = digit //store how much hp/mana was not added to be used in the next loop
loop
// yolo script follows
Where does variable declaration ends? Nested 'if' statement? Haven't noticed.. and so on.
Now, let's space it out a bit:
JASS:
function SetMaxProperty takes unit u, real newMaxProperty, unitstate maxState returns nothing
local integer addMaxProperty // how much hp/mana wasnt added - required for nested loop
local integer digit
local integer index
local integer i // sets loop iteration count
local integer abil
if ( maxState == UNIT_STATE_MAX_LIFE or maxState == UNIT_STATE_MAX_MANA ) then
set addMaxProperty = R2I( newMaxProperty - GetUnitState(u, maxState) )
if ( maxState == UNIT_STATE_MAX_LIFE ) then
set abil = MAX_LIFE_MODIFIER
else
set abil = MAX_MANA_MODIFIER
endif
set index = 5
loop
exitwhen index == 1
// non BJ Modulo ->> Is that really needed?
set digit = addMaxProperty - (addMaxProperty / digitPlace[index]) * digitPlace[index]
set i = (addMaxProperty - digit) / digitPlace[index]
set addMaxProperty = digit
loop
// yolo script follows
Can't belive how different this scripts are? Precisely, the second one tells us the story while first, only a gossip.
function ExchangeItems:
DUMMY_ITEM_ID - preserving slot ids is a very good idea, however, your approach is correct only when dealing with "recide systems" - and given recipe system should have it's own DUMMY_ITEM_ID which you would be using via its interface. There is already a native to move item to correct slot without dummy/backpack overhead: native UnitDropItemSlot takes unit whichUnit, item whichItem, integer slot returns boolean
Whatmore, never take inventory size of 6 for granted. Calculate unit's inventory size and refer to it.
function CopyItemCharges:
The design is not well thought. What if given unit has multiple items of the same type however, with different charges? Correct approach is to check handle ids or direnctly item references. However, due to how items are handled by most spells/systems (e.g stacking/recipe) there are plenty of remove/add item calls. Thus, keeping track of items is rather unreliable, tha'ts why you should avoid getting close to that mess whenever possible.
Your function is also public, which means, user could call it (even if correctly for "body spirits") and begin cascade of chaos. Chosen approach is rather dangerous, what you should have done instead, is to privatize that function and let system handle all the necessary work and only if needed to (usually directly after new body is created/swapped).
Struct DmgHandler is unacceptable. Since you are already using custom systems, DDS should be used instead. Your struct name is too generic and might interfere with whole damage handling process processed by custom, commonly used systems what would effectively break them. Struct Body isn't an exception here
Seems like random methods are private and other portion is public for no real reason. As stated previously, spell's "engine" should never be public.
"next/prev" read-only, yet "head" is private? Method "insert" should be using method "create" directly to create new node. Right now, it looks like you have 2 completely different constructors for your linked list.
method move:
This method is reduntant and should be inlined directly into "moveAll". That's the only place "move" is invokes in entire script.
JASS:
function ElementalBodyHasSoul takes unit u returns integer
local Body this
if tbId.has(GetUnitTypeId(u)) then
set this = tbId[GetHandleId(u)]
if this.hasCaster then
return 2 //returns 2 if unit has caster's soul
else
return 1 //returns 1 if unit does not have caster's soul
endif
else
return 0 //returns 0 if unit is not even an elemental body
endif
endfunction
'else' portion is unnecessary for outter if. Just end it and finish function with return statement.
method onInit:
JASS:
static if not LIBRARY_SpellEffectEvent then
local trigger trg = CreateTrigger()
endif
loop
call TriggerRegisterPlayerUnitEvent(uponSummon, Player(i), EVENT_PLAYER_UNIT_SUMMON, null)
static if not LIBRARY_SpellEffectEvent then
call TriggerRegisterPlayerUnitEvent(trg, Player(i), EVENT_PLAYER_UNIT_SPELL_EFFECT, null)
endif
set i = i + 1
exitwhen i == bj_MAX_PLAYER_SLOTS
endloop
call TriggerAddCondition(uponSummon, Condition(function thistype.onSummon))
static if LIBRARY_SpellEffectEvent then
call RegisterSpellEffectEvent(ELEMENTAL_BODIES, function thistype.onCast)
else
call TriggerAddCondition(trg, Filter(function thistype.onCast))
set trg = null
endif
->>
JASS:
static if not LIBRARY_SpellEffectEvent then
local trigger trg = CreateTrigger()
loop
call TriggerRegisterPlayerUnitEvent(uponSummon, Player(i), EVENT_PLAYER_UNIT_SUMMON, null)
call TriggerRegisterPlayerUnitEvent(trg, Player(i), EVENT_PLAYER_UNIT_SPELL_EFFECT, null)
set i = i + 1
exitwhen i == bj_MAX_PLAYER_SLOTS
endloop
call TriggerAddCondition(trg, Filter(function thistype.onCast))
set trg = null
else
call TriggerRegisterAnyUnitEventBJ(uponSummon, EVENT_PLAYER_UNIT_SUMMON)
call RegisterSpellEffectEvent(ELEMENTAL_BODIES, function thistype.onCast)
endif
call TriggerAddCondition(uponSummon, Condition(function thistype.onSummon))
call TriggerRegisterUnitStateEvent(uponLowHp, u, UNIT_STATE_LIFE, LESS_THAN_OR_EQUAL, 0.404998749)Where have you taken that number from? The design seems complicated when in fact it shouldn't be.
Just check death event instead. The reincarnation check should be done with usage of Unit Event-system or with help of any unit indexer - check for repeated undefend orders with unit-type id returning reasonable values.
call ForGroup(unsilencedBodies, function thistype.silenceEnum)ForGroup instead of FirstOfGroup should be mostly used only to start new thread (if necessary). Use FoG loop-type instead.
method onSummon:
Continuation of "Configurables" thoguht process. Right now, given the circumstances, one of the most meaningful methods takes form of spaghetti with constant spam of static ifs. Then, like it isn't already too much, you inject part of linked-list code which shouldn't be here in the first place (separate that code - isn't not directly part of "body", instead, it's a part of linked-list structure).
Previous statement regarging unitstate event is also valid here. Registering both, "almost dead" and "dead" events or the exact same unit just for the sake of "registering"? Simplify the approach, don't complicate stuff.
I'wont be listing everything here - if you've made it out to this point you probably already know what should or should not be changed.
Overall it's a piece of code with good idea behind that could use a some lifting and a bit of reogranization to make it look as it's a part of one, well thought script. Biggest flaw is the fact that script of yours might interfere with custom DDS engines and function/method namings - in most maps, this code might not compile. Leaving multiple holes, described by me above gives posibility for exprecienced user like me to take advantage of them in-game and cause undesired malfunctions.
20/25
Concept:
I really liked the idea of yours. It’s not super unique, but it’s fun to play with and fits theme perfectly. First thing: you don’t really need to use two abilities in order to „show” user that given ghost is the „soul” (the real one?). The „swap” ability is enough, the passive seems redundant.
What striked me also, is the fact that you aren’t dealing with Channel ability appropretly. The „0.98” thing i.e the animation time matters. It’s not there just because - if you want your spell to jump on another level, pay attention even to such detailts. Usually values in range 1.15-1.5 should do the job. Right now, hero has no real indicator of casting a spell - acts like a tree, without any motion. Some values could be fixed too, current numbers seems.. pretty extra..
To make sure you take care of this: try to avoid implementing custom DDS for a single ability if you are not sure for 100% what are you doing. And let me tell you: your current implementation could cause malfunctions in maps that are using standard DDSers. Portability is important.
14/15
Visuals:
This spell doesn’t need many visuals to be efective, though I must say some additional effects could be added. I’m not speaking about direct special effects only, but sounds too. Proving just enough „extra” even when spell seems like providing non should be fine is what I call „magic”.
8/10
Ghosthunter123
Coding:
Configuration:
Seems like some level-scaling part of configurables is missing, however, I definitely appreciate the fact, that you've chosen correct approach for level-scaling settings that are present in entry. Multipliers instead of table-lvl array, that's a plus. Attachment point for target's effect should be configurable too.
Varible names: 'g', 'un2', 'Lvl' etc. really? I understand that in last step of map optimalization you try to remove unnecessary global variables and reduce their count as much as possible by reusing certain number of number within multiple scripts (treating them like locals).
Yet such step you should always leave to user and user only. Generic variables make script non-portable.
Trigger - Run CPS Creating <gen> (ignoring conditions)
Trigger FT End
Spell trigger shouldn't call display-text functions. Call for <trig>Extra Stuff execution should not be there as well. The later is part of debug/test purpose only, thus is redundant part of base-effect.
By the way, picking every unit on the map (Extra Stuff trigger) is some poor design.
Creating proper spell for community resources means that you apply events/conditions/actions that are required for spell to work efficiently without additional overhead.
Visuals and basic data you leave for user as configurables.
Trigger FR Loop
Again, what are demo/test parts doing inside base spell code? This isn't appropriate.
You should notify user that custom script related to setting animation might not work if dummy type changes.
Playing with units movement via natives might be dangerous unless you handle that correctly. It's best to avoid playing with such and apply slows via dummy abilities instead.
These are also more intuitive for user providing option for dispelling/status icon - better incorporating into gameplay environment
Trigger "Sound the Flames":
Considering previous statement, 'Sound the Flames' isn't an appropriate trigger name either. Another demo-code injection which isn't supposed to be here. Calling timer which are part of non-spell code?
Another enumeration that picks every unit on entire map - this approach should have been changed. 'STF_' prefix speaks to me like this would be part of another spell. Keep it simple, keep it cohesive i.e stick to one, chosen prefix.
16/25
Concept:
Chosen concept does not really sound familiar to me (let’s say I’m a sneaky master of trickery). Dragon’s breath? Fireball? It’s a big part of your ability and it feels like another nuke spell from fire-element spellbook. „Fixed” demo doesn’t help either. Without altering it, there is no real way to test your spell. The only real tricky part lies in the floating texts which tell user if spell hit or missed. Amount of floating text is limited, thats why you shouldn’t be using them in such spamable spell to begin with.
This spell could use some sound effects perhaps? Additional visuals? There is a lot you could have changed/switched to remove floating next from this spell and give this one a breath of uniqueness. Even the dragon figure - meaby force dummy to use different „heads” depending on target chosen/casters state. Darker colours would be an another step into right direction.
9/15
Visuals:
Overall, spell is very eye-candy, and for this I can say: good job. However, this is not how dark-themed (stealth, shadow, trickery) contest entry should looklike. You could have chosen different palete - as stated in „Concept” part.
8/10
Loner-Magixxar: Trick Haunting
Coding:
This spell does not meet criteria to be accepted as efficient and reliable. The approach and overall design directly speak for themselves - creator isn't an experienced one and should refer to Tutorial section in order to get more experience and understanding about basic GUI/triggers and then move on to more advanced techniques.
You never require more than 1 hashtable per map - and even if you want "your own" table for certain ability, 1 per spell is more than enough. Up to 256 hashtables per map and you've initialized 8 of them for purpose of single spell.
Replace EVENT_PLAYER_UNIT_SPELL_CAST and EVENT_PLAYER_UNIT_SPELL_FINISH with EVENT_PLAYER_UNIT_SPELL_EFFECT ('starts effect' one).
I won't be listing everything here, and will refer to all of triggers at once, since most mistakes are repeated through all of them:
(Casting unit) -> (Triggering unit)
(Owner of (Casting unit)) -> (Triggering player)
Don't enforce selection - deal with it "softly" i.e check which units are currently selected, add to selection if needed/ add option to ignore action for orb selection and more.
Store values of frequently used calls into variables and refer to them instead.
Use tables to save spell level-dependant values. The if/then/else spam is totaly unnecessary and reduced readability.
Loops usage would reduce the code size a lot and will improve you as a designer. Combined with previous point, you could speed up and reduce code size by a lot.
'0.01' interval is not a good design. 0.03-0.05 is acceptable.
Do nothing
Does nothing, no need to add that line.
Calculating spell's duration should be done more carefully, 1.00 period is very inaccurate.
Waits compared with usage of globals is dangerous mixture. When multiple casts are going on, triggered by multiple units (with different owners) might collapse your loops and effectively lead to undesired effects/malfunctions.
Same calulcations are repeated many times when in fact, their results are constant - calculate such on map lead and use them in the script.
(Dying unit) -> (Triggering unit)
Unnecessary cascade of triggers i.e one trigger triggering another without having reasonable amount of actions - not well thought division of script with leads to loss of readability and code-split which also makes debugging harder.
Please, read awesome Things That Leak and try to apply those points into your project. Currently, script leaks heavily, especially the positions which happens very often through whole script.
Having this one running for couple of minutes could hinder user map-experience.
Lack of configurables - user can not alter spell's numbers in easy way. Lack of proper code division - makes spell look messy and unreadable. Triggers aren't names in good fashion - you should have prefixed them with shortcut of your spell's name.
The exact same applies to global variables. Their naming is too generic and tells user nothing about spell which are they used in. Clearing hashtables is incorrect - you don't have to clear every single child key. Find parent key which corresponds and "owns" them and flush that key instead.
Test code is questionable. Without altering your map, it is hard to properly test the spell.
8/25
Concept:
Even though trigger execution is not your strong side, the concept seems rather unique - I like it. There could be some twixes here and there: change of behaviour when positive/negative effect is chosen/ provide different indicators for each orbs that are dependent of current orb’s state. Selection issues make it hard for user to controll units while checking the orbs status. Mentioned points would allow user to easily navigate throught units/orbs especially when multiple instances are in place.
Size, colours, bonus effects - all of these could be considered. Most of the effects could be hidden from enemies eyes perhaps? Meaby even orbs themselves? There is alot that you could play with, in order to improve the concept’s execution.
10/15
Visuals:
Statements above sum up most of the changes required to make this entry feel more fresh. Currently, the effects are nice and smooth, but aren’t really showing ability’s full potential.
6/10
Tank-Commander
Coding:
Configurables:
First year: I've made first step, proceeding forward with excitement!
870 years after: (...) .. (...) Can't believe I made it! But honestly, who needs so many configurables? Most spells, hell, engines are shorter than this configuration part. Yes, it is readable, yes, it's properly names and the indentation is proper. However, you will get user lost before he ever reaches the spell-engine part. Whatmore, the configurables are so spread, that it might be hard for user to find exactly what he needs. Make it straight.
The most important things on the very top, group similar types of configurables together. Remove overhead and unnecessary settings (honestly, you don't need half of which are currently present in your entry). Let me show you one example, because example is worth more than a thousand words:
JASS:
constant function PP_PickupTypeDistBase takes nothing returns real
return 160.00
endfunction
// //
constant function PP_PickupTypeDistPerLevel takes nothing returns real
return .0
endfunction
// yolo script follows
// and somewhere deep inside:
call GroupEnumUnitsInRange(udg_PP_TempGroup, x2, y2, PP_PickupTypeDistBase() + (PP_PickupTypeDistPerLevel() * rLevel), null)
This is some bad design. "rLevel" used to calculate level-based bonus in execution code, instead of being used as parameter for API function? Currently, function PP_PickupTypeDistPerLevel() behaves like a global.
->>
JASS:
function PP_GetPickupTypeDist takes integer level returns real // getter
return 160.00 + (level * .0)
endfunction
// yolo script follows
// and somewhere deep inside:
call GroupEnumUnitsInRange(udg_PP_TempGroup, x2, y2, PP_GetPickupTypeDist(rLevel), null)
Seems like sometimes you follow set standard for naming/writing variables/functions/globals and sometimes you don't. Locals should be starting with lower case letters.
PP_Real1.. up to PP_Real14. Same with _Integer and _Boolean. Some variables are so nicely named e.g _AbilityCounter, _CurrentEffect and then, suddenly we meet friend _Real7? Meaby _Real007? Seriously, take a 2-3 months break, don't look at your code - then open WE again, open your entry and try to loudly say which variable is assign for what. This is not only unintuitive for me or random user, but also in a long run for you too.
function InitTrig_<spell name>:
JASS:
//Set up hero casting triggers
loop
//Applies the even to each player in turn
call TriggerRegisterPlayerUnitEvent(PP, Player(index), EVENT_PLAYER_UNIT_SPELL_EFFECT, null)
set index = index + 1
exitwhen index == bj_MAX_PLAYER_SLOTS
endloop
Not really a mistake, but why would you not use single BJ line instead? Inlining function TriggerRegisterAnyUnitEventBJ takes trigger trig, playerunitevent whichEvent returns nothing is only worth if you are planning to register more then 1 event for player.
JASS:
local integer Node
local integer iLevel
Look at declared here "Node" and "iLevel". Currently, it looks like they are not part of the same project - they're following different standards.
JASS:
if UnitAddAbility(udg_PP_Unit[Node], 'Amrf') and UnitRemoveAbility(udg_PP_Unit[Node], 'Amrf') then
endif
What a nice shortcut we got there ;> Is placing UnitRemoveAbility() directly into 'if' statement really necessary (readability should be more important)?
local unit Target = GetSpellTargetUnit()Initialize "Target" only once ability id check has been passed. Currently, that operation is going to be often redundant (usually there are many spells implemented in each custom map, and SPELL_EFFECT event will usually fire for different ability than Punishing Particles) but also, you forgot to null Target at the end of 'if' statement.
function PP_Loop:
There is no way, Warcraft3 spell needs an ~600+ lines callback loop. You've packed everything, almost every single effect that is triggered by your spell into single function. Often functions that are called just once/are rather small should be directly inlines. Here, however, putting everything into one basket reduces code readability tremendously, even that you preserve correct indentaion/spacing throughout whole function.
Divide it, make it easy to see which code part does what. The spam of mr. _Real, mr. _Integer and mr. _Boolean doesn't help either. Such approach also generates redundant 'if' statements and such in order to nativate in "which" effect we currently are. Bless for small comments here and there - I'm a bit lost even with help of those but without, series "Lost" could use me as another attraction. Where to start? First tip: split "recycle part".
JASS:
//This loop is structured in order to be as efficient as possible
//As such, functions ran more often are closer to the top
//So fewer comparisons are run
That's not required. You have advanced knowledge over jass, no need to write down such strings. If you really want to provide such description ask yourself, what "run more often" means, in what circumstances? and what does it means in term of for spell-engine? Provide example, questions - answers. That description will give you nothing when you'll be no longer familliar with the code.
function PP_Move: if not(udg_PP_TargetUnit[Node] == null) thenDon't follow Blizzard GUI standards. if ( udg_PP_TargetUnit[Node] != null ) then
This function seems to have enormous amount of calculations - and jass is rather slow when it comes to rapid calculation of high number of equations. Since your variable naming is unituitive, it's hard for me to provide you a proper approach without studing this, possibly for hours, although keep in mind that you could always reduce accurancy in order to improve efficiency. It's quite possible that running a couple of instances of this spell at once could break thread (PP_Loop) or lower map-experience for user (FPS droprate). call SetUnitAnimationByIndex(udg_PP_Unit[Node], R2I(Atan2((udg_PP_Real2[Node]), Pow((udg_PP_Real3[Node] * udg_PP_Real3[Node]) + (udg_PP_Real4[Node] * udg_PP_Real4[Node]), 0.5)) * bj_RADTODEG + 0.5) + 90), given the native: native SetUnitAnimationByIndex takes unit whichUnit, integer whichAnimation returns nothingNo care to explain what stands for "WhichAnimation"? Meaby it's just unclear for me, but unit animation indexes might be different for different unit-types, so proper explanation could be handy. Especially, when taking look at that code piece after some time.
20/25
Concept:
Interesting entry, though seems a little buggy - I didn't have enough time to validate the code and see where the problem lies, however, sometimes ball just doesn't "explode" (yet the special effect is there), and what's more important - targeting unit directly cases missile to skip effect entirely. Effect itself, could be somewhat hidden (or meaby just different) from enemmy's eyes.
This contest wasn't about creating explosion kind of spell. Although, I must admit, the effect itself is very pleasing and eye-candy You could have chosen other model for the missile, make the spell a little more dark and such. As stated in "Code" part, math should be simplified whereever possible. Currenctly, having multiple instances present at once seems kind of heavy.
12/15
Visuals:
Excellect. However, does they stick with contest's idea? Not so much. Different pallete of colours would make this fit the concept much better. Again, making missile noticiable for user is important, but currently it's also a big blue exclamation mark for an opponent that warns them about upcoming danger.
scope ContestScoring initializer Init
// Config
// Attention for POLL_VOTES_TOTAL:
// Don't count a user twice in case multiple choice is allowed.
globals
private constant integer JUDGE_SCORE_TOTAL = 50
private constant integer POLL_VOTES_TOTAL = 29
private constant integer NUMBER_OF_ENTRIES = 7
endglobals
globals
private string array Name[NUMBER_OF_ENTRIES]
private real array JudgeScore[NUMBER_OF_ENTRIES]
private real array PollVotes[NUMBER_OF_ENTRIES]
private real array FinalScore[NUMBER_OF_ENTRIES]
endglobals
// Write in forumala here
private function CalculateScores takes nothing returns nothing
local integer i = 1
loop
exitwhen (i > NUMBER_OF_ENTRIES)
set FinalScore[i] = (25*PollVotes[i])/POLL_VOTES_TOTAL + (75*JudgeScore[i])/JUDGE_SCORE_TOTAL
set i = i + 1
endloop
endfunction
private function DisplayResults takes nothing returns nothing
local integer i = 1
loop
exitwhen (i > NUMBER_OF_ENTRIES)
call BJDebugMsg(Name[i] + ": " + R2S(FinalScore[i]))
set i = i + 1
endloop
endfunction
private function Init takes nothing returns nothing
set Name[1] = "Ardenian"
set JudgeScore[1] = 30
set PollVotes[1] = 1
set Name[2] = "DEE-BOO"
set JudgeScore[2] = 30
set PollVotes[2] = 7
set Name[3] = "Empirean"
set JudgeScore[3] = 43
set PollVotes[3] = 2
set Name[4] = "Flux"
set JudgeScore[4] = 42
set PollVotes[4] = 3
set Name[5] = "Ghosthunter123"
set JudgeScore[5] = 33
set PollVotes[5] = 1
set Name[6] = "Loner-Magixxar"
set JudgeScore[6] = 24
set PollVotes[6] = 3
set Name[7] = "Tank-Commander"
set JudgeScore[7] = 41
set PollVotes[7] = 12
call CalculateScores()
call DisplayResults()
endfunction
endscope
The contest has come to an end. Thanks for all your entries done, and the effort you've put into them.
Also my thanks goes to Bannar for his juding over all entries.
So congratulations to all winners. The second place was a pretty close one.
Hope to see some of you again up to the future contest.
feel free also to give Bannar as judge a feedback. Noone is perfect and appropriated critique
is always appreciated for all of us. But maybe he also can explain his point of view so you can better understand him.
Though I prefer these detail desussions about a specific point of judging to be of private format, and be not part of this thread.
feel free also to give Bannar as judge a feedback. Noone is perfect and appropriated critique
is always appreciated for all of us. But maybe he also can explain his point of view so you can better understand him.
Though I prefer these detail desussions about a specific point of judging to be of private format, and be not part of this thread.
Well it's not feedback, I just wanted to explain why I did those things he mentioned in his review to my entry. But there is no point since the results are up anyway.
Also, good job Bannar, I can't imagine how you handled that enormous amount of entries.
Congrats to all the winners.
I appreciate Bannar's advice. Very enlightening!
I'm really new to coding, so there's a lot to learn for me. Of course, some problems were due to lack of time so I left them as they were; But for the coding part I really need to improve my skills.
hi, congratulations tank-commander and other winners thank you for participating the event.
Edit: btw i just noticed, in the calculation part my judge score is 42 and in the post its 43 please confirm if this is correct. if its 43 this should make my score 66.22
Depending in what you have "written" your entry, please read other reviews in order to get full insight on my view - approach & reasoning.
GUI overall got better score because users who used it did a better job adapting the "script" and optimalizing it (I'm not counting entries where inexperience is clearly seen). Jass gives you more freedom and possibilities, but there is much more to keep an eye on - thus more mistakes can happen.
I'm happy to reward guys for their good job - and I'm sorry for the delay of the judging. A lot has changed during that time, had many private trips to take on and recently I've successfully qualified for job, let's just say a very good one
Didn't know the pastebin has default lifetime of 7 days - lost the judging text-source and had to rewrite some parts of it. Yey!
Appreciated and wish you Save and Merry Christmas!
This site uses cookies to help personalise content, tailor your experience and to keep you logged in if you register.
By continuing to use this site, you are consenting to our use of cookies.
Contest Thread | Poll
