Wandering Plague v1.1

[Moderator]

12th Dec 2015
IcemanBo: Too long as NeedsFix. Rejected.


11 Nov 2011
Bribe: Even if you are using a scope you need to list the requirements in the in-code comments so the user knows what to import.

GetWidgetLife for checking if a unit is alive is unsafe. The unit's life may have changed after death. You need IsUnitType, UNIT_TYPE_DEAD.

Use a FirstOfGroup instead of a group filter.

You do not require GroupUtils and it adds a lot of overhead to this script which does not need to be there. It is easy to inline "IsUnitInRangeXY" yourself.

"GetOwningPlayer(GetTriggerUnit())" -> GetTriggerPlayer()

SpellEffectEvent would be a good choice for a library requirement.

 

[Magtheridon96]

1- Don't use onDestroy
2- Null the player variables as well (in the destroy method)
3- Use GetWidgetLife (much faster at the C++ level since it only returns one float within a class)
4- Use shorter names for the variables
5- Make the variables "private constant"
6- Make the struct extend an array so that it doesn't generate unnecessary code in the background. (Then you don't need to use allocate and deallocate)
7- Inside the struct, it could be better to use "thistype" instead of the name of the struct.
It's exactly the same, but the only difference is that it allows you to change the struct
name to anything you want without having to pay attention to whatever is inside the
struct. Personally, I like it because it looks good ^^ thistype
8- Null the local trigger in OnInit
9- Try using a library instead of a scope for this spell to make the requirements explicit

Good Job ^^

 

[Justify]

Okay, first feedback. I was sure there will be some mistakes due to the big brake I made.
I always used onDestroy but I'm sure the call can be saved. GetWidgetLife and thistype will be added soon, just forgot them. However, I can't guarantee that they are correct, but if the tests a friend made are correct nulling never-destroyed handles has no effect. Even with thousands of not-nulled references nothing changed.
Oh, and for sure I will add the private constants. For a test I added them to another trigger, a library. When re-copying them I forgot to change it. Thanks.
More feedback would be nice

 

[busterkomo]

Is there any reason why you're doing this:

local Morphed this = GetTimerData(t)
set AFFECTED = this

rather than just this:

set AFFECTED = GetTimerData(t)

?
Since your constants are private, you can shorten their names by a fair amount by avoiding the 'WANDERING_PLAGUE' prefix (ie, 'ID' instead of 'WANDERING_PLAGUE_ID').
Also, GroupUtils is by Rising_Dusk, not Vexorian.

 

[Justify]

There is the reason that I want the syntax. AFFECTED.xyz everywhere is just annoying, the "this"-syntax is much cleaner imo, although I have an useless line due to this.
Will shorten the names, it's true. Another relict from a copy.
Credits will be fixed here with the next upload. In the map the librarie has Rising_Dusks introduction, I think it's clear enough.

 

[baassee]

Instead of creating/removing dummies you should have a static one and give the hex ability 99999 casting range. Create it at ini and it will be the last dummy you'll need.

I really like this one a lot I think I found some use for it.