Code Check

05-30-2009, 02:01 AM

I was wanting to submit a spell but read the rules, and I don't think:

Quote:

If you know/think that your spell/system is not of the best quality, but you are new and would like feedback, you should post it as a thread in the Triggers & Scripts forum instead.

So I don't think my coding is of best quality, so could anyone review?

Hidden information:

JASS:

scope Bomb initializer Init
// Basically makes some bomb at some height with xx seconds before detonation. It drops down and ...

        globals
            // Tree hater? Let this be true! This ability deals 50 damage to destructibles,
            // enough to kill trees, be careful about other destructables such as bridges.
            private constant boolean DESTROYTREES = true
            private constant real    DESTRUCTABLE_DAMAGE = 50.00
            
            // Like a big boom?
            private constant boolean BIGGERBOOMON = false
            //------------------------------------------------------------
            // Edit object rawcodes below here.
            private constant integer OBJECTBOMBID = 'h000'
            private constant integer OBJECTEXID = 'h001'
            private constant integer OBJECTSPELLID = 'A000'
            
            private constant string POOF = "Abilities\\Spells\\Human\\Polymorph\\PolyMorphDoneGround.mdl"
            
            //------------------------------------------------------------
            // Edit spell values below here.
            private constant real SCALE_TIMER_INTERVAL = 0.04   // <-- Timer interval for height and scale stuff.
            
            
            private constant real INITIAL_HEIGHT  = 700.        // <-- How high the bomb is at first.


            private constant real INITIALDISTANCE = 100.00          // <-- How far the bomb is from start location.
            private constant real TEXTTAGSIZE = 10.00 * 0.0023      // <-- (Only modify the 10) How big the texttag is.
            private constant real SECONDS = 5.00                    // <-- How long until detonation. WARNING - 3 to 5 seconds is recommended, 1 will detonate in midair, while 6 - 10 will look weird.
            private constant real AOE = 600.                        // <-- Radius of explosion.
                                                                    // Keep in mind the model size.
            private constant string TEXTTAGCOLOR = "|cffff0000"
            //------------------------------------------------------------
            // Edit the effect in when targets are affected by bomb.


            private constant string SFX = "Abilities\\Spells\\Other\\Incinerate\\FireLordDeathExplode.mdl"
            private constant string SFXPOINT = "origin"
            
            //------------------------------------------------------------
            // Speed slower, size adder, and size of big boom!
            private constant real SCALEADDER = 0.045// <-- Every SCALE_TIMER_INTERVAL, the Bomb increases size by <--
            private constant real BIGBOOM = 35      // <-- Size of the bomb when it bigbooms!

            //------------------------------------------------------------
            // Damage type, attack type, and weapon type.
            private constant attacktype ATKTYP = ATTACK_TYPE_NORMAL
            private constant damagetype DMGTYP = DAMAGE_TYPE_MAGIC
            private constant weapontype WPNTYP = null
            //------------------------------------------------------------
            // Can be left as is.        
            private player PPP    
            private group GGG = CreateGroup()
        endglobals
        
//--------------------------------------------------------------
// GetDamage returns Level * 75 damage... 75/150/225/300/375/450       
    private function GetDamage takes integer Level returns real
        return Level * 75.00
    endfunction

//------------------------------------------------------------
//                    Spells begins here.
//------------------------------------------------------------
    private function Conditions takes nothing returns boolean
        return GetSpellAbilityId() == OBJECTSPELLID
    endfunction

    private function UFilter takes nothing returns boolean
        return IsUnitType( GetFilterUnit(), UNIT_TYPE_DEAD ) == false and IsUnitEnemy( GetFilterUnit(), PPP )
    endfunction
    
    private function DTrees takes nothing returns nothing
        // call KillDestructable( GetEnumDestructable )
        // Kill or set life!
        call SetWidgetLife(GetEnumDestructable(), GetWidgetLife(GetEnumDestructable()) - DESTRUCTABLE_DAMAGE)
    endfunction
    
    private function True takes nothing returns boolean
        return true
    endfunction
    
    //------------------------------------------------------------
    
    private function Parabola takes real acceleration, real velocity, real startheight, real time returns real
        return acceleration*(time*time)+velocity*time+startheight
    endfunction

    private struct BombData
        boolean BounceOn = false    // <-- Bounce 1.
        boolean Bounce2 = false     // <-- Bounce 2.
    
        timer Countdown
        timer BiggerX
        unit Caster
        unit Bomb
        
        real Scale
        real bombx
        real bomby
        
        integer Level
        
        texttag Boomer
        
        player Owner            // <-- Owner of caster.
        real Seconds = SECONDS  // <-- timer counting down in 1 second interval
        real TTT     = SECONDS  // <-- timer counting down
        real rvTTT = 0.00       // <-- reverse timer counting up.
        // The onBoom method actually.
        private method onDestroy takes nothing returns nothing
            local real x = GetUnitX( .Bomb )
            local real y = GetUnitY( .Bomb )
            local unit u
            local destructable T

            debug local integer i = 0
            debug local real ang = 0
            debug local real angDiff = 360 / 25
            //------------------------------------------------------------
            // Removes the bomb, texttag, and creates a boom effect.   
            
            debug loop
            debug    exitwhen i == 25
            debug    call DestroyEffect( AddSpecialEffectLoc( POOF, PolarProjectionBJ( GetUnitLoc( .Bomb ), 600, ang ) ) )
            debug    set ang = ang + angDiff
            debug    set i = i + 1
            debug endloop
            
            if BIGGERBOOMON then
                call KillUnit( .Bomb )
                call SetUnitScale( .Bomb, BIGBOOM, BIGBOOM, BIGBOOM )
            else
                call RemoveUnit( .Bomb )
            
                set u = CreateUnit( .Owner, OBJECTEXID, x, y, 0.00 )
                call SetUnitPathing( u, false )
                call SetUnitPosition( u, x, y )
                call SetUnitAnimation( u, "birth" )
                call SetUnitTimeScale( u, .5 )
                call UnitApplyTimedLife( u, 'BTLF', 1.50 )
            endif
                
            call DestroyTextTag( .Boomer )


            //------------------------------------------------------------
            // Damages the enemies upon boom.
            set PPP = .Owner
            call GroupEnumUnitsInRange( GGG, x, y, AOE, Condition( function UFilter ) )
            loop
                set u = FirstOfGroup( GGG )
                exitwhen u == null
                call GroupRemoveUnit( GGG, u )
                call DestroyEffect( AddSpecialEffectTarget( SFX, u, SFXPOINT ) )
                call UnitDamageTarget( .Caster, u, GetDamage( .Level ), true, false, ATKTYP, DMGTYP, WPNTYP )
            endloop
            
            if DESTROYTREES then
                call EnumDestructablesInRect( Rect( x - AOE, y - AOE, x + AOE, y + AOE ), Condition( function True ), function DTrees )
            endif
            //------------------------------------------------------------
            // Leak-free.
            call ReleaseTimer( .Countdown )
            call ReleaseTimer( .BiggerX )

        endmethod
    endstruct
    


    private function TimerCallback takes nothing returns nothing
        local timer t = GetExpiredTimer()
        local sound Tick = CreateSoundFromLabel( "ChatroomTimerTick", false, true, true, 20, 20 )
        local BombData x = GetTimerData( t ) //GetTimerStructA( t )

        // STUPID NON-3D sound!! Why was it build 2-D only.
        
        call SetSoundPosition( Tick, GetUnitX( x.Bomb ), GetUnitY( x.Bomb ), 0.00 )
        call StartSound( Tick )
        call KillSoundWhenDone( Tick )
        
        set x.Seconds = x.Seconds - 1.00
        if x.Seconds <= 0 then
            call x.destroy()
        endif
        call SetTextTagText( x.Boomer, TEXTTAGCOLOR + I2S( R2I( x.Seconds ) ) + "!|r", TEXTTAGSIZE )
        set t = null
        set Tick = null
    endfunction
    
    private function BiggerBoom takes nothing returns nothing
        local BombData x = GetTimerData( GetExpiredTimer() )
        
        local real d
        local real bombHeight = GetUnitFlyHeight( x.Bomb )
        set x.Scale = x.Scale + SCALEADDER
    
        
        set x.TTT = x.TTT - SCALE_TIMER_INTERVAL
        set x.rvTTT = x.rvTTT + SCALE_TIMER_INTERVAL
        
        if ( SECONDS - 0.6 ) <= x.TTT then
            call DestroyEffect( AddSpecialEffectZ( POOF, x.bombx, x.bomby, GetUnitFlyHeight( x.Bomb ) ) )
        endif
        
        
        if x.BounceOn == false then
            set d = Parabola( -25, 140, INITIAL_HEIGHT, ( x.rvTTT * 3 ) )
            debug call BJDebugMsg( "BounceOn == false" + R2S( d ) )
            call SetUnitFlyHeight( x.Bomb, d, 0 )
        elseif x.BounceOn and x.Bounce2 == false then
            set d = Parabola( -25, 140, 0, ( x.rvTTT * 3 ) )
            debug call BJDebugMsg( "BounceOn, mini-bounce" + R2S( d ) )
            call SetUnitFlyHeight( x.Bomb, d, 0 )
            if d <= 10.00 then
                set x.rvTTT = 0.00
                set x.Bounce2 = true
                debug call BJDebugMsg( "Bounce2 on now betch!" )
            endif
        endif
        
        if x.Bounce2 then
            set d = Parabola( -24, 100, 1, ( x.rvTTT * 2.23 ) )
            debug call BJDebugMsg( "Bounce2 on " + R2S( d ) )
            call SetUnitFlyHeight( x.Bomb, d, 0 )            
        endif
        
        if GetUnitFlyHeight( x.Bomb ) <= 20 and x.BounceOn == false then
            set x.rvTTT = 0.00
            set x.BounceOn = true
        endif
        
        call SetTextTagPos( x.Boomer, x.bombx, x.bomby, bombHeight )
        call SetUnitScale( x.Bomb, x.Scale, x.Scale, x.Scale )
    endfunction
    
    private function Actions takes nothing returns nothing
        local BombData x = BombData.create()
        local location TLoc = GetSpellTargetLoc()
        local real xx = GetLocationX( TLoc )
        local real y = GetLocationY( TLoc )

        set x.Countdown = NewTimer()
        set x.BiggerX = NewTimer()
        
        set x.Scale = 1.00

        set x.Caster = GetTriggerUnit()
        set x.Owner = GetOwningPlayer( x.Caster )
        
        set x.Level = GetUnitAbilityLevel( x.Caster, OBJECTSPELLID )
        
        
        // Create the bomb!!
        set x.Bomb = CreateUnit( x.Owner, OBJECTBOMBID, xx, y, bj_UNIT_FACING )
        // No, not useless.
        call SetUnitPosition( x.Bomb, xx, y )
        call DestroyEffect( AddSpecialEffectZ( POOF, xx, y, INITIAL_HEIGHT ) )
        
        call SetUnitFlyHeight( x.Bomb, INITIAL_HEIGHT, 0 )
        
        set x.bombx = xx
        set x.bomby = y

        // Create the texttag!!
        set x.Boomer = CreateTextTag()
        call SetTextTagText( x.Boomer, TEXTTAGCOLOR + I2S( R2I( SECONDS ) ) + "!|r", TEXTTAGSIZE )
        call SetTextTagPos( x.Boomer, xx, y, INITIAL_HEIGHT )
        
        // Start timer!!
        call TimerStart( x.Countdown, 1.00, true, function TimerCallback )
        call SetTimerData( x.Countdown, x )
        
        call TimerStart( x.BiggerX, SCALE_TIMER_INTERVAL, true, function BiggerBoom )
        call SetTimerData( x.BiggerX, x )
        
        call RemoveLocation( TLoc )
        set TLoc = null
    endfunction

    //===========================================================================
    private function Init takes nothing returns nothing
        local trigger t = CreateTrigger()
        call TriggerRegisterAnyUnitEventBJ( t, EVENT_PLAYER_UNIT_SPELL_EFFECT )
        call TriggerAddCondition( t, Condition( function Conditions ) )
        call TriggerAddAction( t, function Actions )
        set t = null
    endfunction

endscope

OMG! Here goes "Submit New Thread".

Attached Files

(Spell) Bomb 06 - 26th May 2009.w3x (45.8 KB)

05-30-2009, 02:26 AM

cosmicat

Since I didn't post in your "Hiiiiii" thread, I'll welcome you to wc3c here.

It would help if you would upload the map and/or comment object IDs more verbosely. At the moment, I haven't got a clue what OBJECTEXID ('h001') is supposed to be.

Okay, that was kind of a lie - from reading the code it's clear that it's the object ID of a unit used to produce an "explosion" effect (hence "EX"). However, it's generally a good idea to explain things as clearly as possible - and attach a demo map.

05-30-2009, 03:02 AM	#3
BlackRose	Thanks for the welcome. Yes, EX = Explosion. Map attached in first post.

05-30-2009, 12:37 PM	#4
fX_	uses TimerUtils?

05-30-2009, 12:47 PM	#5
Silvenon	You should state somewhere that you're using AddSpecialEffectZ, or maybe you did, but I don't see where. The code looks fine to me.

05-31-2009, 07:47 AM	#6
BlackRose	I removed the documentation on the post above, in the map (If you opened it it has documenting). It really looks fine? At least I get feedback here, rather than another forum I could say, which gives ZERO feedback at all.

05-31-2009, 09:33 AM

Silvenon

Quote:

It really looks fine?

Yeah, if it works go ahead and post it as a script.

Well, I tend to put every external function as a struct method, but that's really a matter of style, not speed. Nice coding, btw :D

06-01-2009, 12:12 AM

Anitarf

It seems like an awfull lot of code for what seems like a simple spell, I could be wrong though.

It mostly seems ok on the bug/leak part, but I ran out of attention at some point so I didn't review it thoroughly. I mostly only have complaints about style.

For example, why use x for the local variable of bomb type? It gets really confusing when you have x and y coordinates on it and you then need to use xx for one of them. Wouldn't it have been a lot simpler to just name the first variable bomb or something?

Second, you make use of the onDestroy method, but not the create method. It would help encapsulation and organisation if you did.

Is it really neccessary to use the CreateEffectZ function, couldn't you just create the effect on the bomb unit instead?

Reading long functions with little comments can be very tedious. It's common courtesy to comment the code at least a bit when submitting it for critique, it's also common sense to comment code in general because that helps you maintain it.

Why do so many people use those parabola equations that I find really difficult to read instead of the intuitive physical approach of position, velocity and acceleration? Instead of having a different parabola for every bounce (hardcoded, no less), you could have a set of configurable physical constants and code using them that would be the same no matter if the bomb was bouncing for the first or the fifth time.

You also really should write a better description of exactly what the spell does, every calibration option should have an explanation of how it fits into the whole spell and you definitely must list what external libraries your code requires.

06-01-2009, 06:44 AM

BlackRose

Quote:

It seems like an awfull lot of code for what seems like a simple spell, I could be wrong though.

It mostly seems ok on the bug/leak part, but I ran out of attention at some point so I didn't review it thoroughly. I mostly only have complaints about style.

> For example, why use x for the local variable of bomb type? It gets really confusing when you have x and y coordinates on it and you then need to use xx for one of them. Wouldn't it have been a lot simpler to just name the first variable bomb or something?
That is true, I'll rename it.

Second, you make use of the onDestroy method, but not the create method. It would help encapsulation and organisation if you did.
Ok.

Is it really neccessary to use the CreateEffectZ function, couldn't you just create the effect on the bomb unit instead?
Does this model actually have an attachment point, though I could I use dummy model.

Reading long functions with little comments can be very tedious. It's common courtesy to comment the code at least a bit when submitting it for critique, it's also common sense to comment code in general because that helps you maintain it.

Why do so many people use those parabola equations that I find really difficult to read instead of the intuitive physical approach of position, velocity and acceleration? Instead of having a different parabola for every bounce (hardcoded, no less), you could have a set of configurable physical constants and code using them that would be the same no matter if the bomb was bouncing for the first or the fifth time.
We're terrible at maths.

You also really should write a better description of exactly what the spell does, every calibration option should have an explanation of how it fits into the whole spell and you definitely must list what external libraries your code requires.

Thanks for your feedback (My reply is in the quote)

06-01-2009, 08:36 PM	#10
General Ray	JASS: private function True takes nothing returns boolean return true endfunction ? May I ask exactly what the point to this is? So many worthless functions and variables.

06-01-2009, 08:50 PM

#11

ploks

Quote:

Originally Posted by General Ray

JASS:

    private function True takes nothing returns boolean
        return true
    endfunction

?

May I ask exactly what the point to this is? So many worthless functions and variables.

Enumerations requires a filter. That function is a filter for the picked destructibles, the effect is that all destructibles inside the radius will be picked.

JASS:

call EnumDestructablesInRect( Rect( x - AOE, y - AOE, x + AOE, y + AOE ), Condition( function True ), function DTrees )

06-01-2009, 11:11 PM	#12
Silvenon	Yeah, I never got that part too. Couldn't you just put null?

06-01-2009, 11:39 PM	#13
azlier	Null boolexprs love to leak in enumerations.

06-02-2009, 11:16 AM

#14

Silvenon

Quote:

Originally Posted by azlier

Null boolexprs love to leak in enumerations.

Really? I didn't know that... Great, tnx.