Shadow Image Blink v1.4

[Moderator]

4th Apr 2016

Your resource has been reviewed by BPower.
In case of any questions or for reconfirming the moderator's rating,
please make use of the Quick Reply function of this thread.

Review:

Overall acceptable coding. Here and there is room for improvement!
Concept and effects are creative and the spell is fun to cast.

You set the casters position before using ShowUnit, therefore the unit appearance
sometimes doesn't match the missile impact point for crowded locations.
It's a visual and not a technical flaw.
A knockback implementation could eventually solve that issue.

Troubleshooting:

• Your whitespaces appear random, thus hurt the read-ability. Honestly it was extremly hard to read your code.
  The code misses structuring or at least a few comments.
  ---
• You are using magic numbers in your code which could preferably be replaced with named constants.
  For example: set angle = angle *.0174533 or set angle = 57.29583 * Atan2(targety - y, targetx - x) + 90.
  ---
• After calling destroy and nulling members your code calls a few native functions which take nulled unit arguments.
  ---
• I don't see the purpose of set StructData[MUI] = -2, because you don't run any debug checks at all.
  --
• local unit f = null. Initializing f isn't required here.

Review changelog:

1. Review 19th Mar 2014; BPower

 

[Druu]

With the video, I noticed the orb goes sometimes too far, sometimes it reaches exact destination. I think you should make it so that if the person blinks 2 feet in front of him have the same animation length but the orb doesn't go too fast. On the other hand, I'd like the orb to go at the normal rate if it's used on max distance.

I love the animation you put into it, looks really good.

 

[Maker]

Use bj_DEGTORAD and bj_RADTODEG. 180/Pi doesn't round to 57.29582 that you use. GetUnitTypeId(filter) != 0 is not needed here. Null Dummy. If you create a unit at x,y and only then disable pathing and add Aloc, there is no guarantee that the unit is at x,y.

 

[TriggerHappy]

• You should probably separate the configurable globals from the ones that aren't.
• dummy_3 may leak becauseUnitApplyTimedLifedoesn't remove the unit.
• I would consider dummy recycling. You can either handle it yourself or use a library like DummyUnitStack (would fix the dummy leak also).
• The "Damage settings" part at the bottom needs to be placed at the top as a constant function.

 

[BPower]

You are a creative mind and I really like the concept.

- Aslong as you don't filter out computer players you could simply use call TriggerRegisterAnyUnitEventBJ.
- I really dislike that you labeled your method onDamage, because on damage indicates actions performed on damage event.
- You could use a FoG, because it's faster the more units are picked.
- You should either use a seconds global block or move the non configurable block into the struct.
- 500 aoe doesn't fit to the effects at all, it is too large.

The "Damage settings" part at the bottom needs to be placed at the top as a constant function.

Indeed!

 

[Cokemonkey11]

http://www.hiveworkshop.com/forums/...on-using-first-group-loop-enumeration-223140/

The video looks great, I love the fading images.

 

[Xonok]

It definitely looks very nice and original.

 

[AlienAtSystem]

You have the small problem of the missile not going correctly from the starting point to the point where the caster appears. One solution would be to adjust the speed of the missile to make it go at the right speed. The other, which I think is computationally much less costly, is to go from the kinematic approach to a purely geometric one:

While the timer loop runs, have a variable t count up from 0 to 1 (so that it reaches 1 exactly when the timer has finished) and set the position of the missile as:

set x1=GetUnitX(caster)
set y1=GetUnitY(caster)
set x2=targetx
set y2=targety
call SetUnitX(u,(1-t)*x1+t*x2)
call SetUnitY(u,(1-t)*y1+t*y2)

 

[nhocklanhox6]

Thank everybody!. For help me improve my coding ..

 

[Malhorne]

        unit caster = null
        unit dummy_1 = null
        unit dummy_2 = null
        unit dummy_3 = null

Maybe I read too fast but what's the use ?

 

[Druu]

Maybe I rode too fast but what's the use ?

isn't rode the past for Ride ?

I think you meant read since read is the past for read but read "red".

 

[Malhorne]

Oh sorry >_<
Fudge my french >_<
It is read my bad ^^

 

[zv27]

nice

 

[GreeN!X]

This one is nice. The sfx is really good, though that delay before the orb explodes kinda bugs me, still gets a 5/5.

 

[Rheiko]

amazing..
i had this idea like 2-3 years ago, but it's before i start modding here
5/5

 

[BPower]

Yeah ...., is this a joke ^^?

                    /**************************Damage settings**********************************/
                    /**/set dmg = damageSetting(GetUnitAbilityLevel(caster,SPELL_ID))        /**/
                    /***************************************************************************/

if not IsTerrainPathable(x,y, PATHING_TYPE_WALKABILITY) then --> You could use TerrainPathability by Rising Dusk or IsTerrainWalkable by Vexorian as requirement.

The effects are nice. The coding looks oke.

Edit:
- Move the timer into the non-configurable part
- Use just one dummy for the stun, there is no benefit in creating one per struct instance only cons.
- TargetFilter for damaged units should be part of the user configuration.
- playe is a horrible variable name --> owner??
- You pass null arguments into functions after destroying the instance. This is bad coding. Note that in many other cases the thread will crash in such a case.
Here it doesn't really matter, but please try to avoid it in future.

Optional:
- Player(15) could be stored
- function Damage ---> function GetDamage
- GetLocalPlayer could be stored.
- 'Aloc' could be stored, you do store 'Armf'

 

[ShadyRO]

Why do i get Shadow image bilnk disabled due to errors ?!

 

[Malhorne]

You're using normal world editor instead of JNGP which is needed for vJASS scripting.

 

[BPower]

You check twice for boolean subtract. This could be merged.
The timer shouldn't be in the user configuration block
You should stick to one naming conventions. We prefer the JPAG --> fade_sep should be fadeSep.

List of horrible variables names and how they actually should be:

• BLI_END --> BLINK_END
• BLI_CAST --> BLINK_CAST
• FAD_VIRTUAL --> FADE_VIRTUAL ???? I didn't know what this is, so I googled it. Apparently a DJ software.
• FAD_SPEED --> FADE_SPEED
• playe --> owner
• ANI_SPEED --> ANIMATION_SPEED
• SEP_SPEED --> SEPERATION_SPEED
• all variables using _ --> Not mandatory, but it's logical to stick to the original Blizzard naming convention.

As the requirements in the Spell sections are not as high as in the JASS section, I'll tend to approve it later without these fixes done.
However I hope you do make those changes mentioned and care more about the naming convention in the future.

 

[Wrda]

loop
  exitwhen i > 15                
  call TriggerRegisterPlayerUnitEvent(t,Player(i),EVENT_PLAYER_UNIT_SPELL_EFFECT,null)                
  set i = i + 1
endloop

What is this for?

TriggerRegisterAnyUnitEventBJ

does the work, it's the same, so you should replace that with it.
Correct me if I'm wrong.

 

[Malhorne]

Both versions are ok.
THe first one is just the inline version of the BJ so it is a bit faster since you don't create the function environment...
But I prefer using the BJ for readability.

@nhock : you can freely keep this it is okay.

 

[BPower]

Old review. Posted for reference reasons.

07:23, 19th Mar 2014
BPower:
Very readable and efficient coding. Good job.
Concept and effects are very creative, which makes the spell really fun to cast.
The null arguments you pass in during the last loop is the only flaw I've found.

08:11, 17th Mar 2014
BPower:

Please fix the variable names in the configuration block and move the timer declaration to the bottom.
This may sound like peanuts to you, but in my eyes the user configuration has to be straightforward and clear.
There is no room for half-done variable names.
(While for instance AID for ability id is fair enough, ABILIT_ID isn't.)

The struct member variables could be fixed too, but it's not mandatory as the code is readable. Furthermore it is very well coded and a cool concept.
Also refer to my last post.