Railgun

[Lt_Hawkeye]

Going to test it out now that it is publicly available.

 

[Chaosy]

Structure makes my eyes bleed.

Indent one step when inside any block, this includes the global block AND function blocks.

Some blank rows between functions would increase readability too

 

[MyPad]

EDIT:

This is a previous review, and has been archived in the next review.

 

[MyPad]

Previous Review

Nitpicks:

• Constants which are configurable could be moved to functions instead.

• Adding some more effects to the railgun when firing would be appreciated.

Notes:

• There is a lot to be desired from readability.

• Try to observe the JPAG standard when submitting resources such as these.
  • Constants should be in UPPER_CASE
  • Non-constants, locals, variables should be camelCase
  • Functions should be LikeThis.

• Do not try to spam the usage of
  GetLocationZ
  . It might provoke desyncs. (See links call GetLocationZ, GetLocationZ {non-synchronous function; may cause desyncs. Should I be worried?}, Weird behaviour: GetLocationZ() does return incorrect value)

• Null your variables that are handles or extensions thereof.

Status:

• [color]Awaiting Update.[/color]

Notes:

• As the code is written in such a way that it would not normally compile on regular World Editor (before patch 1.30), I would suggest enclosing this in a scope.
• Caps-Locked variables are constants in normal JPAG convention, but in the spell, these are not constants.
• Functions can be preceded with a private prefix to save you some time in typing.
• In function Railgun_DamageVictims, a float literal was spotted on the declaration of a local real variable damage.
  local real damage = 0.16*GetUnitState(target,UNIT_STATE_MAX_LIFE)
• In function Railgun_DestructableBlackList, the statement can be shortened from the following:
  

  local unit caster           = GetTriggerUnit()
  local integer destructType  = GetDestructableTypeId(GetFilterDestructable())
     
  //if the destructable is alive and is not a pathblocker of any kind.
  if GetDestructableLife(GetFilterDestructable()) <= 0 then 
      return false
  endif
  if destructType == 'YTlb' then
      call SaveBoolean(RAILGUN_TABLE,GetHandleId(caster),StringHash("Railgun_Rangelimit"),true)
      return false
  endif
  if destructType == 'YTab' then
      call SaveBoolean(RAILGUN_TABLE,GetHandleId(caster),StringHash("Railgun_Rangelimit"),true)
      return false
  endif
  if destructType == 'YTpb' then
      call SaveBoolean(RAILGUN_TABLE,GetHandleId(caster),StringHash("Railgun_Rangelimit"),true)
      return false
  endif
  if destructType == 'YTfb' then 
      call SaveBoolean(RAILGUN_TABLE,GetHandleId(caster),StringHash("Railgun_Rangelimit"),true)
      return false
  endif
  call SaveBoolean(RAILGUN_TABLE,GetHandleId(caster),StringHash("Railgun_Rangelimit"),true)
  return true

  
  to
  
  

  local unit caster           = GetTriggerUnit()
  local integer destructType  = GetDestructableTypeId(GetFilterDestructable())
     
  //if the destructable is alive and is not a pathblocker of any kind.
  if GetDestructableLife(GetFilterDestructable()) <= 0 then 
      return false
  endif
  
  call SaveBoolean(RAILGUN_TABLE,GetHandleId(caster),StringHash("Railgun_Rangelimit"),true)
  return (destructType == 'YTlb') or (destructType == 'YTab') or (destructType == 'YTpb') or (destructType == 'YTfb')

• In function Railgun_UnitBlackList, you can delete the local variable declaration. (It is not particularly useful).
• In functions Railgun_Cast, Railgun_Stop, and Railgun_Fire, you can merge the checking of the correct ability being cast into said functions.
  You can also use TriggerAddCondition instead of TriggerAddAction if only for an incremental performance gain.
  
• In function Railgun_Cast, within the first loop, the variable visualizer is assigned to a newly created unit owned by the Passive player. You can store the passive player into a variable, since its' value is constant.
  
  The local group variable visualization is never nulled.
  
  
• In function Railgun_Stop, the locally declared group g is not nulled.
  
  As much as FirstOfGroup is preferred over ForGroup here, the possibility of ghost units being included may prematurely end the loop.
  
  
• In function Railgun_FireCleanup, using a FirstOfGroup over a ForGroup is preferable, since the dummy units aren't likely to be removed by anything else.
• In function Railgun_Fire, on the second loop, using a FirstOfGroup over a ForGroup is preferable, as the type of iteration is destructive.

Nitpicks:

• Using string hashes may be fine, but beware of using them too often, as they are likely to produce key collisions.
• You might want to add a warning to all who will use this, that this uses up one hashtable out of 255-256.
• The spell could use some constant functions, which store information about a particular aspect of the spell.

Status:

• Awaiting Update

 

[Bribe]

Instead of prefixing everything with Railgun_, use the "private" keyword.

This doesn't need a hashtable. A unit indexer, which many maps already have, would serve better (and faster).

Work towards implementing more of these kinds of changes (such as the ones from MyPad), and I'll return here to give it another look once you've done so. I want to see how many things you spot on your own, first, before I point them out.

This resource looks cool. Your code will get more refined. Lots of room for growth.

 

[Hazop / Sarven]

Is it possible to use a imported asset for the
private string VISUAL_HIT_MODEL = "Objects\\Spawnmodels\\NightElf\\NECancelDeath\\NECancelDeath.mdl"
I tried setting it as war3mapImported\ShrapnelShards.mdx (I'm using the shrapnel shards model) but it gave me an error and said "Invalid Escape character sequence".
Am I doing smth wrong or is it just impossible to set it to an imported model?

 

[Lt_Hawkeye]

You need double slashes like in the default path

 

[Hazop / Sarven]

You need double slashes like in the default path

Ah, I see, thank you!

 

[Cokemonkey11]

Past feedback has been addressed, code is configurable and well-commented. The spell is relatively simple but not one I see often. Approved.

 

[Cokemonkey11]

@Ralle this bundle was updated. How can I see a diff view so I can understand what may be in need of review?

 

[Insanity_AI]

I added a new map to the bundle, the vJASS version is still the same

 

[Ralle]

@Ralle this bundle was updated. How can I see a diff view so I can understand what may be in need of review?

Doesn’t really exist, sorry.