[JASS]Shadow Slashes v1.0

[Moderator]

IcemanBo: Too long as NeedsFix. Rejected.

09:20, 7th May 2014
BPower:
http://www.hiveworkshop.com/forums/spells-569/jass-shadow-slashes-v1-0-a-250246/#post2523356

 

[chobibo]

constant function Ssla_stopOrder takes nothing returns integer
    return OrderId("stop")
endfunction

constant function Ssla_harvestOrder takes nothing returns integer
    return OrderId("harvest")
endfunction

You should replace those OrderId calls with constants, also, try to get rid of those location uses if possible.

Here's the OrderId list

 

[Kazeon]

glad we got new Jasser

CODE:

• call SetUnitTimeScalePercent(udg_Ssla_caster[index],100) => call SetUnitTimeScale(udg_Ssla_caster[index], 1)
  100% in integer equal to 1.00 in real
• call EnumDestructablesInCircleBJ(udg_Ssla_slashRadius[udg_Ssla_level[index]],l,function Ssla_TreeEnumeration)
  don't use BJ
  
  but you can keep that
  call TriggerRegisterAnyUnitEventBJ(gg_trg_Shadow_Slashes,EVENT_PLAYER_UNIT_SPELL_EFFECT)
  ala you only use it for once.

•     set udg_Ssla_mapX[0]=GetRectMinX(GetWorldBounds())
      set udg_Ssla_mapY[0]=GetRectMinY(GetWorldBounds())
      set udg_Ssla_mapX[1]=GetRectMaxX(GetWorldBounds())
      set udg_Ssla_mapY[1]=GetRectMaxY(GetWorldBounds())

  don't use array, use normal variable like minX, minY, maxX, maxY
• start maxindex from -1 and index from 0, but it's okay since you make it GUI-friendly so this is not a problem
• set l=Location(udg_Ssla_mapX[2],udg_Ssla_mapY[2])don't ever try to use location in Jass

• udg_Ssla_mapX[2]
  udg_Ssla_mapY[2]

  if I read your code correctly, it's coordinate of dummy unit, so it's not a good practice, just use local real for that.
  
  and don't use array if you are not using same type too often (in this case you are only using it three times which means you don't need to use array)
• Chobibro is right fix it

•                     set udg_Ssla_mapX[2]=udg_Ssla_orgX[index]+aoe*Cos(angle)
                      set udg_Ssla_mapY[2]=udg_Ssla_orgY[index]+aoe*Sin(angle)
                      if udg_Ssla_mapX[2]<udg_Ssla_mapX[0] then
                          set udg_Ssla_mapX[2]=udg_Ssla_mapX[0]
                      elseif udg_Ssla_mapX[2]>udg_Ssla_mapX[1] then
                          set udg_Ssla_mapX[2]=udg_Ssla_mapX[1]
                      endif

  IMO =>
  

                      set x = udg_Ssla_orgX[index]+aoe*Cos(angle)
                      if x < udg_Ssla_maxX then
                          call SetUnitX(udg_Ssla_caster[index], x)
                      else
                          call SetUnitX(udg_Ssla_caster[index], udg_Ssla_maxX)
                      endif

  and y is the same. x and y is local real
• call GroupRemoveUnit(udg_Ssla_tempGroup,udg_Ssla_caster[index])
  remove it, since if IsUnitEnemy(u,udg_Ssla_owner[index])
• set u=null
  remove it since the loop exitwhen u == null
• change
  • Set Ssla_slashAoE[1] = 500.00
  =>
  
  • Set Ssla_slashMaxDistance[1] = 500.00
  ?
• change
  • Set Ssla_slashRadius[1] = 100.00
  =>
  
  • Set Ssla_slashAoE[1] = 100.00
  ?
• no need to use array for this
  • Set Ssla_attachedEffectPoint[1] = hand left
  • Set Ssla_attachedEffectPoint[2] = hand right

OBJECT:

• none

MISCS:

• Give instructions how to import your spell
• the spell is not overkill and channeling which is nice, I like it.. and I think the concept is not very similliar with other slashes..

SUGGESTION:

• if not IsUnitType(u,UNIT_TYPE_DEAD) and IsUnitEnemy(u,udg_Ssla_owner[index]) and IsUnitType(u,UNIT_TYPE_GROUND) and not IsUnitType(u,UNIT_TYPE_MAGIC_IMMUNE) then
  add custom filtration function for user, so they can configure it
• Maybe give user option to choose wheter they want the target is random or the closest unit.
• Also give option for user if they don't want the caster to slash previous targets (can't slash the same target)
• learn vJass ASAP

 

[Lambdadelta]

Screenshot looks cool

 

[Malhorne]

To stop a unit it is better to use : call IssueImmediateOrderById(u, 851973)

 

[deathismyfriend]

You should not use this to detect dead units. It does not work.

GetWidgetLife(d)>0

Units are dead when they are lower than .405 health.
You should use this to properly detect dead / removed units.

GetWidgetLife( u) > .405 and not IsUnitType( u, UNIT_TYPE_DEAD) and GetUnitTypeId( u) != 0

Replace u with the unit you are checking.

Also as said above never use locations unless you are trying to get somethings Z height.

 

[msongyboi]

haha this is a really cute spell

very good for your first man

some things to consider for the next time:
- have a filter func that filters alive enemy units sth like

filter

    function AliveEnemyUnits takes nothing returns boolean
        return not IsUnitType(GetFilterUnit(), UNIT_TYPE_DEAD) and IsUnitEnemy(GetFilterUnit(), udg_filterReferencePlayer)
    endfunction

    function YourFunc takes nothing returns nothing
        set udg_filterReferencePlayer = (GetOwningPlayer(u))
        call GroupEnumUnitsInRange(g, x, y, 300., Filter(function AliveEnemyUnits)
        loop
            // Now you don't have check if a unit is an alive enemy or not, while looping through your group
            call UnitDamageTarget(blah blah)
        endloop
    endfunction

- consider using vJass and stuff like LinkedList modules and PeriodicTimer modules so you can save time not having to reindex spells

 

[deathismyfriend]

- have a filter func that filters alive enemy units sth like

filter

    function AliveEnemyUnits takes nothing returns boolean
        return IsUnitType(GetFilterUnit(), UNIT_TYPE_DEAD) and IsUnitEnemy(GetFilterUnit(), udg_filterReferencePlayer)
    endfunction

    function YourFunc takes nothing returns nothing
        set udg_filterReferencePlayer = (GetOwningPlayer(u))
        call GroupEnumUnitsInRange(g, x, y, 300., Filter(function AliveEnemyUnits)
        loop
            // Now you don't have check if a unit is an alive enemy or not, while looping through your group
            call UnitDamageTarget(blah blah)
        endloop
    endfunction

The above function does not accurately filter out alive and dead units. Please see my above post for the proper way to filter out units. But yes It's own function is nice as it makes it so the user can add units they do not want to be damaged by the spell.

 

[IcemanBo]

You could create one global location variable and then take usage of...

native MoveLocation takes location whichLocation, real newX, real newY returns nothing

instead of creating a local location that has to be cleaned after usage each time.

GetWorldBounds() can be stored into a variable. Also you don't really need array here. Use 4 normal real variables:

maxX
minX
maxY
minY

 

[BPower]

Haven't read previous comments, so there is a chance I repeat what was already mentioned:

The 4 world boundary variables should be global and non array, label them as IcemanBo said.

call SetUnitTimeScalePercent(...) should be SetUnitTimeScale(...), of course this laso changes the configuration part by factor 100.

set u=null is not required bec ause of the FoG loop.

The location is not required. You could also use EnumDestructablesInRect which only requires one global rect.

You have to stop the harvester in the end and the critical life for trees is 0.405 and not 0.00

udg_Ssla_mapX[2]/udg_Ssla_mapY[2] should be local real variables.

It looks not bad, still I'll set it to Need Fix for now.