Summon the Knight of Light

This bundle is marked as awaiting update. A staff member has requested changes to it before it can be approved.
hello this is my first spell in this case I made a spell in which you invoke a knight of light that is vitally linked with the summoner, that is, if the summoner dies the knight will die but if the invocation dies or his life time expires he will not be nothing will happen to the summoner

JASS:
scope spell initializer KnightOfLight

globals
hashtable triggerT = InitHashtable()
hashtable               KnightOfLightTabla     = InitHashtable()
group                     KnightOfLightGrupo     = CreateGroup()
endglobals

//===========================================================================
// Trigger: KnightOfLight Activacion
//===========================================================================
function KnightOfLight_Activacion_Acciones takes nothing returns nothing
    //Variables Locales
    local unit array u
    local location array p
    local integer n
    local integer i
    local lightning r
    local effect e
    //Fin de Variables Locales
    if ( GetUnitTypeId(GetSummonedUnit()) == 'h000' ) then
        // Guardado de tablas hash
        // Invocador
        set u[1] = GetSummoningUnit()
        // Invocacion
        set u[2] = GetSummonedUnit()
        // Inicio de invocacion animada
        // Metadatos
        set p[1] = GetUnitLoc(u[1])
        set p[2] = GetUnitLoc(u[2])
        set i = GetHandleId(u[1])
        set e = LoadEffectHandle(KnightOfLightTabla,i,StringHash("AlasConjurador"))
        set u[3] = LoadUnitHandle(KnightOfLightTabla,i,StringHash("Invocacion"))
        call KillUnit(u[3])
        call DestroyEffect( e )
        call DestroyLightning(LoadLightningHandle(KnightOfLightTabla,i,StringHash("Relampago")))
        call FlushChildHashtable(KnightOfLightTabla,i )
        call GroupAddUnit( KnightOfLightGrupo, u[1] )
        // Efectos especiales
        set e = AddSpecialEffectTarget("HolyBlastEfecto.mdl", u[1], "chest")
        set r = AddLightningEx("DRAB", true, GetUnitX(u[1]), GetUnitY(u[1]), GetLocationZ(p[1]) + 50, GetUnitX(u[2]), GetUnitY(u[2]), GetLocationZ(p[2]) + 150)
        call SaveBoolean( triggerT, StringHash("Relampagos"), StringHash("Caballero de luz"), true)
        call AddUnitAnimationProperties( u[2], "alternate", true )
        call SetUnitAnimation( u[2], "morph" )
        // Fin de los efectos especiales
        call EnableTrigger( LoadTriggerHandle( triggerT, StringHash("Detonadores"), StringHash("KnightOfLightHechizo") ) )
        call SaveEffectHandle( KnightOfLightTabla, i, StringHash("AlasConjurador"), e)
        call SaveUnitHandle( KnightOfLightTabla, i, StringHash("Invocacion"), u[2] )
        call SaveLightningHandle(KnightOfLightTabla, i, StringHash("Relampago"), r )
        // Limpiando fugas
        set e = null
        set n = 1
        loop
            exitwhen n == 3
            call RemoveLocation(p[n])
            set p[n] = null
            set n = n + 1
        endloop
        set n = 1
        loop
            exitwhen n == 4
            set u[n] = null
            set n = n + 1
        endloop
        else
call DisplayTextToForce( GetPlayersAll(), "TRIGSTR_017" )
    endif
    endfunction

//===========================================================================
// Trigger: KnightOfLight Hechizo
//===========================================================================
function KnightOfLight_Hechizo_AccionesDeGrupo takes nothing returns nothing
    //Variables Locales
    local unit array u
    local location array p
    local effect array e
    local integer i
    local lightning r
    local integer n
    local real x
    local real y
    //Fin de Variables Locales
    set u[1] = GetEnumUnit()
    set i = GetHandleId(u[1])
    set r = LoadLightningHandle( KnightOfLightTabla, i, StringHash("Relampago") )
    set u[2] = LoadUnitHandle(KnightOfLightTabla, i, StringHash("Invocacion"))
    set e[1] = LoadEffectHandle( KnightOfLightTabla, i, StringHash("AlasConjurador"))
    if( GetUnitState(u[1], UNIT_STATE_LIFE) > 0 ) and ( GetUnitState(u[2], UNIT_STATE_LIFE) > 0 ) then
        set p[1] = GetUnitLoc(u[1])
        set p[2] = GetUnitLoc(u[2])
        if ( ( DistanceBetweenPoints(p[1], p[2]) <= 800.00 ) ) then
            call MoveLightningEx(r, true, GetUnitX(u[1]), GetUnitY(u[1]), GetLocationZ(p[1]) + 50, GetUnitX(u[2]), GetUnitY(u[2]), GetLocationZ(p[2]) + 150)
            call SaveBoolean( triggerT, StringHash("Relampagos"), StringHash("Caballero de luz"), true)
        else
            call DestroyEffect( AddSpecialEffectLoc( "HolyBlast.mdl", p[2] ) )
            call DestroyEffect( AddSpecialEffectTarget( "HolyBlast.mdl", u[2], "origin" ) )
            set x = GetLocationX(p[1]) + 256 * Cos(GetUnitFacing(u[1]) * 3.14159/180.0)
            set y = GetLocationY(p[1]) + 256 * Sin(GetUnitFacing(u[1]) * 3.14159/180.0)
            set p[3] = Location(x, y)
            call SetUnitPositionLoc(u[2],p[3] )
            call SetUnitFacing(u[2], GetUnitFacing(u[1]))
            call IssueTargetOrder( u[2], "smart", u[1] )
        endif
        call RemoveLocation(p[1])
        call RemoveLocation(p[2])
        call RemoveLocation(p[3])
    else
        call DestroyLightning( r )
        call SaveBoolean( triggerT, StringHash("Relampagos"), StringHash("Caballero de luz"), false)
        call FlushChildHashtable(KnightOfLightTabla, 1 )
        call GroupRemoveUnit( KnightOfLightGrupo, u[1] )
        // Eliminando Invocacion
        if ( GetUnitState(u[1], UNIT_STATE_LIFE) <= 0 ) then
            call KillUnit( u[2] )
        else
            // Eliminando efecto del invocador
            call DestroyEffect( e[1] )
            set e[1] = null
        endif
        // Limpiador de fugas y desactivador del detonador
        if ( ( CountUnitsInGroup(KnightOfLightGrupo) == 0 ) ) then
            call DisableTrigger( GetTriggeringTrigger() )
            set n = 1
            loop
                exitwhen n == 4
                call DestroyEffect( e[n] )
                set p[n] = null
                set n = n + 1
            endloop
            set n = 1
            loop
                exitwhen n == 3
                set u[n] = null
                set n = n + 1
            endloop
            set r = null
            set e[1] = null
        else
        endif
    endif
endfunction

function KnightOfLight_Hechizo_Acciones takes nothing returns nothing
    call ForGroup( KnightOfLightGrupo, function KnightOfLight_Hechizo_AccionesDeGrupo )
endfunction

//===========================================================================
function KnightOfLight takes nothing returns nothing
    local trigger t = CreateTrigger()
    local integer index
    // Caballero de luz Activacion
    set t = CreateTrigger()
    set index = 0
    loop
        call TriggerRegisterPlayerUnitEvent(t, Player(index), EVENT_PLAYER_UNIT_SUMMON, null )
      
        set index = index + 1
        exitwhen index == bj_MAX_PLAYER_SLOTS
    endloop
    call TriggerAddAction( t, function KnightOfLight_Activacion_Acciones )
    // Caballero de luz Hechizo
    set t = CreateTrigger()
    call DisableTrigger( t )
    call TriggerRegisterTimerEvent(t, 0.03, true)
    call TriggerAddAction( t, function KnightOfLight_Hechizo_Acciones )
    call SaveTriggerHandle( triggerT, StringHash("Detonadores"), StringHash("KnightOfLightHechizo"), t)
    set t = null
endfunction
endscope

-Copy the spell category trigger
-Copy the spell from object editor
-Copy unit from object editor


  • v.1.0 - Release

199684-ffdd97c3ba1b59222c991d5e7a49ee46.png
Contents

Summon the Knight of Light (Map)

Reviews
MyPad
Not bad for a first spell submission. A slave bound to his master through his lifeforce serves in life as he does in death. While the summoned unit can be killed without any issues, killing the summoner will immediately cut off the lifeforce the...

MyPad

Spell Reviewer
Level 21
Joined
May 9, 2014
Messages
1,653
Not bad for a first spell submission. A slave bound to his master through his lifeforce serves in life as he does in death.
While the summoned unit can be killed without any issues, killing the summoner will immediately cut off the lifeforce the
summoned unit is infused with.

There's a lot of things to tackle here, so this might take a while.
  • Since the spell code is already encapsulated within a scope, it is assumed that all of the functions within the scope
    are private. That being said, the functions contained are not private. This can be addressed by adding a "private"
    keyword before the functions themselves. Similarly, this can be applied with the global variables as well.

  • In the initialization function KnightOfLight, the trigger which detects the summoning event is created twice, once
    at the declaration of the variable and another right after that. The second declaration should be removed since that
    results in an unused trigger being generated.

    JASS:
    // Added a private keyword before the function
    private function KnightOfLight takes nothing returns nothing
        local trigger t = CreateTrigger()
        local integer index
        // Please remove the line below
        // set t = CreateTrigger()
        // ...
    endfunction

  • In the same function, there's no need to inline what the BJ function TriggerRegisterAnyUnitEventBJ already does.
    Simply call that function instead, since any gain in performance from the inlined version is negated by the prospect
    of running only once.

  • In JASS, the usage of location handles is highly discouraged, since one can work directly with coordinates instead.

  • The usage of two hashtables for implementing the spell mechanics is odd, since hashtable usage can be avoided in
    most cases. Aside from that, this will leave the user with two less hashtables to use, since there is a limit on the
    number of hashtables that can be generated.

  • In the trigger action that runs on a summoning event KnightOfLight_Activacion_Acciones, there are a lot of things
    that must be addressed.
    • JASS:
          //Variables Locales
          local unit array u
          local location array p
          local integer n
          local integer i
          local lightning r
          local effect e
          //Fin de Variables Locales

      In most cases, I would gloss over the short variable names if their purpose is clearly understandable at a first glance.
      However, using arrays like this leads to confusion on what the variable represents (or references). For example, consider
      two local unit variables named "summoned" and "summoner". From a glance, it is immediately clear what each variable
      represents, making it easier to adjust the code as necessary. This is not easily achievable with local array variables, where
      one has to figure out the contents at a specific index, making it more difficult to update the code.

    • JASS:
          if ( GetUnitTypeId(GetSummonedUnit()) == 'h000' ) then
              // Guardado de tablas hash
              // Invocador
              set u[1] = GetSummoningUnit()
              // Invocacion
              set u[2] = GetSummonedUnit()
              // Inicio de invocacion animada
              // Metadatos
              set p[1] = GetUnitLoc(u[1])
              set p[2] = GetUnitLoc(u[2])
              set i = GetHandleId(u[1])
              set e = LoadEffectHandle(KnightOfLightTabla,i,StringHash("AlasConjurador"))
              set u[3] = LoadUnitHandle(KnightOfLightTabla,i,StringHash("Invocacion"))
              call KillUnit(u[3])
              call DestroyEffect( e )
              call DestroyLightning(LoadLightningHandle(KnightOfLightTabla,i,StringHash("Relampago")))
              call FlushChildHashtable(KnightOfLightTabla,i )
              call GroupAddUnit( KnightOfLightGrupo, u[1] )
              // Efectos especiales
              set e = AddSpecialEffectTarget("HolyBlastEfecto.mdl", u[1], "chest")
              set r = AddLightningEx("DRAB", true, GetUnitX(u[1]), GetUnitY(u[1]), GetLocationZ(p[1]) + 50, GetUnitX(u[2]), GetUnitY(u[2]), GetLocationZ(p[2]) + 150)
              call SaveBoolean( triggerT, StringHash("Relampagos"), StringHash("Caballero de luz"), true)
              call AddUnitAnimationProperties( u[2], "alternate", true )
              call SetUnitAnimation( u[2], "morph" )
              // Fin de los efectos especiales
              call EnableTrigger( LoadTriggerHandle( triggerT, StringHash("Detonadores"), StringHash("KnightOfLightHechizo") ) )
              call SaveEffectHandle( KnightOfLightTabla, i, StringHash("AlasConjurador"), e)
              call SaveUnitHandle( KnightOfLightTabla, i, StringHash("Invocacion"), u[2] )
              call SaveLightningHandle(KnightOfLightTabla, i, StringHash("Relampago"), r )
              // Limpiando fugas
              set e = null
              set n = 1
              loop
                  exitwhen n == 3
                  call RemoveLocation(p[n])
                  set p[n] = null
                  set n = n + 1
              endloop
              set n = 1
              loop
                  exitwhen n == 4
                  set u[n] = null
                  set n = n + 1
              endloop
          else
              call DisplayTextToForce( GetPlayersAll(), "TRIGSTR_017" )
          endif

      Suppose that the summoning event runs, and the unit type summoned does not match. This will result in a message
      being printed ("no unidad"). This appears to be a debug message that was not removed. Removing that debug message
      is recommended.

      The conditional statement can be rewritten such that the action terminates if the condition is not satisfied. This leads
      to the following rewrite

      JASS:
          if ( GetUnitTypeId(GetSummonedUnit()) != 'h000' ) then
              return
          endif
          // Guardado de tablas hash
          // Invocador
          set u[1] = GetSummoningUnit()
          // Invocacion
          set u[2] = GetSummonedUnit()
          // Inicio de invocacion animada
          Metadatos
          set p[1] = GetUnitLoc(u[1])
          set p[2] = GetUnitLoc(u[2])
          set i = GetHandleId(u[1])
          set e = LoadEffectHandle(KnightOfLightTabla,i,StringHash("AlasConjurador"))
          set u[3] = LoadUnitHandle(KnightOfLightTabla,i,StringHash("Invocacion"))
          call KillUnit(u[3])
          call DestroyEffect( e )
          call DestroyLightning(LoadLightningHandle(KnightOfLightTabla,i,StringHash("Relampago")))
          call FlushChildHashtable(KnightOfLightTabla,i )
          call GroupAddUnit( KnightOfLightGrupo, u[1] )
          // Efectos especiales
          set e = AddSpecialEffectTarget("HolyBlastEfecto.mdl", u[1], "chest")
          set r = AddLightningEx("DRAB", true, GetUnitX(u[1]), GetUnitY(u[1]), GetLocationZ(p[1]) + 50, GetUnitX(u[2]), GetUnitY(u[2]), GetLocationZ(p[2]) + 150)
          call SaveBoolean( triggerT, StringHash("Relampagos"), StringHash("Caballero de luz"), true)
          call AddUnitAnimationProperties( u[2], "alternate", true )
          call SetUnitAnimation( u[2], "morph" )
          // Fin de los efectos especiales
          call EnableTrigger( LoadTriggerHandle( triggerT, StringHash("Detonadores"), StringHash("KnightOfLightHechizo") ) )
          call SaveEffectHandle( KnightOfLightTabla, i, StringHash("AlasConjurador"), e)
          call SaveUnitHandle( KnightOfLightTabla, i, StringHash("Invocacion"), u[2] )
          call SaveLightningHandle(KnightOfLightTabla, i, StringHash("Relampago"), r )
          // Limpiando fugas
          set e = null
          set n = 1
          loop
              exitwhen n == 3
              call RemoveLocation(p[n])
              set p[n] = null
              set n = n + 1
          endloop
          set n = 1
          loop
              exitwhen n == 4
              set u[n] = null
              set n = n + 1
          endloop

    • JASS:
          set n = 1
          loop
              exitwhen n == 3
              call RemoveLocation(p[n])
              set p[n] = null
              set n = n + 1
          endloop
          set n = 1
          loop
              exitwhen n == 4
              set u[n] = null
              set n = n + 1
          endloop

      When the number of entries within these array variables are that small, it would be better to inline
      them (typically ranging from 2 to 8), saving up an unnecessary "addition" operation, as well as a
      comparison operation.

    • I think it would be more convenient if the resulting values from the StringHash functions are stored
      in "static" variables ("static" variables being constant variables that are assigned at runtime). That
      said, it would be preferable that the StringHash function is not used at all.

    • Code:
          call SaveBoolean( triggerT, StringHash("Relampagos"), StringHash("Caballero de luz"), true)    call AddUnitAnimationProperties( u[2], "alternate", true )
          call SetUnitAnimation( u[2], "morph" )
          // Fin de los efectos especiales
          call EnableTrigger( LoadTriggerHandle( triggerT, StringHash("Detonadores"), StringHash("KnightOfLightHechizo") ) )

      The trigger stored at triggerT can be stored directly in a global trigger variable instead, reducing unnecessary
      overhead from accessing contents from the triggerT hashtable (which appears to be a wasted hashtable once
      optimized).
  • In the trigger action that runs on a periodic event (0.03 seconds) KnightOfLight_Hechizo_AccionesDeGrupo, there are a lot of things
    that must be addressed.
    • When getting the amount of life of a unit, you can use the following native instead to optimize performance:

      JASS:
      native GetWidgetLife takes widget w returns real
      native SetWidgetLife takes widget w, real amount returns nothing

      What the native above does is get the amount of health of the widget handle. This widget handle can accept
      units, as well as destructables and items. Similarly, SetWidgetLife sets the amount of health of the widget.

    • The DistanceBetweenPoints function involves the usage of a SquareRoot function, which can be quite costly in
      a periodic timer with a short interval. If you'd like to optimize this,
      consider comparing the square of the distance with the square of the maximum distance (which in this case is
      800.00*800.00). Otherwise, you can skip this one.

    • Since the number of units in the group changes only upon death or summoning event, this means that the
      number of units can be easily determined with the usage of a global counter variable (integer). Knowing this,
      one can easily avoid calling the function CountUnitsInGroup, a function that runs in O(n) time which is called
      O(n) times in the periodic, leading to a time complexity of O(n^2), which is not good when dealing with
      a lot of instances.
  • Instead on relying on an internal timer for the periodic event, a timer handle can be created directly to handle the
    same periodic event, freeing up a trigger handle for use.

  • Configurability wise, this spell is a rigid fossil compared to most, having mostly hardcoded constants in place of configurable
    variables or functions. Some examples of hardcoded constants/chunks which can be made configurable:
    • Function KnightOfLight_Activacion_Acciones

      JASS:
      // What if the spell summoned a unit with a different rawcode?
      // This means that the value at the right hand side is a configurable value
      // and should be easily modifiable for the user. Right now, the value
      // at the right hand side is hardcoded to be 'h000'
      if ( GetUnitTypeId(GetSummonedUnit()) == 'h000' ) then
          //...
      //...

      JASS:
      if ( GetUnitTypeId(GetSummonedUnit()) == 'h000' ) then
          //...
          // The attachment effect might not be desirable for some, so
          // assume that they are willing to change the effect. In this
          // case, the path to the model (and the attachment point)
          // is/are hardcoded value/s.
          set e = AddSpecialEffectTarget("HolyBlastEfecto.mdl", u[1], "chest")
      
          // Similarly, the lightning model might not be to the liking of the user, so
          // assume that they are willing to change that.
          set r = AddLightningEx("DRAB", true, GetUnitX(u[1]), GetUnitY(u[1]), GetLocationZ(p[1]) + 50, GetUnitX(u[2]), GetUnitY(u[2]), GetLocationZ(p[2]) + 150)
          // ...
      // ...

    • Function KnightOfLight_Hechizo_AccionesDeGrupo

      JASS:
      if( GetUnitState(u[1], UNIT_STATE_LIFE) > 0 ) and ( GetUnitState(u[2], UNIT_STATE_LIFE) > 0 ) then
              set p[1] = GetUnitLoc(u[1])
              set p[2] = GetUnitLoc(u[2])
              // Perhaps the cut-off range is too long or too short.
              // In any case, this range can be considered a configurable
              // value.
              if ( ( DistanceBetweenPoints(p[1], p[2]) <= 800.00 ) ) then
                  call MoveLightningEx(r, true, GetUnitX(u[1]), GetUnitY(u[1]), GetLocationZ(p[1]) + 50, GetUnitX(u[2]), GetUnitY(u[2]), GetLocationZ(p[2]) + 150)
                  call SaveBoolean( triggerT, StringHash("Relampagos"), StringHash("Caballero de luz"), true)
              else
              // ...
              // At this point, it is a stretch to say that this chunk is configurable
              // but suppose that the user might want something different to happen, like keeping
              // the summoned unit within range only instead of forcing the summoned unit to
              // reposition close to the summoner. Hence, the following chunk can be moved
              // either into a function or a textmacro.
              call SetUnitPositionLoc(u[2],p[3] )
              call SetUnitFacing(u[2], GetUnitFacing(u[1]))
              call IssueTargetOrder( u[2], "smart", u[1] )
      // ...

Due to the number of issues pointed out in the review, the bundle shall be Awaiting Update
 
Top