Ancient Burning Swords

[Moderator]

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

Required changes:

• Add importing instructions into the map file
• Add learn hotkey
• List hotkey in tooltips
• Improve tooltips
• When you declare local lightning variables, you don't have to set them to null
• Change the learned icon position
• The hero should be added to selection when the spell ends
• TriggerSleepAction is not accurate. In single player, a 0.01 second wait is either 0.011, 0.126, 0.151, 0.176 or 0.201 seconds, chosen randomly
• Only remove 'Armf' if the unit didn't have it before adding it
• For what do you need a lightning array and 9 non-array lightning variables?
• 0.405 health is the limit of dead/alive unit

Suggested changes:

• When you create the swords, you could set Cos(t1) and Sin(t1) into variables since now you're calculating them thrice
• I'd like you not to set the paths for effects every time the spell is cast
• In my opinion, you could speed up the spell a bit
• You could increase Art - Follow through time so the caster's cast anim doesn't stop so quickly

 

[Magtheridon96]

- You should use functions to make things (like the special effects configurable:
Example:

function Spell_GetEffect takes nothing returns string
    return "Abilities\\Spells\\Items\\StaffOfSanctuary\\Staff_Sanctuary_Target.mdl"
endfunction

- Don't use TriggerSleepAction, timers are much better to use.
Also, 0.01 second TSA waits 0.27 seconds in reality.
And instead of Actions, you should use Conditions.

You'd just have to get rid of the TSA's and put all your code in the Conditions function then return false at the end (use an if/then/endif block to check if the right spell was casted)

    set hp = GetWidgetLife(ut)
    if hp < 0.4 then
        call DestroyEffect(f1)
        call SetUnitFlyHeight( ut, 0, sp * 4 )
    else
        call SetUnitFlyHeight( ut, 0, sp )
    endif

    set hp = GetWidgetLife(uc)
    set a = 1
    if hp < 0.4 then
        loop
            exitwhen a > 5
            call DestroyEffect(f[a])
            set a = a + 1
        endloop
    endif

Why are you using the ho real if you're only using the variable once?
Do this instead:

                if GetWidgetLife(ut) <= 0.405 then
                    call DestroyEffect(f1)
                    call SetUnitFlyHeight( ut, 0, sp * 4 )
                else
                    call SetUnitFlyHeight( ut, 0, sp )
                endif

                set a = 1
                if GetWidgetLife(uc) <= 0.405 then
                    loop
                        exitwhen a > 5
                        call DestroyEffect(f[a])
                        set a = a + 1
                    endloop
                endif

Still, this is bad. To check if a unit is dead, do this: "IsUnitType(unit, UNIT_TYPE_DEAD)"

- call SetUnitFlyHeight( uc, h1, 10000 )
Use 0 instead of 10000. 0 makes the unit move instantly.

- In the loop, instead of using the SquareRoot function, you could simply compare the distance with 14400 (120*120)

- WEAPON_TYPE_WHOKNOWS -> null
They're the same thing

- Don't use local arrays, they have very bad performance.

- Again, please make more things configurable and use timers for the spell.
The lightning types should be configurable, the height, the damage the effects, the distance, the durations, everything.

This needs to be improved .. a lot.

Oh and the local variable g is useless. We have bj_RADTODEG and bj_DEGTORAD.

constant real bj_RADTODEG = 180./bj_PI
constant real bj_DEGTORAD = bj_PI/180.

 

[shadowvzs]

1 - You should use functions to make things (like the special effects configurable:
Example:

function Spell_GetEffect takes nothing returns string
    return "Abilities\\Spells\\Items\\StaffOfSanctuary\\Staff_Sanctuary_Target.mdl"
endfunction

2 - Don't use TriggerSleepAction, timers are much better to use.
Also, 0.01 second TSA waits 0.27 seconds in reality.
And instead of Actions, you should use Conditions.

3-You'd just have to get rid of the TSA's and put all your code in the Conditions function then return false at the end (use an if/then/endif block to check if the right spell was casted)

    set hp = GetWidgetLife(ut)
    if hp < 0.4 then
        call DestroyEffect(f1)
        call SetUnitFlyHeight( ut, 0, sp * 4 )
    else
        call SetUnitFlyHeight( ut, 0, sp )
    endif

    set hp = GetWidgetLife(uc)
    set a = 1
    if hp < 0.4 then
        loop
            exitwhen a > 5
            call DestroyEffect(f[a])
            set a = a + 1
        endloop
    endif

4 - Why are you using the ho real if you're only using the variable once?
Do this instead:

                if GetWidgetLife(ut) <= 0.405 then
                    call DestroyEffect(f1)
                    call SetUnitFlyHeight( ut, 0, sp * 4 )
                else
                    call SetUnitFlyHeight( ut, 0, sp )
                endif

                set a = 1
                if GetWidgetLife(uc) <= 0.405 then
                    loop
                        exitwhen a > 5
                        call DestroyEffect(f[a])
                        set a = a + 1
                    endloop
                endif

5-Still, this is bad. To check if a unit is dead, do this: "IsUnitType(unit, UNIT_TYPE_DEAD)"

6- call SetUnitFlyHeight( uc, h1, 10000 )
Use 0 instead of 10000. 0 makes the unit move instantly.

7- In the loop, instead of using the SquareRoot function, you could simply compare the distance with 14400 (120*120)

8- WEAPON_TYPE_WHOKNOWS -> null
They're the same thing

9- Don't use local arrays, they have very bad performance.

10- Again, please make more things configurable and use timers for the spell.
The lightning types should be configurable, the height, the damage the effects, the distance, the durations, everything.

This needs to be improved .. a lot.

11-Oh and the local variable g is useless. We have bj_RADTODEG and bj_DEGTORAD.

constant real bj_RADTODEG = 180./bj_PI
constant real bj_DEGTORAD = bj_PI/180.

1. but why more configurable than all string local at begining of trigger in same place?
2. there enough 0.5 value too just i put lower
3. then i declared locals without sense since i must put local declaretion before if else give error to me
4. done
5. done
6. done
7. done
8. ok
9. but harshtable also slower than global variables, so i must use global variable arrays? local arrays slower than harshtable?
10. Timer i understand but lightning height no, coz i tryed move upper and down and both way i think it was ugly or too unrealistic really, anyway i replaced with h2 so its configurable with other object heights, about damage effect i like that also got no ideea how could improve, distance and duration i think also useless because idk if somebody have who want half map ability
11. its shorter if few case, i mean just 1 letter

 

[mckill2009]

- You should use functions to make things (like the special effects configurable:

globals are better

function SetSfx takes nothing retuns nothing
   set udg_s[0] = "Abilities\\Spells\\Items\\StaffOfSanctuary\\Staff_Sanctuary_Target.mdl"
   set udg_ss[1] = "Abilities\\Spells\\NightElf\\Blink\\BlinkCaster.mdl"
   set udg_ss[2] = "Objects\\Spawnmodels\\Human\\HumanBlood\\HumanBloodPeasant.mdl"
   set udg_ss[3] = "Abilities\\Spells\\NightElf\\Blink\\BlinkTarget.mdl"
   set udg_ss[4] = "Abilities\\Spells\\Other\\Doom\\DoomDeath.mdl"
   set udg_ss[5] = "Abilities\\Spells\\Human\\FlameStrike\\FlameStrikeEmbers.mdl"
endfunction

function InitTrig_Magical_sword takes nothing returns nothing
    set gg_trg_Magical_sword = CreateTrigger( )
    call TriggerRegisterAnyUnitEventBJ( gg_trg_Magical_sword, EVENT_PLAYER_UNIT_SPELL_EFFECT )
    call TriggerAddCondition( gg_trg_Magical_sword, Condition( function Trig_Untitled_Trigger_003_Conditions ) )
    call TriggerAddAction( gg_trg_Magical_sword, function Trig_Untitled_Trigger_003_Actions )
   call SetSfx()
endfunction

and you really need to rename this Trig_Untitled_Trigger_003_Actions and a-like coz it's really bad for eyes ...

 

[Magtheridon96]

Yes, globals are better but I was assuming this is a strictly jass resource :>

Hashtables are slower than global and local arrays, but that doesn't mean local arrays are any good. Use a hashtable or use dynamic indexing or even a stack.
Go with dynamic indexing. (I'd recommend it)

The configurables are needed in the form of functions or globals because creating the same constant locals everytime is really inefficient. Putting all the configurables at the top of the code is much better because it gives your code structure and organisation.

 

[shadowvzs]

Yes, globals are better but I was assuming this is a strictly jass resource :>

Hashtables are slower than global and local arrays, but that doesn't mean local arrays are any good. Use a hashtable or use dynamic indexing or even a stack.
Go with dynamic indexing. (I'd recommend it)

The configurables are needed in the form of functions or globals because creating the same constant locals everytime is really inefficient. Putting all the configurables at the top of the code is much better because it gives your code structure and organisation.

if i understand well the only problem with local arrays, is u cant pass the data to another function until global variable+indexing system or harshtable can, right?

ok now i understand that thing about strings, and similiar things what i use often but declare every time.

have anyway to kill TSA, since i use 0,1 value but its indifferent what value i give just must be lower than 0.8, because it is use for loop dont make problem when i run damn much time till target arrive to a location (so exitwhen from loop if a real lower than x), since it is used for slowdown the loop cycle not because i need wait, the only way is the timer? i ask this since really its not like a trigger when need to do something in fixed time, like dot, regen, missile etc

 

[mckill2009]

Yes, globals are better but I was assuming this is a strictly jass resource :>

Mag you should know better that "udg_" is a jass resource ...

EDIT:
the problem with locals also is you cant use it in a timer loop (delay purposes) to recall it's
value, so you might as well use hashtable or UnitIndexer, I wont recommend Hanky's indexing
system in Jass coz Unit indexer is way better and easier...

 

[JollyD]

looks nice but i am not familiar with jass)) good work and i have 500 posts)