Sacred Circle v2.0

[Moderator]

14:39, 23rd Feb 2011
Bribe:

1. Conditions and actions may as well be combined into just conditions

2. The periodic method makes unnecessary function calls and should just be placed inside the "update" method.

3. Inlining the TriggerRegisterAnyUnitEventBJ is completely useless because "null" as a filter no longer leaks.

4. A widget's life is against 0.405, not 0.406, but that's not going to cause any realistic problems.

5. == true and == false are useless.

But it's not the end of the world, just make those changes some day. For now:

Status: Approved

 

[Darkzealot_128]

Post code please.

 

[Laiev]

//¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤
//¤
//¤ ***************** 
//¤ - Sacred Circle - 
//¤ *****************
//¤ 
//¤ By: Daxtreme
//¤ 
//¤ --> How to implement in your map:
//¤     
//¤     1. Copy the game cache variable named "GameCache" in your map.                         
//¤     2. Copy the spell "Sacred Circle" in your map.
//¤     3. Copy everything found in the "Custom script code" section. To do this, click
//¤        on the name of the map in the top-left corner in the trigger editor.
//¤     4. Make a variable called "GameCache".
//¤     5. Copy this trigger into your map.
//¤     6. Import the HolyStrike.mdx model in your map.
//¤
//¤ --> How to customize it:
//¤
//¤     You can configure the spell using the constant functions just below. Change their values.
//¤
//¤ CREDITS:
//¤
//¤     - JetFangInferno's Holy Strike.
//¤     - kenny! for testing, bug-finding, and updating!
//¤
//¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤

scope SacredCircle2

    globals
        private constant integer    ABIL_ID    = 'A000'       // Sacred Circle's ability Id
        private constant integer    ORDER_ID   = 852183
        private constant real       INTERVAL   = 0.04         // Period
        private constant string     EFFECT     = "war3mapImported\\HolyStrike.mdx"      // Spell model art
        private constant attacktype A_TYPE     = ATTACK_TYPE_CHAOS
        private constant damagetype D_TYPE     = DAMAGE_TYPE_UNIVERSAL
        private constant weapontype W_TYPE     = WEAPON_TYPE_WHOKNOWS
        private constant boolean    STOP_FIRST = false
    endglobals
    
    private function Damage_dealt takes integer lvl returns real
        return 100.00 * lvl
    endfunction
    
    private function Damage_radius takes integer lvl returns real
        return 175.00 + (0.00 * lvl)
    endfunction
    
    private function Filter_enemies takes unit filter, unit caster returns boolean
        return GetWidgetLife(filter) > 0.406 and IsUnitEnemy(filter,GetOwningPlayer(caster)) == true and IsUnitType(filter,UNIT_TYPE_MAGIC_IMMUNE) == false
    endfunction
    
    private struct Data
        
        unit    cast = null
        real    time = 0.00
        real    dist = 0.00
        real    ang  = 0.00
        integer lvl  = 0
        boolean stop = false
        
        static Data     array D
        static integer  D_total = 0
        static timer    Timer   = null
        static group    Group   = null
        static boolexpr Filt    = null
        
        static method filt takes nothing returns boolean
            return true
        endmethod
        
        method onDestroy takes nothing returns nothing
            set .cast = null
        endmethod
        
        method search takes nothing returns nothing
            local integer i = 1
            
            loop
                exitwhen i > Data.D_total
                if Data.D[i].cast == .cast then
                    if STOP_FIRST then
                        set Data.D[i].stop = true
                    else
                        set .stop = true
                    endif
                endif
                set i = i + 1
            endloop
        endmethod
        
        method periodic takes nothing returns boolean
            local real x = GetUnitX(.cast)
            local real y = GetUnitY(.cast)
            local unit u = null
            
            if GetUnitCurrentOrder(.cast) != ORDER_ID or .time <= 0 then
                return true
            else
                set x = x + (50.00 + .dist * 10.00) * Cos(.ang * bj_DEGTORAD)
                set y = y + (50.00 + .dist * 10.00) * Sin(.ang * bj_DEGTORAD)
                
                call DestroyEffect(AddSpecialEffect(EFFECT,x,y))
                call GroupEnumUnitsInRange(Data.Group,x,y,Damage_radius(.lvl),Data.Filt)
                loop
                    set u = FirstOfGroup(Data.Group)
                    exitwhen u == null
                    call GroupRemoveUnit(Data.Group,u)
                    if Filter_enemies(u,.cast) then
                        call UnitDamageTarget(.cast,u,Damage_dealt(.lvl),false,false,A_TYPE,D_TYPE,W_TYPE)
                    endif
                endloop
                
                set .ang  = (.ang  + 24.00 - .dist / 6.00)
                set .dist = (.dist + 1.00)
                set .time = (.time - INTERVAL)
            endif
            
            set u = null
            
            return false
        endmethod            
        
        static method update takes nothing returns nothing
            local integer i = 1
            
            loop
                exitwhen i > Data.D_total
                
                if Data.D[i].stop or Data.D[i].periodic() then
                    call Data.D[i].destroy()
                    set Data.D[i] = Data.D[Data.D_total]
                    set Data.D_total = Data.D_total - 1
                    set i = i - 1
                endif
                
                set i = i + 1
            endloop
            
            if Data.D_total <= 0 then
                call PauseTimer(Data.Timer)
                set Data.D_total = 0
            endif
        endmethod
        
        static method actions takes nothing returns boolean
            local Data d = Data.create()
            
            set d.cast = GetTriggerUnit()
            set d.lvl  = GetUnitAbilityLevel(d.cast,ABIL_ID)
            set d.time = 100.00 * INTERVAL
            call d.search()
            
            set Data.D_total = Data.D_total + 1
            set Data.D[Data.D_total] = d
            if Data.D_total == 1 then
                call TimerStart(Data.Timer,INTERVAL,true,function Data.update)
            endif
            
            return false
        endmethod
        
        static method conditions takes nothing returns boolean
            return GetSpellAbilityId() == ABIL_ID
        endmethod
            
        static method onInit takes nothing returns nothing
            local trigger trig = CreateTrigger()
            local integer i    = 0
            
            set Data.Timer = CreateTimer()
            set Data.Group = CreateGroup()
            set Data.Filt  = Filter(function Data.filt)
            
            loop
                call TriggerRegisterPlayerUnitEvent(trig,Player(i),EVENT_PLAYER_UNIT_SPELL_EFFECT,Data.Filt)
                set i = i + 1
                exitwhen i == bj_MAX_PLAYER_SLOTS
            endloop
            
            call TriggerAddCondition(trig,Condition(function Data.conditions))
            call TriggerAddAction(trig,function Data.actions)
        endmethod
        
    endstruct

endscope

 

[Lambdadelta]

You ought to comment your code more, the variable STOP_FIRST... I have no idea what it does. Neither do I understand what ORDER_ID means.

 

[Laiev]

ORDER_ID is used to check if the current order is the current one...

example... you cast blizzard... then you're ordering to blizzard... if you order anything else (stop) it will return false and will stop the spell

 

[Pharaoh_]

Well, it's not anything fancy I guess, code-wise. There are lots of spells like this one. If I recall well, I have seen it before, I guess you've updated the code into supporting the 1.24 patch, but the maximum rating I'd give is useful. Lovely effect, nonetheless :]

 

[daxtreme]

I just browsed the approved spells section and sure there are a lot of nova spells and the such, but this isn't exactly a nova, look closely.

All I know is that, back when it was approved, people liked it and enjoyed using it. I don't see why, now that it's edited so that it works again, things should be different.

 

[Pharaoh_]

Don't get me wrong, it's just that it has a really basic vJass coding. That's all. Anyway, I won't shed any poison here, it was no aim of mine in the first place. As I said, the effect is nice, yet the coding is simple :]

 

[daxtreme]

Haha, criticism is always appreciated, don't worry.

Did anyone encounter flaws while testing it?

 

[watermelon_1234]

Note: I haven't tested the spell yet.

Short Code Review

• Your onDestroy method is pretty much useless since cast will be initialized as being null.
• You could try to remove the FirstOfGroup loop by doing things directly in the filter method or by using ForGroup
• You could just use TriggerRegisterAnyUnitEventBJ instead of doing it manually.
• GetWidgetLife(filter) > 0.406?
  Shouldn't it be 0.405?
• You could preload the special effect.
• I think you could make other things configurable as well, like the distance the special effect would be created.
• In the periodic method, you don't need to null u since it would become null if the group is empty.
• If the caster doesn't move at all, you could try storing the caster's coordinates instead of having to find them all the time. This could bug if the caster moves, but that shouldn't really be a problem since it would most likely interrupt the order anyway.
• You don't really need to make GroupUtils a requirement but it would be nice to see if you supported that. (It can be easily done with some static-ifs.)

 

[daxtreme]

> Your onDestroy method is pretty much useless since cast will be initialized as being null.

Removing it.

> You could try to remove the FirstOfGroup loop by doing things directly in the filter method or by using ForGroup

I prefer my method of looping since, if I recall correctly, there are less function calls. But I haven't tested though if there are truly less.

> You could just use TriggerRegisterAnyUnitEventBJ instead of doing it manually.

Not really. It's one less function call by doing it my way.

> Shouldn't it be 0.405?

Both lead to the same results, as far as I know.

> You could preload the special effect.

Indeed. If I recall correctly I was preloading it in the previous version. But since I started from scratch, I forgot this time, thank you.

> I think you could make other things configurable as well, like the distance the special effect would be created.

I'll see what I can do! Time should definitely be configurable.

> In the periodic method, you don't need to null u since it would become null if the group is empty.

Will take care of that.

> If the caster doesn't move at all, you could try storing the caster's coordinates instead of having to find them all the time. This could bug if the caster moves, but that shouldn't really be a problem since it would most likely interrupt the order anyway.

True that. Will fix this.

> You don't really need to make GroupUtils a requirement but it would be nice to see if you supported that. (It can be easily done with some static-ifs.)

See below, I think I won't have a choice.

> Stick with radians instead of needlessly switching between them and degrees.

I will modify the code so instead of showing Sin(.ang * bj_DegtoRad) it will be displayed as Sin(.ang * (3.14159 / 180.)) so that's a couple less global variables to seek.

> Use GroupUtils instead of your own global group.

Will try it!

> Time should be configurable.

Indeed.

Alright, so I will work on this. How do I submit it again as "Pending" instead of "Needs fix" when the fixes are done?

 

[watermelon_1234]

I prefer my method of looping since, if I recall correctly, there are less function calls. But I haven't tested though if there are truly less.

Alright, but it's much more inefficient to do it your way. The reason I suggested that is because FirstOfGroup loops are slow.

See below, I think I won't have a choice.

That's kind of the reason why I left that comment; however, it's fine if you decide to use GroupUtils.

Alright, so I will work on this. How do I submit it again as "Pending" instead of "Needs fix" when the fixes are done?

Once you update the spell, I think it will be reset to Pending.

 

[daxtreme]

Updated!

I added many configuration options! I also fixed all the issues I could.

 

[watermelon_1234]

When busterkomo said

Use GroupUtils instead of your own global group.

he was talking about using ENUM_GROUP instead of having your own group variable. Just get rid of the Group member in the struct and use ENUM_GROUP for the group enumeration.

Use radians directly. Instead of 360, replace with Pi*2. Then you don't need to convert to radians when using Cos/Sin

Let me try to explain what I said about the periodic method. You don't need set u = null at the bottom. Since you're using the FirstOfGroup loop, u would already be set to null once the group is empty. Setting it to null afterward is just redundant.

I think you could preload effects with just Preload.

 

[daxtreme]

> he was talking about using ENUM_GROUP instead of having your own group variable.

Will do.

> Use radians directly. Instead of 360, replace with Pi*2. Then you don't need to convert to radians when using Cos/Sin

I could change that in there, but look below in the code.

If you look at it down there in the increment section, my calculations and incremental values for the angles are based off angle values in degrees too, so that means I would need to change my whole formula, since my expanding circle doesn't expand in a normal angled shape, but increments instead.

Doing all that just because of a mere multiplication inside the Sin/Cos value is counterproductive. That is, if my new formula even works to begin with. So, that's why I am converting the degrees into radians inside the Sin/Cos, and working in degrees everywhere else. Keep in mind this impressive "conversion" is actually only a multiplication, as I shortened it up and deleted all references to global variables by using the value directly (.017453278).

> Since you're using the FirstOfGroup loop, u would already be set to null once the group is empty. Setting it to null afterward is just redundant.

Indeed. Removed.

> I think you could preload effects with just Preload .

Silly me and old-style coding.

-------------

Updated again!

EDIT: To clarify the degree/radian thing, please look at this part:

set .ang  = ( .ang  + ( 360. / NB_BOLTS ) - .dist / 6. )

Replacing 360. with pi/2 wouldn't work there, since I am also incrementing my value based on the distance between the current position and the caster. Right now, it scales perfectly with the values in degree. When I converted the whole thing in radians, including that part ( * .017453278) it scaled awkwardly and didn't quite work as intended.

 

[watermelon_1234]

Okay, you made me download your spell just to show that it can be done in radians. (Nice spell by the way. ;D)

I was wrong about only replacing 360 with Pi*2; you also need to convert 6 to radians. So, setting .ang should look like this:

set .ang  =   .ang  +  6.283185 / NB_BOLTS - .dist *0.002909

Just make sure to remove that conversion in Cos/Sin

I'm just obsessing over radians so if you really don't want to change your formula, then don't.

 

[daxtreme]

Thanks, updated!

 

[cpjok]

What if you got a spell with that rawcode already as every jass skill on hive all use A000 i tried 1 thing

change id code in skill to a different one and when pasted skill in object editor set that as the code

but then the map dont work at all

 

[mohamedhnfy]

Nice Spell, Like Nevermore coils in DOTA, it's more awsome