Sealing the Keyhole (Kingdom Hearts / SSBU)

[MyPad]

Spoiler: Previous Review 1.0

Hmm, based on the original mechanic of the featured spell, this might look interesting. Anyway, on with the review.

Spoiler: Code Review

Sealing the Keyhole

• You can merge the conditions and actions block into one, so that you only have to use TriggerAddCondition or TriggerAddAction, though the former is usually preferred.
  • TriggerSleepAction is not allowed for use in spells, because of how it can affect the simulation of the game state (game sessions may differ from replays).
    
    
  • Normally, working around TriggerSleepAction in the context of spell usage means employing timers for data attachment. However, for this particular spell, you can use the player unit event EVENT_PLAYER_UNIT_SPELL_FINISH or EVENT_PLAYER_UNIT_SPELL_ENDCAST to stun and damage all affected targets, subsequently cleaning up the effects as needed. You'll also need a data structure to attach the gate special effect and the target group to the caster.
    • Use EVENT_PLAYER_UNIT_SPELL_FINISH to deal the stun and damage. Use EVENT_PLAYER_UNIT_SPELL_ENDCAST for the cleanup process. Since EVENT_PLAYER_UNIT_SPELL_FINISH will not always run, EVENT_PLAYER_UNIT_SPELL_ENDCAST should be the event used for cleaning up generated special effects and groups.
      
• I think this block of code can be made into its separate function, allowing users to configure the target parameters easily, and making the chunk more readable:
  
  Original:
  

  private function onCast takes nothing returns nothing
      // Custom stuff...
      // The block of code that can be made into a separate function.
      if IsUnitEnemy(u, GetOwningPlayer(c)) and not IsUnitType(u, UNIT_TYPE_MAGIC_IMMUNE) and not BlzIsUnitInvulnerable(u) and UnitAlive(u) and not IsUnitType(u,UNIT_TYPE_STRUCTURE) then
      // Rest of the body
  endfunction

  
  New:
  

  private function FilterTarget takes unit u, unit c returns boolean
      return IsUnitEnemy(u, GetOwningPlayer(c)) and not IsUnitType(u, UNIT_TYPE_MAGIC_IMMUNE) and not BlzIsUnitInvulnerable(u) and UnitAlive(u) and not IsUnitType(u,UNIT_TYPE_STRUCTURE)
  endfunction
  
  // Other functions
  
  private function onCast takes nothing returns nothing    // Custom stuff...
      // Based on the function name, you can immediately tell that the iterated unit "u" is being filtered as a viable target.
      if FilterTarget(u, c) then
      // Rest of the body
  endfunction

  
  
• (Optional) I think you can merge the following two separate chunks from the original code into one with the help of multi-line comments.
  
  Original:
  

  if IsUnitType(u,UNIT_TYPE_HERO) and (KILL_PERCENT_HERO < 0.0 or GetUnitLifePercent(u) <= KILL_PERCENT_HERO) then
      call SetUnitX(u,x)
      call SetUnitY(u,y)
      call InstantKill(GetOwningPlayer(c),u,GetUnitX(c),GetUnitY(c))
  elseif not IsUnitType(u,UNIT_TYPE_HERO) and (KILL_PERCENT_UNIT < 0.0 or GetUnitLifePercent(u) <= KILL_PERCENT_UNIT) then
      call SetUnitX(u,x)
      call SetUnitY(u,y)
      call InstantKill(GetOwningPlayer(c),u,GetUnitX(c),GetUnitY(c))
  else
      call DummyCaster['A001'].castTarget(GetOwningPlayer(c),1,STUN_OID,u) //Comment this out and use your own stuns system here if desired
      call UnitDamageTarget(c, u, enddamage, false, true, ATTACK_TYPE_NORMAL, DAMAGE_TYPE_MAGIC, null)
  endif

  
  New
  

  if (IsUnitType(u,UNIT_TYPE_HERO) and (KILL_PERCENT_HERO < 0.0 or GetUnitLifePercent(u) <= KILL_PERCENT_HERO)) or /*
  */ (not IsUnitType(u,UNIT_TYPE_HERO) and (KILL_PERCENT_UNIT < 0.0 or GetUnitLifePercent(u) <= KILL_PERCENT_UNIT)) then
      call SetUnitX(u,x)
      call SetUnitY(u,y)
      call InstantKill(GetOwningPlayer(c),u,GetUnitX(c),GetUnitY(c))
  else
      call DummyCaster['A001'].castTarget(GetOwningPlayer(c),1,STUN_OID,u) //Comment this out and use your own stuns system here if desired
      call UnitDamageTarget(c, u, enddamage, false, true, ATTACK_TYPE_NORMAL, DAMAGE_TYPE_MAGIC, null)
  endif

Status:

• Awaiting Update

Version Reviewed:

• v.1.0

 

[KitsuneTailsPrower]

Hmm, based on the original mechanic of the featured spell, this might look interesting. Anyway, on with the review.

Spoiler: Code Review

Sealing the Keyhole

• You can merge the conditions and actions block into one, so that you only have to use TriggerAddCondition or TriggerAddAction, though the former is usually preferred.
  • TriggerSleepAction is not allowed for use in spells, because of how it can affect the simulation of the game state (game sessions may differ from replays).
    
    
  • Normally, working around TriggerSleepAction in the context of spell usage means employing timers for data attachment. However, for this particular spell, you can use the player unit event EVENT_PLAYER_UNIT_SPELL_FINISH or EVENT_PLAYER_UNIT_SPELL_ENDCAST to stun and damage all affected targets, subsequently cleaning up the effects as needed. You'll also need a data structure to attach the gate special effect and the target group to the caster.
    
    • Use EVENT_PLAYER_UNIT_SPELL_FINISH to deal the stun and damage. Use EVENT_PLAYER_UNIT_SPELL_ENDCAST for the cleanup process. Since EVENT_PLAYER_UNIT_SPELL_FINISH will not always run, EVENT_PLAYER_UNIT_SPELL_ENDCAST should be the event used for cleaning up generated special effects and groups.
      
      
• I think this block of code can be made into its separate function, allowing users to configure the target parameters easily, and making the chunk more readable:
  
  Original:
  

  private function onCast takes nothing returns nothing
      // Custom stuff...
      // The block of code that can be made into a separate function.
      if IsUnitEnemy(u, GetOwningPlayer(c)) and not IsUnitType(u, UNIT_TYPE_MAGIC_IMMUNE) and not BlzIsUnitInvulnerable(u) and UnitAlive(u) and not IsUnitType(u,UNIT_TYPE_STRUCTURE) then
      // Rest of the body
  endfunction

  
  New:
  

  private function FilterTarget takes unit u, unit c returns boolean
      return IsUnitEnemy(u, GetOwningPlayer(c)) and not IsUnitType(u, UNIT_TYPE_MAGIC_IMMUNE) and not BlzIsUnitInvulnerable(u) and UnitAlive(u) and not IsUnitType(u,UNIT_TYPE_STRUCTURE)
  endfunction
  
  // Other functions
  
  private function onCast takes nothing returns nothing    // Custom stuff...
      // Based on the function name, you can immediately tell that the iterated unit "u" is being filtered as a viable target.
      if FilterTarget(u, c) then
      // Rest of the body
  endfunction

  
  
• (Optional) I think you can merge the following two separate chunks from the original code into one with the help of multi-line comments.
  
  Original:
  

  if IsUnitType(u,UNIT_TYPE_HERO) and (KILL_PERCENT_HERO < 0.0 or GetUnitLifePercent(u) <= KILL_PERCENT_HERO) then
      call SetUnitX(u,x)
      call SetUnitY(u,y)
      call InstantKill(GetOwningPlayer(c),u,GetUnitX(c),GetUnitY(c))
  elseif not IsUnitType(u,UNIT_TYPE_HERO) and (KILL_PERCENT_UNIT < 0.0 or GetUnitLifePercent(u) <= KILL_PERCENT_UNIT) then
      call SetUnitX(u,x)
      call SetUnitY(u,y)
      call InstantKill(GetOwningPlayer(c),u,GetUnitX(c),GetUnitY(c))
  else
      call DummyCaster['A001'].castTarget(GetOwningPlayer(c),1,STUN_OID,u) //Comment this out and use your own stuns system here if desired
      call UnitDamageTarget(c, u, enddamage, false, true, ATTACK_TYPE_NORMAL, DAMAGE_TYPE_MAGIC, null)
  endif

  
  New
  

  if (IsUnitType(u,UNIT_TYPE_HERO) and (KILL_PERCENT_HERO < 0.0 or GetUnitLifePercent(u) <= KILL_PERCENT_HERO)) or /*
  */ (not IsUnitType(u,UNIT_TYPE_HERO) and (KILL_PERCENT_UNIT < 0.0 or GetUnitLifePercent(u) <= KILL_PERCENT_UNIT)) then
      call SetUnitX(u,x)
      call SetUnitY(u,y)
      call InstantKill(GetOwningPlayer(c),u,GetUnitX(c),GetUnitY(c))
  else
      call DummyCaster['A001'].castTarget(GetOwningPlayer(c),1,STUN_OID,u) //Comment this out and use your own stuns system here if desired
      call UnitDamageTarget(c, u, enddamage, false, true, ATTACK_TYPE_NORMAL, DAMAGE_TYPE_MAGIC, null)
  endif

Status:

• Awaiting Update

Version Reviewed:

• Unknown, possibly v.1.0

Thanks for the review, updated accordingly

 

[MyPad]

A delayed AOE stunning ability, useful for securing kills and interrupting channeled abilities alike. While the concept of delayed damage isn't new, the spell itself doesn't do too much to stand out from that core idea. Replicating the skill concept more faithfully would be advised.

Anyway, on to the code review:

Spoiler: Code Review

• function onCast
  • Reference leak (unit c) detected in case the ability itself (Sealing the Keyhole) isn't cast.
    
    
  • Multiple instances of GetUnitUserData(c) were found. You could store its value in a local integer variable and refer to that instead.

Looks like there isn't much to address or fix here. I will leave it to you to update it with the fixes.

Status:

• Useful

Version Reviewed:

• v.1.1