Frostfire Bolt [custom swirly bolt] v1.0.2

[Moderator]

18:23, 14th Oct 2013
PurgeandFire: Approved! This spell is great for whatever swirly projectile abilities you desire.

(old) Review:
http://www.hiveworkshop.com/forums/2431630-post20.html

 

[iAyanami]

The resource should be encapsulated within a scope or a library.

private                     real                boltScale                   = FFB_BOLT_SCALE
private                     real                boltSpeed                   = FFB_BOLT_SPEED
private                     real                spinSpeed                   = FFB_BOLT_SPIN_SPEED
private                     real                verAmp                      = FFB_AMPLITUDE_VERTICAL
private                     real                horAmp                      = FFB_AMPLITUDE_HORISONTAL
private                     real                boltOffset                  = FFB_BOLT_OFFSET

Do these members remain constant throughout the duration of the spell? If so, you don't need these members, just use the constants directly.

readonly                    widget              target                      = null
readonly                    unit                caster                      = null
readonly                    boolean             active                      = true

Any reason why these members are readonly?


I see that you're using a linked list of some sort to iterate through the instances for your periodic effect? You have unneeded static members for your linked list.

struct Test
    private thistype next
    private thistype prev

    // looping through the list
    private static method periodic takes nothing returns nothing
        local thistype this = thistype(0)
        
        loop
            set this = this.next
            exitwhen this == 0

            ...
        endloop
    endmethod

    // destroy an instance
    private method destroy takes nothing returns nothing
        set this.next.prev = this.prev
        set this.prev.next = this.next

        // count check, to determine if timer needs to be paused
        if thistype(0).next == 0 then
            // pause timer
        endif

        call this.deallocate()
    endmethod

    // create an instance and insert into list
    private static method create takes nothing returns thistype
        local thistype this = thistype.allocate()

        set this.next = thistype(0)
        set this.prev = thistype(0).prev
        set thistype(0).prev.next = this
        set thistype(0).prev = this
        
        return this
    endmethod

    // init method
    private static method onInit takes nothing returns nothing
        // this is probably unneeded as next and prev already points to thistype(0) by default, but just doing it anyways
        set thistype(0).next = thistype(0)
        set thistype(0).prev = thistype(0)
    endmethod
endstruct

You won't need the variables thiss, first, last and count. And actually, rather than coding your own list from scratch, you could actually use CTL for periodic timers with a timeout of 0.03125.


You should allow ATTACK_TYPE and DAMAGE_TYPE to be configurable, rather than null. WEAPON_TYPE as well if you want.


You don't need to dynamically create caster dummies. Simply have 1 static dummy created at init to perform all the necessary dummy casts.

 

[Tal'Rarity]

I am pretty sure this is VJass and not Jass.

 

[Lender]

The resource should be encapsulated within a scope or a library.

sorry i was in a hurry, I just missed that

Do these members remain constant throughout the duration of the spell? If so, you don't need these members, just use the constants directly.

you got the point

Any reason why these members are readonly?

I was thinking of adding events but changed my mind and forgot to rename them

You won't need the variables thiss, first, last and count. And actually, rather than coding your own list from scratch, you could actually use CTL for periodic timers with a timeout of 0.03125.

Since I already scratched it up I don't realy want to rewrote that part but will take your advice and get rid of thiss, first and last. I always thinked of linked lists as a line not a circle thanks

will update the map as soon as I can. +rep for the great feedback iAyanami .

 

[nhocklanhox6]

if your spell is private, you must private all members on globals, you don't need to private all members of struct one by one, just private the struct, like this:

private struct FrostFireBolt

 

[iAyanami]

if your spell is private, you must private all members on globals, you don't need to private all members of struct one by one, just private the struct, like this:

private struct FrostFireBolt

I still use private members even when I use private structs. So I guess it's more of a personal preference.

 

[Lender]

Almost done with update but the jass helpers compiler shows error on method destroy saying it had been redeclared... Any idea why?

 

[iAyanami]

Almost done with update but the jass helpers compiler shows error on method destroy saying it had been redeclared... Any idea why?

Hmm, that's weird. Did you happen to declare destroy twice? Perhaps from a module or something?

 

[Lender]

Nope .. Checked everything no double declaration no moduls and alikes :/
Only can think it's my we but I use jngp 5d with a standard (not patched) game so that should be ok

 

[nhocklanhox6]

Private struct are private all members inside and the named, even using static, you can't call it outside, that weird if you use private members twice , i never seen what happen if private twice, i'll do it.
If you using method "destroy" inside struct, it will cause an error, i think "onDestroy" is right or you missing some thing.

 

[Lender]

Hm I'll try that one. But the strange thing is the create method works perfectly as intended

 

[edo494]

Private struct are private all members inside and the named, even using static, you can't call it outside, that weird if you use private members twice , i never seen what happen if private twice, i'll do it.
If you using method "destroy" inside struct, it will cause an error, i think "onDestroy" is right or you missing some thing.

I shouldnt, JassHelper is not very intelligent but he is good enough to find if user uses his own destroy/create methods, if he does, it implements them, if he doesnt it inlines them to .allocate()/.deallocate()

fix the indentation in onInit method please

Also instead of looping through all players inside onInit, you should use http://www.hiveworkshop.com/forums/jass-resources-412/snippet-registerplayerunitevent-203338/

if SquareRoot(.ampX*.ampX+.ampY*.ampY)>=.verAmp then

into

if (this.ampX*this.ampX+this.ampY*this.ampX) >= this.verAmp*this.verAmp) then

Your method of checking if unit is alive is not reliable, if I use tranquility it will heal dead units, so your condition will pass, use either if IsUnitType(unit, UNIT_TYPE_DEAD) then or if IsUnitType(unit, UNIT_TYPE_DEAD) or GetWidgetLife(widget) >= 0.405 then

If you can, order units by Id instead of string, its faster(recommandation, not like Im pushing you into it)

call SetUnitFacingTimed(.frostDummy,.angle*57.29577951,0.01)
call SetUnitFacingTimed(.fireDummy,.angle*57.29577951,0.01)

is irrelevant, just use SetUnitFacing, because there is hardcoded time that it takes the unit to turn around, alternativelly you can call SetUnitMoveSpeed(unit, 0.0), which makes unit unmovable, untrunable but the unit will cast and attack instantly under any angle I believe

 

[nhocklanhox6]

Blizzard hide the method "destroy" inside any struct, that why it cause an error, use onDestroy to access that method
@ed: you don't need to use "this" on any members of struct, just "." or leave this keyword

 

[edo494]

Blizzard hide the method "destroy" inside any struct, that why it cause an error, use onDestroy to access that method

blizzard does not, blizzard has no control over vJass, because it was not blizzard that made it but Vexorian, and he is former admin of wc3c

Believe me, if you implement your destroy method, it shouldnt cause compile error, onDestroy is only used in case with inheritance when you destroy object of type A inheriting type B, then B's onDestroy will be called.

Destroy and create cant be private tho, and if you dont provide any version of method called create and/or destroy, JassHelper will generate one(which inlines to .allocate()/.deallocate() call

 

[iAyanami]

Nope .. Checked everything no double declaration no moduls and alikes :/
Only can think it's my we but I use jngp 5d with a standard (not patched) game so that should be ok

Try re-downloading JNGP in that case. It should work. Get this version if you haven't btw.

Blizzard hide the method "destroy" inside any struct, that why it cause an error, use onDestroy to access that method

Err no. Like mentioned above, declaring your own destroy method works perfectly fine. In fact, onDestroy is slower than using destroy. onDestroy really should only be used when working with inheritance.

 

[nhocklanhox6]

@ed: Hmn, okay, now i've learned something , thanks.

 

[Lender]

Than I guess I'll try reinstalling jngp and see if it works

Ed: ok I got it to work thanks for the help everyone

Update soon

 

[Lender]

ed: solved error problem

 

[Lender]

Updated, works properly.

 

[PurgeandFire]

Review:

• You can initialize the timer "t" with CreateTimer() in the struct:
  

  struct Example
      private static timer t = CreateTimer()
  endstruct

  Basically, you don't have to do it in the onInit.
• Your indenting is a bit weird in "onInit". You indent the TriggerAddCondition() and the loop even though they aren't in a block.
• You may want to register on SPELL_EFFECT rather than SPELL_CAST. Spell effect will fire after mana is used and stuff (probably won't be an issue for this spell, but for future spells you may want to use SPELL_EFFECT)
• This condition check:
  

  if GetSpellAbilityId() == FFB_SPELL_ID and GetSpellTargetUnit() != GetSpellAbilityUnit() and not IsUnitType(GetSpellTargetUnit(),UNIT_TYPE_DEAD) then

  Seems a bit odd. Shouldn't you just check the targets in the object editor? E.g. targets allowed: Enemy, Alive
• In the method create, the sound path should be a constant.
• For future reference, GetTriggerPlayer() is the same as GetOwningPlayer(GetTriggerPlayer()) for spell casts**, at least in the condition/actions part (not in a separate timer or group call though).
• In create, you should null snd.
• For SetUnitScale, you can safely put in 0 for Y and Z (if i recall correctly). It isn't necessary but it might look nicer. Just another tip for future spells.

• if not IsUnitType(.utarg,UNIT_TYPE_DEAD) then

  In general, you should add a check to make sure the unit wasn't removed:
  

  if not IsUnitType(.utarg, UNIT_TYPE_DEAD) and GetUnitTypeId(.utarg) != 0 then

• Anything that you don't need to store over the timer's period should be local. For example, preX and preY can simply be inlined since you don't need really *need* to save those values. Once you use them for that loop iteration, you're done with them. You can either make them locals or just inline them. (this is in the method "positioning")
• Unless I'm mistaken, I don't think you use "boolean b" in the method "positioning". You can remove it.
• Btw, why aren't you using private for the constants? It would allow you to eliminate prefixing it with FFB and the code might look a bit neater.
• About this:
  

  set current = this.prev
  call .destroy()
  set this = current

  Technically, when you call .destroy(), this.prev is still maintained in memory (we don't bother to assign it to 0 because it'll usually be overwritten anyway by a different spell cast). So technically, you could do:
  

  call this.destroy()
  set this = this.prev

  While this's prev and next no longer point to "this", "this" will still point to its prev and next links. For example, you have a list of members A B C:
  A <-> B <-> C
  You remove B through B.destroy():
  A <-> C; A <- B -> C;
  Note that A and C are now linked (to allow for proper looping), but B still maintains some linking to A and C. Neat, huh?

That's about it. Cool spell otherwise. I was thinking about making a similar spell while I was in math. It reminds me of arcane barrage (the way they swirl):
http://www.wowhead.com/spell=44425

 

[Lender]

Thanks for the update
Sad I hadn't got time to optimize it since upload
Damn I got a lot tests and exams

call this.destroy() 
set this = this.prev

very nice thx for the tip

Fixes will be done tomorrow hopefully

 

[PurgeandFire]

Once you null the variable "snd" at the end of "create", I'll approve. Send me a VM if you can so I'll know that you updated it.