Summon the Knight of Light

MyPad · Aug 26, 2021

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

Summon the Knight of Light

Summon the Knight of Light (Map)

MyPad

MyPad

Resources

Similar threads