Holy Shock 1.1

Moderator · Jun 16, 2014

PM me with comments and concerns.

General

Coding

Concept

Design

Misc

Visuals

Required Changes

Purgeandfire :: 30 October 2014

Criteria (Spells):

Coding (20): efficiency and potential improvement.
Concept (20): originality and features.
Design (10): modularity, importability, ease of use, and approach.
Misc (10): test map, image, hive submission, misc.
Visuals (10): effects and appeal.

Score:

Coding: 18 / 20
Concept: 16 / 20
Design: 9 / 10
Misc: 9 / 10
Visuals: 8 / 10
Total Score: 60 / 70

Approved

Current Rating

4 / 5

65 - 70 becomes 5 / 5
55 - 65 becomes 4 / 5
40 - 55 becomes 3 / 5
30 - 40 becomes 2 / 5
0 - 30 becmes 1 / 5

http://www.hiveworkshop.com/forums/2566133-post26.html
http://www.hiveworkshop.com/forums/2608229-post34.html
http://www.hiveworkshop.com/forums/2608520-post36.html

Instead of using "Is unit group empty", you should
just use an integer. When a caster is added to the
unit group, increment the integer. When a caster is
removed, decrement the integer. Then change the conds
to check if the integer is 0 (i.e. group is empty).
The "Group is empty" GUI function is very expensive
since it goes through the ENTIRE group just to check
if it is empty. [-2]

Score: 18 / 20

Neat concept. I love the knock-up idea.
Simple but effective channeled AoE spell.

Score: 16 / 20

+Nice configurability.
I don't think HS_trigger and HS_trigger2 are necessary as variables. [-1]

Score: 9 / 10

The unit will sometimes attack during the cast.
Increase the follow-through time in the object
editor (channeling time). [-1]
Sometimes the units will be knocked up, and die
in the process, but their bodies appear for about
a second before they play their death animation.
(confirmed: bug related to other system)
Great documentation, and I love the video!

Score: 9 / 10

Lovely visuals.
Nice sounds.

Score: 8 / 10

None

Chaosy · Jun 16, 2014

I will post the changelogs in this post as I know the moderator will have a few *cough* opinions about the triggers.

~reserved

First release.

-the local hashtable is now nulled at the end of the trigger(s)
-the loop trigger is now turned off if the unit group is empty
-the loop speed is now a real variable
-the size of the areas used when randomize direction of the shockwaves is now configurable

-the caster is no longer stored into the hashtable, I use picked unit to get the caster instead.
-the local handle variable now gets its value set a little later in order to prevent one function call.
-one condition in the cancel trigger has been removed
-the picked unit is set sooner in the loop trigger.

rarigate · Jun 16, 2014

Your loop trigger is broken in the description, fix pls.

GywGod133 · Jun 16, 2014

i recommend you use 0.03125 sec instead using 0.03

it helps you to give a exact number that you need

custom script: set udg_real = 0.03125
add trigger real second event

:v dah Daffa and me use this function lol

Daffa · Jun 16, 2014

Can't trace the damn said leak.
Would be nice if you oatch the trigger tag for the loop.
The cancel is nice as add on as well

IcemanBo · Jun 16, 2014

Definition of max/min angle radius would be cool in config trigger.

Hashtables is an agent and should be nulled if declared localy. But here it's not even needed to declare it localy because you already use a globals hashtable. It's just an useless overhead.

You should initialisize your HandleId variable after you set your unit variable to prevent one more "GetTriggerUnit()" function call.

In your cancel trigger you simply could check if unit is in the UnitGroup in the trigger condition.

Haven't read the periodic trigger yet, because of the broken tag.

Spell looks good for me, nice GIF. Fix the description tags soon, so we can review :thumbs_up:

Chaosy · Jun 16, 2014

@Rarigate
fixed

@AKA.GywGod133
I guess I can do that, I don't really see the difference but I will do it.

@Daffa the Mage
The second unitgroup in the loop trigger

and thanks

@icemanBo
I was thinking of it, but somehow I didn't do it. Will fix.

oh yes, I am just used to use the integer which shouldn't be nulled.

what do you mean? I use that at the top of the trigger, I can't use it earlier?

will do..

fixed.

IcemanBo · Jun 16, 2014

what do you mean? I use that at the top of the trigger, I can't use it earlier?

You can seperate declaration and initialization.

You could just store the caster's location into hashtable isntead of X/Y values, because you anyway create a new location afterwards out of these X/Y coordinates.

Set HS_direction = (Random real number between (HS_facing - 70.00) and (HS_facing + 70.00))

-->

Set HS_direction = (Random real number between (HS_facing - HS_cone) and (HS_facing + HS_cone))

In periodic trigger "PickedUnit" can be stored into a variable to avoid more function calls.

Instead of using "(HS_group is empty) Equal to True" I recommend to use a custom integer counter, and check if the counter equals null.

In your periodic trigger you declare each iteration a new hashtable.

... You really should get rid of this overhead

In cancel trigger: Is " HS_is_casting Equal to True" this really needed? Because you already know the unit is in this UnitGroup. So think maybe this next check can be avoided, no?

Chaosy · Jun 16, 2014

1. Either I don't get what you mean or that seems pretty pointless to me.. (no offense)
2. You can't remove location leaks if you store a location into a hashtable, the X / Y prevents a leak.
3. that's fixed already

4. there's 2 picked unit used so I guess that should be a variable

will do
5. I try to keep my spells in as much GUI as possible, for example

JASS:

call SaveReal(hash, h, 7, udg_HS_counter)
==>
call SaveReal(hash, h, 7, udg_HS_counter + 0.03)

6. how dare you suggest to remove my local hashtable! (easier to type!)
7. true, I will do that.

IcemanBo · Jun 16, 2014

1. Either I don't get what you mean or that seems pretty pointless to me.. (no offense)

I don't take as offense, no problem.

but I mean this ...

Custom script: local intger h = GetHandleId(GetTriggerUnit())
Set unitVariable = TriggeringUnit

to avoid one function call you can do this -->

Custom script: local intger h
Set unitVariable = TriggeringUnit
Custom script: set h = GetHandleId(udg_unitVariable)

2. You can't remove location leaks if you store a location into a hashtable, the X / Y prevents a leak.

Sorry, haven't used it by mself with location in hashtable. Is the function bugged with locations or why you can't clean it from hashtable?

6. how dare you suggest to remove my local hashtable! (easier to type!)

I agree it's simpler to write, but it's just a senseless overhead for me that's why I suggested to remove.
This problem only occurs because of with mixing GUI with JASS stuff. Indexing all instances could remove this issue for you and furthermore it would be some more efficient than looping through a UnitGroup.
(just saying... but of course it's absolutly ok to use UnitGroups)

Chaosy · Jun 16, 2014

I see.. well that's something I could do.

If I store location X into my hashtable
and then call Removelocation(udg_storedLocation) it always crash for me at least.

I might consider removing it, should be too bad anyway.

IcemanBo · Jun 17, 2014

It looks something like this? : call RemoveLocation(LoadLocationHandle(udg_hash, parentKey, childKey))

But hmm, strange that it crahes the game. :/ ... but might be, I never worked with it just was a guess :thumbs_up:

Daffa · Jun 18, 2014

As far as I'm aware, nulling the Hashed data aren't necessary.
When you destroy the handle from the hash, you clean the leak. That's my logic however.

nhocklanhox6 · Jun 21, 2014

This is a cool spell. I love it. 5/5..

This is using for?

Custom script: local hashtable hash = udg_HS_hash

. You can just simply use udg_HS_hash.

Custom script: set udg_HS_direction = LoadReal(udg_HS_hash, h, 5)
- Custom script: set udg_HS_level = LoadInteger(udg_HS_hash, h, 6)
- Custom script: set udg_HS_counter = LoadReal(udg_HS_hash, h, 7)
- Set HS_counter = (HS_counter + 0.03)
- Custom script: call SaveReal(udg_HS_hash, h, 7, udg_HS_counter)
- Custom script: set udg_HS_explosion_count = LoadInteger(udg_HS_hash, h, 8)
- Set HS_explosion_count = (HS_explosion_count + 1)
- Custom script: call SaveInteger(udg_HS_hash, h, 8, udg_HS_explosion_count)
- Custom script: set udg_HS_X = LoadReal(udg_HS_hash, h, 3)
- Custom script: set udg_HS_Y = LoadReal(udg_HS_hash, h, 4)

Use hashtable isn't a good idea, indexing faster than hashtable. Your trigger will save load save load during period, it will cause a bit slower, but very slower if it casting too much at the same time. Of course, you can use hashtable but keep it less

, just save anything you need to do with.

And use ForGroup instead of Indexing isn't a good idea too. I just show you a one thing: a additional part is GetHandleId or GetUnitUserData and some stuff go with them. And maybe indexing is faster than ForGroup, I haven't checked it, but I believe it

..You can correct me if I wrong >.<..

Use call DestroyGroup instead of Custom script: set bj_wantDestroyGroup = true because it is safer and faster than.

I think you have to use timer to run period trigger, because timer is faster than trigger timer (Executable trigger) (Optional suggestion from me)

And can you explain your configurable ^^!

Chaosy · Jun 21, 2014

@nhocklanhox6

1. the local hashtable is just to save time (aka easier to type) and improve the readability slightly.
2. you're certainly right, a 1d array is obviously faster than a 2d array. However I don't use index because I hate how it looks in GUI. Besides with todays technology that does hardly mater, I mean unless it's a map that lasts for over an hour I can play a map without any leak removals at all due my high RAM. Same with the proccesor.
3+. I have problems understanding everything here, but since this is all about speed I'll just answer them all. Isn't that to go over board, I mean my spell is jassish as it is with all those hashtable saves and loads as it is.

nhocklanhox6 · Jun 21, 2014

1. the local hashtable is just to save time (aka easier to type) and improve the readability slightly.

It doesn't make sense, udg_HS_Hash and hash are the same, you're improve nothing >.<, you're making period trigger use more memory.

Did you think ForGroup is look cute than indexing, I had a think opposed with you

.

2. you're certainly right, a 1d array is obviously faster than a 2d array. However I don't use index because I hate how it looks in GUI. Besides with todays technology that does hardly mater, I mean unless it's a map that lasts for over an hour I can play a map without any leak removals at all due my high RAM. Same with the proccesor.

I know

, even you play it for more hours, it still okay. But one thing is if handled too much, can cause overheating for RAM and reduce FPS over time. You may feel after some time playing, performance dropped markedly. Slow or fast isn't a matter, the matter is a programmer really needs to know to improve performance optimization each resources they created

. I call it responsibility and stance.

3+. I have problems understanding everything here, but since this is all about speed I'll just answer them all. Isn't that to go over board, I mean my spell is jassish as it is with all those hashtable saves and loads as it is.

The things that you don't understand is?.

Untitled Trigger 001
- Events
- Conditions
- Actions
  - Custom script: call set bj_wantDestroyGroup=true
  - Unit Group - Pick every unit in (Units in (Playable map area)) and do (Actions)
    - Loop - Actions

. This is unsafer and slower than DestroyGroup, so why?

Because after a while using call bj_wantDestroyGroup=true, you can feel your FPS reduce very fast than DestroyGroup. Some people doesn't know what's inside call bj_wantDestroyGroup=true. So think again

. Slow or fast doesn't matter, but you have to know it. Slow and hard code are the enemies of the device. Computer or phone are the same.

Please do not resist any word that I have spoken

IcemanBo · Jun 21, 2014

I think in GUI it's ok to use ForGroupBJ(), even indexing is more efficient.

nhocklanhox6 · Jun 21, 2014

IcemanBo said:
I think in GUI it's ok to use ForGroupBJ(), even indexing is more efficient.

Yes, you're right about ForGroupBJ(), but I'm talking about call bj_wantDestroyGroup=true

..

Chaosy · Jun 21, 2014

1. I don't know.. I just can't stand GUI indexing. I don't review spells/systems that use it since I get a headache by looking at it. Tbh I think forgroup in GUI looks really good but in jass.. oh god why.

2/3. but the FPS reduce is caused by the RAM being breaked. (aka when the computer starts to use memory from the normal drives.) But you're correct optimization is important, but if we talk public recources I think it's better to make it more GUI friendly. For the GUI friendly part it's towards the timer and GetHandleId() in the last post. I can change the unit group leak removal method not a big deal.

IcemanBo · Jun 21, 2014

@nhocklanhox6, you were talking about "ForGroup", even he doesn't use it. So I mentioned "ForGroupBJ" because I thought you meant it, no?
You said better to use "DestroyGroup" instead setting "bj_wantDestroyGroup = true". And bj_wantDestroyGroup only works with the BJ function.

nhocklanhox6 · Jun 21, 2014

1. I don't know.. I just can't stand GUI indexing. I don't review spells/systems that use it since I get a headache by looking at it. Tbh I think forgroup in GUI looks really good but in jass.. oh god why.

Yup!, Everyone has a personal preference and style, so never mind!. Ahaha, it still cute by the way

.

2/3. but the FPS reduce is caused by the RAM being breaked. (aka when the computer starts to use memory from the normal drives.) But you're correct optimization is important, but if we talk public recources I think it's better to make it more GUI friendly. For the GUI friendly part it's towards the timer and GetHandleId() in the last post. I can change the unit group leak removal method not a big deal.

Breaked ? >.<, I believe FPS reduced when the computer not enough time to processing image. So this will drop FPS until it has enough time. Yup! trigger timer is very friendly to GUI users, but think again. Using

Custom Script

too much is very hard for GUI users, who ONLY understand about original GUI, right?

@Ice: ForGroup and ForGroupBJ are the same interface, I just simply use ForGroup to explain it (I'm a big fan of pink color), I think Chaosy, who I'm talking about is understand what I am saying ^^~, but you're right, this is my mistake. I will fix it someday

.

GUI Interface

Unit Group - Pick every unit in (Units in (Playable map area)) and do (Actions)
- Loop - Actions

JASS Interface
ForGroup

Chaosy · Jun 21, 2014

Yeah break. Once there isn't enough of RAM the computer starts to use memory from the other hard drives on the computer. And THAT is the definition of slow, and since it's slow there isn't enough time to process images. ( at least that's how I got it explained to me )

Yup, 90% of the GUI users don't know a single shit about JASS.

this is getting a little off topic

nhocklanhox6 · Jun 21, 2014

Once there isn't enough of RAM the computer starts to use memory from the other hard drives on the computer. And THAT is the definition of slow, and since it's slow there isn't enough time to process images.

I've read about those things for a while, but never mind!

..

Ahaha, So, I will say good ye to this topic

..

GywGod133 · Jun 22, 2014

You forgot something chaosy

you are using duration in your spell
So, in 0.03125 sec you add 0.03 in HS_counter
So you must change it, HS_counter = HS_counter + 0.03 to HS_counter = HS_counter + HS_loop_speed
to see the differents using 0.03 and 0.03125, just use DISPLAY MESSAGE

Chaosy · Jun 22, 2014

ah, I actually tried to do "HS_counter = HS_counter + 0.03125" but that didn't work so I just left it be. Will make sure to fix it very soon.

BPower · Aug 2, 2014

Replace local hashtable has with udg_HS_hash ......

The configuration trigger could be outlined with comments.

You don't have to set the unit animation via trigger, it can be done in the object editor aswell.

Using the ForGroup native as function for the loop is not the best solution, but an acceptable one.

I guess the system KUS is not an approved system in our spell data base.
It has flaws, for example it doesn't properly reset the UnitProperWindow.

The IsDestructableTree version only works for trees with the id 'LTlt'.
You also have a location leak here.

Chaosy · Aug 8, 2014

1. fine D:
2. I explain what every variable is used for in an trigger. Why would I need to add more comments?
3. you might want to change it depending on the unit type etc. Should I still change it?
4. it is approved.
5. oh.. it's outdated. I will change that.

edit: updated!

Kazeon · Aug 12, 2014

according to your last update:

1. Dead units are still knocked up. And they played "attack" animation while being knocked which is weird.

2.

Set HS_caster_loc = (Position of HS_caster)
Custom script: call SaveReal( udg_HS_hash, h, 3, GetLocationX(udg_HS_caster_loc))
Custom script: call SaveReal( udg_HS_hash, h, 4, GetLocationY(udg_HS_caster_loc))

=>

Custom script: call SaveReal( udg_HS_hash, h, 3, GetUnitX(udg_HS_caster))
Custom script: call SaveReal( udg_HS_hash, h, 4, GetUnitY(udg_HS_caster))

3.

Set HS_level = (Level of HS_ability for HS_caster)
Custom script: call SaveInteger( udg_HS_hash, h, 6, udg_HS_level)

=>

Custom script: call SaveInteger( udg_HS_hash, h, 6, GetUnitAbilityLevel(udg_HS_caster, udg_HS_ability))

4.

-------- ------------------------------------ --------
-------- ------------------------------------ --------
If (All Conditions are True) then do (Then Actions) else do (Else Actions)
- If - Conditions
  - HS_counter Greater than or equal to HS_time
- Then - Actions
  - Set HS_counter = 0.00
  - Custom script: call SaveReal( udg_HS_hash, h, 7, udg_HS_counter)
- Else - Actions

=>

-------- ------------------------------------ --------
-------- ------------------------------------ --------
If (All Conditions are True) then do (Then Actions) else do (Else Actions)
- If - Conditions
  - HS_counter Greater than or equal to HS_time
- Then - Actions
  - Custom script: call SaveReal( udg_HS_hash, h, 7, 0.0)
- Else - Actions

Basically, just don't use variable if you don't use it at all. Just directly save the value into hashtable. This one is an example. You are not using udg_HS_counter after it and just save the value in it to the hashtable. Same thing happens for several points above (my review). I explain so I don't need to point out same mistakes.

5.

Unit Group - Pick every unit in (Units within HS_detect_area of HS_temp_loc matching (((Matching unit) belongs to an enemy of (Owner of HS_caster)) Equal to True)) and do (Actions)
- Loop - Actions

Save (Owner of HS_caster) into hashtable and store it into variable before use.

6.

If (All Conditions are True) then do (Then Actions) else do (Else Actions)
- If - Conditions
  - HS_knockup Equal to True
- Then - Actions
  - Set KUS_effect = <Empty String>
  - Set KUS_height = 300.00
  - Set KUS_speed = 5.00
  - Set KUS_animation = Attack
  - Trigger - Run prepare <gen> (checking conditions)
- Else - Actions

Allow user to configure them.

BPower · Aug 12, 2014

Please note that:

Custom script: call SetUnitPropWindow(udg_KUS_target, GetUnitDefaultPropWindow(udg_KUS_target))

doesn't properly reset the units PropWindow. In order to get the correct value you need to multiply it with bj_DEGTORAD

Chaosy · Aug 15, 2014

Thanks both of you! I will get to it. And update this post when I am finished.

edit: updated!

I did as you said, BPower.

@Dalvengyr

1. Dead units can no longer be knocked up. [fixed] and I agree on the attack animation. But it looks lame if they just stand in the air too, sadly there is no "take damage" animation or something like that.
2. fixed
3. fixed
4. fixed
5. I don't really see the point of that.
6. While I get where you're coming from, I don't think it's an good idea. Then I'd use 2 pointers.. (I think that's what you call it)

Kazeon · Aug 30, 2014

5.
- First save the owner of caster into hashtable
- Before unit picking, load the player to a non-array player variable

set (non-array player variable) = Load player from hashtable

- then:

Unit Group - Pick every unit in (Units within HS_detect_area of HS_temp_loc matching (((Matching unit) belongs to an enemy of (non-array player variable)) Equal to True)) and do (Actions)

To avoid repetitive FC
6.

Set KUS_effect = <Empty String>
- Set KUS_height = 300.00
- Set KUS_speed = 5.00

they (effect, height, etc) are better can be configured by users

IcemanBo · Sep 22, 2014

Some new notes:

Set HS_level = (Level of HS_ability for HS_caster)

Set HS_caster_loc = (Position of HS_caster)
^These lines are not needed anymore in your cast trigger.
I still suggest using a counter instead of doing this:
(HS_group is empty) Equal to True
Custom script: set udg_HS_counter = LoadReal( udg_HS_hash, h, 7)

Set HS_counter = (HS_counter + 0.03)
-->
Custom script: set udg_HS_counter = LoadReal( udg_HS_hash, h, 7) + udg_HS_loop_speed

Custom script: set udg_HS_explosion_count = LoadInteger( udg_HS_hash, h, 8)
Set HS_explosion_count = (HS_explosion_count + 1)

-->

Custom script: set udg_HS_explosion_count = LoadInteger( udg_HS_hash, h, 8) + 1

Custom script: set udg_HS_X = LoadReal( udg_HS_hash, h, 3)
Custom script: set udg_HS_Y = LoadReal( udg_HS_hash, h, 4)
Set HS_caster_loc = (Point(HS_X, HS_Y))

-->

Custom script: set udg_HS_caster_loc = Location(LoadReal( udg_HS_hash, h, 3), LoadReal( udg_HS_hash, h, 4))

^You anyway use custom scripts to load the values, so there is no need for these two extra variables for x/y.

Damge - and AttackType could be configurable.
Special Effect - Create a special effect attached to the chest of (Picked unit) using HS_effect2
-->
Special Effect - Create a special effect attached to the chest of KUS_target using HS_effect2
^"chest" might also be part of user configuration.
Custom script: set udg_HS_charge_counter = LoadInteger( udg_HS_hash, h, 9)

Set HS_charge_counter = (HS_charge_counter + 1)
-->
Custom script: set udg_HS_charge_counter = LoadInteger( udg_HS_hash, h, 9) + 1

Maker · Oct 15, 2014

Disable the cancel trigger when the unit group is empty.

PurgeandFire · Oct 29, 2014

PM me with comments and concerns. Scores are subject to change based on revisions.

General

Coding

Concept

Design

Misc

Visuals

Required Changes

Purgeandfire :: 29 October 2014

Criteria (Spells):

Coding (20): efficiency and potential improvement.
Concept (20): originality and features.
Design (10): modularity, importability, ease of use, and approach.
Misc (10): test map, image, hive submission, misc.
Visuals (10): effects and appeal.

Score:

Coding: 8 / 20
Concept: 16 / 20
Design: 7 / 10
Misc: 5 / 10
Visuals: 8 / 10
Total Score: 44 / 70

Needs Fix :: see Required Changes for approval.

Current Rating

3 / 5

65 - 70 becomes 5 / 5
55 - 65 becomes 4 / 5
40 - 55 becomes 3 / 5
30 - 40 becomes 2 / 5
0 - 30 becmes 1 / 5

http://www.hiveworkshop.com/forums/2566133-post26.html

You do not need to save the X/Y in the hashtable.
Since it is a channeled spell, it is assumed that
the X/Y will be constant. Loading the value from
a hashtable should be slower than loading it from
the unit itself. [-1]
As IcemanBo said, you can just set the caster_loc
directly to the X and Y of the caster.
You do not need to save HS_direction in the hash.
Just set it at the top of the unit-group-loop.
- Set HS_direction = (Random real number between (HS_facing - HS_cone) and (HS_facing + HS_cone))
Consequently, you don't need to set it in HS cast. [-1]
In addition to the above note, you never load HS_facing.
You need to load it for MUI. [-3]
You have a leak in the destructable kill loop. You
have to use call RemoveLocation(udg_HS_temp_loc)
after the "Then - Actions" line. Otherwise, you'll
keep reassigning the variable, and the previous locs
in memory will remain there forever, unremoved. [-3]
Disable HS cancel when the unit group is empty.
Enable HS cancel when the spell starts. [-1]
Instead of using "Is unit group empty", you should
just use an integer. When a caster is added to the
unit group, increment the integer. When a caster is
removed, decrement the integer. Then change the conds
to check if the integer is 0 (i.e. group is empty).
The "Group is empty" GUI function is very expensive
since it goes through the ENTIRE group just to check
if it is empty. [-2]
You can remove udg_HS_is_casting. [-1]
Check out IcemanBo's points as well.

Score: 8 / 20

Neat concept. I love the knock-up idea.
Simple but effective channeled AoE spell.

Score: 16 / 20

+Good configurability.
Add damage/attack type configurability.
I don't think HS_trigger is necessary as a variable.

Score: 7 / 10

Needs a DISBTN. [-1]
The unit will sometimes attack during the cast.
Increase the follow-through time in the object
editor (channeling time). [-1]
The test map should have a way to reset health,
mana, and cooldowns.
The test map should have more than one hero to
be able to test MUI. [-2]
Sometimes the units will be knocked up, and die
in the process, but their bodies appear for about
a second before they play their death animation. [-1]
Is that a bug related to the knock-up system?
Great documentation, and I love the video!
In the tooltip, you should add a space before "12".

Score: 5 / 10

Lovely visuals.
Nice sounds.

Score: 8 / 10

Coding: #4, #5, #6, #7, #8
Misc: #4, I would like a response to #5 so we can get that
sorted out

Chaosy · Oct 29, 2014

Seems like a lot to work on.

I will start to fix all the stuff today, hopefully I will finish today too ^^

thanks very much for the comments!

edit:
ok fixed a bunch of stuff. I was writing down exactly what I changed.. but I press back accidently.. and it was lost. I am too lazy to re-write all of it. But in total I did most of the things suggested.

But yeah, that's a issue with the knockup system, not this spell

(#5)

edit2: I just noticed a rather heavy bug.. the effect is sometimes created randomly beside the lines.. I don't know what's up with that.. any ideas?

PurgeandFire · Oct 29, 2014

Updated review. See Misc #4 for the reason behind that bug.

General

Coding

Concept

Design

Misc

Visuals

Required Changes

Purgeandfire :: 29 October 2014

Criteria (Spells):

Coding (20): efficiency and potential improvement.
Concept (20): originality and features.
Design (10): modularity, importability, ease of use, and approach.
Misc (10): test map, image, hive submission, misc.
Visuals (10): effects and appeal.

Score:

Coding: 15 / 20
Concept: 16 / 20
Design: 8 / 10
Misc: 7 / 10
Visuals: 8 / 10
Total Score: 54 / 70

Needs Fix :: see Required Changes for approval.

Current Rating

3 / 5

65 - 70 becomes 5 / 5
55 - 65 becomes 4 / 5
40 - 55 becomes 3 / 5
30 - 40 becomes 2 / 5
0 - 30 becmes 1 / 5

The destructable kill loop still leaks. The RemoveLocation
should be placed above "Set HS_temp_loc", so that it first
gets rid of this:
- Set HS_temp_loc = (HS_caster_loc offset by (HS_speed[HS_level] x (Real(HS_explosion_count))) towards HS_direction degrees)
If you put the RemoveLocation above this line:
- Custom script: call RemoveLocation(udg_HS_temp_loc)
- Set HS_temp_loc = (Position of ISD_d)
Then it should all work out just fine in the end, since you have
a RemoveLocation() at the end of the trigger to handle that
last location assigned. [-3]
Instead of using "Is unit group empty", you should
just use an integer. When a caster is added to the
unit group, increment the integer. When a caster is
removed, decrement the integer. Then change the conds
to check if the integer is 0 (i.e. group is empty).
The "Group is empty" GUI function is very expensive
since it goes through the ENTIRE group just to check
if it is empty. [-2]

Score: 15 / 20

Neat concept. I love the knock-up idea.
Simple but effective channeled AoE spell.

Score: 16 / 20

+Good configurability.
I don't think HS_trigger and HS_trigger2 are necessary as variables. [-1]

Score: 8 / 10

Needs a DISBTN. [-1]
The unit will sometimes attack during the cast.
Increase the follow-through time in the object
editor (channeling time). [-1]
Sometimes the units will be knocked up, and die
in the process, but their bodies appear for about
a second before they play their death animation.
(confirmed: bug related to other system)
You should check if the destructable is dead before
killing it and making the effect. [-1]
Great documentation, and I love the video!
In the tooltip, you should add a space before "12".

Score: 7 / 10

Lovely visuals.
Nice sounds.

Score: 8 / 10

Coding: #1
Misc: #4

Chaosy · Oct 29, 2014

Ok ran the few small fixed.

1. fixed the path for the DISBTN icon
2. actually fixed the leak
3. checks if destructible is alive.

I still fail to find that object editor field though..

PurgeandFire · Oct 30, 2014

General

Coding

Concept

Design

Misc

Visuals

Required Changes

Purgeandfire :: 30 October 2014

Criteria (Spells):

Coding (20): efficiency and potential improvement.
Concept (20): originality and features.
Design (10): modularity, importability, ease of use, and approach.
Misc (10): test map, image, hive submission, misc.
Visuals (10): effects and appeal.

Score:

Coding: 18 / 20
Concept: 16 / 20
Design: 9 / 10
Misc: 9 / 10
Visuals: 8 / 10
Total Score: 60 / 70

Approved

Current Rating

4 / 5

65 - 70 becomes 5 / 5
55 - 65 becomes 4 / 5
40 - 55 becomes 3 / 5
30 - 40 becomes 2 / 5
0 - 30 becmes 1 / 5

Instead of using "Is unit group empty", you should
just use an integer. When a caster is added to the
unit group, increment the integer. When a caster is
removed, decrement the integer. Then change the conds
to check if the integer is 0 (i.e. group is empty).
The "Group is empty" GUI function is very expensive
since it goes through the ENTIRE group just to check
if it is empty. [-2]

Score: 18 / 20

Neat concept. I love the knock-up idea.
Simple but effective channeled AoE spell.

Score: 16 / 20

+Nice configurability.
I don't think HS_trigger and HS_trigger2 are necessary as variables. [-1]

Score: 9 / 10

The unit will sometimes attack during the cast.
Increase the follow-through time in the object
editor (channeling time). [-1]
Sometimes the units will be knocked up, and die
in the process, but their bodies appear for about
a second before they play their death animation.
(confirmed: bug related to other system)
Great documentation, and I love the video!

Score: 9 / 10

Lovely visuals.
Nice sounds.

Score: 8 / 10

None

Anderhil · Sep 12, 2015

Great job on the spell, it was amazing

Holy Shock 1.1

Just another Warcraft III map (Map)

Moderator

Moderator

General

Coding

Concept

Design

Misc

Visuals

Required Changes

General

Coding

Concept

Design

Misc

Visuals

Required Changes

General

Coding

Concept

Design

Misc

Visuals

Required Changes

General

Coding

Concept

Design

Misc

Visuals

Required Changes

Similar threads