Lightning v1.5

Moderator · May 21, 2012

11th June 2012

The Reborn Devil:

The code is okay now, but I still feel the if d > 0 then should be moved or just removed entirely. This is because it's actually redundant in its current position, due to the exit condition in the loop. Anyway, it meets the criteria, so it's approved.

Status: Approved
Rating: Useful

9th June 2012

The Reborn Devil:

It seems most issues have been dealt with, although I'm still stumped by this: if d > 0 then
If the AOE being less than 0 really is a possibility wouldn't it be better to have that check further up? Not too far up you have this line: call GroupEnumUnitsInRange(g, x, y, d, null) which wouldn't work afaik if the AOE (the variable d) is less than 0.

There's also another issue I didn't spot before. Your effect loop could potentially run indefinitely if this condition is not met: d == 0 which could easily happen if the AOE is not divisible by 10 (which is the amount you decrement it by). Simple example would be if d is 5. Decrementing it by 10 would result in -5, which is not 0. Changing the exit condition to d < 0 would easily solve that.

You also forgot to null "target", which is necessary now that it's a local.

8th June 2012

The Reborn Devil:

The first I notice is that the names of these constants could be better:

JASS:

private constant integer    A_CODE
private constant attacktype A_TYPE
private constant damagetype D_TYPE

At first glance it may seem that A_CODE and A_TYPE are somehow connected, while they're not. Having understandable names, especially for constants is key.

You may want to move some of the functions around a bit to help people find what they need to easily customize things. Something like this:

JASS:

    globals
        private constant real       DAMAGE            = 50 //the damage dealt (you can set this)
        private constant real       AREA_OF_EFFECT    = 250 //how large is the lightning AoE(you can set this)
        private constant string     SFX               = "Abilities\\Spells\\Other\\Monsoon\\MonsoonBoltTarget.mdl" //the SFX model path
        private constant integer    A_CODE            = 'A001' //the ability ID
        private constant attacktype A_TYPE            = ATTACK_TYPE_NORMAL
        private constant damagetype D_TYPE            = DAMAGE_TYPE_NORMAL
    endglobals

    private constant function areaEffect takes integer level returns real
        return AREA_OF_EFFECT*level
    endfunction

    private constant function damageInflict takes integer level returns real
        return DAMAGE*level
    endfunction

    private constant function GetFilter takes unit caster, unit u returns boolean
        return IsUnitEnemy((u),GetOwningPlayer(caster))/* 
                */and UnitAlive(u)/* 
                */and not IsUnitType((u), UNIT_TYPE_STRUCTURE)/* 
                */and not IsUnitType((u), UNIT_TYPE_MAGIC_IMMUNE)
    endfunction

    //The rest of the spell continues from here

Do you really need these to be globals:

JASS:

private unit caster  
private unit target 
private real d
private real damage

?

Also, this line: set angle = 0 which is near the end of "lightningActions" is unnecessary.

Last, but not least:

JASS:

if d >=d  then
    <code>
endif

..Wat? Maybe I'm incredibly stupid, but seems to me that this if-then does absolutely nothing. The variable "d" is of course equal to itself.

Other than that it seems the issues Bribe pointed out have been solved, and as soon as you've fixed the points I've listed, it should be ready for approval.

26 May 2012
Bribe: You are nulling the locals now but for some reason you aren't removing them, so you have a handle leak x2 in that function now.

You should be using coordinates instead of locations if you're using JASS, and also should be using a global group instead of a dynamic group for this.

21st May 2012
Bribe: Please analyze the highlighted changes I suggest for your spell:

JASS:

//////////////////////////////////////////////////////////////////////////////
//                    LIGHTNING 1.1                                         //
//                                                                          //
//This is a very simple spell but could be useful for maps.                 //
//What you will just do is to copy all these codes into your map and then   //
//create an ability for this. I recommend you use Acid Bomb as base ability //
//==========================================================================//
/////////////////////////////////////////////////////////////////////////////
library LightningSpell@ initializer Init@
    @native UnitAlive takes unit id returns boolean@
//==========================================================
    @private @function Conditions takes nothing returns boolean
        return  GetSpellAbilityId() == 'A001'
    endfunction
//==========================================================
    globals
//THE GLOBALS BELOW CAN BE CONFIGURED
        private real dam = 50 //the damage dealt (you can set this)
        private real AoE = 250 //how large is the lightning AoE(you can set this)
        private string o = "origin" //you might want to configure the attachment point
        private string s = "Abilities\\Spells\\Other\\Monsoon\\MonsoonBoltTarget.mdl" //the SFX model path
        private  integer a_code = 'A001' //the ability ID
//END OF CONFIGURATIONS
        private  unit caster
        private  unit target
        private  location loc
        private  real d
        private  real damage
    endglobals
//==============================================================
//Don't touch anything below
@private @function Actions takes nothing returns nothing
    local real angle = 0
    local group g = CreateGroup()
    local unit u
    local real x=GetSpellTargetX()
    local real y=GetSpellTargetY()
    set caster = GetTriggerUnit()
    set target = GetSpellTargetUnit()
    set loc = GetUnitLoc(target)
    set d = AoE*GetUnitAbilityLevel(caster,a_code)
    set damage = dam*GetUnitAbilityLevel(caster,a_code)
//==============================================================
//The damage part
    call GroupEnumUnitsInRangeOfLoc(g,loc,d,null)

        loop
            set u = FirstOfGroup(g)
            exitwhen u == null
            if IsUnitEnemy(u,GetOwningPlayer(caster)) and @UnitAlive(u)@ and @not@ IsUnitType(u, UNIT_TYPE_STRUCTURE) and @not@ IsUnitType(u, UNIT_TYPE_MAGIC_IMMUNE) then
                call UnitDamageTarget(caster,u,damage,true,false,ATTACK_TYPE_NORMAL,DAMAGE_TYPE_NORMAL,null)
            endif
                call GroupRemoveUnit(g, u)
        endloop
//==============================================================
//For some SFX---------------
        if d >=d then
            loop
                set d = d - 10
                set angle = angle + 50
            exitwhen d == 0
                call DestroyEffect(AddSpecialEffect(s,x+d*Cos(angle*bj_DEGTORAD),y+d*Sin(angle*bj_DEGTORAD)))
            endloop
                set angle = 0
        endif
        call RemoveLocation(loc)
        @set loc = null@
        call DestroyGroup(g)
        @set g = null@
    endfunction
//===========================================================================
    @private @function Init takes nothing returns nothing
        local trigger L
        set L = CreateTrigger( )
        call TriggerRegisterAnyUnitEventBJ( L, EVENT_PLAYER_UNIT_SPELL_EFFECT )
        call TriggerAddCondition( L, Condition( function Lightnings_Conditions ) )
        call TriggerAddAction( L, function Lightning_Actions )
    endfunction
endlibrary
//====================================================================
//Code by eubz//

Even still, this is not exactly ready for approval. Creation/destruction of groups where unnecessary, creation/destruction of locations where unnecessary, in these cases, respectively, use a global group and use coordinates.

eubz · May 21, 2012

I wonder if I have to still remove the location here.

Almia · May 21, 2012

Nice Work sir Eubs(of the Phils)ill give you sir 5/5 because of the effects

maddeem · May 21, 2012

Get rid of the BJs!

JASS:

call DestroyEffect(AddSpecialEffectLocBJ( PolarProjectionBJ(loc, d, angle), s )

I see what you did there, didn't know that was possible. Very neat

Its very refreshing to see a plain o'l jass spell, fantastic work

Luorax · May 21, 2012

eubz said:
I wonder if I have to still remove the location here.

Yes, you have; locations are bad in general, especially combined with a polar projection call (which is one of the worst BJ's I know about; there's a reason why BJ's were named after "Bad Job" [/jk])

JASS:

local real x=GetSpellTargetX()
local real y=GetSpellTargetY()
...

call DestroyEffect(AddSpecialEffect(s,x+d*Cos(angle*bj_DEGTORAD),y+d*Sin(angle*bj_DEGTORAD)))

sonofjay · May 21, 2012

Nice effects, and you're owning every possible modding(modelling, Icons, mapping, spell making,) :psmile:

Adiktuz · May 21, 2012

it uses globals though so its not plain jass... XD...

maybe you should just combine the conditions and actions and just put the spell id as a global so that the user can still change it without going deep into the code...

and I would recommend channel as a base ability as that spell was made for triggered spells...

also, you have a group leak... it would be better to just use one global group instead of creating a local one...

and for better configuration, the filter could be made into a separate function placed at the top of the other codes so the user can easily modify the units that can be hit by this...

the damage formula and the AoE formula should also be placed as a separate function for better configurability...

and the configurable globals should come before the non-configurable ones...

eubz · May 21, 2012

Thanks for the comments.
EDIT:
I will update this ASAP.

Edit:
Thanks to Lourax for providing me with a simple code that gets rid of BJs.
Spell Updated.

EDIT:
Spell Updated.

iAyanami · May 27, 2012

JASS:
native UnitAlive takes unit id returns boolean
The native UnitAlive breaks map optimizer, from what I heard. I believe using UNIT_TYPE_DEAD would be fine.

You don't need the Action function. Simply merge it with the Condition. Also, I recommend you to take a look at this for spell cast event.

JASS:

    globals
        private constant real dam = 50 //the damage dealt (you can set this)
        private constant real AoE = 250 //how large is the lightning AoE(you can set this)
        private constant string o = "origin" //you might want to configure the attachment point
        private constant string s = "Abilities\\Spells\\Other\\Monsoon\\MonsoonBoltTarget.mdl" //the SFX model path
        private constant integer a_code = 'A001' //the ability ID
        private constant attacktype a_type = ATTACK_TYPE_NORMAL
        private constant damagetype d_type = DAMAGE_TYPE_NORMAL
    endglobals

Constant variables should be in CAPITAL_CASE. Just naming conventions for better readability.

Your spell configuration is kind of awkward. Perhaps this would be better.
JASS:
private constant function GetDamage takes integer level returns real
    return 50. level
endfunction

You could use just local variables for this spells. The globals aren't really needed here. However, you do still need the global group for enumeration. However, you don't need to create a group. You can simply do:
JASS:
private group G = bj_lastCreatedGroup

Your filter function should be configurable as well. What if I wanted the lightnings to hit magic immune enemies as well? Something like this:
JASS:
private constant function GetFilter takes unit source, unit u returns boolean
    return IsUnitEnemy((u),GetOwningPlayer(source)) /*
    */ and UnitAlive(u) /*
    */ and not IsUnitType((u), UNIT_TYPE_STRUCTURE) /*
    */ and not IsUnitType((u), UNIT_TYPE_MAGIC_IMMUNE)
endfunction

Adiktuz · May 27, 2012

you don't need the group variable, you can use bj_lastCreatedGroup directly... XD

iAyanami · May 27, 2012

Adiktuz said:
you don't need the group variable, you can use bj_lastCreatedGroup directly... XD

You could do that as well. Though I like putting it inside a variable

eubz · May 28, 2012

Thanks to you guys.

xyzier_24 · Jun 2, 2012

NICE WORK eubz !
i'll rate this 5 of 5 for the cool skill ...

eubz · Jun 7, 2012

thanks.
EDIT:
Spell updated.
EDIT:
Spell updated to version 1.5

zv27 · Jun 11, 2012

This is actually vjass category not jass so...hmmm

I noticed that a lot of vJASS spells is in JASS category,well, that's wrong!
Many times I have reviewed both categories,mann there is no order,everything is mixed up.
Maybe some people here think that vjass and jass one and the same,but simply not,the differences are obvious.

I think the moderators should pay attention to such things

KhaosMachine · Jun 14, 2012

Simple but nice!
Change this:

JASS:

private constant real       AREA_OF_EFFECT    = 250 //how large is the lightning AoE(you can set this)

with this:

JASS:

private function areaOfEffect takes integer level returns real
    return 250.
endfunction

This make the spell much better!

MORE CONFIGURABLE = MORE USEFUL

iAyanami · Jun 14, 2012

KhaosMachine said:
Simple but nice!
Change this:

JASS:

private constant real AREA_OF_EFFECT = 250 //how large is the lightning AoE(you can set this)

with this:

JASS:

private function areaOfEffect takes integer level returns real return 250. endfunction

This make the spell much better!

MORE CONFIGURABLE = MORE USEFUL

private constant function would be better.

eubz · Jun 17, 2012

really true.

Soul_EaterS · Jul 16, 2012

can you remake this to Mui or Gui

maddeem · Jul 20, 2012

Soul_EaterS said:
can you remake this to Mui or Gui

This IS Mui, but why would someone downgrade their spell to GUI?

Lightning v1.5

Lightning v1.5 (Map)

Moderator

Moderator

Similar threads