Holy v1.03

[Moderator]

12th Dec 2015
IcemanBo: Too long as NeedsFix. Rejected.

22:47, 2nd Aug 2014
BPower: Need Fix

http://www.hiveworkshop.com/forums/spells-569/holy-v1-03-a-253676/index2.html#post2561062

16:55, 22nd Jul 2014
BPower:
Not approve-able aslong as it depends on ExortKit.

 

[GywGod133]

Sir, This is English Site so use in english

 

[gorillabull]

this is a good spell but i doubt you made it seeing how the documentation is in english and you dont seem to speak it

 

[deathismyfriend]

He could of read rules and thought spells had to be submitted in English bit that you can post in any language. Although it is a stretch it is possible. I am a little on your side though gorilla.

It has a lot of inefficient coding. And uses GetLocalPlayer rather than gettriggeringplayer.

 

[PoWerJin]

one of the most awesome looking spells on hive, very original, i like it!

 

[dhguardianes]

I surprised!, when you sign Exorkit in here!.

 

[Tal'Rarity]

He knew to post the triggers/code. . . lol

 

[nhocklanhox6]

Very clear coding . But you have to rename library and method c and f if it isn't a part of frame and please explain your configurable.

If this is not a part of your habit, remove this. in front of variables

set this.SIN1[i] = Sin((i*bj_PI)/MOVE_TIME) set this.SIN2[i] = Sin((i*bj_PI)/(2*MOVE_TIME)) set this.temp = this.temp + this.SIN1[this.tack]*RADIUS*Cos(this.rand[this.i])*Cos(this.angle - bj_PI/2) set this.temp = this.temp + this.SIN1[this.tack]*RADIUS*Cos(this.rand[this.i])*Sin(this.angle - bj_PI/2)

Looks like i*bj_PI,RADIUS*Cos(this.rand[this.i])*Sin(this.angle - bj_PI/2) used twice during period, save it to variable and bj_PI/2 = 1.57078

set this.dummy[this.i] = CreateUnit(GetOwningPlayer(this.caster), DUMMY, xc, yc, 270)

Save GetOwningPlayer as a member of struct.

GetUnitState(this.target, UNIT_STATE_LIFE) > 0 => GetWidgetLife, this is faster than GetUnitState. You can simply use UnitAlive or IsUnitType(whichUnit,UNIT_TYPE_DEAD) and GetUnitTypeId(whichUnit) != 0 to assure a better death check.

Save ATTACK_TYPE_NORMAL, DAMAGE_TYPE_NORMAL on configurable. Get rid of BJs

This is using for? set this.fly = IsUnitType(this.target, UNIT_TYPE_FLYING) , if a unit have a fly height or SetUnitPathing=true without UNIT_TYPE_FLYING or a target is going to fly while this spell is casting?.

set this.facing[i] = fa + (2*bj_PI*(i - 1))/MAX_DUMMY

2*bj_PI = 6.28318

local integer i = 1 and (2*bj_PI*(i - 1))

=>

local integer i = 0 and (2*bj_PI*i)

set this.SIN2[i] = Sin((i*bj_PI)/(2*MOVE_TIME))

Save 2*MOVE_TIME and i*bj_PI to variable and this too set this.SIN3[i] = Sin((i*bj_PI)/(2*MOVE_TIME2))

I think you have to put function f to function c since function c is faster than function f (Conditions => Action) and then simply check it.

I think the member of hive, they are using JNPG and Regular WE than XDesign Pack v1.06.

Use timer is fast enough.

Inside library ExortKit has some useless function such as:

Click

function HideUnitInMap takes unit u returns nothing
        call UnitSetUsesAltIcon(u, true)
    endfunction

    function ShowUnitInMap takes unit u returns nothing
        call UnitSetUsesAltIcon(u, false)
    endfunction
function PrintPlayer takes player p, string s, real d returns nothing
        if d == 0 then
            call DisplayTextToPlayer(p, 0, 0, s)
        else
            call DisplayTimedTextToPlayer(p, 0, 0, d, s)
        endif
    endfunction
function Print takes string s, real d returns nothing
        if d == 0 then
            call DisplayTextToPlayer(GetLocalPlayer(), 0, 0, s)
        else
            call DisplayTimedTextToPlayer(GetLocalPlayer(), 0, 0, d, s)
        endif
    endfunction
 function GetChance takes real r returns boolean
        if (r >= 100) or GetRandomReal(0.00, 100.00) <= r then
            return true
        endif
        return false
    endfunction
       
    function IsGameDay takes nothing returns boolean
        if(GetTimeOfDay() > 6.00 and GetTimeOfDay() < 18.00)then
            return true
        else
            return false
        endif
    endfunction
       
    function GetPID takes player whichPlayer returns integer
        return GetPlayerId(whichPlayer)
    endfunction
       
    function GetUID takes unit u returns integer
        return GetPlayerId(GetOwningPlayer(u))
    endfunction
function GetHandleU takes unit u returns integer
        return GetHandleId(GetOwningPlayer(u))
    endfunction
       
    function GetHandleP takes player p returns integer
        return GetHandleId(p)
    endfunction
function GetWDB takes widget a, widget b returns real
        return GetDB(GetWidgetX(a), GetWidgetY(a), GetWidgetX(b), GetWidgetY(b))
    endfunction

    function GetWAB takes widget a, widget b returns real
        return GetAB(GetWidgetX(a), GetWidgetY(a), GetWidgetX(b), GetWidgetY(b))
    endfunction
function IsPlayerOnline takes player p returns boolean
        return GetPlayerSlotState(p) == PLAYER_SLOT_STATE_PLAYING and GetPlayerController(p)==MAP_CONTROL_USER
    endfunction
   
    function IsPlayerLeft takes player p returns boolean
        return GetPlayerSlotState(p) == PLAYER_SLOT_STATE_LEFT and GetPlayerController(p)==MAP_CONTROL_USER
    endfunction
   
    function IsPlayerEmpty takes player p returns boolean
        return (GetPlayerSlotState(p) == PLAYER_SLOT_STATE_EMPTY)
    endfunction

function PlayerId takes player whichPlayer returns integer
        return GetPlayerId(whichPlayer) + 1
    endfunction

//This function is using for?. Each spell has a different way of creating.

function CreateDummy takes player owner, integer unitid, real x, real y, real face, real dur returns unit
        local unit u = CreateUnit(owner, unitid, x, y, face)
        call SetUnitPathing(u, false)
        call SetUnitX(u, x)
        call SetUnitY(u, y)
        call UnitApplyTimedLife(u, 'BTLF', dur)
        return u
    endfunction
function SetFastAnimation takes unit whichUnit, real percentScale returns nothing
        call SetUnitTimeScale(whichUnit, percentScale * 0.01)
    endfunction
       
    function RemoveUnitEx takes unit c returns nothing
        call UnitApplyTimedLife(c, 'BTLF', 0.01)
    endfunction

And more...

They can do it without those function .

Using interface isn't a good idea.

More function => more compile => more spaces.

This spell is cool by the way, so 5/5 .

 

[bgcy_ro002]

I do will improve Holy code in the future

 

[kunkkakute]

I do will improve Holy code in the future

ok bro :3

 

[Wrda]

Uh... Holy spell seems very doll. I mean, it only causes damage, nothing more. It should have something special.

 

[edo494]

not compilable.

private struct data extends frame

you didnt provide frame anywhere

 

[Wrda]

not compilable.

private struct data extends frame

you didnt provide frame anywhere

He does. (Correct me if I'm wrong)

call SetFrameData(this)

 

[edo494]

he didnt provide implementation for interface/struct frame at the time of my post. He did include the ExortKit however, which contains the definition for interface frame

 

[btdonald]

Very Good )

 

[Bannar]

Not really "very good". Much can be improved, in current state it's unacceptable.

At first, and foremost: make it independant of "ExortKit". It's bad design to make script requiring some random pack of functions which share names with multiple popular libraries api which handle such operations better and more convenient than your wierd lib.

Struct fields and global variables count can definitely be reduced. Your approach can also use some improvements.
Merge conditions and actions -> apply whole expression via conditions, it is a common way to script spells and it has been for years now.

GetUnitState(this.target, UNIT_STATE_LIFE) > 0 into GetWidgetLife(target) > 0.405. Drop UnitDamageTargetBJ for native instead.

Code is unreadable, escpecially because of "this" spam. You could place functions into struct as static methods - when "this" has been defined, it will be automatically included before each of struct fields greatly improving readability.

Use one global timer instead of your triggers/evaluations/periodic event. Uhh..

 

[BPower]

Please code your spell without the ExortKit. Bannar basically pointed out the main reasons why your submission is not approve-able, aslong as it depends on the ExortKit.
Function packs are not very welcome in general. A good rule is that external libraries should only cover excacly their purpose e.g. a Timer library should only cover timer related code.
Of course this applies for public resources, what you prefer/do within your own map/coding is totally up to you.

 

[bgcy_ro002]

I have just updated my spell without depending on the ExortKit system. I'm very busy and this is the first time i've posted my spells in hive.
If u guys still dissatisfied at my spell, i will delete it.

 

[Bannar]

If you do so, the whole point of spell uploading is lost.
We do want you to upload new spells, join the community, have fun, spam jokes, find new friends.

However, we are also into teaching ppl new/better/more efficient/more convenient ways of scripting within war3 environment. Wouldn't you be better satisfied with your work if at the end you heard "good job, bgcy_ro002"? Surely you would. None of us is here to deny yours, or anybody else' work - contrary, we are here to help you develop your skills.

At the end its both, better for you and community.
Don't be afraid to ask any questions that may seem to trouble you.

 

[BPower]

Okay let's get started:

1. The AddEvent function is not really nessesary, because it doesn't add any extra efficiency or could be used as wrapper because your spell is a scope. If you wish to do so you could use RegisterPlayerUnitEvent or better SpellEffectEvent from our JASS section.
If not you could just add what AddEvent does into private function int.
Furthermore remove the line TriggerAddAction and do the integer comparison for the correct SpellAbilityId() directly in function f.
Talking about the naming a little bit more generic function names are appreciated. private function f --> private function run/spell/action/....

The current version of JNGP 2.x does highlight int and bool in its standart settings, it would be very nice if you avoid those in your code.

    private function int takes nothing returns nothing
        local trigger t = CreateTrigger()
        local integer index = 0
        loop
            call TriggerRegisterPlayerUnitEvent(t, Player(index), EVENT_PLAYER_UNIT_SPELL_EFFECT , null)
            set index = index + 1
            exitwhen index == bj_MAX_PLAYER_SLOTS
        endloop
        call TriggerAddCondition(t, Condition( function c ) )
    endfunction

2. I can only recommend to stick to the JPAG ruleset. It makes your code much more readable and clean looking.
e.g. set this.DAMAGE =... --> set this.damage =...
if bool then --> please relabel bool into a more self-explanatory variable name. You may also add one line in the globals and explain what this boolean actually does.

    private function f takes nothing returns boolean
        local real      fa
        local integer   i       =   1
        local data      this  
        
        if GetSpellAbilityId() == SPELL_ID then
            set this = data.create()
            .....
        endif
        return false
    endfunction

I will think about a better solution for your update function when I have more time. Currently it looks messy.

Don't feel discouraged, people here are just trying to help you. The spell itself looks cool, judging from the screenshot and video.

 

[bgcy_ro002]

ok, i will try to make it better

 

[Kazeon]

this is cool, coding is super neat. But you know what? Moderators would be loved to see some documentations in it.

And I believe it will be improved as the time passed by. So 5/5

This is a good start, keep going bro.

 

[Gismo359]

I really like the concept, and it looks awesome, too! Good luck in improving it!

EDIT: Also 100 posts yay! Triple digits all tha way accross the sky!

 

[BPower]

Nothing changed since my last review.

I would like to add a couple of things:

call UnitDamageTargetBJ(this.caster, this.target, this.DAMAGE, ATTACK_TYPE_NORMAL, DAMAGE_TYPE_NORMAL ) --> replace the BJ with native UnitDamageTarget

if IsUnitEnemy(this.target, GetOwningPlayer(this.caster)) and GetUnitState(this.target, UNIT_STATE_LIFE) > 0 and not IsUnitType(this.target, UNIT_TYPE_STRUCTURE) then --> This filter should be part of the user configuration.

Instead of Runtime you could use the much more convenient library TimerUtils.

 

[Deaod]

MOVE_TIME should be called MOVE_TICKS. Alternatively, you make MOVE_TIME behave as the name indicates and let the users configure a time directly there. The latter option is better for users, by the way.

Why do you filter again when damaging the unit? Shouldnt that already have been handled by the allowed targets of the ability?

Why do you use this.i as an index variable instead of a local variable?

 

[Bloody Wolf]

is the name suposed to be only Holy? i think it would be better to to slap something like Holy Strike or Holy/Divinine Judgement

 

[rarigate]

I don't care if this is 'Needs Fix', because this spell is great.