View Single Post
Old 11-23-2009, 05:41 AM   #919
Pyrogasm
Lackadaisically Absent.
 
Pyrogasm's Avatar


Respected User
 
Join Date: Sep 2006
Posts: 4,523

Submissions (9)

Pyrogasm is a splendid one to behold (638)Pyrogasm is a splendid one to behold (638)Pyrogasm is a splendid one to behold (638)Pyrogasm is a splendid one to behold (638)Pyrogasm is a splendid one to behold (638)Pyrogasm is a splendid one to behold (638)Pyrogasm is a splendid one to behold (638)

Hero Contest - Fourth place

Send a message via ICQ to Pyrogasm Send a message via AIM to Pyrogasm Send a message via MSN to Pyrogasm Send a message via Yahoo to Pyrogasm
Default

Alright, you've summoned me out of my Calculus-II studying lair to answer this because I do still get notifications from this thread.

First off, most of what you've written looks like badly-optimized GUI-conversion JASS. Mostly because of the abundant use of BJs, but also due to your use of locations, stupid if-statements, and an absence of vJASS.

So, here's where we start (from scratch): We need some structure for this code:
Collapse JASS:
scope BlazeArmor initializer Init
    globals
        //Globals go here
    endglobals


    private function HasItemFilter takes nothing returns boolean
        //This function will filter out the units that have Blaze Armor
    endfunction

    private function DamageFilter takes nothing returns boolean
        //This function filters out the units to be damaged
    endfunction

    private function PeriodicActions takes nothing returns nothing
        //This is the meat of the script; it will deal damage to all units around units with Blaze Armor
    endfunction

    private function Init takes nothing returns nothing
        //This does our initialization stuff
    endfunction
endscope

Whoo! That looks cleaner. Alright, now let's start adding some stuff that actually does what we want:
Collapse JASS:
scope BlazeArmor initializer Init
    globals
        //Globals go here
    endglobals


    private function HasItemFilter takes nothing returns boolean
        return ( UnitHasItemOfTypeBJ(GetFilterUnit(), 'I004') == true )
    endfunction

    private function DamageFilter takes nothing returns boolean
        if ( IsUnitType(GetFilterUnit(), UNIT_TYPE_STRUCTURE) == false ) and ( IsUnitEnemy(GetFilterUnit(), Player(8)) == true ) and ( IsUnitAliveBJ(GetFilterUnit()) == true ) then
            return true
        else
            return false
        endif
    endfunction

    private function PeriodicActions takes nothing returns nothing
        local unit ItemUnit //  is the unit carrying the Blaze Armor
         local unit DamageUnit // is the unit that is taking damage from Blaze Armor
         local group BlazeGroup // This is the group of units carrying Blaze Armor
         local group DamageGroup // This is the group that is getting damaged by Blaze Armor
         local location Point1 // This is the location of SomeUnit2
        set BlazeGroup = GetUnitsInRectMatching(GetPlayableMapRect(), Condition(function BlazeGroupConditions))
        loop
            call RemoveLocation(Point1)
            call DestroyGroup(DamageGroup)
            set ItemUnit = FirstOfGroup(BlazeGroup)
             set Point1 = GetUnitLoc(ItemUnit)
            set DamageGroup = GetUnitsInRangeOfLocMatching(400.00, Point1, Condition(function DamageGroupConditions))
            exitwhen ItemUnit == null
                //Do stuff
                loop
                    set DamageUnit = FirstOfGroup(DamageGroup)
                     exitwhen DamageUnit == null
                    //Do Stuff
                    call UnitDamageTargetEx(ItemUnit, DamageUnit, 50.00, ATTACK_TYPE_HERO, DAMAGE_TYPE_EXTRA, false) // False ignores armor
                    call AddSpecialEffectTargetUnitBJ( "head", DamageUnit, "Abilities\\Spells\\NightElf\\Immolation\\ImmolationDamage.mdl" )
                    call DestroyEffectBJ( GetLastCreatedEffectBJ() )
                    call GroupRemoveUnit(DamageGroup, DamageUnit)
                endloop
            call GroupRemoveUnit(BlazeGroup, ItemUnit)
        endloop
        call RemoveLocation(Point1)
        call DestroyGroup(BlazeGroup)
        call DestroyGroup(DamageGroup)
        set ItemUnit = null
        set DamageUnit = null
        set BlazeGroup = null
        set DamageGroup = null
        set Point1 = null
    endfunction

    private function Init takes nothing returns nothing
        set gg_trg_Blazing_Armor = CreateTrigger(  )
        call TriggerRegisterTimerEventPeriodic( gg_trg_Blazing_Armor, 1.00 )
        call TriggerAddAction( gg_trg_Blazing_Armor, function Blaze_Armor_Actions )
    endfunction
endscope

Alright, now the first two functions are very easy to clean up, so let's do that. We can remove unnecessary parenthesis (because they don't affect the code in this case), replace BJs (for efficiency and clarity reasons), get rid of some boolean comparisons (because making sure something is true is the same thing as making sure true == true), and consolidate our return statement.
Collapse JASS:
    globals
        private constant integer ITEM_ID = 'I004'
    endglobals

    private function HasItemFilter takes nothing returns boolean
        return UnitHasItemOfType(GetFilterUnit(), ITEM_ID)
    endfunction

    private function DamageFilter takes nothing returns boolean
        return not(IsUnitType(GetFilterUnit(), UNIT_TYPE_STRUCTURE)) and IsUnitEnemy(GetFilterUnit(), Player(8)) and IsUnitAliveBJ(GetFilterUnit())
    endfunction
Notice how I took out the Rawcode for the item and made it into a constant, so it's easy to change later on in case you need to use a different rawcode. The only thing that's wonky with this now is the use of GetFilterUnit() and the comparison IsUnitAliveBJ(). The latter function is not reliable for checking to see if a unit is alive; instead, you should check to see that the unit's life is > 0.405. Additionally, we can save some function calls by using a variable for GetFilterUnit() and the output of the function:
Collapse JASS:
    private function DamageFilter takes nothing returns boolean
        local unit F = GetFilterUnit()
        local boolean B = not(IsUnitType(F, UNIT_TYPE_STRUCTURE)) and IsUnitEnemy(F, Player(8)) and GetWidgetLife(F) > 0.405
        set F = null
        return B
    endfunction

Okay, so that's cleared up. Let's look at the main function:
Collapse JASS:
    private function PeriodicActions takes nothing returns nothing
         local unit ItemUnit //  is the unit carrying the Blaze Armor
         local unit DamageUnit // is the unit that is taking damage from Blaze Armor
         local group BlazeGroup // This is the group of units carrying Blaze Armor
         local group DamageGroup // This is the group that is getting damaged by Blaze Armor
         local location Point1 // This is the location of SomeUnit2
        set BlazeGroup = GetUnitsInRectMatching(GetPlayableMapRect(), Condition(function BlazeGroupConditions))
        loop
            call RemoveLocation(Point1)
            call DestroyGroup(DamageGroup)
            set ItemUnit = FirstOfGroup(BlazeGroup)
            set Point1 = GetUnitLoc(ItemUnit)
            set DamageGroup = GetUnitsInRangeOfLocMatching(400.00, Point1, Condition(function DamageGroupConditions))
            exitwhen ItemUnit == null
                //Do stuff
                loop
                    set DamageUnit = FirstOfGroup(DamageGroup)
                     exitwhen DamageUnit == null
                    //Do Stuff
                    call UnitDamageTargetEx(ItemUnit, DamageUnit, 50.00, ATTACK_TYPE_HERO, DAMAGE_TYPE_EXTRA, false) // False ignores armor
                    call AddSpecialEffectTargetUnitBJ( "head", DamageUnit, "Abilities\\Spells\\NightElf\\Immolation\\ImmolationDamage.mdl" )
                    call DestroyEffectBJ( GetLastCreatedEffectBJ() )
                    call GroupRemoveUnit(DamageGroup, DamageUnit)
                endloop
            call GroupRemoveUnit(BlazeGroup, ItemUnit)
        endloop
        call RemoveLocation(Point1)
        call DestroyGroup(BlazeGroup)
        call DestroyGroup(DamageGroup)
        set ItemUnit = null
        set DamageUnit = null
        set BlazeGroup = null
        set DamageGroup = null
        set Point1 = null
    endfunction
Everything red is either bad or a consequence of something that's bad. Fun, eh? Well, here's what's up:
  • JASS is not exactly the most efficient of languages, so improvements are always good. Instead of using/allocating two new groups each iteration, let's just use two global groups that we contain within the scope. This might seem like a useless improvement until you realize that when you use non-BJ GroupEnum functions, the functions don't return a group but instead add units to a previously created group.
  • Once again, we like to improve our code, so we can do that by deprecating locations in favor of x/y coordinates. Most native functions take coordinates anyway, so this goes hand-in hand with removing the BJ calls.
  • In your damage call (what library supplies the EX function anyway, BTW?) and special effects, you can take some of those values out and turn them into constants so that they may be easily modified later.
  • Regarding the special effects, we have a cool trick that involves destroying the effect and creating it in the same line: call DestroyEffect(AddSpecialEffect(...))
So, let's see how I'd write the function:
Collapse JASS:
globals
        private constant integer ITEM_ID = 'I004'
        private constant real DAMAGE = 50.00
        private constant real RANGE = 400.00
        private constant string EFFECT = "Abilities\\Spells\\NightElf\\Immolation\\ImmolationDamage.mdl"
        private constant string EFFECT_ATTACH = "head"

        private group BlazeGroup = CreateGroup()
        private group DamageGroup = CreateGroup()
    endglobals

    private function PeriodicActions takes nothing returns nothing
        local unit ItemUnit //  is the unit carrying the Blaze Armor
        local unit DamageUnit // is the unit that is taking damage from Blaze Armor
        local real X
        local real Y

        call GroupEnumUnitsInRect(BlazeGroup, bj_mapInitialPlayableArea, Condition(function HasItemFilter))

        loop
            set ItemUnit = FirstOfGroup(BlazeGroup)
            exitwhen ItemUnit == null

            set X = GetUnitX(ItemUnit)
            set Y = GetUnitY(ItemUnit)
            call GroupEnumUnitsInRange(DamageGroup, X, Y, RANGE, Condition(function DamageFilter))

            loop
                set DamageUnit = FirstOfGroup(DamageGroup)
                exitwhen DamageUnit == null

                call UnitDamageTargetEx(ItemUnit, DamageUnit, DAMAGE, ATTACK_TYPE_HERO, DAMAGE_TYPE_EXTRA, false) // False ignores armor
                call DestroyEffect(AddSpecialEffectTarget(EFFECT, DamageUnit, EFFECT_ATTACH)
                call GroupRemoveUnit(DamageGroup, DamageUnit)
            endloop

            call GroupClear(DamageGroup)
            call GroupRemoveUnit(BlazeGroup, ItemUnit)
        endloop

        call GroupClear(BlazeGroup)
        set ItemUnit = null
    endfunction

The final thing to fix is the way you're running the main function periodically. There's no reason to do it with a trigger, so let's just use a timer instead. Doing that and putting it all together gives you this, which (barring any extraneous typing errors or my own failure at functions) ought to work just fine:
Collapse JASS:
scope BlazeArmor initializer Init
    globals
        private constant integer ITEM_ID = 'I004'
        private constant real DAMAGE = 50.00
        private constant real RANGE = 400.00
        private constant string EFFECT = "Abilities\\Spells\\NightElf\\Immolation\\ImmolationDamage.mdl"
        private constant string EFFECT_ATTACH = "head"
        private constant real PERIOD = 1.00

        private group BlazeGroup = CreateGroup()
        private group DamageGroup = CreateGroup()
    endglobals


    private function HasItemFilter takes nothing returns boolean
        return UnitHasItemOfType(GetFilterUnit(), ITEM_ID)
    endfunction

    private function DamageFilter takes nothing returns boolean
        local unit F = GetFilterUnit()
        local boolean B = not(IsUnitType(F, UNIT_TYPE_STRUCTURE)) and IsUnitEnemy(F, Player(8)) and GetWidgetLife(F) > 0.405
        set F = null
        return B
    endfunction

    private function PeriodicActions takes nothing returns nothing
        local unit ItemUnit //  is the unit carrying the Blaze Armor
        local unit DamageUnit // is the unit that is taking damage from Blaze Armor
        local real X
        local real Y

        call GroupEnumUnitsInRect(BlazeGroup, bj_mapInitialPlayableArea, Condition(function HasItemFilter))

        loop
            set ItemUnit = FirstOfGroup(BlazeGroup)
            exitwhen ItemUnit == null

            set X = GetUnitX(ItemUnit)
            set Y = GetUnitY(ItemUnit)
            call GroupEnumUnitsInRange(DamageGroup, X, Y, RANGE, Condition(function DamageFilter))

            loop
                set DamageUnit = FirstOfGroup(DamageGroup)
                exitwhen DamageUnit == null

                call UnitDamageTargetEx(ItemUnit, DamageUnit, DAMAGE, ATTACK_TYPE_HERO, DAMAGE_TYPE_EXTRA, false) // False ignores armor
                call DestroyEffect(AddSpecialEffectTarget(EFFECT, DamageUnit, EFFECT_ATTACH)
                call GroupRemoveUnit(DamageGroup, DamageUnit)
            endloop

            call GroupClear(DamageGroup)
            call GroupRemoveUnit(BlazeGroup, ItemUnit)
        endloop

        call GroupClear(BlazeGroup)
        set ItemUnit = null
    endfunction

    private function Init takes nothing returns nothing
        local timer T = CreateTimer()
        call TimerStart(T, PERIOD, true, function PeriodicActions)
    endfunction
endscope

BAM.
__________________
Quote:
Originally posted by Rising_Dusk
Your spells are mostly ignored because they are not very cool so we aren't very excited to review/approve them, but you are incredibly persistent and won't give us an excuse to graveyard it. That is generally what results in a resource being ignored for a long time.

The Spell Request Thread Done for, unless someone else wants to revive it...
It lasted a damn long time.

Please; Ask for Help Appropriately














Quote:
Originally posted by Kyrbi0
Huh. Almost makes me wish I had a girlfriend, to take advantage of today (wait, no, that's not what I meant... I mean, take advantage of the fact that it is international women's day... gah, never mind).
Quote:
Originally posted by Pyrogasm
Rome may not have been built in a day, but the Romans sure as hell didn't say "look at this great city we built guys!" when they had nothing more than a bit of stone and some cottages.
Pyrogasm is offline   Reply With Quote