Piercer of Heaven 4.5

[Moderator]

Piercer of Heaven 4.5 | Reviewed by BPower | 13.06.2015 [COLOR="gray" [COLOR="gray" [COLOR="gray" [COLOR="gray" [COLOR="gray" Concept[/COLOR]] Shoots a missle towards the target cast point. Damaged units will get stunned and dragged along with the missile. Damage and speed increases over the fly duration. It's a missile flying from A to B. Stun, dragging and optional tree destruction spice up the spell. Overall sufficient concept for a spell submission. Code[/COLOR]] The spell is MUI and leakless. ( except 1/2 group handles ) Once critical issues are solved, this spell can be approved. Destroying trees is optional, but you always do it within the loop. In the loop you have an hardcoded distance check of <= 100. Why is it not configurable? I would be suprised if group == null is not working ( I have to test this myself ) Overall code is good. Objects[/COLOR]] Object data looks ok. The aoe indicator is a bit confusing, as it is no AOE spell. Effects[/COLOR]] Beside the missile model, there are no extra effects. There is no death animation display of the missile, which looks a bit abrupt to me. You could simply destroy a special effect on the missiles x/y, once it is removed. Rating[/COLOR]] CONCEPT CODE OBJECTS EFFECTS RATING STATUS 3/5 4/5 4/5 3/5 3.5/5 NEED FIX

 

[Chaosy]

Remove comments... please.. it makes the code like 3x longer.

 

[Daffa]

You should delete this line :

• Game - Display to (All players) the text: KBInstances: 0

Since it's not related to the spell directly and users don't want their screen filled with this.

Only one of these can be applied :

• Set GT_LoopTime = 0.03
• Custom script: set udg_GT_LoopTime = 0.03

I suggest delete the first one and set the below one to 0.03125, nothing much except 1.33 operation difference, which is not a big deal. But you really need to choose only one, as the second always override the first one.

Great spell, and I see the name is taken from Bleach Anime (Getsuga Tenshou aka Heaven's Piercer/Piercer of Heaven).

Also, I really appreciate that you used my TreeKiller ^^ thanks

 

[IcemanBo]

There is a corrupted custom script in loop. (at least for me)
I can't compile the code correctly. I would appreciate if someone else tests it to compile.

You don't index properly. Increase the counter in begining, not in the end.
In case of a deindex it might mix up at the moment.
-> not MUI.

Why do you count knock back instances? Also, when one instance ends you remove
all units from knock back group and decrease counter. That's illogical.
-> not MUI.

Your "Some functions" library provides two functions witha common name.
They may get in conflict with functions by user. It needs to be changed.

Please use constant bjs intsead of RADTODEG real value... much more readable and easier to understand.

 

[IcemanBo]

If dummy is out of bounds no further actions should be executed.
The only next thing to happen is deindexing.

Using a global knockback group for all instances is no best solution.
You remove all units in range at deindexing from the knockback group
-> if unit is affected by 2 knock backs it will bug.

Why you don't stun if you detect that picked unit is not in knockback group yet?

Custom script: set udg_GT_TempUnit = CreateUnit(GetTriggerPlayer(),'n003',x,y,bj_RADTODEG *a)

^Do not hardcode. Put the unit type into a configurable variable.
Flying height should be configurable.

If caster is not in game anymore spell should end on next interval, as the caster is the unit that will deal damage.

I recommend you to put a distance limitation over the time limitation.
If you won't anyway you could make it depending on level of ability.

Describe in comments for variables that will be influenced by ability level that they are influenced by ability level.
User can't know this by himself.

Change dummy's death type to "can't raise, can't decay".

 

[GywGod133]

Suggestion

• Custom script: local real x = GetUnitX(GetTriggerUnit())
• Custom script: local real y = GetUnitY(GetTriggerUnit())

Its better to save the GetTriggerUnit in a variable so become like this

• Set TempUnit = Triggering Unit
• Custom script: local real x = GetUnitX(TempUnit)
• Custom script: local real y = GetUnitY(TempUnit)

 

[IcemanBo]

"Skip remaining actions" is not a good action to use in loops, because it will ignore all further instances.
Could be completly avoided with structering with help of then/else of an if-statement.

Directly check with the duration if caster is dead/alive. And then you can directly deindex.
So you would not need an extra if-statement in your "Then" action part.

• Set GT_Time = (GT_Distance / GT_InitialSpeed)

^This calculation is not correct, because your speed increases. It isn't constant.
Also the speed and inc_speed is level depending, so you would need to re-calculate it anyways.

The knockback doesn't work. Following check is faulty, when you pick units in range:

• (GT_Target is in GT_KB_Group[GT_ID]) == True

If you want to stun only units that are in range then only stun consequently units in range for next period. Don't stun over whole spell duration:

• Set STE_DURATION = (GT_Duration[GT_ID] + 10.00)

Why even +10?
Also if you only would stun until next period, then you don't need to "unstun" all units on deindex.
"unstun" all units on deindex is wrong, as it you don't know what happened. Just no new stun should be applied.
So you should stun units when they comes in range first time with missle, but also continiosly when knocking back. But only until next period.

 

[Elprede]

@IcemanBo: I will address all your points except

• Set GT_Time = (GT_Distance / GT_InitialSpeed)

^This calculation is not correct, because your speed increases. It isn't constant.
Also the speed and inc_speed is level depending, so you would need to re-calculate it anyways.

I have already resolved this with the following line in the loop.

• Set GT_Duration[GT_ID] = (GT_Duration[GT_ID] / GT_Speed[GT_ID])

I plan to either remove that time variable and calculate the time at cast or keep it to allow users to choose between time only or both.

Edit: Never mind. I see now an issue with the formula. I will fix it as well.

 

[IcemanBo]

• Set GT_UpdateMissile = True

^I personaly don't see much sense having this in config.
But you can let it, if you think it's senseful.

Groups with index 0 and 1 already exist in a GUI group array.
You leak them onInit when you create a new group and overwrite the variable.

Please note if values are depending onInterval. (config)

TestMap works not very fluent for me because you overloaded the demo with heroes that cast.
Sometimes less is more.

 

[Elprede]

Having that option really is senseless, since I have two other variables that would become fodder if that boolean was set to false.

If I were to check whether groups are null, the spell refuses to work properly, i.e. targets are hit multiple times and become immune to knockback.

 

[IcemanBo]

Having that option really is senseless

What do you mean?

If I were to check whether groups are null, the spell refuses to work properly, i.e. targets are hit multiple times and become immune to knockback.

You had a solution, but you created unit groups before so you leaked again. Does it work now correctly though?

 

[Elprede]

What do you mean?


You had a solution, but you created unit groups before so you leaked again. Does it work now correctly though?

I was referring to your comment on GT_UpdateMissile.

Yes, the spell works correctly as long as I do not check whether groups are null.
Don't worry about the unit group leaks as I seldom get 1 assigned to the custom value of a missile in my tests.

How would separating the knockback movement into its own loop sound?

 

[IcemanBo]

The spell is based on an AoE spell, but should be based on an other spell like shockwave for example,
as it's effect does't affect units in an area, but makes a straight shot in wanted direction.

Don't worry about the unit group leaks as I seldom get 1 assigned to the custom value of a missile in my tests.

It's just good practice if we exclude memory leaks in our approved submissions,
even it won't happen too often or is not really critical.

How would separating the knockback movement into its own loop sound?

You have problems with it atm?

 

[BPower]

A nice demo map is very welcome, but too many not required resources are
not helpful for users who just want to import what they need for the spell.

Add just what is really required for Piercer of Heaven and a minimum of demo code.

 

[Daffa]

Seeing the critical issue is pointed at the flaw of my system, I decided to shed some help :
Instead of running the pick in each loop, insert an ITE and place the destructable loop in the THEN block, then add if GT_DestroyTrees == true on the condition. This effectively eliminates the loop of destructable.

My deepest apologies for the very slow recommendation.

 

[Daffa]

Seeing this having nothing happening after 5 years, it felt like I am at fault for the whole reason it is not approved yet.

The solution related to the requirement given by @BPower 5 years ago... now that I re-read it:

• -------- Destroy trees if enabled --------
• Set GT_TargetPoint = (Point(GT_MissileX, GT_MissileY))
• Destructible - Pick every destructible within GT_AOE of GT_TargetPoint and do (Actions)
  • Loop - Actions
    • Set Dest = (Picked destructible)
    • Set TreeKill = GT_DestroyTrees
    • Trigger - Run TreeKiller GUI <gen> (ignoring conditions)

To fix the issue, the solution is to pass the GT check for trees before the destructible loop:

• -------- Destroy trees if enabled --------
• Set GT_TargetPoint = (Point(GT_MissileX, GT_MissileY))
• If (All Conditions are True) then do (Then Actions) else do (Else Actions)
  • If - Conditions
    • GT_DestroyTrees Equal to True
  • Then - Actions
    • Destructible - Pick every destructible within GT_AOE of GT_TargetPoint and do (Actions)
      • Loop - Actions
        • Set Dest = (Picked destructible)
        • Set TreeKill = GT_DestroyTrees
        • Trigger - Run TreeKiller GUI <gen> (ignoring conditions)
  • Else - Actions

Actually, even without this fix, the spell is working correctly, and TreeKiller won't destroy trees if GT_DestroyTrees is set to false (and if it does, that is my systems' problem, and should be held accountable to me as the provider). Indeed the loop will run, but the checker will block the tree-killing from taking effect in situations it is set to False.
@KILLCIDE @PurgeandFire @MyPad