Dark Lightning Nova v1.3

Moderator · Jun 16, 2013

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

Changes made. Approved.

Chaosy · Jun 16, 2013

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 · Jun 16, 2013

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

msongyboi · Jun 16, 2013

Chaosy said:
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

deathismyfriend said:
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 · Jun 16, 2013

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 · Jun 17, 2013

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)

JASS:

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

->

JASS:

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 · Jun 17, 2013

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.

Almia said:
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 · Jun 17, 2013

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

EDIT:

Almia said:
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 · Jun 18, 2013

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 · Jun 18, 2013

deathismyfriend said:
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 · Jun 18, 2013

msongyboi said:
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

msongyboi said:
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.

msongyboi said:
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.

msongyboi 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 · Jun 18, 2013

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:

JASS:

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 · Jun 18, 2013

Almia said:
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:

JASS:

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 · Jun 18, 2013

I mean if next[0] = 0

JASS:

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

PurgeandFire · Jun 23, 2013

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:

JASS:

    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:

JASS:

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:

JASS:

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 · Jun 23, 2013

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

FRENGERS · Jun 23, 2013

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

+ REPT

PurgeandFire · Jun 23, 2013

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

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

msongyboi · Jun 23, 2013

done thank you guys

Dark Lightning Nova v1.3

Dark Lightning Nova v1.3 (Map)

Moderator

Moderator

Similar threads