Esdo's "Demonic Pentagram" spell v1.1a

[Moderator]

IcemanBo: Too long as NeedsFix. Rejected.

22:35, 5th May 2014
BPower:
http://www.hiveworkshop.com/forums/spells-569/esdos-demonic-pentagram-spell-v1-1a-250990/#post2522746

 

[Kazeon]

• those globals inside configuration must be constant like private constant integer SOME_VARIABLE and check carefully how I write the variable name. it's a standart vJass writing convention
  
• use FoG looping instead of ForGroup
  

  loop
      set u = FirstOfGroup(g)
      exitwhen u == null
      // do actions here
      call GroupRemoveUnit(g, u)
  endloop

• not a big deal, but you may start the private integer Index = 0 from -1
  

•         local location l = GetSpellTargetLoc()
  
          set x = GetLocationX( l )
          set y = GetLocationY( l )

  please, dont use location on Jass.. use this native: GetSpellTargetX() and GetSpellTargetY()
  
• call TimerStart( Periodic, 0.03, true, function DemonicPentagramDraw )
  that periodic timer (0.03) can be included in the configuration
  private constant real PERIODIC = 0.03
  

•             call DisplayTextToPlayer( GetLocalPlayer(),0,0, "TimerFired")
          endif
          call DisplayTextToPlayer( GetLocalPlayer(),0,0, "Aesd bacchannalia")

  I think you forgot to remove that debug message?
  
• you should merge Trig_DemonicPentagram_Actions into Trig_DemonicPentagram_Conditions
  and that's just a bad function name
  
• set a = GetUnitFacing( Data[i].Draw )
  instead of getting the face on each loop isn't that better to store it into a variable array like (yup, I'm speed freaky):
  

                  call KillUnit( Data[i].Draw )
                  set Data[i].Draw = CreateUnit( p, DummyID , nx, ny, Facing[i] )
                  set Facing[i] = Facing[i] - 144

• local effect s = AddSpecialEffect( "Environment\\LargeBuildingFire\\LargeBuildingFire1.mdl", x, y )
  that effect path should be configurable
  
• if ( IsPlayerAlly( GetOwningPlayer( Data[Expire].Caster ), GetOwningPlayer(u)) == false ) then
  you could add a configurable filtering function
  
• private real TriSideSize = ( 0.95 * Size ) * 2
  it should be a constant
  private constant real TRI_SIDE_SIZE = ( 0.95 * Size ) * 2
  

•         //
          // "AttributeUsed" defines the used attribute for damage. You should edit only the latter
          // part of the var, that is, the "_" and the last three letters. Use _STR for strenght,
          // _AGI for agility and _INT for intelligence.
          private integer AttributeUsed = bj_HEROSTAT_STR
          //
          // "Damage" defines the damage dealt by the spell. This value is multiplied
          // by the casters "AttributeUsed".
          private real Damage = 4
          //
          // "DmgLvl" multiplies total damage done by the level of the spell fo the caster.
          // The final damage equasion is:
          // Hero Attribute * (( Damage var ) + ( Level of ability * DmgLvl var ))
          private real DmgLvl = 1

  it can be improved:
  

  // that private real DmgLvl is not neccesary
  // remove global Damage..
  // use this function so that user could calculate the damage by themselves
  
  private function getDamage takes integer level, integer att returns real
      // see that the function takes integer att which it's caster's used attribute
      // and imo it's better to improve the damage as we got higher ability level, isn't it?
      return 4 * att * level
  endfunction

  then you may call it on damage event:
  

  call UnitDamageTarget( Data[Expire].Caster, u, getDamage(abilityLevel, USED_ATTRIBUTE), true, false, ATTACK_TYPE_CHAOS, DAMAGE_TYPE_NORMAL, WEAPON_TYPE_WHOKNOWS)

  and weapon/damage/attack type should be configurable though..
  
• local effect s = AddSpecialEffect( "Environment\\LargeBuildingFire\\LargeBuildingFire1.mdl", x, y )
  afaik you should null it

I'm sorry for unreadable review.. I'm in hurry

it's edited. I'm sorry if there is maybe some mistakes because I typed them manually I'm not on my PC atm
and hey, you got a good start here keep going

 

[deathismyfriend]

You should inline this. GetHeroStatBJ

Since you are clearing the group every time it is used make it a global group. Create it once. You should also use FirstOfGroup loop rather than ForGroup loop. FirstOfGroup is faster and more efficient than forgroup. It will also allow you to get rid of a function call.

Don't use locations.
GetSpellTargetLoc()
Use these instead.
GetSpellTargetX() and this GetSpellTargetY()

 

[esdo]

UPDATED. Fixed all of the above but couldn't find the "GetSpellTargetX/Y()" native on my WE.

 

[deathismyfriend]

UPDATED. Fixed all of the above but couldn't find the "GetSpellTargetX/Y()" native on my WE.

They are not listed in JNGP. Use them and it will work.

You also never made a global group.

SetUnitX / Y are also faster than SetUnitPosition.

 

[Kazeon]

        //
        // "SPELL_DAMAGE" defines the damage dealt by the spell. This value is multiplied
        // by the casters "ATTRIBUTE_USED".
        private constant real SPELL_DAMAGE = 5
        //
        // "DAMAGE_LEVEL" multiplies total damage done by the level of the spell fo the caster.
        // The final damage equasion is:
        // ( Hero Attribute * SPELL_DAMAGE var ) * ( Level of ability * DAMAGE_LEVEL var )
        private constant real DAMAGE_LEVEL = 2

you forgot to remove it

merge those two configurable global declarations. and write damage function right after global declaration..

    private function GetDamageDealt takes real attribute, real level returns real
        return attribute * level // <== This will be the damage done.
    endfunction

level must be integer, idk about the attribut but as far as I remember it's an integer too..

some haven't fixed yet..

 

[esdo]

Okay now it's fixed. I think.

The att/level can be reals. If I change them to Integer the function's (return real) wont work, so I'll just leave them like that.

Removed unnecessary vars and locations.

I think that merging the global blocks would make the config page look abit messy.

 

[deathismyfriend]

Make groups global.
Make a struct variable for owner of caster since you use it a lot.

Remove the BJ GetHeroStatBJ

 

[Heinvers]

Awesome.

 

[BPower]

- set p = GetOwningPlayer( Data[Index].Caster ) --> set p = GetTriggerPlayer()
p could be a member of the struct, label it with "owner"
- call DestroyGroup( Data[Index].Effects ) This group does not exist at that point.
- You could use SetUnitX/Y instead of SetUnitPosition, because it is superior in terms of speed.
- Instead of creating multiple local groups, you could use one global group for the FoG enumeration.
- The BJ could be replaced.
- in IsUnitAlly you don't have to compare the condition with the boolean false --> if IsUnitAlly(...) then is enough

 

[deathismyfriend]

- in IsUnitAlly you don't have to compare the condition with the boolean false --> if IsUnitAlly(...) then is enough

This.
if ( IsPlayerAlly( GetOwningPlayer( Data[i].Caster ), GetOwningPlayer(u)) == false ) then
Should be this for a false boolean.
if not IsPlayerAlly( GetOwningPlayer( Data[i].Caster ), GetOwningPlayer(u)) then

 

[BPower]

1. Remove all generated variables from tactical dragon fire.

2. gg_trg_DemonicPentagram = CreateTrigger() --> local trigger t = CreateTrigger()

3. As you are using vJass, your function names should follow the JPAG. This also means that function names like Trig_DemonicPentagram_Conditions should be for instance Check or AbilityCondition.

4. SPELL1 and SPELL2 are not really suitable variable names.

5. call DestroyGroup( Data[Index].Effects ) might not exist in the first place.

6. In the loop you create a lot of local groups, which are not used at all.

7. if ( IsPlayerAlly( GetOwningPlayer( Data.Caster ), GetOwningPlayer(u)) == false ) then should be configurable. Furthermore GetOwningPlayer( Data[i].Caster ) is already set to local player variable p.
You also don't have to do a boolean comparison == false, just if ( IsPlayerAlly( p, GetOwningPlayer(u))) then is enough.

8. Struct members should be nulled properly.

9. Remove the BJ.