Soul Tear Aura V1.01

[Moderator]

Changed were made.

It is a well coded and decent spell with good visuals. Approved.

Old Review

13:08, 5th Jan 2015
IcemanBo:
To get it approved make these few changes: http://www.hiveworkshop.com/forums/spells-569/soul-tear-aura-v1-00-a-255799/index2.html#post2636003
At least the change of checking if a unit is dead is needed.

 

[Bannar]

I'm curious why you all are submitting your resources before contest actually ends ^)^
Anyway, I'll get to your spell review tomorrow. However, this I'll state immidiately: you overexpressed the description i.e too much text (aka bloat) is as bad as no description at all. If you want to be as informative as you shown here, please separate triggers and actual documentation.

Good night!

 

[Tank-Commander]

I'm curious why you all are submitting your resources before contest actually ends ^)^

People's individual reasons are probably all very different, public advertising, putting your entry more directly in view, gain attraction for the contest in general, etc. all seem like fair enough reasons. Eitherway it's pretty commonplace regardless of the form of contest

If you want to be as informative as you shown here, please separate triggers and actual documentation.

I would have done, and have done so in the past, if it weren't for the overwhelming number of people who post/pm issues with my spells, which were actually explained on how to solve (particularly when it came to importing/adjusting the spell) So I integrate it now in order to force people to actually read the readme's for once XD, though Indeed I'm aware it's quite bloaty. I've had both positive and negative responces on my method of documentation so I choose to stick with it on my own personal preference.

 

[Deleted member 219079]

Why not use vJass? Globals could help you

So many comments lol

 

[chobibo]

^ Better portability.

 

[Bannar]

Meaby he has his own reasons to stick with vanilla jass. This does not require JNPG I imagen, but frankly everything that is written as script, most GUIers recognize as chaos and pain made flesh. For most of them vJass or not - would not make a difference.

Yeah, if you implemented whole script in jass you could as well provide portable version with help of vJass.

 

[IcemanBo]

Being independent from JNGP is nothing bad I think. There are still guys out there who use vanilla WE.

 

[Tal'Rarity]

Being independent from JNGP is nothing bad I think. There are still guys out there who use vanilla WE.

Definitely.

 

[Kazeon]

//Checks if there are no iterations of the aura left //this is ,mostly an "in-case" part of the script, as it //is unlikely that this ever returns true (only if a unit unlearns the ability) if (udg_STA_AbilityCounter == 0) then call DestroyTimer(T) endif it can also occurs if the unit has been removed (in case caster is not a hero), so you should also check whether the unit is still existing in the map or not. Which as fas as I have scanned through your code, you are not checking it. if GetUnitTypeId(u) != 0 then

loop //Moves to the next node set TempInt = TempInt + 1 exitwhen TempInt > udg_STA_AbilityCounter set Node = udg_STA_NextNode[Node] //If the unit exists as a caster in the linked list, don't add it if (u == udg_STA_Caster[Node]) then set scan = true endif endloop Can we just use a global unit group variable instead of that lousy slow loop checking? => //If the unit wasn't found as a caster, add it as one if not(IsUnitInGroup(u, udg_STA_CasterGroup)) then set udg_STA_AbilityCounter = udg_STA_AbilityCounter + 1 //Checking for recycleable Nodes if (udg_STA_RecyclableNodes == 0) then set udg_STA_NodeNumber = udg_STA_NodeNumber + 1 set Node = udg_STA_NodeNumber else set udg_STA_RecyclableNodes = udg_STA_RecyclableNodes - 1 set Node = udg_STA_RecycleNodes[udg_STA_RecyclableNodes] endif ..... call GroupAddUnit(udg_STA_CasterGroup, u) Much faster

How about new created units with the aura ability? You didn't register them.

That's all I can say, I'm just pointing major flaws. Other than that, it looks like spagghetti with mayonnaise. I mean awesome.

EDIT: I believe the contest is over and you are not allowed to update the entry so here is my review. Also there are several improvements above.

 

[Tank-Commander]

it can also occurs if the unit has been removed (in case caster is not a hero), so you should also check whether the unit is still existing in the map or not. Which as fas as I have scanned through your code, you are not checking it.

The spell works properly in the way it is when units are removed (even if it's a unit ability on a unit and not a hero) thus this change is not needed, but the timer as you correctly point out will keep going if you do this - not a major flaw since removing units is very rare (beyond dummy units) but I'll update the spell to account for it

Can we just use a global unit group variable instead of that lousy slow loop checking?

Certainly - it would function in pretty much the same way, checking a unit is in a unit group requires enumeration through that unit group, the code would be shorter, but I doubt the processor difference is major (if any) and I'd have one more variable for the purpose of a function which isn't ran very often. I opted for the method I used for personal preference, if it's that big of a deal I'd comfortably change it for the sake of minor efficiency improvements

How about new created units with the aura ability? You didn't register them.

Somebody didn't read the readme, I did account for this

The bit you missed

//----------------------------------------------------------------//
//  Check Timer Periodic: Determines whether or not the Check     //
//  Timer runs constantly all the time without stopping, thus     //
//  allowing it to pick up more than pre-placed units, this       //
//  should always be set to false if no units will gain this      //
//  ability through any means other than learning it, or being    //
//  preplaced unit on the map, as it would just be wasteful       //
constant function STA_CheckTimerPeriodic takes nothing returns boolean
    return false
endfunction

Setting this to true will make the system pick up units placed in the map via either training or create unit functions

Edit: Also yes, it's in the polling stage at this point - I can't update my entered version, I can however update this one without infringing any rules (if the need arises)

Edit2: Changes made (too small to warrant a different version number)

 

[Vinz]

The special effects are awesome as usual. Good job TC!

 

[Kazeon]

The spell works properly in the way it is when units are removed (even if it's a unit ability on a unit and not a hero) thus this change is not needed, but the timer as you correctly point out will keep going if you do this - not a major flaw since removing units is very rare (beyond dummy units) but I'll update the spell to account for it Does removed unit stay in the list?

Certainly - it would function in pretty much the same way, checking a unit is in a unit group requires enumeration through that unit group, the code would be shorter, but I doubt the processor difference is major (if any) and I'd have one more variable for the purpose of a function which isn't ran very often. I opted for the method I used for personal preference, if it's that big of a deal I'd comfortably change it for the sake of minor efficiency improvements Are you sure unit group checking works that way? (I dunno)

Somebody didn't read the readme, I did account for this Yup, I didn't read all of your narrative texts Oh, you do check them periodically, then I suggest not to, it does uneeded processes. Just check units when they entered the map area. Creating one another trigger wont hurt. native TriggerRegisterEnterRegion takes trigger whichTrigger, region whichRegion, boolexpr filter returns event and this one too maybe? TriggerRegisterAnyUnitEventBJ(t, EVENT_PLAYER_UNIT_TRAIN_FINISH) One or two additional lines but it does better job.

And one day ago, someone told me that: if (GetWidgetLife(udg_STA_Caster[Node]) <= 0.405) then is not a proper way to check whether a unit is alive or not. Just use: if not IsUnitType(u, UNIT_TYPE_DEAD) and GetUnitTypeId(u) != 0 then I dunno, just sharing my experience

 

[Tank-Commander]

Does removed unit stay in the list?

No

Are you sure unit group checking works that way? (I dunno)

Fairly sure, eitherway I've already made the change

Yup, I didn't read all of your narrative texts
Oh, you do check them periodically, then I suggest not to, it does uneeded processes. Just check units when they entered the map area. Creating one another trigger wont hurt.
One or two additional lines but it does better job.

I think that would be uneeded, note that a unit can have the ability constantly removed/added, a simple "when entering the map" condition would not account for this, the periodic check was chosen as a universal method, and to be efficient is why I made it being periodic optional, this was a low on memory, higher in processing solution and kept the code shorter. If somebody wanted to use an alternative method, that's what the "manual adding" documentation was for and that functionality can be added in a minute (I didn't do this in the test map, because the case was not applicable to the scenario in it)

And one day ago, someone told me that:

if (GetWidgetLife(udg_STA_Caster[Node]) <= 0.405) then

is not a proper way to check whether a unit is alive or not. Just use:

if not IsUnitType(u, UNIT_TYPE_DEAD) and GetUnitTypeId(u) != 0 then

I dunno, just sharing my experience

I was told the opposite in my own experience, regardless if you see it fail feel free to tell me so, otherwise it would be a "proper way" and less processing

 

[IcemanBo]

If a unit dies, it's life can be set higher again via triggers. (probably accidently)
Checking for life < 0.405 is the common way, but that's why Dalvengyr suggested an other method.

 

[Kazeon]

//----------------------------------------------------------------//
//                        AESTHETICS                              //
//----------------------------------------------------------------//
//  Note: Effects must have valid pathnames and must not be set   //
//  to null values, otherwise graphical errors are likely to      //
//  occur, if an effect can be left null, it will be marked       //
//----------------------------------------------------------------//
//  Aura Effect: This determines the model used for your main     //
//  aura, you want to use the model path and paste it into the    //
//  double quotes or " markers, if the path has only single       //
//  slashes (\) you'll need to changeb it to double slash (\\)    //
//  before you save, the spell will not work if you do not do     //
//  this (if you acidentally save with the single slash (\) it    //
//  will still save but you may find yourself having a lingering  //
//  progress bar at full percentage, you can ignore it  for the   //
//  most part, but if you have it, you probably haven't entered   //
//  this field correctly. Get the model paths from the Object     //
//  editor - find the model you want (the model, not the unit,    //
//  in the models list, select it and hit enter twice, then       //
//  select the path (will look similar to this default value,     //
//  and paste in it here) I suggest doing this with a unit of no  //
//  value (changing the model to get the model path) and then     //
//  reset the field afterwards to get the unit back to normal,    //
//  this function must return a valid value, to make "remove"     //
//  effects from the spell (for those without "Use" functions     //
//  to disable them) you can enter the path of dummy.mdl (this    //
//  can differ from map to map depending on how you imported it)  //
//  and this will cause the system to create invisible effects    //
//  so that the spell can still run properly                      //

Man, this is not just a spell. This is TRAINING COURSE! Even better than that xD
(The main code itself seemingly to only takes 33% of the whole code)

Sorry, for the off-topic... Anyway, I think this should be approved soon.. ^)^

 

[IcemanBo]

• One global timer can be used, instead of recreating/destroying timers.
• Use IsUnitType(u, UNIT_TYPE_DEAD) or GetUnitTypeId(u) == 0 or UnitAlive native to check if a unit is dead.
  Using (GetWidgetLife(u) > 0.405) is not recommended because the life of a dead unit can still be changed.
• In function STA_AuraEnd there is no loop needed to find the correct node. It's already given as parameter.

Some minor notes:

• GetOwningPlayer(udg_STA_Caster[Node]) can be stored into a variable.
• In function STA_SoulProjectileDamage you set your player variable twice.
• You can directly return the expression in your filter function.
• No need to declare a local variable for only one function call. function LearnAbility
• For unit creation Player(14) could be a constant.
• No need to null players.
• No need to null a trigger that is never destroyed.

Well made visuals and everything is alright in OE. Nicely documentated, even a bit too much for me.
It's a great spell and I can see the work you've put it!

 

[Tank-Commander]

• One global timer can be used, instead of recreating/destroying timers.
• Use IsUnitType(u, UNIT_TYPE_DEAD) or GetUnitTypeId(u) == 0 or UnitAlive native to check if a unit is dead.
  Using (GetWidgetLife(u) > 0.405) is not recommended because the life of a dead unit can still be changed.

Will get on that, granted the global timer isn't much of a major thing - the odds of the timer being destroyed is pretty low in any given play

• In function STA_AuraEnd there is no loop needed to find the correct node. It's already given as parameter.

The loop is needed - it's not looking for the node which is passed (the unit which is losing the aura) but rather all the projectiles and effects associated with that aura instance so they can be removed

Some minor notes:

• GetOwningPlayer(udg_STA_Caster[Node]) can be stored into a variable.
• In function STA_SoulProjectileDamage you set your player variable twice.
• You can directly return the expression in your filter function.
• No need to declare a local variable for only one function call. function LearnAbility
• For unit creation Player(14) could be a constant.
• No need to null players.
• No need to null a trigger that is never destroyed.

1 - I'll get on straight away
2 - Ah, my mistake: must've missed it when skimming over it
3 - I kept it seperate so that it can be modified by users if they want to make it not affect/start affecting other certain types of units (I was recommended to do that with my abilities some time ago by a different moderator)
4 - I thought unit variables leaked even if only called once? (hence the need to null them? if not then sure I'll change that)
5 - I'll change that
6 - I was told otherwise (so I suppose it's a preference thing)
7 - Was never told that before, but sure I'll change that

 

[IcemanBo]

I am sorry, you were correct with using the loop in function function STA_AuraEnd. Thanks for mentioning it.

Needed changes were done. Approved.

I thought unit variables leaked even if only called once? (hence the need to null them? if not then sure I'll change that)

Once you initialize a local variable, you are right, it has to be nulled. But you only use the variable once, so actually there is no need to declare it:

set u = GetTriggerUnit()
call STA_NewAura(u)
set u = null

can be shortened to ->

call STA_NewAura(GetTriggerUnit())

I was told otherwise (so I suppose it's a preference thing)

It is because players can be seen as constants that are never destroyed in game. So it's ID can never recycle. -> No need to null it. (same for triggers that are never destroyed)

 

[Vinz]

Did you... forget to approve it? xD

 

[JC Helas]

I love all your Resources dude so cool,great,unique

 

[JC Helas]

I love all your resources dude so cool,great,unique

 

[aider]

Anywhere I can set the duration of the buff to last forever? Right now it's applied by the projectiles(?) but the buff lasts forever as long as the enemy is within AoE of my hero. As fast as they leave the AoE of the hero, they lose the buff. So you can stand on the edge of the AoE and projectiles will shoot out in an OP speed.

I thought first the affected units made their own aura and it had an X duration. But for me at least it seems the hero is giving out the aura
Such a good description, but I can't find anything on the duration of the debuff.

 

[Tank-Commander]

You're correct in that the debuff automatically happens whenever a unit leaves the AOE of the hero with the aura - I'm aware this can lead to projectile spamming. Though unless vs. Ais it's unlikely somebody is going to stand around and let you do it, making the projectiles deal 0 damage is a way around any sort of encouragement for doing it as well

 

[Tal'Rarity]

Could just add a buff duration and a config for it couldn't you?

 

[Tank-Commander]

Yes and No - it would require some code structure changes (the buff is simply removed at this point, to add a duration I'd have to add a timer for removal when they leave the AOE which would then also have to check if they have come back within the AOE since but before the timer runs out (with the timeout being the configurable) anybody is more than welcome to do that themselves if it's what they need but it'll add a lot of extra processing if I did it to the general resource