Implosion Bomb v 1.2.0

[Moderator]

12th Dec 2015
IcemanBo: Too long as NeedsFix. Rejected.

BPower 14.1.2014
Aslong as you upload a vJass and a GUI-friendly JASS version, we have to take both into consideration.
For this review the focus lies on the second one.

http://www.hiveworkshop.com/forums/spells-569/implosion-bomb-v-1-2-0-a-235096/index3.html#post2466790

In addition I like to refer to Makers post #37.

Old review

Implosion Bomb v 1.1.0 | Reviewed by Maker | 4th Jun 2013

NEEDS FIX

Trying to reset the caster's prop window with GetUnitDefaultPropWindow results in giving the unit a wrong prop window value The follow through time is way too long and the caster keeps channeling even when the spell is over Get rid of the unneeded degtorad and radtodeg conversions Improve the tooltips, look at default Blizzard abilities for reference Adjust command card positions Add importing intructions into the map file I recommend adding locust to the dummy in object editor and removing upgrades used and unneeded abilities from it

[tr]

Old review Implosion Bomb v1.0.0.6 | Reviewed by Maker | 1st Jun 2013 NEEDS FIX The dummy uses food and upgrades, has build ability, gives vision Learn icon is missing There is no attention to command card positions The tooltips are lacking You could make the ability true channeling ability that the caster can break out from You are calling the polar projection functions too often. As the angle won't change, the sin/cos*angle*bj_RADTODEG is always the same Why is the angle converted away from radians in the first place? [tr]

 

[Maker]

Theres a debug message in the code.

Using GetUnitDefaultPropWindow doesn't reset the propulsion window correctly. You can verify that with debug messages.

 

[deathismyfriend]

ok how do u reset the propulsion window then ? i was told to use the get default one. updated it to wat u suggested in the other post.

 

[Almia]

YAY! First spell from DIMF

Review:
For the vJASS:
1) I see you use a lot of array variables.Best suggestion? Structs.
2)

       else
            if durCounter[ L] < durInt1[ L] then

You know what's the problem here right? ehem..... elseif.

3) The space in arrays makes the code ugly . [ L] vs [L]
4) Combine setup with condition. As what Bribe always say: "Keep it simple".
5)

private function GroupCondition takes nothing returns boolean
    return GetFilterUnit() != caster[ maxIndex] and GetFilterUnit() != UNIT_TYPE_STRUCTURE and /*
    */ GetUnitState( GetFilterUnit(), UNIT_STATE_LIFE) > 0.405 and GetFilterUnit() != dummy[ maxIndex]
endfunction

Cache filter unit into a global(because local leaks).
6)you don't need to null unit u. Booleans are already nulling them(im referring the FirstOfGroup loop)
7)Do the condition(optional) in the FirstOfGroup loop than in GroupEnumUnits blah blah.

8)Make "origin" configurable.
9)Make those special effects models configurable err i mean in the globals.

This is all of what i got. Later the GUI.

Btw, the funniest thing I saw here is the Idle Worker Icon spawning while channeling the spell XD

 

[deathismyfriend]

YAY! First spell from DIMF

Review:
For the vJASS:
1) I see you use a lot of array variables.Best suggestion? Structs.
2)

       else
            if durCounter[ L] < durInt1[ L] then

You know what's the problem here right? ehem..... elseif.

3) The space in arrays makes the code ugly . [ L] vs [L]
4) Combine setup with condition. As what Bribe always say: "Keep it simple".
5)

private function GroupCondition takes nothing returns boolean
    return GetFilterUnit() != caster[ maxIndex] and GetFilterUnit() != UNIT_TYPE_STRUCTURE and /*
    */ GetUnitState( GetFilterUnit(), UNIT_STATE_LIFE) > 0.405 and GetFilterUnit() != dummy[ maxIndex]
endfunction

Cache filter unit into a global(because local leaks).
6)you don't need to null unit u. Booleans are already nulling them(im referring the FirstOfGroup loop)
7)Do the condition(optional) in the FirstOfGroup loop than in GroupEnumUnits blah blah.

8)Make "origin" configurable.
9)Make those special effects models configurable err i mean in the globals.

This is all of what i got. Later the GUI.

Btw, the funniest thing I saw here is the Idle Worker Icon spawning while channeling the spell XD

1) ya i should convert to structs id still use the same variables tho lol.

2) not sure how i did that one.

3) i like it that way its easier to read / write / correct anything.

4) i could do that only reason i do it with the condition function is so it doesnt create the locals. i dont think ill change that.

5) i was told that the local doesnt leak. are u sure it does leak ? my handle counter didnt go up at all.

6) i null the unit u in case the group doesnt run. that way i know its still nulled.

7) umm not sure what u mean lol

8) that makes sense i will do that

9) good point

 

[Almia]

I was wrong. Only Handle Locals XD

As i told you, booleans automatically nulls the handle. exitwhen u == null does the job.

About 7,do the filtering in the loop,not in GroupEnum action,but that is optional.

 

[deathismyfriend]

I was wrong. Only Handle Locals XD

As i told you, booleans automatically nulls the handle. exitwhen u == null does the job.

About 7,do the filtering in the loop,not in GroupEnum action,but that is optional.

ok lol ill change the null thing lol. i didnt even think about that lol.

about 7 i guess i could do it in the group loop it would get rid of a function. and the less efficient GetFilterUnit calls.

edit: updated a lot of the code. especially the vJass. now it uses a struct.

 

[Orthane]

function InitTrig_Implosion_Bomb_Cast takes nothing returns nothing
    local trigger t = CreateTrigger()
    set udg_timerImpBomb = CreateTimer()
    call TriggerRegisterAnyUnitEventBJ( t, EVENT_PLAYER_UNIT_SPELL_CAST)
    call TriggerAddCondition( t, Condition( function ImplosionBombCastConditions))
    set t = null
endfunction

You should know better then to use that TriggerREgisterAnyUnitEventBJ, I have replaced it with the proper way.

Also you should try not making the spell depend on the cast, it should depend on the effect stage.

Here:

function InitTrig_Implosion_Bomb_Cast takes nothing returns nothing
    local trigger t = CreateTrigger()
    local integer index
    set udg_timerImpBomb = CreateTimer()
    set index = 0
    loop
        call TriggerRegisterPlayerUnitEvent(t, Player(index), EVENT_PLAYER_UNIT_SPELL_CAST, null)
        set index = index + 1
        exitwhen index == bj_MAX_PLAYER_SLOTS
    endloop
    call TriggerAddCondition( t, Condition( function ImplosionBombCastConditions))
    set t = null
endfunction

Also I do believe you should be able to remove that entire Trigger Conditon line by adding it to the null location on the new event.

as this:

call TriggerRegisterPlayerUnitEvent(t, Player(index), EVENT_PLAYER_UNIT_SPELL_EFFECT, Filter(function ImplosionBombCastConditions))
//which will allow you to delete this:
call TriggerAddCondition( t, Condition( function ImprosionBombCastConditions))

function ImplosionBombGroupCondition takes nothing returns boolean
    return GetFilterUnit() != udg_casterImpBomb[ udg_maxIndexImpBomb] and GetFilterUnit() != UNIT_TYPE_STRUCTURE and GetUnitState( GetFilterUnit(), UNIT_STATE_LIFE) > 0.405 and GetFilterUnit() != udg_dummyImpBomb[ udg_maxIndexImpBomb]
endfunction
//I do not see you detecting if the victims are going to be Hostile to the caster or not to toss it.

Overall, the trigger is not neat in any way, you are also using very ugly and long variable names. As long as you make a Key Table for the Short Names for people to copy paste and look at when they are editing the spell, you should be allowed to dramaticaly shorten it so it is way easier to read.
You also don't tell some people where the cords are ended up at.


I have several issues with the spell (Preformance wise)

1. You have no way to stop the spell after its cast, Every spell should stop when the caster stops since it is a channeling spell.
2. the spell doesn't check to see if some units are stunned, sleeping or have a way to get removed from the spells effects if they change forces.
3. The spell is ideal to conflict with many other spells.

 

[deathismyfriend]

that BJ function is perfectly fine to use. i dont like putting the condition in the event. Its ugly to me.
The units dont get tossed. Also as i stated this spell is a bomb and there are no friendly free bombs, so thats how i made it.

The long variable names can be shortened by the user in the variable editor. It changes the jass script when changing them. They are long so they dont conflict with any other spells. its GUI cant do much about that lol. if u want short names go to vJass.

1) this spell isnt channeling. The caster gets stuck in the implosion also.
2) as i said its a bomb. Every unit gets hit.
3) how is that ?

 

[Orthane]

that BJ function is perfectly fine to use. i dont like putting the condition in the event. Its ugly to me.
The units dont get tossed. Also as i stated this spell is a bomb and there are no friendly free bombs, so thats how i made it.

The long variable names can be shortened by the user in the variable editor. It changes the jass script when changing them. They are long so they dont conflict with any other spells. its GUI cant do much about that lol. if u want short names go to vJass.

1) this spell isnt channeling. The caster gets stuck in the implosion also.
2) as i said its a bomb. Every unit gets hit.
3) how is that ?

1) I disapprove lol, the spell is set up like a channel spell since he is standing still, also if you don't want it to be a channel spell then allow it to be a cast at a target area with range, like an instant walk away. It would make more sense. Plus if you did that, it would be nice to have something in the center grow then explode lol.

 

[deathismyfriend]

he is standing still but it doesnt mean he is channeling lol

 

[Almia]

The Spell is a Channeling spell.

Review:

• Structs shouldn't have any constants inside it. Move all of the constants in the globals block.
• You should now learn how to create your own alloc bro
• Move the timer to the global.
• I think linked list is good than your indexing. But that is optional
• Make struct private.
• You don't need this.

      private static method polarProjectionX takes real x, real dist, real angle returns real // returns x
          return x + dist * Cos( angle*bj_DEGTORAD)
      endmethod
  
      private static method polarProjectionYtakes real y, real dist, real angle returns real // returns y
          return y + dist * Sin( angle*bj_DEGTORAD)
      endmethod

• Instead of a method,make this a function:
  

      private static method distBetweenXY takes real x1, real y1, real x2, real y2 returns real
          local real x = x2 - x1
          local real y = y2 - y1
          return SquareRoot(x*x + y*y)
      endmethod
  
      private static method getAngleBetweenXY takes real x1, real y1, real x2, real y2 returns real
          return bj_RADTODEG * Atan2( y1 - y2, x1 - x2)
      endmethod

• if GetUnitState( this.target, UNIT_STATE_LIFE) < 0.405 then
  Use GetWidgetLife.
• Attack Types and Damage Types should be at least constants.
• The booleans in UnitDamageTarget should be false.

• globals
      private integer array structIndex
  endglobals

  You don't need this,make this a struct global
• I suggest that you use SpellEffectEvent by Bribe.
• Y u remove this?
  if u != c and u != UNIT_TYPE_STRUCTURE and GetUnitState( u, UNIT_STATE_LIFE) > 0.405 and u != dum then
• CTL? no,its an option.
• local integer abiLvl = GetUnitAbilityLevel( c, GetSpellAbilityId())
  Replace GetSpellAbilityId() with the ability constant you use.

That's all i can see.

@Orthane
RegisterAnyUnitEventBJ is fine. In fact,it is the most useful BJ.

 

[deathismyfriend]

The Spell is a Channeling spell.

Review:
[*]Structs shouldn't have any constants inside it. Move all of the constants in the globals block.
[*]You should now learn how to create your own alloc bro
[*]Move the timer to the global.
[*]I think linked list is good than your indexing. But that is optional
[*]Make struct private.

changed the constants.
I like how the module is already set up. so y index structs by myself when im gonna do it the exact same way ? lol
moved the timer.
i dont think ill use linked list.
struct is now private. wonder how i missed that haha

[*]You don't need this.

    private static method polarProjectionX takes real x, real dist, real angle returns real // returns x
        return x + dist * Cos( angle*bj_DEGTORAD)
    endmethod

    private static method polarProjectionYtakes real y, real dist, real angle returns real // returns y
        return y + dist * Sin( angle*bj_DEGTORAD)
    endmethod

i do need that lol look at the move method

[*] Instead of a method,make this a function:

    private static method distBetweenXY takes real x1, real y1, real x2, real y2 returns real
        local real x = x2 - x1
        local real y = y2 - y1
        return SquareRoot(x*x + y*y)
    endmethod

    private static method getAngleBetweenXY takes real x1, real y1, real x2, real y2 returns real
        return bj_RADTODEG * Atan2( y1 - y2, x1 - x2)
    endmethod

i rather that be a method.

[*] if GetUnitState( this.target, UNIT_STATE_LIFE) < 0.405 then
Use GetWidgetLife.
[*]Attack Types and Damage Types should be at least constants.
[*]The booleans in UnitDamageTarget should be false.

lol i honestly couldnt remember the getwidget life name haha.
changed them to false

[*]

globals
    private integer array structIndex
endglobals

You don't need this,make this a struct global
[*]I suggest that you use SpellEffectEvent by Bribe.
[*]Y u remove this?
if u != c and u != UNIT_TYPE_STRUCTURE and GetUnitState( u, UNIT_STATE_LIFE) > 0.405 and u != dum then
[*]CTL? no,its an option.
[*] local integer abiLvl = GetUnitAbilityLevel( c, GetSpellAbilityId())
Replace GetSpellAbilityId() with the ability constant you use.

i do need that for looping purposes.
id rather not use that system as i dont believe this needs it.
i didnt remove that i just moved it into the group loop like previously stated lol.
CTL ? not sure what u mean
changed that to a constant.

Edit: updated the triggers and the test map

Edit2: fixed bug with effects for the jass / gui triggers.

 

[Almia]

CTL = Constant Timer Loop 32 by Nes.

For the polar methods,i mean,you can just do it directly without creating a method,that's why i said "you don't need this"

 

[deathismyfriend]

ooo lol. i think its good the way it is lol

edit: updated

 

[deathismyfriend]

The dummy uses food and upgrades, has build ability, gives vision
....umm not sure y this matters ? i said to make ur own dummy unit.
Learn icon is missing
....not sure y this matters either ? its a spell. the icon doesnt matter they can make it what they want.
There is no attention to command card positions
....not sure what u mean ?
The tooltips are lacking
....again not sure why this matters ?
You could make the ability true channeling ability that the caster can break out from
....i dont want the ability true channeling. Its a bomb, not a channel ability.
You are calling the polar projectuion functions too often.
As the angle won't change, the sin/cos*angle*bj_RADTODEG is always the same
....this i will change as that makes sense.
Why is the angle converted away from radians in the first place?
....is there a better way to do it ?

 

[Maker]

The dummy uses food and upgrades, has build ability, gives vision
....umm not sure y this matters ? i said to make ur own dummy unit.

Not everyone knows how to make a proper dummy unit. In fact the vast majority don't know that. If a spell uses a dummy unit, it must provide a proper dummy unit for that reason. Spells should be useable and glitchelss without the user having to edit it.

Learn icon is missing
....not sure y this matters either ? its a spell. the icon doesnt matter they can make it what they want.
The tooltips are lacking
....again not sure why this matters ?

Spells are not just triggering/coding. Tooltips and icons are a part of the complete package. Sure people can change them to their liking, but you should still provide a complete spell.

There is no attention to command card positions
....not sure what u mean ?

The learn icon position doesn't match the position of the learned ability. Additionally, the learned icon shows up where no abilities show up for units. By default, the bottom row is used for learned ability icons.

You could make the ability true channeling ability that the caster can break out from
....i dont want the ability true channeling. Its a bomb, not a channel ability.

Then why do you have such long follow through time? The caster keeps the order way too long after casting the ability.

Why is the angle converted away from radians in the first place?
....is there a better way to do it ?

You only need degrees for facings of unit, but you are not setting any facing in that spell. Therefore you should only use radians. In getAngleBetweenXY you convert radians to degrees only to convert the same angle from degrees back to radians in polarProjectionX/Y. You are no using the degree value anywhere.

 

[deathismyfriend]

Not everyone knows how to make a proper dummy unit. In fact the vast majority don't know that. If a spell uses a dummy unit, it must provide a proper dummy unit for that reason. Spells should be useable and glitchelss without the user having to edit it.

ok i already fixed this.

Spells are not just triggering/coding. Tooltips and icons are a part of the complete package. Sure people can change them to their liking, but you should still provide a complete spell.

i fixed this in the update also

The learn icon position doesn't match the position of the learned ability. Additionally, the learned icon shows up where no abilities show up for units. By default, the boom row is used for learned ability icons.

umm not sure y that matters ? or how to change it.

Then why do you have such long follow through time? The caster keeps the order way too long after casting the ability.

i have the follow through time high because i always set it like that. it could be shortened it really doesnt matter either way IMO.

You only need degrees for facings of unit, but you are not setting any facing in that spell. Therefore you should only use radians. In getAngleBetweenXY you convert radians to degrees only to convert the same angle from degrees back to radians in polarProjectionX/Y. You are no using the degree value anywhere.

u said that already but how do i change that ? what are the formulas that u would use so it didnt do that ? thats the way ive always seen angles and polar projections done.

 

[Maker]

1) Art - Button positions
2) Follow through time matter. It sounds to me you don't know what it does. It is the time that the unit keeps looping the cast animation and keeps the ability order. The unit won't auto-attack enemies for example during follow thorugh time, so it does matter a lot.
3) Remove all degtorad/radtodeg conversions.

 

[deathismyfriend]

1) Art - Button positions
2) Follow through time matter. It sounds to me you don't know what it does. It is the time that the unit keeps looping the cast animation and keeps the ability order. The unit won't auto-attack enemies for example during follow thorugh time, so it does matter a lot.
3) Remove all degtorad/radtodeg conversions.

1) ok will fix it.
2) ik what follow through time does. i will change it to the length of the spells.
3) again what are the other formulas without them ????? u keep telling me to remove them. im asking for the formulas

 

[Maker]

So what becomes of these when you remove degtorad and radtodeg conversions?

    private static method getAngleBetweenXY takes real x1, real y1, real x2, real y2 returns real
        return bj_RADTODEG * Atan2( y1 - y2, x1 - x2)
    endmethod

    private static method polarProjectionX takes real x, real dist, real angle returns real // returns x
        return x + dist * Cos( angle*bj_DEGTORAD)
    endmethod

    private static method polarProjectionY takes real y, real dist, real angle returns real // returns y
        return y + dist * Sin( angle*bj_DEGTORAD)
    endmethod

Answer

    private static method getAngleBetweenXY takes real x1, real y1, real x2, real y2 returns real
        return Atan2( y1 - y2, x1 - x2)
    endmethod

    private static method polarProjectionX takes real x, real dist, real angle returns real // returns x
        return x + dist * Cos( angle)
    endmethod

    private static method polarProjectionY takes real y, real dist, real angle returns real // returns y
        return y + dist * Sin( angle)
    endmethod

 

[Almia]

So what becomes of these when you remove degtorad and radtodeg conversions?

    private static method getAngleBetweenXY takes real x1, real y1, real x2, real y2 returns real
        return bj_RADTODEG * Atan2( y1 - y2, x1 - x2)
    endmethod

    private static method polarProjectionX takes real x, real dist, real angle returns real // returns x
        return x + dist * Cos( angle*bj_DEGTORAD)
    endmethod

    private static method polarProjectionY takes real y, real dist, real angle returns real // returns y
        return y + dist * Sin( angle*bj_DEGTORAD)
    endmethod

Answer

    private static method getAngleBetweenXY takes real x1, real y1, real x2, real y2 returns real
        return Atan2( y1 - y2, x1 - x2)
    endmethod

    private static method polarProjectionX takes real x, real dist, real angle returns real // returns x
        return x + dist * Cos( angle)
    endmethod

    private static method polarProjectionY takes real y, real dist, real angle returns real // returns y
        return y + dist * Sin( angle)
    endmethod

Ahaha

 

[deathismyfriend]

updated.

 

[deathismyfriend]

made the changes to the spell.

 

[Maker]

EVENT_PLAYER_UNIT_SPELL_CAST - no
0.00 follow through time - no
GetWidgetLife - no
call SetUnitPropWindow( this.caster, GetUnitDefaultPropWindow( this.caster)) - no

Are damage- and attack types configrable? - no
Is there a hotkey listed? - no

Needs fix

 

[deathismyfriend]

EVENT_PLAYER_UNIT_SPELL_CAST - no
0.00 follow through time - no
GetWidgetLife - no
call SetUnitPropWindow( this.caster, GetUnitDefaultPropWindow( this.caster)) - no

Are damage- and attack types configrable? - no
Is there a hotkey listed? - no

Needs fix

1) ok ill change that
2) y not ? its not a channeling spell.
3) forgot to remove that
4) forgot to change that
5) ok i will make that configurable.
6) y does a hotkey matter ? they can put there own if they want.

 

[deathismyfriend]

Updated with changes as stated above.

 

[Malhorne]

Oh I just saw it you're double privating !

private struct impBombSpell extends array
    implement Alloc
    
    private real aoeTotal
    private unit caster
    private real casterX
    private real casterY
    private real decScale
    private real damToDeal
    private unit dummy
    private integer durCounter
    private integer durInt1
    private integer durInt2
    private effect eff
    private real propWindow
    private real scale
    private unit target
    private real distanceMX
    private real distanceMY
    private real targetMX
    private real targetMY
    private real targetSX
    private real targetSY

Even the methods ^^

 

[deathismyfriend]

Oh I just saw it you're double privating !

private struct impBombSpell extends array
    implement Alloc
    
    private real aoeTotal
    private unit caster
    private real casterX
    private real casterY
    private real decScale
    private real damToDeal
    private unit dummy
    private integer durCounter
    private integer durInt1
    private integer durInt2
    private effect eff
    private real propWindow
    private real scale
    private unit target
    private real distanceMX
    private real distanceMY
    private real targetMX
    private real targetMY
    private real targetSX
    private real targetSY

Even the methods ^^

Lol thanks for that. I made this a long time ago i didn't even think to check for that stuff.

 

[Malhorne]

No problem at all !
Anyway good job there +rep

 

[deathismyfriend]

It actually seems to parse the same way whether or not they are privated. As long as the struct is private everything becomes privated.

 

[Malhorne]

Oh there is no problem with the longer name with double privating :O ?
I wanna see this too xD
*Going back to test*

EDIT : Actually you're right.
But how amazing this can be.
When parsing private members get nothing more in their names.
That means that private struct against regular struct with everything private is slower :O

 

[Maker]

-The vjass version doesn't seem to be working
-Use effect event, not cast
-Worker icon shows up
-No need to check unit_type_id == 0

 

[deathismyfriend]

-The vjass version doesn't seem to be working
-Use effect event, not cast
-Worker icon shows up
-No need to check unit_type_id == 0

Fixed both versions. Had a small bug.
Fixed effect event.
Worker icon not there anymore.
I was told if you remove a unit it still counts that unit as alive unless you use unit type id == 0.

 

[Maker]

I was told if you remove a unit it still counts that unit as alive unless you use unit type id == 0.

Only if you store a group and loop through the units later.

If you instantly pick units and loop through them, there is no need for that check. You can't pick removed units.

 

[deathismyfriend]

Only if you store a group and loop through the units later.

If you instantly pick units and loop through them, there is no need for that check. You can't pick removed units.

Ok i will change it and update. Thanks for that info.

 

[Maker]

-No follow through time
-Lackluster tooltip
-No learn hotkey
-If a unit is close to the caster while the spell is on and I order the caster to move, the caster is moved away from where the spell was cast
-I would make the spell channeled. Now the unit just casts it and it goes on an on and the caster can not move but it can do anyting else
-getAngleBetweenXY should be yx not xy, since you return the angle from y to x
-You could do like this: call SetUnitScale( this.target, this.scale, 0, 0)
The y and z won't make a difference
-Instead of SquareRoot(x*x + y*y) > VELOCITY/2, you could do (x*x + y*y) > VELOCITY*VELOCITY/2. The vel*vel could be precalculated. Just an optimization to avoid sqrt calls
-The dummy should have Locust in object editor, and it does have it. So no need to add it with triggers

 

[deathismyfriend]

-No follow through time
-Lackluster tooltip
-No learn hotkey
-If a unit is close to the caster while the spell is on and I order the caster to move, the caster is moved away from where the spell was cast
-I would make the spell channeled. Now the unit just casts it and it goes on an on and the caster can not move but it can do anyting else
-getAngleBetweenXY should be yx not xy, since you return the angle from y to x
-You could do like this: call SetUnitScale( this.target, this.scale, 0, 0)
The y and z won't make a difference
-Instead of SquareRoot(x*x + y*y) > VELOCITY/2, you could do (x*x + y*y) > VELOCITY*VELOCITY/2. The vel*vel could be precalculated. Just an optimization to avoid sqrt calls
-The dummy should have Locust in object editor, and it does have it. So no need to add it with triggers

As i said above this is a bomb. It will never be channeling. That is entirely my choice.
I don't care about hotkeys click the button.
I'll change the spell so the caster can move after it casts. That makes more sense than a channeled bomb.
I always label it XY. I don't see a reason to change it.
If y and z don't do anything then there is no reason to change it.
As long as it works the same I will change the velocity code. // This adjustment seems to screw up a lot of the calculations so i am leaving the old square root calculations.
I must've forgot to remove from code after i added it in OE.

Will edit soon.

 

[BPower]

As i said above this is a bomb. It will never be channeling. That is entirely my choice.

Follow through time is required to display the animation used for the spell, for instance "attack" or "spell". If you set it to 0 the spell will go off, but the casters animation will immediately end on spell cast. It's just a matter of eyecandy.

I always label it XY. I don't see a reason to change it.

To improve read-ability and because it is logical. You could also name your variables and functions aa, ab, ac, ad, ... with the argument most people use a map optimizer in the end anyways.

Edit: I don't see a reason for the last these lines. I can't imagine a case, where a dummy tries to flee ^^. Use dum if you initialize that local unit.

    set udg_dummyImpBomb[ m] = CreateUnit( GetOwningPlayer( c), udg_dummyUnitImpBomb, x, y, 0)
    set dum = udg_dummyImpBomb[ m]
    call SetUnitPropWindow( dum, 0.00)
    call SetUnitX( udg_dummyImpBomb[ m], x)
    call SetUnitY( udg_dummyImpBomb[ m], y)

- call SetUnitScale( udg_targetImpBomb[ L], udg_scaleImpBomb[ L], udg_scaleImpBomb[ L], udg_scaleImpBomb[ L]) --> units can only be scaled in x-axis. You could simple pass in 1 for y and z.
It doesn't change anything, but reading 1 should be faster than reading the array.
- call DestroyEffect( udg_effectImpBomb[ L]) null that and also this one call RemoveUnit( udg_dummyImpBomb[ L])
- call SetUnitPropWindow( udg_casterImpBomb[ L], GetUnitDefaultPropWindow( udg_casterImpBomb[ L])) This will not reset the PropWindow properly, also the casters prop window is never changed in the first place.
- The tooltips could be improved.

I like the effects of the spell aswell as the concept itself. The caster could perform the full cast animtion and once the bomb is set new units can run through it without getting involved :/.

 

[zv27]

This is totally unnecessary and can be optimized with four simple lines

function DistanceBetweenXYImpBomb takes real x1, real y1, real x2, real y2 returns real
    local real x = x2 - x1
    local real y = y2 - y1
    return SquareRoot(x*x + y*y)
endfunction

function GetAngleBetweenXYImpBomb takes real x1, real y1, real x2, real y2 returns real
    return Atan2( y1 - y2, x1 - x2)
endfunction

function PolarProjectionXImpBomb takes real x, real dist, real angle returns real // returns x
    return x + dist * Cos( angle)
endfunction

function PolarProjectionYImpBomb takes real y, real dist, real angle returns real // returns y
    return y + dist * Sin( angle)
endfunction

But that does not mean it is wrong!

If you already use vJass,then do not use this

TriggerRegisterAnyUnitEventBJ

Again I say, it is not wrong but it looks ugly.

 

[Malhorne]

It is better to inline the maths funcs.
And simplify the dist ^^

ANyway the BJ is fine.

 

[Wrda]

Why when I cast the spell and when units are in the center of the position of the caster, it starts a sound of a peasant hitting someone hard? joke, but you forgot to change attacks enable to none.

 

[Almia]

If you are just naming functions for your library, why not shorter it?

Mag said that the longer the function name is, the slower it is parsed.

 

[Malhorne]

Yes the longer the name is the slower it is and it significantly increase every 4 chars.