Blight Curse 1.01

[Moderator]

BPower: The way you shift groups is not working ( You can test it by displaying the handle id of each group. You will realise you destroy 1 handle twice )

Your spell does not provide any tooltip, which is really unprofessional.
Cleanup your code, make a proper demo, then you can upload the new version.

If you need help with your code feel free to make a post in Triggers&Scripts

Need Fix

BPower: Lot of things to fix

15:02, 24th Jun 2015
IcemanBo: Check my post in thread.

 

[BPower]

Please post the code into the description.

 

[IcemanBo]

Hey, please provide a decent description of your spell.

Also you should add basic documentation. Especially when it comes
to configuration for the user, documentation becomes very important.

Sometimes it's better to use constant functions over constant values,
when it comes to certain aspects in configuration, for example for damage:

private static constant real DAMAGE = 20
->

private constant function Damage takes integer level returns real
    return 20. //*level for example can be used.
endfunction

x, and y coordinates of unit can stored into locals onCast.

In your enum you don't filter out for examle dead units.
You also might want filter for enemy units.

private group targets
private group temp
private group swap

^Three groups for an instance can be avoided here.
Basicly:
Temporary enums only, then: private static group myGroup
-> is completly enough. No need to destroy/re-create it over and over again.
Need to keep track of certain units for each instance, then: private group myGroup
-> Now each instance has it's own group, that can be used for damage only once for example.
Your case is a bit special now, because you use the fresh swapping method
in combination with FoG loops. But here also static help groups can be used.
Read last chapter here: http://www.hiveworkshop.com/forums/...on-using-first-group-loop-enumeration-223140/

When it comes to checks if an expression is true or false, you don't
need to compare it with true or false explicitly.
Just let the expression for true, and for false use not before it.
You also don't say "it is rainy == true" -> "it is rainy"
You also don't say "it is rainy == false" -> "it is not rainy"
The extra comparison becomes redundant.

this.tick1 == 32
-> I guess you want to wait 1 second. But as the timeout is configurable,
the value in comparison also should depend on it, and should not be a hardcoded constant.

private static timer iterator = CreateTimer()
^Nothing serious, but I personaly think "iterator" is a strange name for a timer variable.
I personaly would go with just something like "tim", or "clock".

Agents, and only agents need to be nulled when they fall out of scope.
You don't null groups onDeindex, but instead following operations are not needed:

set this.infected = FALSE
set this.tick1 = 0
set this.tick2 = 0
set this.level = 0
set this.infections = 0

An instance should probably be deindex if this.caster is not in game/not alive anymore. (?)

I see you are new on hive, so welcome on THW!
Please read the post carefuly before next update.

 

[fireblasts]

not really new and all

I will look at the spell and see what i can do to improve it ty for feed back

 

[IcemanBo]

Oh, yes. Idk why I thought you are a new user, sorry. :s

 

[fireblasts]

will work on changing some of the other stuff but for now i fixed some of the easyer stuff

 

[BPower]

Let's begin:

1.) Please put your code in a scope/library. Spells normally are scopes.
Encapsulation is very important to avoid code conflicts.

2.) You could add a small description above the configuration, explaining what the spell does.
The configuration part could be in a global block, to be better seperated from the rest of the code.
Furthermore the comments for the configuration part are poor made. i.e:

    private static constant real RANGE = 300
   /// range///

3.) Using static integer count is repeated unnecessarily.
Instead make use of your linked list data structure if this.next == 0 then

Directly related code issues:

1.) GetOwningPlayer(this.caster) --> GetTriggerPlayer()

2.) Store GetUnitX/Y into a local variable to reduce computation time.
( function calls are more expensive than variable look-ups )

3.) this.infected == false --> not this.infected

4.) Due to shifting groups back and forth you have a lot of group handle leaks.
Find a better solution. I quickly demonstrate the current problem

group temp
group swap 
group targets

--->

set swap    = targets// ok
set targets = temp// ok
set temp    = swap// means temp becomes original targets. Leak original swap handle.

-->

call DestroyGroup(targets)// Destroys temp
call DestroyGroup(temp)// Destroys targets
call DestroyGroup(swap)// Destroys targets

--> swap is never destroyed.

 

[fireblasts]

ty

ty very much i will now begin fixing as you have posted. i will also take conesterision of eveything else

 

[Hashjie]

First off let me state that I give fireblasts full permission to upload this resource, I'm the one that coded it.

^Three groups for an instance can be avoided here.
Basicly:
Temporary enums only, then: private static group myGroup
-> is completly enough. No need to destroy/re-create it over and over again.
Need to keep track of certain units for each instance, then: private group myGroup
-> Now each instance has it's own group, that can be used for damage only once for example.
Your case is a bit special now, because you use the fresh swapping method
in combination with FoG loops. But here also static help groups can be used.
Read last chapter here: http://www.hiveworkshop.com/forums/spells-569/blight-curse-1-01-a-266978/j...ration-223140/

Could you perhaps provide us with an example on how this is done? I don't seem to quite understand your workaround.

4.) Due to shifting groups back and forth you have a lot of group handle leaks.
Find a better solution.

Does this mean that this tutorial http://www.hiveworkshop.com/forums/...on-using-first-group-loop-enumeration-223140/ would have the same group leak?
I basically copied what was done there. I would like some clearance on how this can be avoided in the future. Perhaps what IcemanBo suggested can help but I still need some guidance on what exactly is meant with the static help group. I can't wrap my head around it.

An instance should probably be deindex if this.caster is not in game/not alive anymore. (?)

no because the units must still be damaged if the effect hasn't worn off yet even when the player unit has already died.

 

[IcemanBo]

The difference is in his tutorial he is using always the same global variables.
You are not using globals, but member of instances, so array. (global == static)

In here, each instance own 3 groups for it's own, which is not neeed for swap.