Moderator
M
Moderator
11th June 2012
The Reborn Devil:
The code is okay now, but I still feel the
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 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:
There's also another issue I didn't spot before. Your effect loop could potentially run indefinitely if this condition is not met:
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:
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:
Do you really need these to be globals:
?
Also, this line:
Last, but not least:
..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.
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
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
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:
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.
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.