Dark Lightning Nova v1.3

[Moderator]

16:36, 22nd Jun 2013
PurgeandFire: See my review:
http://www.hiveworkshop.com/forums/2365426-post15.html

Changes made. Approved.

 

[Chaosy]

1. the API is odd (have not seen something like that before)
2 why you got 2 global blocks? you can combine them

effects seem nice however

 

[deathismyfriend]

U should get rid of all locations. U should almost never need locations in jass. Also u should inline the GetPlayableMapRect function.

 

[msongyboi]

1. the API is odd (have not seen something like that before)
2 why you got 2 global blocks? you can combine them

effects seem nice however

I have no prior programming knowledge/experience before JASS, I don't know the norms and conventions so I just copy the styles or do what I think looks cute.

It's just to separate the configurables with the ones that are not lol

U should get rid of all locations. U should almost never need locations in jass. Also u should inline the GetPlayableMapRect function.

Gotcha, and sorry what does inline mean?

 

[deathismyfriend]

this should help with inlining http://www.hiveworkshop.com/forums/triggers-scripts-269/inlining-168433/

also i believe there is a tutorial that tells u the conventions.

 

[Almia]

1) Make all globals of this spell private.
2) Remove the prefixes if you did above
3) Make the timer global not constant(well)
4) Make all constant functions and functions private.
5) Use http://www.hiveworkshop.com/forums/jass-resources-412/snippet-worldbounds-180494/
6) Index the caster owner ( player array)
I see you have indexed the owner,but you are not using it in the If-blocks in the loop.
7) If you want a this syntax, why not do this in struct then?
8) move the exitwhen this == 0 above the loop code(after the loop) because you are always running an end instance.
9) Instead of checking the instances with prev[this] to start a timer,use a instance counter instead,whereas every instance,it increments the counter, every instance ends, it decrements.
10)

       local location lvLoc = GetSpellTargetLoc()
        local real x2 = GetLocationX(lvLoc)
        local real y2 = GetLocationY(lvLoc)

->

local real x2 = GetSpellTargetX()
local real y2 = GetSpellTargetY()

Its a nice spell,but it has to do the above changes. Btw, Your Comments are not indented properly.

 

[Mr_Bean]

Firstly, nice documentation (although the comment blocks shouldn't be indented as much; for readability).

As mentioned above, you should make all your globals private. Then you can remove the prefix from them. You may want to make the spell's raw ID public if you need to access it externally, but that is rarely done.

Btw, Your Comments are not indented properly.

There is no standard for commenting, although it is more readable if the comment blocks aren't indented.

 

[msongyboi]

^^ Okay Almia and Mr_Bean, thanks a lot for the comments I'll get those things done.

EDIT:

1) Make all globals of this spell private.
2) Remove the prefixes if you did above
3) Make the timer global not constant(well)
4) Make all constant functions and functions private.
5) Use [Snippet] WorldBounds
6) Index the caster owner ( player array)
I see you have indexed the owner,but you are not using it in the If-blocks in the loop.
7) If you want a this syntax, why not do this in struct then?
8) move the exitwhen this == 0 above the loop code(after the loop ) because you are always running an end instance.
9) Instead of checking the instances with prev[this] to start a timer,use a instance counter instead,whereas every instance,it increments the counter, every instance ends, it decrements.
10)

1. done
2. done
3. does it matter?
4. done
5. does it matter?
6. I use it for caster damage target and isunitenemy player
7. I wanted to prove to myself that structs aren't as magical as I thought they were
8. hm.. not sure it matters whether its at top or very bottom, it'll exit before any evaluations are made won't it?
9. sorry I don't understanddddd wai I have to do that.
10. done

 

[deathismyfriend]

2) yes it will make things shorter and faster.
5) it will help narrow things down to a better area for any map this is used on. so it will make it easier for the ppl that use this.
7) structs are nice to use. yes u can do it without them but using them is both faster and more efficient.
8)if its at the top it will run one less instance
9) that is basic indexing. it should keep from bugs happening.

 

[msongyboi]

2) yes it will make things shorter and faster.
5) it will help narrow things down to a better area for any map this is used on. so it will make it easier for the ppl that use this.
7) structs are nice to use. yes u can do it without them but using them is both faster and more efficient.
8)if its at the top it will run one less instance
9) that is basic indexing. it should keep from bugs happening.

2. constant Timer or function prefixes? I took care of the prefixes
5. gotcha, how can I set this up?

do I do something like if
if x[this] > WorldBounds.maxX then
set x[this] = WorldBounds.maxX
endif

EDIT: constant timer vs timer - if there is only one timer running does it even matter which one i use?
5 is done. I changed the WorldBounds to bj_mapInitialPlayableArea so game doesn't crash for people
okay I honestly don't understand 7, 8, and 9
I'm doing what a struct does for me in vJASS compiler, what difference will it make if I do it or the computer does it.

local integer i = 0

loop
exitwhen i == 1
blah
blah
blah
set i = i + 1
endloop

vs

loop
blah
blah
blah
set i = i + 1
exitwhen i = 1
endloop

mine's the 2nd loop, you're telling me the 1st loop will exit first - even though in order for the exit evaluation to trigger it needs to go back to the top of the loop? Plus it looks like the same thing to me - unless there's something about loops i dont understand.
how do I achieve 9, can I get an explicit example?

but please, I'm not trying to be rude - I just don't get it and I'm asking questions - correct me.

 

[deathismyfriend]

2. constant Timer or function prefixes? I took care of the prefixes
5. gotcha, how can I set this up?

do I do something like if
if x[this] > WorldBounds.maxX then
set x[this] = WorldBounds.maxX
endif

EDIT: constant timer vs timer - if there is only one timer running does it even matter which one i use?
5 is done. I changed the WorldBounds to bj_mapInitialPlayableArea so game doesn't crash for people
okay I honestly don't understand 7, 8, and 9
I'm doing what a struct does for me in vJASS compiler, what difference will it make if I do it or the computer does it.

it creates extra code when u dont use structs. structs compile into faster simpler code if u use the extends array.
also it does the indexing for u so its less variables as well.
try to look at some of my systems in my sig. they all have structs in them

local integer i = 0

loop
exitwhen i == 1
blah
blah
blah
set i = i + 1
endloop

vs

loop
blah
blah
blah
set i = i + 1
exitwhen i = 1
endloop

in that case it doesnt really matter but the conventional way is keep it at the top.

mine's the 2nd loop, you're telling me the 1st loop will exit first - even though in order for the exit evaluation to trigger it needs to go back to the top of the loop? Plus it looks like the same thing to me - unless there's something about loops i dont understand.
how do I achieve 9, can I get an explicit example?

no that was a miscommunication made by me. it was just for convention.
i thought u had a nested loop lol i didnt really look at the code. i just looked at the questions u had on what almia said.

but please, I'm not trying to be rude - I just don't get it and I'm asking questions - correct me.

thats ok asking for help is never rude. i dont mind helping ppl lol. kinda y i have so many posts on this site.

 

[Almia]

The reason why I want you to move that to the top is because it is a Linked List.
If an instance ends,you are still running the function even though it is already ended. That is in my opinion. Its better if you use exitwhen as the first code in the loop.

Its like:

loop
    exitwhen this == 0 // If is 0,below codes are not called.
    call BJDebugMsg("HAHAHA")
    set this = n[this]
endloop]
VS
[code=jass]
loop
    call BJDebugMsg("HAHAHA") //even though the instance is already ended,it is still executed.
    set n[this] = this
    exitwhen this == 0

Btw,it also execute the functions even though the instances are empty.

 

[msongyboi]

The reason why I want you to move that to the top is because it is a Linked List.
If an instance ends,you are still running the function even though it is already ended. That is in my opinion. Its better if you use exitwhen as the first code in the loop.

Its like:

loop
    exitwhen this == 0 // If is 0,below codes are not called.
    call BJDebugMsg("HAHAHA")
    set this = n[this]
endloop]
VS
[code=jass]
loop
    call BJDebugMsg("HAHAHA") //even though the instance is already ended,it is still executed.
    set n[this] = this
    exitwhen this == 0

Btw,it also execute the functions even though the instances are empty.

I guess you are right about it running once no matter what. But I don't think I can really do anything to change that.

But what do you mean functions will run when instances are empty?

btw Almia, thanks for your comments - I appreciate it a lot.

 

[Almia]

I mean if next[0] = 0

loop
    call BJDebugMsg("HAHAHAHA") //called even though next[0] = 0
    set this = next[this]
    exitwhen 0 == this
endloop

 

[PurgeandFire]

• In the initialization function, call TriggerAddCondition(t, function dlnNova) should be call TriggerAddCondition(t, Condition(function dlnNova)), or else it might not work on certain compilers.
• This function:
  

      private function dlnNova takes nothing returns boolean
          local unit lvUnit = GetTriggerUnit()
          local real x1 = GetUnitX(lvUnit)
          local real y1 = GetUnitY(lvUnit)
          local real x2 = GetSpellTargetX()
          local real y2 = GetSpellTargetY()
          local real lvAng = bj_RADTODEG * Atan2(y2 - y1, x2 - x1)
          
          if GetSpellAbilityId() == ID then
              call dlnCreate(lvUnit, x2, y2, lvAng)
          endif
          
          set lvUnit = null
          return false
      endfunction

  Can be compressed down to:
  

  private function dlnNova takes nothing returns boolean
      local unit lvUnit = GetTriggerUnit()
      local real x2 = GetSpellTargetX()
      local real y2 = GetSpellTargetY()
      local real lvAng = bj_RADTODEG * Atan2(y2 - GetUnitY(lvUnit), x2 - GetUnitX(lvUnit))
          
      if GetSpellAbilityId() == ID then
          call dlnCreate(lvUnit, x2, y2, lvAng)
      endif
      return false
  endfunction

  Alternatively, you can compress the entire thing down to:
  

  private function dlnNova takes nothing returns boolean
      local unit u = GetTriggerUnit()
      if GetSpellAbilityId() == ID then
          call dlnCreate(u, GetSpellTargetX(), GetSpellTargetY(), bj_DEGTORAD*Atan2(GetSpellTargetY() - GetUnitY(u), GetSpellTargetX() - GetUnitX(u)
      endif
      set u = null
      return false
  endfunction

  That one might be slightly slower since it has extra function calls, but it is viable depending on your guidelines for declaring locals. For some people, 2 or more calls of the same function (such as GetTriggerUnit()) indicates that you should declare a local. For others, it is 3 or more calls. I personally would choose the one that has the locals, since the difference is negligible.
• In dlnCreate, set play[this] = GetOwningPlayer(caster[this]) can simply be set play[this] = GetTriggerPlayer()
• In the function dlnLight, you use GetOwningPlayer(caster[this]) when you can just use play[this]
• When you make lvAng, you convert radians to degrees. However, when you actually use it in the loop script, you convert the degrees to radians. You should just leave lvAng as radians and then remove the multiplication in the loop script.
• When you have this: elseif dist[this] > MAX_DIST(LEVEL(caster[this])) then, I'm assuming you are ending that instance because that spell instance is over. However, after that block you still end up creating a dummy unit and setting its x and y, etc. You shouldn't create the dummy if the spell instance is over.

Make those changes and I think it should be approvable.

 

[msongyboi]

Thank you Purge, I've done as yout outlined ^^

 

[FRENGERS]

You should give some colour on your description :/ :/

+ REPT

 

[PurgeandFire]

Thank you Purge, I've done as yout outlined ^^

Make sure you update the code in the description. Then I'll approve.

 

[msongyboi]

done thank you guys