Water Ball v1.13

Moderator · Apr 6, 2010

Water Ball v1.11 | Reviewed by Maker | 29th Sep 2013

APPROVED

A cool spell that is leakless and MUI

Locust doesn't need to be configurable, add it in object editor.
Units with locust do not need invulnerability ability
The pathing type could be checked when the order is issued
Instead of GetWidgetLife, use UNIT_TYPE_DEAD check
If the angle and side do not change, you can precalcuate stuff here
set x2 = x2 + d*Cos(data.ang[i]+(data.side[i]*0.5*bj_PI)) set y2 = y2 + d*Sin(data.ang[i]+(data.side[i]*0.5*bj_PI))

[tr]

Water Ball v1.10 | Reviewed by Maker | 28th Sep 2013

NEEDS FIX

You are lacking importing instructions (xebasic dummy unit type confguration)
Some of the projectiles fly not to the targeted area,
but several hundreds of distance units away

Consider using Spell Effect Event
You could make it optional
You could merge actions with conditions, leaving you only with
condition functions
You don't need p variable in precast_t
The pathing type could be checked when the order is issued
The whole precast_t could be wrapped in static if
and don't create a trigger or register an event if the boolean is false
Instead of GetWidgetLife, use UNIT_TYPE_DEAD check

[tr]

07:45, 14th Apr 2010
TriggerHappy:

Not very original, but it's approvable.

You should be using TimerUtils or something for your timer attachment. Even better is it includes recycling which is far better than you destroying timers.
Remove those FirstOfGroup loops, they are fairly slow.
Improve your group usage by either using a global group or recycling groups (GroupUtils).
You can inline start_t.
Change if ON_PATH to static if ON_PATH then

Inferior · Apr 6, 2010

Ok Feedback.

Code looks ok, just some minor things.

no need to inline TriggerRegisterAnyUnitEventBJ!

maybe lower the Period_T from 0.04 to 0.03 because it looks kinda weird with itsi bitsi pauses in the movement

Maybe add a Channeling Animation to the Caster. He just watches the balls flying :O

JASS:

private function PreloadAbil takes integer id returns nothing
        local unit u = CreateUnit(Player(PLAYER_NEUTRAL_PASSIVE),DUMMY_ID,0,0,0)
        call UnitAddAbility(u,id)
        call UnitRemoveAbility(u,id)
        call RemoveUnit(u)
        set u = null
    endfunction

Pls don't make sth like that

. Import xepreload and use a static if.

Maybe i'll give you some more, only skimmed the code

Don't destroy Groups and Timers. Use TimerUtils and GroupUtils pls

Use SetUnitX and SetUnitY instead of SetUnitPosition

This:

JASS:

private function Enemy_Check takes nothing returns boolean
        local unit v = GetFilterUnit()
        local boolean b = false
        if IsUnitEnemy(v,TEMP_PLAYER) and GetWidgetLife(v) > .405 and IsUnitType(v,UNIT_TYPE_MAGIC_IMMUNE)==false and IsUnitType(v,UNIT_TYPE_FLYING) == false then
            set b = true
        endif
        set v = null
        return b
    endfunction

Can be:

JASS:

private function Enemy_Check takes nothing returns boolean
        return IsUnitEnemy(GetFilterUnit(),TEMP_PLAYER) and GetWidgetLife(GetFilterUnit()) > .405 and not IsUnitType(GetFilterUnit(),UNIT_TYPE_MAGIC_IMMUNE) and not IsUnitType(GetFilterUnit(),UNIT_TYPE_FLYING)
    endfunction

watermelon_1234 · Apr 7, 2010

Inferior said:
Use SetUnitX and SetUnitY instead of SetUnitPosition

You don't need to follow this suggestion if you want the unit's pathing to be considered, but you should know that using SetUnitX/Y is faster.

Inferior said:

JASS:

private function Enemy_Check takes nothing returns boolean
     return IsUnitEnemy(GetFilterUnit(),TEMP_PLAYER) and GetWidgetLife(GetFilterUnit()) > .405 and not IsUnitType(GetFilterUnit(),UNIT_TYPE_MAGIC_IMMUNE) and not IsUnitType(GetFilterUnit(),UNIT_TYPE_FLYING)
endfunction

Actually, it might be better if it was done your way with a few modifications since you use GetFilterUnit() multiple times.

JASS:

private function Enemy_Check takes nothing returns boolean
     local unit v = GetFilterUnit()
     local boolean b = IsUnitEnemy(v,TEMP_PLAYER) and GetWidgetLife(v) > .405 and IsUnitType(v,UNIT_TYPE_MAGIC_IMMUNE)==false and IsUnitType(v,UNIT_TYPE_FLYING) == false
     set v = null
     return b
endfunction

I think you should also know that the IsUnitType bug is fixed now.

You don't need two trigger variables in your init function, just reuse the first one.

FirstOfGroup loops are slower than doing the actions directly in the filter or with a ForGroup function.

This is my opinion, but you have many variables in your struct that don't really need to be there.

You could put constant in front of some of the functions, like Duration.

I think you don't need to null player variables.

PurgeandFire · Apr 7, 2010

Instead of using all those struct members, you should just save a member of ".lvl" and use that. (Actually, you don't even need to save a .lvl, you can just use the saved unit and then get the level when you need to)

Also try group recycling instead of creating a group each time. (Also switch from FirstOfGroup loops to Filter enumerations)

Basically:

JASS:

scope GroupExample

globals
    private constant group heheGroup = CreateGroup() //just create one group and use it
endglobals

function FilterEnumeration takes nothing returns boolean //returns boolean since it's used as a condition
    if GetWidgetLife(GetFilterUnit())>.405 then  //use GetFilterUnit(), this part is your boolean check
        //do your normal actions, pretend that it was a "ForGroup()"
        //except instead of using GetEnumUnit(), you use GetFilterUnit()
    endif
    return false //return false at the end, since it has to return boolean
endfunction

function UsageExample takes nothing returns nothing
    call GroupEnumUnitsInRect(g,GetWorldBounds(),Condition(function FilterEnumeration))
    //this is where you enumerate, just an example
    //the "Condition(function xxxx)" is where you are going to do all your actions
endfunction
//no need for destroying or clearing, enumerations auto-clear
//and you don't need to destroy since you'll just reuse the group

endscope

Just as an example. No need to really use GroupUtils since that is basically doing the above for you. (and it was to workaround with boolexpr null and leaks, but conditions/filters are auto-recycled and nulling as an input no longer leaks, so as long as you don't destroy boolexprs and know what you're doing, it should be fine)

You can also use TimerUtils as Inferior mentioned, but it isn't completely necessary.

Otherwise, nice job.

EDIT: I just looked at the spell and it looks awesome. Great eyecandy. =D

axleion · Apr 7, 2010

wow thanks for all of the feedback...
and yes, I will fix the code like you all suggested after I pass the exams week.
once again thank you....

Itachi009 · Apr 9, 2010

good job, idea is good (maybe add knockback when explode)
i like your coding style too

Lambdadelta · Apr 10, 2010

A spell I made for BlizzVault Spell Contest#1, unfortunately the contest looks to be shutdown before judging. So now I upload this spell here as my first resource (yippie!) as well want some judging about it. Thanks...

I was waiting for that competition to finish. Now it's dead.

FUJI · Sep 28, 2013

You updated it since in 2010 to 2013 xD.!

axleion · Sep 28, 2013

[HIGHLIGHT]Update v1.10[/code]

Changes

- Now uses TimerUtils, GroupUtils, xe.
- Remove abilitypreload function
- Changed missile movement from linear movement to parabolic movement
- Reworked from using one timer per instance of missile into one timer for all missiles
- Old spell code provided in test map for comparison

All suggested changes have been considered and necessary changes is implemented.
except ForGroup, because for me, FirstOfGroup looping is better than ForGroup enumeration as it avoid the need to transfer to other function.

jakeZinc said:
You updated it since in 2010 to 2013 xD.!

well yes, better late than never, its not funny for someone who encourage others to update their people spell if he didn't do it too. about the timespan, college can be time-consuming.

iAyanami · Sep 28, 2013

You could merge your Action codes into the Condition for both your precast and postcast.

JASS:

// this part does the pathingtype check and cancel the spell respectively.
    private function precast_t takes nothing returns nothing
        local unit u
        local player p

        if GetSpellAbilityId() == ABIL_ID then
            set u = GetTriggerUnit()

            static if ON_PATH then
                if IsTerrainPathable(GetUnitX(u),GetUnitY(u),PATH_TYPE) then
                    call PauseUnit(u,true)
                    call IssueImmediateOrder(u,"stop")
                    call PauseUnit(u,false)
                    set p = GetOwningPlayer(u)
                    call SimError(p,ERROR_MSG)
                    set p = null
                endif
            endif
        endif
        set u = null

        return false
    endfunction

private function init takes nothing returns nothing
        local trigger precast  = CreateTrigger()
        local trigger postcast = CreateTrigger()

        //this part setup for the pre-cast check
        call TriggerRegisterAnyUnitEventBJ( precast, EVENT_PLAYER_UNIT_SPELL_CAST )
        call TriggerAddCondition( precast , Condition(function precast_t) )
        ...
endfunction

Your static if for ON_PATH should be checked when you actually create the trigger, rather than on every cast. This would prevent unnecessary codes from being fired every time the spell is cast when ON_PATH is false.

Is there a reason why you're not using methods? Since you're using a struct, might as well make use of methods (I personally like using thistype

). Also, consider using array structs.

Your configuration functions could be constant functions.

precast_t
You don't need to declare the player variable, p. You're only calling GetOwningPlayer once.

effect_t
For movement timers, it's better utilize a timeout of 0.03125. I'd recommend you use either Timer32 or CTL. I prefer CTL though.

move_t
I realized you use data.g as a temporary group. If so, it's unnecessary for it to be a member. In fact, you don't even need to create a group. Simply use bj_lastCreatedGroup for all FirstOfGroup enumerations.

The way you're currently using FirstOfGroup is actually much slower than a ForGroup. FirstOfGroup is only faster if your filter is null. If not, that defeats the whole purpose of using a FirstOfGroup. Your "filtering" should be done within the FirstOfGroup loop itself, rather than using a enumeration filter.

You can leave weapon type for UnitDamageTarget as null.

You forgot to null your unit v.

axleion · Sep 28, 2013

iAyanami said:

You could merge your Action codes into the Condition for both your precast and postcast.

JASS:

// this part does the pathingtype check and cancel the spell respectively.
    private function precast_t takes nothing returns nothing
        local unit u
        local player p

        if GetSpellAbilityId() == ABIL_ID then
            set u = GetTriggerUnit()

            static if ON_PATH then
                if IsTerrainPathable(GetUnitX(u),GetUnitY(u),PATH_TYPE) then
                    call PauseUnit(u,true)
                    call IssueImmediateOrder(u,"stop")
                    call PauseUnit(u,false)
                    set p = GetOwningPlayer(u)
                    call SimError(p,ERROR_MSG)
                    set p = null
                endif
            endif
        endif
        set u = null

        return false
    endfunction

private function init takes nothing returns nothing
        local trigger precast  = CreateTrigger()
        local trigger postcast = CreateTrigger()

        //this part setup for the pre-cast check
        call TriggerRegisterAnyUnitEventBJ( precast, EVENT_PLAYER_UNIT_SPELL_CAST )
        call TriggerAddCondition( precast , Condition(function precast_t) )
        ...
endfunction

Your static if for ON_PATH should be checked when you actually create the trigger, rather than on every cast. This would prevent unnecessary codes from being fired every time the spell is cast when ON_PATH is false.

good idea, will do.

iAyanami said:
Is there a reason why you're not using methods? Since you're using a struct, might as well make use of methods (I personally like using thistype ). Also, consider using array structs.

I'm still learning about using method, so it might be better to left it in current way for now until I can implement method in it. by the way, I heard about "struct extend array" is better or what, but it require manual allocator/deallocator, and to implement it will require no struct method, so probably I will left it as it is for now and rework it whole later.

iAyanami said:
Your configuration functions could be constant functions.

I do remember constant configuration function mean that it can and will be inlined during compile time, probably will left it as it is, so users can modify the function to include several math operations. but I guess that can be done to the GetParabolaZ as its a fixed function, will update soon.

iAyanami said:
precast_t
You don't need to declare the player variable, p. You're only calling GetOwningPlayer once.

the only player variable p exist is only at precast_t which will be gone when changes above done.

iAyanami said:
effect_t
For movement timers, it's better utilize a timeout of 0.03125. I'd recommend you use either Timer32 or CTL. I prefer CTL though.

honestly, I refrain myself from using CTL or T32 or any other timer system other than TimerUtils. about the interval, that will be at user's discretion when implemented in their map, but default value won't hurt, will do.

iAyanami said:
move_t
I realized you use data.g as a temporary group. If so, it's unnecessary for it to be a member. In fact, you don't even need to create a group. Simply use bj_lastCreatedGroup for all FirstOfGroup enumerations.

The way you're currently using FirstOfGroup is actually much slower than a ForGroup. FirstOfGroup is only faster if your filter is null. If not, that defeats the whole purpose of using a FirstOfGroup. Your "filtering" should be done within the FirstOfGroup loop itself, rather than using a enumeration filter.

well that temp group is to maintain everything by specific instance of spell, will check if it won't hurt for multiple cast instance. about the FirstOfGroup loop, its slower factor is negligible, ForGroup enumeration will need another global variable to transfer the struct into enumeration, which I avoid doing so.

iAyanami said:
You can leave weapon type for UnitDamageTarget as null.

well it doesn't matter much because weapon type is used for sound on damage, but I think null won't hurt too, will do.

iAyanami said:
You forgot to null your unit v.

something I learn about FirstOfGroup loop is, to exit the loop, unit v must be null, so its already done automatically. no need to null it again.

Thank you for reviewing, iAyanami.

iAyanami · Sep 28, 2013

Dark_Axl said:
I do remember constant configuration function mean that it can and will be inlined during compile time, probably will left it as it is, so users can modify the function to include several math operations. but I guess that can be done to the GetParabolaZ as its a fixed function, will update soon.

Constant functions are basically functions that do not call any non-constant functions and do not use the set operation. Thus, mathematical operations still qualify the function to be constant.

Dark_Axl said:
honestly, I refrain myself from using CTL or T32 or any other timer system other than TimerUtils. about the interval, that will be at user's discretion when implemented in their map, but default value won't hurt, will do.

Is there a reason for not using the systems? Or is it just personal preference. CTL and T32 would be more efficient than TimerUtils when a lot of instances of spells are simultaneously occurring.

Dark_Axl said:
well that temp group is to maintain everything by specific instance of spell, will check if it won't hurt for multiple cast instance. about the FirstOfGroup loop, its slower factor is negligible, ForGroup enumeration will need another global variable to transfer the struct into enumeration, which I avoid doing so.

Yeah, you won't need to use ForGroup. Simply put the filter within the FirstOfGroup loop and set the filter parameter to null in the GroupEnumUnitsInRage function. Will give you slightly better performance that way (better performance is always better right?

). You'd need to change your filter function though (won't be able to use GetFilterUnit). Use a filter with 2 unit parameters, something like private function EnemyFilter takes unit caster, unit enemy returns boolean.

Dark_Axl said:
something I learn about FirstOfGroup loop is, to exit the loop, unit v must be null, so its already done automatically. no need to null it again.

Ah yes, forgot about that FirstOfGroup loop. My bad

Dark_Axl said:
Thank you for reviewing, iAyanami.

No problem

axleion · Sep 28, 2013

iAyanami said:
Constant functions are basically functions that do not call any non-constant functions and do not use the set operation. Thus, mathematical operations still qualify the function to be constant.

that part which hinder the use of set is why I avoid constant function, but user who use this should understand basic JASS, so yes, will do on all possible functions.

iAyanami said:
Is there a reason for not using the systems? Or is it just personal preference. CTL and T32 would be more efficient than TimerUtils when a lot of instances of spells are simultaneously occurring.

that's fully because of personal preference, but maybe when I'm convinced to use it, I'll add it.

iAyanami said:
Yeah, you won't need to use ForGroup. Simply put the filter within the FirstOfGroup loop and set the filter parameter to null in the GroupEnumUnitsInRage function. Will give you slightly better performance that way (better performance is always better right? ). You'd need to change your filter function though (won't be able to use GetFilterUnit). Use a filter with 2 unit parameters, something like private function EnemyFilter takes unit caster, unit enemy returns boolean.

actually I've done it like that before in my other map here, I found that the downside is the amount of function calls when a lot of units is inside the group to loop. that's why I use GroupEnumUnitsInRange with enum filter now.

[HIGHLIGHT]EDIT[/code]
I would like to respond to Maker's review,

Some of the projectiles fly not to the targeted area, but several hundreds of distance units away

I've already fixed that in my comp, its because I forgot to reindex data.x2 and data.y2 during reindexing.

The pathing type could be checked when the order is issued

I don't understand what this mean, judging from another review point, does that mean instead of catching spell cast event, it will be better to catch the orderid for pathing check?

Instead of GetWidgetLife , use UNIT_TYPE_DEAD check

I read that UNIT_TYPE_DEAD check is more unreliable because it has an update delay before labeled dead, unlike GetWidgetLife although latter has downside with setting dead unit's hitpoint above that 1.

[HIGHLIGHT]Update v1.11[/code]

Changes

- Fixed missile movement sometimes going off its trajectory
- Changed configurable function to constant function
- Changed precast trigger creation to be based on ON_PATH
- Changed the default interval period of movement timer
- Changed damage's weapontype from WEAPON_TYPE_WHOKNOWS to null
- Added SpellEffectEvent as optional library
- Reformat the trigger to support more readable trigger
- Added "How to Import" instruction inside the map and site

Maker · Sep 29, 2013

Dark_Axl said:
I don't understand what this mean, judging from another review point, does that mean instead of catching spell cast event, it will be better to catch the orderid for pathing check?

A unit is issued an order event.

Dark_Axl said:
I read that UNIT_TYPE_DEAD check is more unreliable because it has an update delay before labeled dead

Where did you read that? I don't believe it until I see proof.

I tested with this, unit was labelled dead when it was supposed to.

Untitled Trigger 001
- Events
  - Player - Player 1 (Red) Selects a unit
  - Unit - A unit Dies
- Conditions
- Actions
  - Unit - Kill (Triggering unit) // Enable/disable
  - Unit - Set life of (Triggering unit) to 0.00% // Enable/disable
  - Custom script: if IsUnitType(GetTriggerUnit(), UNIT_TYPE_DEAD) then
  - Game - Display to Player Group - Player 1 (Red) for 5.00 seconds the text: DEAD
  - Custom script: else
  - Game - Display to Player Group - Player 1 (Red) for 5.00 seconds the text: NOT DEAD
  - Custom script: endif

axleion · Sep 29, 2013

Maker said:
Where did you read that? I don't believe it until I see proof.

I don't remember the exact post, but I remember read that in a thread in World Editor Help Zone few months ago. yeah, probably I misread that. but still, it won't matter much using GetWidgetLife on this matter unless there's certain drawback with its use in this ability.

about the latest review, will do another update regarding that angle calculation, and locust ability.

about that pathing check on order issued, haven't test this but if the caster is issued order to cast the ability to target point outside its casting range, will the check happen when start moving to get into range or after the caster get into casting range? and does it have advantage than using unit begin cast event?

axleion · Oct 4, 2013

[HIGHLIGHT]Update v1.12[/code]

Changes

- Fixed spell didn't actually release timer and destroy struct after spell ends
- Changed how the missile speed defined, now it use missile life time to calculate missile speed
- Now use IsUnitType(UNIT_TYPE_DEAD) check instead of GetWidgetLife
- Now precalculate some value which remains constant for each missile
- Added locust to dummy unit

[HIGHLIGHT]Update v1.13[/code]

Changes

- Now only use 1 struct per cast, all previous versions use 2 struct per cast.
- Optimized precast function
- Cleaned up the spell code from garbage left from older version
- Added Handle Counter in test map to check possible leak
- Attached screenshot to show spell in effect from different point of view

coolyoshi1 · Mar 22, 2019

Seems this spell doesn't work anymore. Doesn't deal damage when cast on enemies.

Pyrogasm · Mar 22, 2019

coolyoshi1 said:
Seems this spell doesn't work anymore. Doesn't deal damage when cast on enemies.

Hilarious that in 6 years nobody bothered to see if this spell actually did anything. The last update uses the following filter function to find targets:

JASS:

    private function Enemy_Check takes nothing returns boolean
        local unit u = GetFilterUnit()
        local boolean b = IsUnitEnemy(u,TEMP_PLAYER) and IsUnitType(u,UNIT_TYPE_DEAD) and not IsUnitType(u,UNIT_TYPE_MAGIC_IMMUNE) and not IsUnitType(u,UNIT_TYPE_FLYING)
        set u = null
        return b
    endfunction

If you read that it actually only targets dead enemy non-flying units. Adding a "not" before the dead check will return the spell to normal working order, but be aware that it by default doesn't affect air units so if you test it on the dragons in the map it won't deal damage to them. The fixed line looks like this:

local boolean b = IsUnitEnemy(u,TEMP_PLAYER) and not IsUnitType(u,UNIT_TYPE_DEAD) and not IsUnitType(u,UNIT_TYPE_MAGIC_IMMUNE) and not IsUnitType(u,UNIT_TYPE_FLYING)

axleion · Sep 5, 2019

Pyrogasm said:
Hilarious that in 6 years nobody bothered to see if this spell actually did anything. The last update uses the following filter function to find targets:

JASS:

private function Enemy_Check takes nothing returns boolean local unit u = GetFilterUnit() local boolean b = IsUnitEnemy(u,TEMP_PLAYER) and IsUnitType(u,UNIT_TYPE_DEAD) and not IsUnitType(u,UNIT_TYPE_MAGIC_IMMUNE) and not IsUnitType(u,UNIT_TYPE_FLYING) set u = null return b endfunction

If you read that it actually only targets dead enemy non-flying units. Adding a "not" before the dead check will return the spell to normal working order, but be aware that it by default doesn't affect air units so if you test it on the dragons in the map it won't deal damage to them. The fixed line looks like this:

local boolean b = IsUnitEnemy(u,TEMP_PLAYER) and not IsUnitType(u,UNIT_TYPE_DEAD) and not IsUnitType(u,UNIT_TYPE_MAGIC_IMMUNE) and not IsUnitType(u,UNIT_TYPE_FLYING)

LOL, after 6 years, thanks for noticing that Pyro, that's what happen when I follow suggestion before actually fully understanding the suggestion first :grin:

Water Ball v1.13

Water Ball v1.13 (Map)

Moderator

Moderator

Similar threads