MountainBoulder v1.01

This bundle is marked as approved. It works and satisfies the submission rules.
Mountain Boulder ver 1.01

Required: GUI Unit Indexer 1.2.0.2 by Bribe (or other)
UNIT INDEXER
WorldBounds Library

Instalation
copy & paste: 2 units and 2 abilities
copy & paste this trigger, set raw codes, change settings if you want

Spell cancellation
You can cancel all spell instances any time by calling this function:
call CancelSpell_MountainBoulder()
It may be useful for Cinematics, when moving heroes in arena etc.

**Mountain Boulder**
spell type: not channeling, unit-target, MUI

Hero throws a boulder which follows the target. On its way boulder damages enemies by 1/4 damage, additionally pieces of rock break off and damage random enemy by 10dmg every 1sec. If boulder reach the target it explodes and deals 100/200/300 damage to all enemies (and buildings) in 250aoe and immobilize target (only) for 2/3/4sec. If boulder doesn't reach the target in 20/22/24sec (or target dies) boulder will explode and not deals damage. Damage type: physical (can attack magic-immune units, applies armor value) note: spell based on "channel" orderID: 'thunderbolt' /can be changed/

code:
JASS:
//------- Spell **Mountain Boulder** ver1.01 by ZibiTheWand3r3r
// spell type: not channeling, unit-target, MUI
//
// ------- Description:
// Hero throws a boulder which follows the target. On its way boulder damages enemies by 1/4 damage,
// additionally pieces of rock break off and damage random enemy by 10dmg every 1sec.
// If boulder reach the target it explodes and deals 100/200/300 damage to all enemies (and buildings) in 250aoe
// and immobilize target (only) for 2/3/4sec. If boulder doesn't reach the target in 20/22/24sec (or target dies)
// boulder will explode and not deals damage. Damage type: physical (can attack magic-immune units, applies armor value)
// note: spell based on "channel" orderID: 'thunderbolt' /can be changed/
//
//------- Requirements:
//     GUI Unit Indexer 1.2.0.2 by Bribe (or other)
//       http://www.hiveworkshop.com/forums/spells-569/gui-unit-indexer-1-2-0-2-a-197329/
//     WorldBounds
//       https://github.com/nestharus/JASS/blob/master/jass/Systems/WorldBounds/script.j
//
//-------- Instalation:
// copy & paste: 2 units (DUMMY, BOULDER_MISSLE)
// copy & paste: 2 abilities (ABI_MOUNTAIN_BOULDER, ABI_SHARDS)
// copy & paste this trigger, change settings

//--------Spell cancellation:
// You can cancel all spell instances any time by calling this function:
//      call CancelSpell_MountainBoulder()
// It may be useful for Cinematics, when moving heroes in arena etc.
//

library MountainBoulder initializer Init uses WorldBounds   
    private function IsUnitAlive takes unit u returns boolean
        return not IsUnitType(u, UNIT_TYPE_DEAD) and GetUnitTypeId(u) != 0
    endfunction
   
    // -----start of user configuration ---------------------------
   
    globals    
        // please make sure these raw codes are the same as copied into your map:
        private constant integer DUMMY = 'n000'
        // main ability id :
        private constant integer ABI_MOUNTAIN_BOULDER = 'A000'
        private constant integer BOULDER_MISSLE = 'e000'
        // additional ability ABI_SHARDS is based on PhoenixFire: it shoots random enemy every 1sec/10dmg
        // attack speed and damage can be changed in OE :
        private constant integer ABI_SHARDS = 'A001'
        // by default missle deals physical damage (can attack magic-immune units, applies armor value)
        private constant attacktype ATTACK_TYPE = ATTACK_TYPE_MELEE     
        private constant damagetype DAMAGE_TYPE = DAMAGE_TYPE_NORMAL
        // effect: enemies hit during movement of the missle
        private constant string ON_IMPACT_FX = "Abilities\\Weapons\\AncientProtectorMissile\\AncientProtectorMissile.mdl"
        // effect: immobilize main target on hit
        private constant string IMMOBILIZE_FX = "Abilities\\Spells\\Orc\\StasisTrap\\StasisTotemTarget.mdl"
        // effect: when missle hit main target and explodes
        private constant string ON_EXPLOSION_FX = "Abilities\\Spells\\Human\\ThunderClap\\ThunderClapCaster.mdl"
    endglobals
   
    // this damage is deal upon impact, by default it is 100/200/300dmg for 3 levels spell
    private constant function Damage takes integer level returns real
        return level * 100.00
    endfunction
    // this damage is deal when missle travels, by default it is 25/50/75dmg for 3 levels
    private constant function PartialDamage takes integer level returns real
        return level * 25.00
    endfunction
    //set how long missle exists (by default it is: 20/22/24sec for 3 levels)
    private constant function MissleDuration takes integer level returns real
        return (2.00*level) + 18.00
    endfunction
    // set immobilize duration; when target is hit he won't be able to move for this amout of time
    // by default it is 2/3/4sec for 3 levels:
    private constant function ImmobilizeDuration takes integer level returns real
        return level + 1.00
    endfunction
    //  missle speed, suggested range: 7-25 (example: value 8.44 is equal to unit 270 speed)
    // higher value=faster speed; recomended: 11.00
    private constant function MissleSpeed takes nothing returns real
        return 11.00
    endfunction
    // visual effect: missle scale; on higher spell levels missle will be bigger
    // for high level spell (~10 or more levels) use 0.05 instead of 0.10
    private constant function MissleScale takes integer level returns real
        return (0.10*level) + 1.50
    endfunction
    //when missle reach its target and explode, it deals aoe damage
    private constant function ExplosionRadius takes nothing returns real
        return 250.00
    endfunction
    //during movement missle deals aoe damage
    private constant function MissleOnMoveDmgRadius takes nothing returns real
        return 200.00
    endfunction
    //filter target units
    private function TargetUnitFilter takes unit u, player pla returns boolean
        return IsUnitEnemy(u, pla) and IsUnitAlive(u)
    endfunction
   
    // -----end of user configuration ----------------------------   
   
    globals
        private unit array spellMissle
        private unit array spellTargetUnit
        private real array spellDuration
        private integer array spellLevel
        private effect array spellEffect
        private group array spellGroupVictims
        private group spellGroup
        private group ug
        private timer spellTimer
    endglobals
   
    private function Cancel takes nothing returns nothing
        set spellDuration[GetUnitUserData(GetEnumUnit())] = 0.00
    endfunction
    function CancelSpell_MountainBoulder takes nothing returns nothing
        call ForGroup(spellGroup, function Cancel)
    endfunction
    //-----------------------------------------------------------
    // ----- START MOUNTAIN BOULDER SPELL ------------
    //-----------------------------------------------------------
    private function LoopEnum takes nothing returns nothing
        local unit d = GetEnumUnit()
        local integer id = GetUnitUserData(d)
        local player pla = GetOwningPlayer(d)
        local unit u
        local real x = GetUnitX(spellMissle[id])
        local real y = GetUnitY(spellMissle[id])
        local real Tx = GetUnitX(spellTargetUnit[id])
        local real Ty = GetUnitY(spellTargetUnit[id])
        local real angle = Atan2(Ty-y, Tx-x)
        local real Tx1 = x + MissleSpeed() * Cos(angle)
        local real Ty1 = y + MissleSpeed() * Sin(angle)
   
        if not IsUnitAlive(spellTargetUnit[id]) then
            set spellDuration[id] = 0.00
        elseif IsUnitAlive(spellMissle[id]) then
            call SetUnitFacing(spellMissle[id],  bj_RADTODEG * angle)
            if IsUnitInRangeXY(spellMissle[id], Tx, Ty, 64.00) then // missle reach its target:
                call DestroyEffect(AddSpecialEffect(ON_EXPLOSION_FX, Tx, Ty))
                call GroupEnumUnitsInRange(ug, x, y, ExplosionRadius(), null)
                loop
                    set u = FirstOfGroup(ug)
                    exitwhen u == null               
                    if TargetUnitFilter(u, pla) then
                        call UnitDamageTarget(d, u, Damage(spellLevel[id]), true, false, ATTACK_TYPE, DAMAGE_TYPE, WEAPON_TYPE_WHOKNOWS)
                        if u == spellTargetUnit[id] and not IsUnitType(u, UNIT_TYPE_STRUCTURE) then
                            call SetUnitPropWindow(spellTargetUnit[id], 0.00) //immobilize
                            set spellEffect[id] = AddSpecialEffectTarget(IMMOBILIZE_FX, spellTargetUnit[id], "overhead")
                        endif
                    endif
                    call GroupRemoveUnit(ug, u)
                endloop
                call KillUnit(spellMissle[id])
                call UnitRemoveAbility(d, ABI_SHARDS)
                if spellDuration[id] > 0.00 then
                    set spellDuration[id] = ImmobilizeDuration(spellLevel[id])
                endif
            // move missle, if coordinates are inside map:
            elseif Tx1 >= WorldBounds.minX and Tx1 <= WorldBounds.maxX and Ty1 >= WorldBounds.minY and Ty1 <= WorldBounds.maxY then
                call SetUnitX(spellMissle[id], Tx1)
                call SetUnitY(spellMissle[id], Ty1)
                call SetUnitX(d, Tx1)
                call SetUnitY(d, Ty1)           
                call GroupEnumUnitsInRange(ug, Tx1, Ty1, MissleOnMoveDmgRadius(), null) // make damage while missle moving
                loop
                    set u = FirstOfGroup(ug)
                    exitwhen u == null
                    if TargetUnitFilter(u, pla) and (not IsUnitInGroup(u, spellGroupVictims[id])) and u != spellTargetUnit[id] then
                        call DestroyEffect(AddSpecialEffectTarget(ON_IMPACT_FX, u, "origin"))
                        call UnitDamageTarget(d, u, PartialDamage(spellLevel[id]), true, false, ATTACK_TYPE, DAMAGE_TYPE, WEAPON_TYPE_WHOKNOWS)
                        call GroupAddUnit(spellGroupVictims[id], u)
                    endif
                    call GroupRemoveUnit(ug, u)
                endloop
            else // new target point outside map:
                set spellDuration[id] = 0.00
            endif
        endif

        set spellDuration[id] = spellDuration[id] - 0.03125
        if spellDuration[id] <= 0.00  then
            // Clean up any data attached to this spell
            call SetUnitPropWindow(spellTargetUnit[id], GetUnitDefaultPropWindow(spellTargetUnit[id]) * bj_DEGTORAD) //allow move
            call DestroyGroup(spellGroupVictims[id])
            set spellGroupVictims[id] = null
            call DestroyEffect(spellEffect[id])
            call GroupRemoveUnit(spellGroup, d)
            call KillUnit(spellMissle[id])
            call KillUnit(d)
            set spellMissle[id] = null
            set spellTargetUnit[id] = null
            if FirstOfGroup(spellGroup) == null then
                call PauseTimer(spellTimer)
            endif
        endif
        set u=null
        set d=null  
    endfunction
    //---------------------
    private function Loop takes nothing returns nothing
        call ForGroup(spellGroup, function LoopEnum)
    endfunction
    //---------------------
    private function OnCast takes nothing returns nothing
        local unit u = GetTriggerUnit()
        local player pla = GetTriggerPlayer()
        local unit d = CreateUnit(pla, DUMMY, GetUnitX(u), GetUnitY(u), 0.00)
        local integer id = GetUnitUserData(d)
        call UnitAddAbility(d, ABI_SHARDS)
        set spellMissle[id] = CreateUnit(pla, BOULDER_MISSLE, GetUnitX(u), GetUnitY(u), 0.00)
        set spellTargetUnit[id] = GetSpellTargetUnit()
        set spellLevel[id] = GetUnitAbilityLevel(u, ABI_MOUNTAIN_BOULDER)
        set spellDuration[id] = MissleDuration(spellLevel[id])
        call SetUnitScale(spellMissle[id], MissleScale(spellLevel[id]), 0.00, 0.00)
        set spellGroupVictims[id] = CreateGroup()
        if FirstOfGroup(spellGroup) == null then
            call TimerStart(spellTimer, 0.03125, true, function Loop)
        endif
        call GroupAddUnit(spellGroup, d)
        set u=null
        set d=null
    endfunction
    //---------------------
    private function Cond takes nothing returns boolean
        if GetSpellAbilityId() == ABI_MOUNTAIN_BOULDER then
            call OnCast()
        endif
        return false
    endfunction
    //-----------------------------------------------------------
    // -----  END MOUNTAIN BOULDER SPELL --------------
    //-----------------------------------------------------------
    private function Init takes nothing returns nothing
        local trigger t = CreateTrigger()
        call TriggerRegisterAnyUnitEventBJ(t, EVENT_PLAYER_UNIT_SPELL_EFFECT)
        call TriggerAddCondition(t, Condition(function Cond))
        set t=null
        set ug = CreateGroup()
        set spellGroup = CreateGroup()
        set spellTimer = CreateTimer()
    endfunction
endlibrary

ver1.01: implemented Flux valuable suggestions, thanks!
ver1.02: implemented one
two
three
01-04-2016: pack splitted to 3 spells
17-04-2016: ver 1.01 implemented BPower's suggestions, thanks

Keywords:
mountain boulder, earth spell
Contents

MountainBoulder 1.01 (Map)

Reviews
17th 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: It's a simple homing projectile spiced up with...

Moderator

M

Moderator

17th Apr 2016

General Info

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:

It's a simple homing projectile spiced up with the effects of phoenix fire.
The immobilize effect on impact gives the spell the certain extra.
Very simplified trajectory coding, which may look strange on uneven terrain.
Targeting flying units will not give the wanted visual effects.

Still this is a great improvement towards the previous version.
Nearly all hardcoded fields have been replaced by constants or functions. Good job.

Overall rating between 2 and 3. Approved.

Troubleshooting:

  • local unit u doesn't have to be nulled. It's already null at the end of the function.
    ---
  • The hardcoded collision size of 64. could be replaced by a descriptive constant.
    ---
  • CancelSpell_MountainBoulder will cancel all running instances. That is
    contradictory, as the spell itself is Multiple Unit Instanceability ( MUI ).

Review changelog:
  1. [self=http://www.hiveworkshop.com/forums/spells-569/mountainboulder-v1-00-a-276726/index2.html#post2808141]29th Mar, BPower[/self]
  2. [self=http://www.hiveworkshop.com/forums/spells-569/mountainboulder-v1-00-a-276726/index2.html#post2808141]3rd Apr, BPower[/self]
 
Level 11
Joined
Jul 25, 2014
Messages
490
A lot of the stuff is not configurable via code, but in object editor. You should try to mimic the spells by coding for easier and more complex configuration.

Use TriggerRegisterTimerEvent(trig, timeout, true)

Use 0.03125 instead of GUI periodic timer which is 0.03.

Why do you use UnitDamageTargetBJ when there is a UnitDamageTarget native?
--

I don't have a lot of experience in this, but that's what I got from one quick look.
 
Level 17
Joined
Dec 11, 2014
Messages
2,009
Why do you use UnitDamageTargetBJ when there is a UnitDamageTarget native?

Because of these long parameters, probably.

JASS:
function UnitDamageTargetBJ takes unit whichUnit, unit target, real amount, attacktype whichAttack, damagetype whichDamage returns boolean
    return UnitDamageTarget(whichUnit, target, amount, true, false, whichAttack, whichDamage, WEAPON_TYPE_WHOKNOWS)
endfunction

On the rest I totally agree.
 
Level 10
Joined
Aug 21, 2010
Messages
317
Very poor vjass

A lot of the stuff is not configurable via code, but in object editor. You should try to mimic the spells by coding for easier and more complex configuration.

Use TriggerRegisterTimerEvent(trig, timeout, true)

Use 0.03125 instead of GUI periodic timer which is 0.03.

Why do you use UnitDamageTargetBJ when there is a UnitDamageTarget native?
--

I don't have a lot of experience in this, but that's what I got from one quick look.

Absolutely no difference between .031250000 and 0.03 Simply the human eye can not spot the difference. It means nothing more than a classic stupid comment.

Explain what is wrong with UnitDTargBJ. (I already know what you're gonna say!)
 
Last edited by a moderator:

KILLCIDE

Spell Moderator
Level 36
Joined
Jul 22, 2015
Messages
3,488
Absolutely no difference between .031250000 and 0.03 Simply the human eye can not spot the difference. It means nothing more than a classic stupid comment.
Because 32 times a second is easier to calculate than 33.333...

Explain what is wrong with UnitDTargBJ. (I already know what you're gonna say!)
What is the point of calling a function that directly calls to the native? Just call the native. He didn't even say anything was wrong, just suggesting minor improvements.
 
Level 10
Joined
Aug 21, 2010
Messages
317
Because 32 times a second is easier to calculate than 33.333...


What is the point of calling a function that directly calls to the native? Just call the native. He didn't even say anything was wrong, just suggesting minor improvements.

1.So the only difference is that it is easier to calculate 32. omg hahahah...
It is true But that's no bad influence on the game.
That's what I'm saying.
So, 0.03 is quite ok.


2. So, this makes no sense - TriggerRegisterAnyUnitEventBJ and all use it ...

I opened more than 100 protected maps and a bunch of them has a UDTBJ and 0.03 and all working perfectly.
You know what,you can use whatever you want I do not care.
 
Last edited:

KILLCIDE

Spell Moderator
Level 36
Joined
Jul 22, 2015
Messages
3,488
So, this makes no sense - TriggerRegisterAnyUnitEventBJ and all use it ...
That's because TriggerRegisterAnyUnitEventBJ() isn't just a single native call.
JASS:
function TriggerRegisterAnyUnitEventBJ takes trigger trig, playerunitevent whichEvent returns nothing
    local integer index

    set index = 0
    loop
        call TriggerRegisterPlayerUnitEvent(trig, Player(index), whichEvent, null)

        set index = index + 1
        exitwhen index == bj_MAX_PLAYER_SLOTS
    endloop
endfunction

It's one of the few BJs that are decent if you're using very few events. You would have to create an extra local, a counter, a loop, etc for so little.

I opened more than 100 protected maps and a bunch of them has a UDTBJ and 0.03 and all working perfectly.
Again, I'm not quite sure where you got the idea that anyone ever said it was wrong to use either. It's a suggestion.
 
Last edited:
Level 22
Joined
Feb 6, 2014
Messages
2,468
I don't have JNGP at the moment, but here are some things I noticed upon skimming your code.
  • IsUnitGroupEmptyBJ(SpellGroup[3]) -> FirstOfGroup(SpellGroup[3]) == null

  • TriggerRegisterTimerEventPeriodic -> TimerStart
    ^That way, you don't have to create a trigger and you can run the code directly. Btw, those triggers should be private but it doesn't matter anyway since you should be using timers instead. Also, if you will use TimerStart, instead of disabling triggers, you will be pausing timers instead.

  • Why do you use UnitDamageTargetBJ when there is a UnitDamageTarget native?
    Doesn't matter, it will get inlined anyway since it's a vJASS spell.
    EDIT: BJs don't get inlined. Proof

  • Very poor vjass
    Very constructive feedback

  • I suggest following JPAG for your variable names.

  • GetSpellTargetLoc() -> GetSpellTargetX() and GetSpellTargetY()

  • You used GetTriggerPlayer() twice.

  • Note that GroupClear(ug) isn't necessary since you already removed all the group's units. Plus GroupEnumUnitsInRange(...) also clears the group first before it runs.

  • You might wanna store your TIMEOUT (in your case 0.03) to a private constant variable so users can change it without changing all other parts in the code.

  • The spell itself is not configuration-friendly. Please refer to other approved vJASS spells how configuration is done.
 
Last edited:
Level 22
Joined
Feb 6, 2014
Messages
2,468
Does anyone know why I have not in Trigger Editor in Function List:
Code:
GetSpellTargetX()
it is in black color (not purple like other natives) when I type it manually;
anyway when I try to add GetSpellTargetX() and save map is saved without errors.
I have Jass helper version 0.A.2.B

I think you don't have the latest JNGP which is found in blizzmodding website (requires you to register).
 
Level 17
Joined
Nov 21, 2012
Messages
836
made some updates,
I belive IsUnitGroupEmptyBJ(SpellGroup[3]) -> FirstOfGroup(SpellGroup[3]) == null will be save since user has no access to SpellGroup[3] to remove unit from game w/o removing from group. Btw does it leak:
'if FirstOfGroup(SpellGroup[3]) == null then' ?

main change is: I abandoned loop triggers in favor of timers like Flux suggested. So now 1 timer per spell-type instead 1 loopTrigger ;]
other:
UnitDamageTargetBJ-->UnitDamageTarget
0.03-->0.03125
configuration is fine in my opinion, I wont give user possibility to change TIMEOUT because of spell construction, also half of configuration can be changed in OE, I think its good to use what OE offers, not only code all, at all cost;]
thanks Flux
 
Level 22
Joined
Feb 6, 2014
Messages
2,468
made some updates,
I belive IsUnitGroupEmptyBJ(SpellGroup[3]) -> FirstOfGroup(SpellGroup[3]) == null will be save since user has no access to SpellGroup[3] to remove unit from game w/o removing from group. Btw does it leak:
'if FirstOfGroup(SpellGroup[3]) == null then' ?

I'm not getting what you mean.

And no it does not leak since it doesn't generate an object handle.
 
Level 19
Joined
Mar 18, 2012
Messages
1,717
You're indentation is not consistent. A fact I don't like so much.

function Init hopefully becomes a private function Init in your next update.

Consider using TerrainPathability as requirement for your spell package.

GetSpellAbilityUnit() could / should be GetTriggerUnit().

In a event player unit trigger action GetOwningPlayer(caster) is less good than GetTriggerPlayer()

Your spells are semi MUI. A unit can only have one active instance during the same time.
You should step back from using UnitUserData to index your spells.
If it's your own map it may be totally fine. For a public spell I would appreciate that
a unit can have x running instances during the same time.

The overall configuration level is poor, instead many fields are hardcoded.
( max distance, speed, effect models, filters .... ).
 
Last edited:
Level 17
Joined
Nov 21, 2012
Messages
836
The overall configuration level is poor, instead many fields are hardcoded.
( max distance, speed, effect models, filters .... ).
Does it mean its not allowed for me to use spells based on shockwave/carrionswarm and use OE fields like missle speed, distance, damage? I feel its crazy to code shockwave when it is already in OE..

Your spells are semi MUI
Are you sure? I know its old way to code Mui spells using UnitUserData, heh I'm like a Bribe's pupil;] I learned form his 'unit indexer' and 'spell template'. Please note in 'Earth Spells pack' dummy is created every time spell is casted, then I refer to dummy's custom value. Cause of Unit Indexer there is no way to have to units with identical custom value. If I'm not mistaken spells are Mui.

GetTriggerUnit(), GetTriggerPlayer(), link to wc3c --> thanks!, I think I'll use TerrainPathability library for my private and any public uploads.
 
BPower explains that there is still room for more instanceability.

It's MUI in terms of wording, but a unit can be registered only once.
If it gets registered while it is already registered, then it will cause bugs as "custom value" is the
only key to both registered instances, which is obviously the very same constant for a unit. So it can't be distinguished anymore.

This can happen when a cooldown is very for example, so the unit is able to register multiple times.

Dynamic indexing is immune towards this point.
 
Level 17
Joined
Nov 21, 2012
Messages
836
friends..
I did it this way:
JASS:
//
private function LoopShockwaveEnum takes nothing returns nothing
    local unit d = GetEnumUnit() // this is dummy /NOT a caster/
    local integer id = GetUnitUserData(d)
//......actions
endfunction
//-----
private function LoopShockwave takes nothing returns nothing
    call ForGroup(spellGroup[2], function LoopShockwaveEnum)
//every 0,03125 sec enum group 2
endfunction
//------
private function ShockwaveOnCast takes nothing returns nothing
    local unit d = CreateUnit(pla, DUMMY, Tx, Ty, 0.00)
    local integer id = GetUnitUserData(d)
//each time spell is casted dummy is created and [U]his id[/U] is used to remember spell parameters, (not a caster id)
//.....actions
//dummy is added to spellGroup[2]
//in this group i keep all dummy units related to all instances of this spell (Returning Shockwave)
endfunction
 
Level 38
Joined
Sep 26, 2009
Messages
8,458
Tip: when checking distance, instead of using sqrt(x2 + y2) == h, have it as x2 + y2 == h2. SquareRoot is expensive and can be easily avoided here. Also, wtf, 200.00 tolerance? The differential should be 0, but even if a tolerance were included it should be at most 8-16 (see: TerrainPathability). I don't allow tolerance in my Knockback 2D resource and the pathing is far superior to when I allowed a tolerance. When done correctly, you would simply compare "x == GetWidgetX" and the same for "y == GetWidgetY"
 
Level 10
Joined
Aug 21, 2010
Messages
317
Oh, I indeed did not check it how you exactly used custom value. Sorry.
What needs to be done on each enumeration, is that it's checked if caster is still alive before using him, or his coordinates


First of all, personally checking whether something is true or not.

2.You brought yourself in a situation to comment on(and also you support)unverified information(based on an incorrect interpretation of someone else's)

3.This is not the first time that you bring contradictory or unverified information.

In this way you can bring the people into believing that this is totally wrong and it's not true.

Nothing personal just a little more professionalism.
 
Level 17
Joined
Nov 21, 2012
Messages
836
If I won't touch this
The overall configuration level is poor, instead many fields are hardcoded.
( max distance, speed, effect models, filters .... ).
but I will change other indicated issues is there a chance for this to be approved? I just ask to not waste Moderator's and mine time.
Btw thanks all who take part in comments here for tips, thanks zv27 for support
 
Level 19
Joined
Mar 18, 2012
Messages
1,717
but I will change other indicated issues is there a chance for this to be approved? I just ask to not waste Moderator's and mine time.
^I dislike approving a halfway finished submission. Actually it takes more time, because reading well formated code goes very fast.
Basically I could approve it anyways, then however expect a rating between 2 and 3.

You should null all handle variables. For spells which use a standard indexing method it's
optional, because the next cast probably overrides previous references.
You use unit user data, which means that you eventually end up with many handle leaks,
as indexes can be widely distributed.

call DisplayTimedTextToPlayer(pla, 0, 0, 1, "Returning Shockwave error: caster to close to map border.")
^Add a debug keyword to this line.

I would seperate the spell into different libraries and show a bit more passion
about configuration options.

I can highly recommend SpellEffectEvent to everyone who
has more than a couple of custom spells in his / her map. The performance gain can be significant.

To me it makes more sense when the casting unit deals the damage instead of a dummy unit.
It creates a better synergy with any DDS system.

JASS:
private function IsUnitAlive takes unit u returns boolean
    return GetUnitTypeId(u) != 0 and not IsUnitType(u, UNIT_TYPE_DEAD)
endfunction
^Optimized varation.

JASS:
private function Cancel takes nothing returns nothing
    set spellDuration[GetUnitUserData(GetEnumUnit())] = 0.00
endfunction
function CancelSpell_MountainBoulder takes nothing returns nothing
    call ForGroup(spellGroup[1], function Cancel)
endfunction
function CancelSpell_ReturningShockwave takes nothing returns nothing
    call ForGroup(spellGroup[2], function Cancel)
endfunction
function CancelSpell_TaurenWave takes nothing returns nothing
    call ForGroup(spellGroup[3], function Cancel)
endfunction
^They cancel ALL running instances. Btw the spellDuration can be override if the missile is in target range.
 
Last edited:
Level 17
Joined
Nov 21, 2012
Messages
836
You should null all handle variables
yes, of course, but Im just looking and can't find not nulled variables, maybe you refer to "private function LoopTaurenWaveEnum .." there is local unit u, not nulled, Ill fix this if it is what you mean.

I would seperate the spell into different libraries and show a bit more passion about configuration options.
separate such a simple spells? Tauren Wave spell is based on original shockwave. How should I add configuration options if missle model/speed is set in OE? So you suggest to create missle, move it every 0,03sec, and so on? Second spell "Returning Shockwave" is based on original 'carrionswarm'. The same problem as before. 3rd spell "Mountain Boulder" there are options for damage, BOULDER_MISSLE, and ABI_SHARDS. This last ability is based on 'Phoenix Fire' and used to auto attack (every1sec). In OE missle model/damage etc can be changed.

I can highly recommend SpellEffectEvent to .. yes.. but requirements for this simple spell pack will grow from one (unit indexer) to 4 .. (UI + TerrainPathability + SpellEffectEvent + RegisterPlayerUnitEvent) quite a lot..

To me it makes more sense when the casting unit deals the damage instead of a dummy unit. It creates a better synergy with any DDS system. You refer to 'Mountain Boulder' spell. Idea was to not destroy missle when caster dies. Missle travels up to 20sec (target can run away from it) If caster supposed to damage target then I should check if caster is alive, and kill missle/end spell if caster dies. Its not natural, thrown boulder can be still on its way regardless if caster dies or not.

They cancel ALL running instances Yes, please look at description it was mentioned.
Btw the spellDuration can be override if the missile is in target range
Yes, you right Ill fix it by adding
JASS:
if spellDuration[id] > 0.00 then 
    set spellDuration[id] = I2R(spellLevel[id]) + 1.00
endif
you also mentioned about intendation. Is it about globals block and function declaration in the same column as library ?
 
Level 17
Joined
Nov 21, 2012
Messages
836
spellCaster and spellTargetUnit should be nulled when a spell instance expires.
yes, I completely miss that.. thanks

Ill try learn about degrees/radians to switch to radians. And I'll cut this into 3 pieces;]

Im still not sure about this:
JASS:
if FirstOfGroup(spellGroup[3]) == null then
  //something..
endif
// or like that?
set u = FirstOfGroup(spellGroup[3])
if u == null then
  //something..
endif
set u = null

and how you do that:
^Add a debug keyword to this line.
when "debug" is black font on white background, here on forum?
 
Level 19
Joined
Mar 18, 2012
Messages
1,717
Old review. Posted into comments for reference reasons.


Overall impression​
Review:
  • Some parts of the spell are hardcoded. Something which I normally try to avoid in my code.
    Others are even more strict about that issue. It's also documented as no-go in our Spell Submission Rules.
  • I recommend that you seperate each spell into an own scope or library.
  • Better configuration options are also on my wish list.
  • These spells require some more passion.
TaurenWave​
Review:

The spell reminds me a bit of DotA's puck without the option to decide whether to port or not.
  • In my opinion the teleport process could be spiced up by an extra special effect.
  • The unit knockback looks a bit rough. I highly recommend the usage of a knockback system.
    For example Bribes GUI Knockback2.5D is very solid.
  • The tooltip says that the wave / impact deals damage which is not true.

Troubleshooting:

  • spellRadius[id] should store the angle in radians instead of degrees. The variable name Radius is a bit confusing.
    ---
  • IsTerrainWalkable(Tx, Ty) returns a boolean and can be directly used in an if then condition.
    This here: if ((Tx-TerrainPathability_X) * (Tx-TerrainPathability_X) + (Ty-TerrainPathability_Y) * (Ty-TerrainPathability_Y)) < 16.00*16.00 then
    just creates unnecessary overhead. --> Go with if IsTerrainWalkable(Tx, Ty) then instead.
    ---
  • spellCaster and spellTargetUnit should be nulled when a spell instance expires.
    ---
  • You cache the ability level, but it has no meaning for that spell as you didn't implement level based actions.

Shockwave​
Review:

It's a bouncing shockwave.

Troubleshooting:

  • call DisplayTimedTextToPlayer(pla, 0, 0, 1, "Returning Shockwave error: caster to close to map border.")
    ^Add a debug keyword. Players don't want to read that message ingame.
    Btw if that cases happens you'll end up with a created dummy unit.
    ---
  • set angle = bj_RADTODEG * Atan2(y-GetSpellTargetY(), x-GetSpellTargetX()) //angle.
    ^First of all: Why in degree? Also angle = angle + bj_PI or in your case angle = angle + bj_PI*bj_RADTODEG.
    ---
  • spellRadius[id] should store the angle in radians instead of degrees. The variable name Radius is a bit confusing.
    ---
  • Again no array handle variables are nulled.

MountainBoulder​
Review:

  • Basically lives from the phoenix fire effects.

Troubleshooting:

  • Problems are equal to the spells above
    ---
  • No array handle variables are nulled.




It's simple an homing projectile spiced up with the effects of phoenix fire.
The stun on impact gives the spell the certain extra.
The missile motion is coded very simple and may look strange for non circular models or on uneven terrain.

The configuration options got better, yet there are fields which are still hardcoded.
Read more in troubleshooting and the Spell Submission Rules.

Troubleshooting:

  • I advise you to place to global configuration above the array variables.
    Eventually seperate them into an extra global block below the configurable functions.
    It's easier to setup and read for the user.
    ---
  • SetUnitScale only affects the scaling in x,
    you could optimize your code it by passing in 0. for y and z.
    Example: SetUnitScale(unit, GetMissileScale(level), 0., 0.)
    ---
  • Collision radius ( 250. ) and in collision range ( 64*64 ) could be configurable.
    There is also the very performant native IsUnitInRange(source, target, range)
    ---
  • The on impact fx should be configurable.
    ---
  • if IsUnitEnemy(u, pla) then is a bit poor. It also affects dead units.
    I recommend to setup a filter function, which the user can configurate.
    ---
  • It's not required to clear a group before destroying it.
    ---
  • JASS:
        private function Cancel takes nothing returns nothing
            set spellDuration[GetUnitUserData(GetEnumUnit())] = 0.00
        endfunction
        function CancelSpell_MountainBoulder takes nothing returns nothing
            call ForGroup(spellGroup, function Cancel)
        endfunction
    ^Will cancel all active spell instances. Is that wanted?
    ---

 
Last edited:
Level 17
Joined
Nov 21, 2012
Messages
836
Thank you for approving, BPower ;]
I think I learned something by making this spell, also hope spell will be usefull for some mappers.

You wrote
Very simplified trajectory coding, which may look strange on uneven terrain.
The projectile facing angle is never updated, hence the spell only works for circular shaped models.
I belive missle movement looks ok, cause in OE it using Movement type "hoover" and Movement Height "60.00". So it follows terrain deformations, cliff levels.
In
private function LoopEnum takes nothing returns nothing
there's missle face update:
call SetUnitFacing(spellMissle[id], bj_RADTODEG * angle)

Targeting flying units will not give the wanted visual effects.
The last thing I agree with you, I forgot about flying units, so because missle has constant height 60, it will not looks perfect when follows and/or hits flying units.
 
Top