Chernobyliss v1.02

[Moderator]

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

10:31, 4th Feb 2014
BPower:
- Get rid of the ForGroup enumeration and use a FirstOfGroup loop instead.
- The timer should be periodic, also pass in TIMER_INTERVAL and not 1/TIMER_INTERVAL
- One spell cast easily allocates 40 instances, hence use one timer per spell cast coupled with a stack (could use Table by Bribe or Vex) or one timer for the whole scope with a linked list and a stack.
- Add a player member to the struct and get rid of CASTER, SPELL_X and SPELL_Y
- You could pass a player and a unit variable into FilterUnits to get rid of the GetOwningPlayer(..)
- You don't have to null ug, it gets nulled by the FoG loop you are using.
- Move the UnitAlive native to the top, because Cohadars JassHelper allows only one declaration.
- You don't need the Cbliss x and y member at all.
- R2I(ChanceToSpread(lvl)) is constant per cast, you could add a variable for it.
- Since you change the vertex color, RGB should be configurable

The concept and object are good and I would give you a rating of ~3.5 for it.
The current coding is ~1.5 and needs to be changed.

 

[TriggerHappy]

• Remove the Actions function altogether and place it directly in the GetSpellAbilityId() block.
• You should be nullingCASTERas well as your struct members to prevent leaks.

• TimerStart(...,1. / TIMER_INTERVAL)
  // shouldn't it be
  TimerStart(..., TIMER_INTERVAL)

• There should be a bit more documentation for things like importing and spell description.

 

[Strikest]

but CASTER isn't a local? It's a global variable, like udg? And people keep telling me null struct members, but how do you do that? I never learned because if I recall it's a fine practice and does not leak anything even remotely noticeable.

1./TIMER_INTERVAL is so it's easier for people to change the timer interval, as it will be a whole number. For example, my TIMER_INTERVAL is 32, so 1./32. = 0.03125.

I dunno about merging the actions and conditions... makes it more confusing for people to learn my code.

 

[TriggerHappy]

but CASTER isn't a local? It's a global variable, like udg? And people keep telling me null struct members, but how do you do that? I never learned because if I recall it's a fine practice and does not leak anything even remotely noticeable.

They leak, so just null them.

To null struct members just define your own destroy method

method destroy takes nothing returns nothing
    this.caster = null
    this.fx = null
    // ect..
endmethod

1./TIMER_INTERVAL is so it's easier for people to change the timer interval, as it will be a whole number. For example, my TIMER_INTERVAL is 32, so 1./32. = 0.03125.

So then put that directly in the global otherwise it will confuse people. It's also more efficient.

private constant integer TIMER_INTERVAL = 0.03125
// or
private constant integer TIMER_INTERVAL = 1./32.

I dunno about merging the actions and conditions... makes it more confusing for people to learn my code.

It's just another function call that's not needed. I don't think it makes it any less legible.

 

[BPower]

OnSpell and Actions could be merged into one.
GetUnitState(data.t,UNIT_STATE_LIFE) --> GetWidgetLife(data.t)
It is not a rule, but why aren't you working on the pending resources of yours before posting new ones? Me and and others pointed out many issues that should/must be changed.
I get the feeling, you want someone to double check your stuff rather than contributing it to the community.

Again consider this JPAG. To get one thing clear JPAG is not inconsequential.

 

[Strikest]

I get the feeling, you want someone to double check your stuff rather than contributing it to the community.

When you assume you make an ass of u and me. If I just wanted someone to double check my other spell, I wouldn't have updated it seven times. Also, I fixed most of the things that others pointed out, The only things I didn't fix are the changes I didn't understand, which I asked them to explain more.

 

[BPower]

• TIMER_INTERVAL should be 0.03125 and not 1/32
• functions names should be DamagePerSecond and not DAMAGE_PER_SECOND ( important )
• OnSpell and Actions should be merged into one
• I recommend a FirstOfGroup loop over the ForGroup you are currently using.
• CASTER as variable is not nessesary at all.
• Use a player variable as member of the struct
• The timers should be periodic. As alternative you could use only one timer for everything and a linked list.
• life / mlife calculate that once and store it to a local real.
• null the struct members cast, t, attach once they are destroyed or the struct instance is deallocated.
• The spell should have a maximum of 4 levels otherwise it prolongs the map loading time
• The tooltip could be improved, use the blizzard standart hero abilities as template (color, hotkey)

The effects are cool, but the coding is not efficient. That is one of the reasons for fps drops once more than a few units are infected by the poison.
The other reason is the huge amount of special effects. (Well that one can't be changed)

Maybe I have time on the weekend, to show you how to improve your code. Or maybe someone else here does.

 

[Haley]

chernobyl disaster! +rep

 

[Malhorne]

I strongly suggest to merge the three func actions and use FoG loop : this will lead to faster executions.
Some variables are unneeded like SPELL_X SPELL_Y CASTER ....

local unit ug = null
Why ?

Don't use TimerUtils but use only one single timer for everything xD
One timer every 0.0312500 is helly performance eater :/

 

[Strikest]

How could I do this with one timer? ;o That sounds awesomely efficient indeed.

 

[Malhorne]

With indexing you can do it.
Create a struct array and index through it ^^

See the tutorial in my sign

 

[Strikest]

Updated to v.101, fixing a bug that made the spell lag alot, and also fixed most of the things TriggerHappy and BPower mentioned, including:

Merging Actions and Onspell into one.
Nulling struct members.
Changing the function names to the "standard"
Changed GetUnitState to GetWidgetLife.
Set the life/mlife calculation to a variable.
Updated the spell to have 4 levels.
Updated the spell tooltips.

I did not understand, why do i have to add a player member to the struct?

And how do I use periodic? I never really used them, restarting timers felt like the same thing to me.

I did not change the timer interval because at the end of the day it's a matter of preference, 1/32 or .03125, the end result is 1/32 or .03125. Also I believe FoG groups are optional since the performance difference from GroupEnumUnitsInRange is negligible.

 

[BPower]

Do not use method onDestroy.

There's another known method, but please, if you ever see someone use it, you
should flame them give them a link to this tutorial:

method onDestroy takes nothing returns nothing
    // do stuff when you destroy
endmethod

This method is bad because it generates useless functions.
Instead of creating this method that runs when you call destroy,
why don't you just write the destroy method?
Isn't that logical?

 

[Malhorne]

onDestroy method generates TriggerEvaluate if I remember so yes only use destroy method.
BPower is right.

 

[Strikest]

Okay I fixed it.

 

[BPower]

To achieve a acceptable performance I would recommend a radical change of the whole code. On a second view I think you don't have to use a linked list for the timer.

Using TimerUtils and one timer per spell cast is fair enough. Note currently you are using one timer per struct instance.
One way would be to use two structs one for the main cast and the other for each chernoblala projectiles ^^ plus a Table array.
It is my favorized style for coding spells with many proectiles. But I don't know how to guide you here, it would basically mean that I write the whole code for you.

The current problem is, that you use ~50 timers per spell instance or more after all it is ENFO.

 

[Strikest]

So currently the way I did it is the most optimized it can be, and to make it even more optimize I have to recode it your way? Can I code it with an integer array for one struct, and then use the second struct recall the integer array for the spell effect? Sort of like what you do to make spells MUI in GUI, using a disabled loop that activates when you cast the spell.

 

[Malhorne]

If there is around 20 casts there will be 20 timers with a 0.0312500 period...
If the map isn't laggy it will be ok otherwise ....