Magic Light v1.3

[Moderator]

18th Apr 2016

Your resource has been reviewed by BPower.
In case of any questions or for reconfirming the moderator's rating,
please make use of the Quick Reply function of this thread.

Review:

Overall cool concept and presentation.
The code is so-so and leaves room for improvement. Read more in troubleshooting.
You could have considered that the caster can die while the spell is running.
The configuration options are a bit lousy for a level based spell.

You overdo white spaces without an obvious pattern. It hurts a bit the read-ability of your code.

Troubleshooting:

• You don't run any debug checks, hence set PSM[PM] = -2 is totally useless.
  ---
• You have tons of unnecessary GetUnitX/GetUnitY calls when creating the magic lights.
  ---
• Stick to one function naming convention. For example the JPAG.
  In your code I found function Heal() right after function getDamage(). Terrible.
  ---
• public struct MagicLightPlugin could preferably be a private struct.
  ---
• You could merge the creation of magic lights into one seperate function to shorten your code.
  ---
• The way you sorted your code compiles in a terrible amount of pseudo-code.
  Change the position of the two structs.
  ---
• You are using magic numbers in your code which could preferably be replaced with named constants.
  For example the value .0174533 or 99999.
  ---
• The dummy units could be paused to save extra computation time.
  ---

• private constant function filterFunc takes unit filterUnit returns boolean
      return not IsUnitType(filterUnit,UNIT_TYPE_DEAD) and GetOwningPlayer(filterUnit) != SKIP_PLAYER and GetUnitTypeId(filterUnit) != 0
  endfunction

  ^It beats me that the JassHelper compiles that function. It shouldn't be constant.

Review changelog:

1. -

 

[Malhorne]

Nothing much to say for a first look on the code :
You're still maniac by declaring null local handle ^^ !
Anyway that's nothing

 

[Heinvers]

WOW. Talk about being a star I'm not a coder but that looks great

 

[Malhorne]

WOW. Talk about being a star

What do you mean xD ?

 

[muzzel]

You should allow the user to use his own damage/heal formula. Instead of this:

private constant real DAMAGE_BASE = 20.
private constant real HEAL_BASE = 10.

use something like that:

private function damage takes unit caster, unit target returns real
        return 20.
endfunction
private function heal takes unit caster, unit target returns real
        return 20.
endfunction

Didnt read the rest of the code.

 

[Malhorne]

Ah yes you're right didn't read the globals just checked if there was constant where there should be constant and by the way you forgot the constant keyword in you example muzzel

 

[muzzel]

by the way you forgot the constant keyword in you example muzzel

What do you mean? Constant functions dont exist..

 

[TriggerHappy]

What do you mean? Constant functions dont exist..

Yes they do.

 

[muzzel]

Couldnt find them here: http://www.wc3c.net/vexorian/jasshelpermanual.html

Anyway the function is not supposed to be constant, because thats the exact point of using functions instead of constant variables - they can return something which depends on the passed arguments. Something like "return 10.+GetHeroInt(caster, true)*0.1".

 

[TriggerHappy]

Couldnt find them here: http://www.wc3c.net/vexorian/jasshelpermanual.html

Anyway the function is not supposed to be constant, because thats the exact point of using functions instead of constant variables - they can return something which depends on the passed arguments. Something like "return 10.+GetHeroInt(caster, true)*0.1".

That's because it's a JASS feature.

constant function foo takes nothing returns real
    return 5
endfunction

 

[Heinvers]

It was just an expression Malhorne.

 

[Adiktuz]

what's the point of making these formula functions constant? I kind of already forgot why people always say to make them constant.

 

[muzzel]

You dont make it constant, if you do you wont be able to call non-constant functions inside it, which is a useless restriction.

 

[BPower]

You could store GetUnitX/Y before the loop.
Also you could make the unit filter part of the user configuration, not only because many tend to use UnitAlive over other is unit dead checks.

I think this one is actually constant for all casts -> da = 360./LIGHT_COUNT*.0174533

Concept and effects are great. Based on a quick scan, the code is neat and readable.

 

[Malhorne]

I don't remember all the bonuses from constant funcs : but they're inlined if in one line and they also are faster.
TriggerHappy quoted something interesting in another topic but sadly I don't remember...

 

[muzzel]

Non-constant functions can be inlined too.
But really, you cannot make the function constant because it would be a useless restriction. (wouldnt allow non-constant functions to be called inside the function).

 

[Malhorne]

Non-constant functions aren't automatically inlined dude...

 

[BPower]

Non-constant functions aren't automatically inlined dude...

And constant functions are? Afaik the're not.

 

[muzzel]

1. Jasshelper inlines SOME stuff. Its rules for inlining are:

How to make a function inlineable? The current algorithm basically follows these rules (which are subject to change to allow more functions to be considered inlineable in the future):

The function is a one-liner
If the function is called by call the function's contents must begin with set or call or be a return of a single function.
If the inlined function is an assigment (set) it should not assign one of its arguments.
Every argument must be evaluated once and only once by the function, in the same order as they appear in the arguments list.
If the function contains function calls, they should all be evaluated after the arguments UNLESS the function is marked as non-state changing, at the moment some very few native functions and also return bug exploiters are considered as non-state changing.

(source)

As far as i can see it is completely irrelevant to the inliner whether a function is constant or not.

2. Other optimizers: There are other optimizers which can be run after or instead of JassHelper, for instance the Froptimizer from WurstPack. Froptimizer does a lot more optimizing than JassHelper (source) and its inliner is much more intelligent. But also for froptimizer it doesnt matter if a function is constant or not.

Maybe you confused constant functions with constant variables. Constant variables can of course be replaced (inlined) by their value.

 

[TriggerHappy]

Hey, nice spell I can see your getting better at JASS, but as always here's some comments.

• Inside onPeriodic, you could store GetUnitX/Y because it's called twice.
• This has no requirements, therefor you should be usingscopeinstead oflibrarywhich is pretty standard for spells anyway.
• I can't see where you destroy any MagicLight instance, so after 8191 casts this spell would break. Unless of course, I missed it? MagicLightPlugin is the only one that gets destroyed. Also, make sure to null all struct members that are handles (that get destroyed).
• Consider using GroupUtils'sENUM_GROUPinstead of creating a new group each spell. You can make this optional by using static if's.

As far as i can see it is completely irrelevant to the inliner whether a function is constant or not.

I'm pretty sure you're right.

Constant functions are useful for configuration in vanilla JASS, and they may provide a small performance benefit.

 

[BandolXD]

Could be useful in PVP Maps or RPG !
Lights everywhere < 3
3/5

 

[Malhorne]

Constant funcs : They provide a performance boost.

 

[muzzel]

Constant funcs : They provide a performance boost.

1. If you come up with these quotes you need to tell us where you found that. Otherwise its not authentic.

2. Its completely irrelevant to this topic because making the function constant would completely destroy its purpose (as i said like 3 times before).

 

[Malhorne]

Just benchmarked it with Sharpcraft :

TestCode

scope Stopwatch initializer onInit
    
    globals
        private constant integer ITERATIONS = 4000
    endglobals
    
    private function GetValue takes nothing returns integer
        return 5
    endfunction
    
    private constant function GetValu2 takes nothing returns integer
        return 5
    endfunction
    
    private function Actions takes nothing returns boolean
        local integer sw
        local integer i
        local real array ticks
        local string output
        local integer t

        set i  = 0
        set sw = StopWatchCreate()
            
        loop
            exitwhen i == ITERATIONS
            set t = GetValue()
            set i = i + 1
        endloop
            
        set ticks[0] = StopWatchTicks(sw)
        set output = I2S(ITERATIONS) + " iterations of Test #1 took " + I2S(StopWatchMark(sw)) + " milliseconds to finish.\n"
        call StopWatchDestroy(sw)
            
        set i  = 0
        set sw = StopWatchCreate()
            
        loop
            exitwhen i == ITERATIONS
            set t = GetValu2()
            set i = i + 1
        endloop
            
        set ticks[1] = StopWatchTicks(sw)
        set output = output + I2S(ITERATIONS) + " iterations of Test #2 took " + I2S(StopWatchMark(sw)) + " milliseconds to finish.\n\n"
        call StopWatchDestroy(sw)
            
        if (ticks[0] > ticks[1]) then
            set ticks[2] = 100 - (ticks[1]/ticks[0] * 100)
            set output = output + "Test #2 was " + R2S(ticks[2]) + "% faster than Test #1\n\n"
        else
            set ticks[2] = 100 - (ticks[0]/ticks[1] * 100)
            set output = output + "Test #1 is " + R2S(ticks[2]) + "% faster than Test #2\n\n"
        endif
        
        call DisplayTextToPlayer(GetLocalPlayer(), 0, 0, output)
        
        return false
    endfunction

    //===========================================================================
    private function onInit takes nothing returns nothing
        local trigger t = CreateTrigger()
        call TriggerRegisterPlayerEvent(t, Player(0), EVENT_PLAYER_END_CINEMATIC)
        call TriggerAddCondition(t, Condition(function Actions))
        set t=null
    endfunction

endscope

Result :
Test #2 was 9.365% faster than Test #1

On only 4000 iterations.
Now you have your proof.

 

[Adiktuz]

9% boost vs usability...

 

[muzzel]

9% boost vs usability...

No, if you dont use a non-constant function you can just use a constant global, which is even faster than constant functions.
Conclusion: use constant global or non-constant function, but constant function is pointless.

And not sure if 9% is still correct if the function has arguments... didnt test it.

 

[Malhorne]

Omg guys xD !!
When you can use constant functions without getting annoyed just use them it is free performance :O !!!

Testing with arguments....


EDIT : See the picture.
This is not a static percentage but the lowest I got is around 3% faster and the biggest was about 79% :O
The middle value is around 20-30%...

Test code :

TestCode

scope Stopwatch initializer onInit
    
    globals
        private constant integer ITERATIONS = 4000
    endglobals
    
    private function GetValue takes integer l returns integer
        return l*2
    endfunction
    
    private constant function GetValu2 takes integer l returns integer
        return l*2
    endfunction
    
    private function Actions takes nothing returns boolean
        local integer sw
        local integer i
        local real array ticks
        local string output
        local integer t

        set i  = 0
        set sw = StopWatchCreate()
            
        loop
            exitwhen i == ITERATIONS
            set t = GetValue(i)
            set i = i + 1
        endloop
            
        set ticks[0] = StopWatchTicks(sw)
        set output = I2S(ITERATIONS) + " iterations of Test #1 took " + I2S(StopWatchMark(sw)) + " milliseconds to finish.\n"
        call StopWatchDestroy(sw)
            
        set i  = 0
        set sw = StopWatchCreate()
            
        loop
            exitwhen i == ITERATIONS
            set t = GetValu2(i)
            set i = i + 1
        endloop
            
        set ticks[1] = StopWatchTicks(sw)
        set output = output + I2S(ITERATIONS) + " iterations of Test #2 took " + I2S(StopWatchMark(sw)) + " milliseconds to finish.\n\n"
        call StopWatchDestroy(sw)
            
        if (ticks[0] > ticks[1]) then
            set ticks[2] = 100 - (ticks[1]/ticks[0] * 100)
            set output = output + "Test #2 was " + R2S(ticks[2]) + "% faster than Test #1\n\n"
        else
            set ticks[2] = 100 - (ticks[0]/ticks[1] * 100)
            set output = output + "Test #1 is " + R2S(ticks[2]) + "% faster than Test #2\n\n"
        endif
        
        call DisplayTextToPlayer(GetLocalPlayer(), 0, 0, output)
        
        return false
    endfunction

    //===========================================================================
    private function onInit takes nothing returns nothing
        local trigger t = CreateTrigger()
        call TriggerRegisterPlayerEvent(t, Player(0), EVENT_PLAYER_END_CINEMATIC)
        call TriggerAddCondition(t, Condition(function Actions))
        set t=null
    endfunction

endscope

 

[muzzel]

When you can use constant functions

You cant, thats the problem, you lose flexibility.

If you dont care about that flexibility you can just use constant globals which are even faster than constant functions.

Read my posts again, i explained it like 3 times.

 

[Malhorne]

And I just proved right above that you can use constant func when damage depend on arguments too so please read....

 

[muzzel]

I think you misunderstood what the difference between a constant and a non-constant funtion is. A constant function cannot make calls to non-constant functions, so inside the body of a constant function you can only use other constant functions.

So if you simply want to return 20 damage then a constant integer variable is the fastest solution:

// Slow:
function damage takes ... returns integer
    return 20
endfunction

// Faster:
constant function damage takes ... returns integer
    return 20
endfunction

// Fastest:
constant integer DAMAGE = 20

Now lets say you want to allow the user to use an arbitrary damage formula, then you obviously cannot use a constant variable. So you can use:

// Slow:
function damage takes unit caster, unit target returns integer
    return // ...
endfunction

// Faster:
constant function damage takes unit caster, unit target returns integer
    return // ...
endfunction

Now the problem you apparently dont understand is that in constant functions you can only call other constant functions. So lets say the spell should deal 3 times the casters intelligence, then this is what our code looks like:

// Slow:
function damage takes unit caster, unit target returns integer
    return GetHeroInt(caster, true) * 3
endfunction

// Compile Error:
constant function damage takes unit caster, unit target returns integer
    return GetHeroInt(caster, true) * 3
endfunction

Now while the non-constant function works just fine the constant function will throw a compiler error:
"Call to non-constant function GetHeroInt in constant function"

This is because "GetHeroInt()" is a non-constant function: native GetHeroInt takes unit whichHero, boolean includeBonuses returns integer (see common.j)

 

[Malhorne]

Yes but only if the user wants to use non constant func then you can remove it but the performance boost isn't negligable....
Otherwise why in nearly each submission every time we say do make them constant zzz ??

 

[muzzel]

Yes but only if the user wants to use non constant func then you can remove it but the performance boost isn't negligable....

Id say if the user wants additional speed he should add the keyword, but adding it by default will cause "random" compile errors. Not such a cool thing..

Anyway, if a function can be made constant a good optimizer will do that for you.

Otherwise why in nearly each submission every time we say do make them constant zzz ??

The "because we always did" argument isnt very good. There are lots of stupid things we do daily..

 

[BPower]

In onCast you have local real o for no reason.
Basically everything used more than twice should be stored into a local varriable. Variable lookups are just so much faster than function calls. I'm talking about GetUnitX/Y.

Store Player(15) into a player variable.

For super duper speed you could do : private static constant real inBetween = 360./LIGHT_COUNT*.0174533.

I know you use it in all submissions, but now it's time to tell you that the word "intervar" does not exist in the english language.

 

[Malhorne]

Constant should be in globals block rather than inside the struct just conventional stuff ^^'

 

[muzzel]

Its not a configuration constant.

 

[Malhorne]

Using constant inside the struct is not that good pratice ...
In terms of definition of what's a struct/class ...

 

[muzzel]

Its really not important, but i think if a constant is only used by a certain structs methods it should be a member of the struct too.
I never heard that this is bad practice and i cant imagine why. Feel free to enlighten me if you have a reason.

 

[Malhorne]

EDIT : Nvm I have to find something before xD

 

[iAyanami]

Using constant inside the struct is not that good pratice ...
In terms of definition of what's a struct/class ...

I personally think it would make more sense to encapsulate the constant within the struct if the specific constant were to be only used inside the struct and non-configurable. A good example would be let's say a constant called PERIOD. If you declare 2 structs within a library that requires a PERIOD constant with different values, you'd need to declare PERIOD1 and PERIOD2 separately in the global block.

And it's perfectly normal for classes/structs to have constants. For example, in Java, the Math class contains PI as a constant double. Not really sure why you would consider it a bad practice.

 

[Malhorne]

Yeah you're right don't know what I drank this day xD

 

[MasterTrainer]

How on earth are you guys able to control shape movement without leaking or other problems like that?

 

[Malhorne]

Because THIS is vJASS XD !
Anyway I think we should close this off-topic.

 

[Ivanaruto]

I am new to this and I have a question ...

Because I install it as it says here, and then when I try to save the map I get hundreds of errors and tells me the detonator example "magic light" to been disabled ... Then I want to try and as expected no casts the spell .. . makes movement nothing more but the spell and / or effect never does, because many lines get error what is wrong? or what I'm doing wrong?

From already thank you friend

 

[Malhorne]

You have to use JNGP, because this is coded in vJASS.

Link to JNGP : http://blizzardmodding.info/4263/the-jass-newgen-pack-jngp-2-0/