View Single Post
Old 04-18-2008, 04:11 PM   #21
Rising_Dusk
Obscurity, the Art


Projects Director
Project Leader: OD
 
Join Date: Feb 2006
Posts: 9,729

Submissions (27)

Rising_Dusk has a reputation beyond repute (1192)Rising_Dusk has a reputation beyond repute (1192)Rising_Dusk has a reputation beyond repute (1192)Rising_Dusk has a reputation beyond repute (1192)Rising_Dusk has a reputation beyond repute (1192)Rising_Dusk has a reputation beyond repute (1192)Rising_Dusk has a reputation beyond repute (1192)Rising_Dusk has a reputation beyond repute (1192)Rising_Dusk has a reputation beyond repute (1192)

Hero Contest #3 - 1st PlaceApproved Map: Desert of ExileApproved Map: Advent of the ZenithHero Contest #2 - 1st PlaceHero Contest - Third place>

Send a message via AIM to Rising_Dusk Send a message via MSN to Rising_Dusk
Default

Collapse JASS:
private constant integer SpellID = 'A000' //Spell ID, based on Trueshot Aura Ability
It doesn't have to be based on Trueshot, though, as a matter of fact having the buff is actually very ugly for such a passive. I think it would be much better and less confusing to users the less they have to configure. Base it on evasion with 0% or something like that.
Collapse JASS:
private function GetUnitsAngle takes unit a, unit b returns real
    return Atan2(GetUnitY(b)-GetUnitY(a), GetUnitX(b)-GetUnitX(a))
endfunction
Man, inline that call wherever you use it and save the lines of code.
Collapse JASS:
set Cindexes[Cindex] = integer(C)
That typecast isn't necessary, structs are integers anyways.
Collapse JASS:
    set SonicFX = GetAbilityEffectById(SpellID, EFFECT_TYPE_SPECIAL, 0) //Sonic effect model
    set BloodFX = GetAbilityEffectById(SpellID, EFFECT_TYPE_SPECIAL, 1) //Blood effect model
Make these into configurable strings so people can change the path as normal. It's not totally intuitive how someone would change the effects right now, so this change is pretty important.
Expand JASS:
Whoa, man, what are you doing? Go look at how I re-index things in Shrapnade, all of this looping stuff is horrendously inefficient. You could be saving all of these loops and everything. Simply put the ith indexed entry where this' indexed entry is upon removal. Easy, man. :p
Collapse JASS:
    private method onDestroy takes nothing returns nothing
        call ForGroup(.Units, function Data.Release)
        call RemoveUnit(.dummy)
        set .caster = null
        set .dummy = null
        call GroupClear(.Units)
        call .RemoveIndex()
    endmethod
The caster/dummy vars are re-used globals in arrays, they don't need to be nulled if you properly clean up the rest of it. (Which you do)
Collapse JASS:
        call DestroyEffect(AddSpecialEffectTarget(BloodFX, GetEnumUnit(), "chest"))
Needs to be configurable where to attach the effects.
Collapse JASS:
    if IsUnitInRangeXY(GetEnumUnit(), GetUnitX(D.dummy) + Radius * D.cos, GetUnitY(D.dummy) + Radius * D.sin, Radius) and not IsUnitInGroup(GetEnumUnit(), D.Units) then
This function internally uses distance math to return true or false, just do a (x2-x1)*(x2-x1)+(y2-y1)*(y2-y1)<= MaxDist*MaxDist mathematical check and save yourself that trouble. Also, don't be afraid to use local real variables, makes your code loads easier to read.

I'm not a big fan of the loop/search method to get the desired index of an array, there are cleaner ways of handling it. I won't be picky about it, though, since it won't be an issue until we're talking more than 30 or so instances at once.
__________________
Rising_Dusk is offline   Reply With Quote