Doom Fuse

[Pyrogasm]

It's 2019 and vJASS has now been fully integrated into the editor for multiple large patches. I don't think it's too much to ask to take advantage of the simplest features:

• Encapsulate your spell in a library/scope
• Use globals blocks instead of relying on a variable creation trigger and GUI variables
• Use an initializer instead of the default InitTrig_ function, which will break if the trigger is renamed.

 

[JC Helas]

It's 2019 and vJASS has now been fully integrated into the editor for multiple large patches. I don't think it's too much to ask to take advantage of the simplest features:

• Encapsulate your spell in a library/scope
• Use globals blocks instead of relying on a variable creation trigger and GUI variables
• Use an initializer instead of the default InitTrig_ function, which will break if the trigger is renamed.

Thanks for your wonderful opinion bro but i have to stay on this lvl,, we all starts from gui and I dont think no one has left behind every month or year may have new editor I hope you get what I mean :V,, about the initTrig i guess you have not notice on how it works,, well if ever they change the triggers' name at folder automatically it will change so XD why not try it? Witness how the function change its self.. XD

 

[Pyrogasm]

There's literally no reason no to use the most basic and simple features that improve accessibility and ease of use for everyone. You're actively making it harder for anyone who wishes to use your resource, and yourself during development. Anyone who doesn't have access to a 1.29+ editor either pirated the game and can't update (ffs it's like $15) or is willingly running an old patch, and in that old patch either WEX or JNGP will still work.

Witness how the function change its self.

It never used to do this, it's a new addition to the editor I was unaware of. Thank you.

 

[JC Helas]

There's literally no reason no to use the most basic and simple features that improve accessibility and ease of use for everyone. You're actively making it harder for anyone who wishes to use your resource, and yourself during development. Anyone who doesn't have access to a 1.29+ editor either pirated the game and can't update (ffs it's like $15) or is willingly running an old patch, and in that old patch either WEX or JNGP will still work.

It never used to do this, it's a new addition to the editor I was unaware of. Thank you.

Its okay bro I rely to the power of "COPY" "PASTE" i guess im not alone XD even how long that perfix are XD

 

[MyPad]

It was once said that those who dare wander at night will behold an enigmatic and predatory luminescence. It was thought to be a spirit, an impish pixie, an impudent fairy, phantom, or even an alien sighting. As this eerie presence grips its' audience, a woeful song can be heard; the wailing of maidens gone mad, an ear-piercing howl, despair made manifest. Woe to those who wander to these lights, for none shall grant them rest, a requiem that can never know its' end.

Now, onto the review: (vJASS)

• In struct DoomFuse, there is no need to inline TriggerRegisterAnyUnitEventBJ, since it is only going to be called once.
  
  
• Local variables do not appear to be in camelCase. (This is somewhat minor, so feel free to ignore this)
  
  
• Having SpellEffectEvent and RegisterPlayerUnitEvent appears to be overkill for this spell. (Good thing they're optional).
  
  
• IsUnitType(unit, UNIT_TYPE_DEAD) can and should be replaced with not UnitAlive(unit).
  
  
• Instead of using raw numbers to indicate wave states, you could use some global variables that will clearly describe the wave state, instead of relying on presumptions made on your end on what each wave state actually is. (This is to aid development down the line when a user wants to modify the spell.)

It appears that a lot of work will have to be done before approval, so this will be moved to Awaiting Update

 

[JC Helas]

It was once said that those who dare wander at night will behold an enigmatic and predatory luminescence. It was thought to be a spirit, an impish pixie, an impudent fairy, phantom, or even an alien sighting. As this eerie presence grips its' audience, a woeful song can be heard; the wailing of maidens gone mad, an ear-piercing howl, despair made manifest. Woe to those who wander to these lights, for none shall grant them rest, a requiem that can never know its' end.

Now, onto the review: (vJASS)

• In struct DoomFuse, there is no need to inline TriggerRegisterAnyUnitEventBJ, since it is only going to be called once.
  
  
• Local variables do not appear to be in camelCase. (This is somewhat minor, so feel free to ignore this)
  
  
• Having SpellEffectEvent and RegisterPlayerUnitEvent appears to be overkill for this spell. (Good thing they're optional).
  
  
• IsUnitType(unit, UNIT_TYPE_DEAD) can and should be replaced with not UnitAlive(unit).
  
  
• Instead of using raw numbers to indicate wave states, you could use some global variables that will clearly describe the wave state, instead of relying on presumptions made on your end on what each wave state actually is. (This is to aid development down the line when a user wants to modify the spell.)

It appears that a lot of work will have to be done before approval, so this will be moved to Awaiting Update

:O woaahh,,

In vJass, that BJ line you mention bro? Hehehe maybe in jass okay i will rid that bj..

UnitAlive bro? i never seen that native before please bro tell me which one XD

I dont get what you mean that i have to make it global variable for user to understand the state clearly >_<

Edit:
Do you mean bro that i have to write down documentary for each wave?

Bytheway thanks bro for putting the owners name of those system and reviewing my spell
-Appreciated

 

[MyPad]

UnitAlive bro? i never seen that native before please bro tell me which one XD

It is just UnitAlive. To declare such a native, copy and paste the following line:
native UnitAlive takes unit id returns boolean

I dont get what you mean that i have to make it global variable for user to understand the state clearly >_<

Do you mean bro that i have to write down documentary for each wave?

There's no need to write a long documentary on what each wave state means. It can be done like this.

// Extra code
if this.wave == WAVE_PHASE_ONE then
    //Insert code here
    //Insert some condition when proceeding to another phase.
    set this.wave = WAVE_PHASE_TWO
elseif this.wave == WAVE_PHASE_TWO then
   // Extra code
endif
// Extra code

Those are just examples of naming each wave state. You can think of names better than the example at conveying the mode.

In vJass, that BJ line you mention bro? Hehehe maybe in jass okay i will rid that bj..

No, that would not be necessary. There's no need to copy and paste what the function already does to save performance at a point where it isn't necessary to do so (It will only be called once, on map initialization, so it would just be inconvenient). I am suggesting the opposite, to use that BJ (

TriggerRegisterAnyUnitEventBJ) in the vJASS version, for the reason stated above.

 

[JC Helas]

It is just UnitAlive. To declare such a native, copy and paste the following line:
native UnitAlive takes unit id returns boolean



There's no need to write a long documentary on what each wave state means. It can be done like this.

// Extra code
if this.wave == WAVE_PHASE_ONE then
    //Insert code here
    //Insert some condition when proceeding to another phase.
    set this.wave = WAVE_PHASE_TWO
elseif this.wave == WAVE_PHASE_TWO then
   // Extra code
endif
// Extra code

Those are just examples of naming each wave state. You can think of names better than the example at conveying the mode.



No, that would not be necessary. There's no need to copy and paste what the function already does to save performance at a point where it isn't necessary to do so (It will only be called once, on map initialization, so it would just be inconvenient). I am suggesting the opposite, to use that BJ (

TriggerRegisterAnyUnitEventBJ) in the vJASS version, for the reason stated above.

the spell has been updated,, if you include to add those libraries for 1.31 well i dont have,, my warcraft is still 1.27b,, i wish to update but we dont have wifi now a days,, i should wait until we have..

 

[JC Helas]

It is just UnitAlive. To declare such a native, copy and paste the following line:
native UnitAlive takes unit id returns boolean
Bro its been updated


There's no need to write a long documentary on what each wave state means. It can be done like this.

// Extra code
if this.wave == WAVE_PHASE_ONE then
    //Insert code here
    //Insert some condition when proceeding to another phase.
    set this.wave = WAVE_PHASE_TWO
elseif this.wave == WAVE_PHASE_TWO then
   // Extra code
endif
// Extra code

Those are just examples of naming each wave state. You can think of names better than the example at conveying the mode.



No, that would not be necessary. There's no need to copy and paste what the function already does to save performance at a point where it isn't necessary to do so (It will only be called once, on map initialization, so it would just be inconvenient). I am suggesting the opposite, to use that BJ (

TriggerRegisterAnyUnitEventBJ) in the vJASS version, for the reason stated above.

Bro its update

 

[FUJI]

I like the concept, those revolving projectiles are absolutely a well-made eye candy.

However, there are some major and minor things you could consider for an improvement;

• In some parts of the code, you're following JPAG while there's also a parts you didn't.
• The spell is an area of effect, therefore you can use the radius indicator or 'Targeting Image' option by default.
• The combinations of special effects can be improved, especially on the lightning and explosion models to fit the actual theme of the concept. I find the healing wave and cannon missile model by default a bit contrived to a dreaded theme.
• On vJASS version, you're replacing the ball model with a vacuum model... while on JASS version there's no vacuum model but the ball model stays within the whole duration. Is this intentional?
• You're nulling a nulled local handle on JASS; set u=null <- at the end of the function is not needed because of exitwhen u==null
• Why's the creation of lightning is separated from its main data structure? You can just simply integrate it inside the periodic function and set its Waves to another stage. In this way, you save yourself from creating another timer and another function even variables. ;_;
• The WaveEnd stage contains some complicating conditions.
• You don't even need to reset the duration to 0 when it ends.
• You should account for terrain heights when setting lightning's z.
• You should check for map boundaries, at this current state... it can crash a map easily.

You're just redoing conversions here which makes no sense x)

set a=Atan2(y-udg_Doom_Fuse_SpellY[i],x-udg_Doom_Fuse_SpellX[i])*bj_RADTODEG
call SetUnitX(u,x-Doom_Fuse_VacuumForce()*Cos(a*bj_DEGTORAD))
call SetUnitY(u,y-Doom_Fuse_VacuumForce()*Sin(a*bj_DEGTORAD))

Also,

set a=GetRandomReal(0,359)
set x=udg_Doom_Fuse_SpellX[i]+GetRandomReal(20.0,Doom_Fuse_CenterBallHeight())*Cos(a * bj_DEGTORAD)
set y=udg_Doom_Fuse_SpellY[i]+GetRandomReal(20.0,Doom_Fuse_CenterBallHeight())*Sin(a * bj_DEGTORAD)

->

set a=GetRandomReal(-bj_PI,bj_PI)
set x=udg_Doom_Fuse_SpellX[i]+GetRandomReal(20.0,Doom_Fuse_CenterBallHeight())*Cos(a)
set y=udg_Doom_Fuse_SpellY[i]+GetRandomReal(20.0,Doom_Fuse_CenterBallHeight())*Sin(a)

You're making the radius here hardcoded and basing it on the dummy's height which doesn't makes sense either. :/

if LeftRight then
    set LeftRight=false
else
   set LeftRight=true
endif
// ->
set LeftRight = not(LeftRight)

This is pointless;

if udg_Doom_Fuse_AngleW23[i]==360 then
    set udg_Doom_Fuse_AngleW23[i]=0
endif

• You don't even need a group variable here unless you're keeping some units.
• Why call GroupClear(GroupDamage) ? you're already removing the units when enumerating them.
• (360/Doom_Fuse_PostNumbers())(360/(Doom_Fuse_RotateSpeed()/Doom_Fuse_Periodic())) can be constants instead of recalculating them everytime. (these are not the only ones inside the code)
• On cast function, you could've just use locals or a variable of index 0 as a single reference to the functions you're repeatedly calling. (JASS)

        call UnitAddAbility(udg_Doom_Fuse_Dummy[udg_Doom_Fuse_Index],Doom_Fuse_FlyID())
        call UnitRemoveAbility(udg_Doom_Fuse_Dummy[udg_Doom_Fuse_Index],Doom_Fuse_FlyID())

can be

if UnitAddAbility(udg_Doom_Fuse_Dummy[udg_Doom_Fuse_Index],Doom_Fuse_FlyID()) then
    call UnitRemoveAbility(udg_Doom_Fuse_Dummy[udg_Doom_Fuse_Index],Doom_Fuse_FlyID())
endif
// or
if UnitAddAbility(udg_Doom_Fuse_Dummy[udg_Doom_Fuse_Index],Doom_Fuse_FlyID()) and UnitRemoveAbility(udg_Doom_Fuse_Dummy[udg_Doom_Fuse_Index],Doom_Fuse_FlyID()) then
endif

Just want you to know if you're changing a unit's height in the future.
At this case, these are not needed for dummy units unless their movement type are not 'Fly'.

On SetTime method

local thistype this
set this=create()
// ->
local thistype this = create()

• You can omit declaring this local LightningLifeTime lfx
• You forgot this Doom_Fuse_BallModelAttach() ->AddSpecialEffectTarget(Doom_Fuse_BallModel(),udg_Doom_Fuse_Dummy[udg_Doom_Fuse_Index],"origin")
• As MyPad said, you can ignore inlining this TriggerRegisterAnyUnitEventBJ
• The configuration values lack the ability to change per level.
• The lightning properties can be configurable.
• You can shorten the prefixes, 2-4 letters are already enough to distinguish. I find udg_ is already repulsive prefix to read on JASS... why make it more lengthy ;_;
• Make your documentations more understandable // Loop Periodic PS: Changing this may cause bigger change <- ?
• You can just remove those TESH comments on the header.

I recommend linked list data structure on JASS, they're much more cleaner and deindexing on a large-scale variables will be as easy as pie than this current one you're opted to. Of course, it's up to you to consider these things