Sharp Needles v1.0

[Moderator]

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

12:06, 2nd Sep 2011
Bribe: You don't need the "h" trigger, just drag the init hashtable
line to the InitTrig_ function in your main trigger.

If this is meant to be used with vanilla JASS you need to list all of
the required udg_ variables, make a "variables" trigger that can
be copy pasted to auto generate the variables, or you can be super
awesome cool and combine the two and use GUIJASS like I do.

Put the code in hidden tags and add a better description including
import instructions.

 

[Deuterium]

I only skimmed through the code... seems fine (i guess).

However:

1- it lacks documentation on how & what to import
2- When you first cast the spell, the direction of the needle's are pointed outwards instead of rotation direction
3- As for special effects, there are a few advices you could follow:
a. Try letting the needles' start location be the unit itself & then expand to their supposed locations (check this out: Comradesphip v1.03 -- not advertising my spell but this shows you what I'm talking about)
b. Do one of two things: in your case, all needles go at the same polar speed (i.e. the ones on the outside goes faster than the ones closer to the caster) making them rotate together and stay in synch with each other. Either, you let them rotate at the same actual speed (thus giving the impression that those closer and those farther away from the caster rotate at different speeds, OR actually let each rotate at a randomly picked speed and at a randomly picked radius and randomly picked direction which would look MUCH AWESOMER!
c. Try using another model for the needles 'cause this one looks odd: the trace left behind is actually a part of the needle model rather than a real trace, so it rotates with the needle instead of leaving a nice circular shaped trace.

Good job however


EDIT:
Use GetUnitX(or Y) instead of GetWidgetX(or Y)... the first is faster

EDIT 2:
Make your code adjustable by setting global variables at the top which can be easily changed (as mentioned below by Magtheridon96)

 

[Magtheridon96]

local real x = GetWidgetX(caster)
local real y = GetWidgetY(caster)

->

local real x = GetUnitX(caster)
local real y = GetUnitY(caster)

local group g = CreateGroup()

This is a huge problem.
Just use a global group and clear it at the end.

call GroupEnumUnitsInRange(g,x2,y2,120.,Filter(function IgloUltFilt))
call ForGroup(g,function IgloUltGroup)

Also, here, why don't you just do whatever you want with the units in the filter function using an if/then/endif statement? You could return false at the end then.
ForGroup is virtually useless here.

Tip: Don't destroy the global group, just clear it.


Also:

    loop   
        exitwhen i > 4 * lvl + 5   
        if i <= 3 then   
            set d[i] = -240. * i * i + 1040. * i - 640.   
            set h[i] = 70. * i - 30.   
        else
            set a = 3
            loop
                exitwhen a > 18    
                if i > a and i <= 3 + a then   
                    set d[i] = -240. * (i - a) * (i - a) + 1040. * (i - a) - 640.   
                    set h[i] = 70. * (i - a) - 30.    
                endif
                set a = a + 3   
            endloop
        endif
        set i = i + 1   
    endloop

You could do this at map initialization for all levels.
Declaring local arrays is as bad as creating a local gruop and destroying it >.<

function InitTrig_Igloult takes nothing returns nothing
    set gg_trg_Igloult = CreateTrigger()
    call TriggerRegisterAnyUnitEventBJ(gg_trg_Igloult,EVENT_PLAYER_UNIT_SPELL_EFFECT)
    call TriggerAddCondition(gg_trg_Igloult,Condition(function Trig_Igloult_Conditions))
    call TriggerAddAction(gg_trg_Igloult,function Trig_Igloult_Actions)
endfunction

Don't use TriggerAddAction, just add the actions as a condition and use an if statement to make sure the correct spell is being cast.
Conditions are much faster.


And, it's better if you use a local trigger here because that Global variable is dependent of the name of the trigger.

And in your actions function, you should declare a local integer for GetHandleId(t) since you're repeating that function call several times..

edit
And I agree with Deuterium about how your spell needs to be configurable.
Since it isn't vJASS, I'd recommend using functions that return the values instead of globals to keep this plain Jass

 

[Deuterium]

Oh yeah didn't notice it wasn't vjass... woops :s

 

[mckill2009]

instead of a local timer and destroying it, use a looping timer with a global group, get the values of it using GetEnumUnit()...make also an integer checker to check if that looper runs or not...

 

[Deuterium]

well mckill2009 what u say makes sense about the timer... but a local timer isn't a problem.. & honestly i prefer having a local timer since it's a spell to be imported into a map; in that case, even if u got a global timer, it's gna be only for this spell and thus would be almost as equally efficient as having the local timer.
(that is unless the mapper tweaks the triggers)

right?

i misread what u said mckill2009... so just scratch that

 

[mckill2009]

I'm really in no position to argue with a respected user like you Deuterium, but I did not say global timer, but "TriggerRegisterTimerEvent" and "ForGroup"...

 

[Deuterium]

hahah i'm more like a rusty user by now
& yeah i misread that... woops & sorry

yeah ur totally right then

 

[Undeadducky]

Love the spell
It could be very usefull!