Brutal Blizzard v1.5

Moderator · Jul 8, 2015

Submission:	Brutal Blizzard v1.5
Date:	12.07.2015
Status:	Approved
Rating:	4/5
Review	Link
Moderator:	IcemanBo

Rheiko · Jul 8, 2015

Looks cool.
Not yet capable of reviewing Jass code though, sadly. </3
It's nice to see you again, pal. :b

Ofel · Jul 8, 2015

Hi thanks!
I thought you were gone too.
Nice to see (?) you too. No need to review, rate maybe?

Kazeon · Jul 8, 2015

Just some quick hints

JASS:

constant function Bblz_DummiesOwner takes nothing returns player
    return Player(15) // Neutral Passive
endfunction

Just use a variable just like the other configurations, since "Player()" is basically a function call which can slow things down.

I don't know how the spell works but call PauseUnit(u,true) should be avoided. Try to use a stun system instead or create yours.

This is not enough to check is unit dead:

JASS:

if GetWidgetLife(udg_Bblz_Target[index])<=0.405 then

Should be:

JASS:

if GetWidgetLife(udg_Bblz_Target[index])<=0.405 and IsUnitType(udg_Bblz_Target[index], UNIT_TYPE_DEAD) then

I do not agree on this part since I think it's just for bloating effects:

JASS:

                set i=0
                loop
                    exitwhen i>udg_Bblz_ExplosionSFXNumber
                    call DestroyEffect(AddSpecialEffect(udg_Bblz_ExplosionSFX[i],x,y))
                    set i=i+1
                endloop

This could be added in configuration part:

JASS:

if GetWidgetLife(u)>0.405 and @IsUnitEnemy(u,udg_Bblz_Owner[index]) and not IsUnitType(u,UNIT_TYPE_FLYING) and not IsUnitType(u,UNIT_TYPE_MAGIC_IMMUNE)@ then

But it's rather optional to do.

After recycling the indexes, you have to null handles at the max index.

JASS:

.... // deindex part
            set udg_Bblz_Caster[udg_Bblz_MaxIndex]=null
            set udg_Bblz_Dummy[udg_Bblz_MaxIndex]=null

JASS:

    set bj_enumDestructableCenter=Location(x,y)

You can create one global temporary location variable on initialization and simply move it around using MoveLocation(loc, x, y) to avoid re-creating locations over and over again. But wait, you don't even use that location so just remove it.

JASS:

function Bblz_DestroyTrees takes nothing returns nothing
    local destructable tree=GetEnumDestructable()
    call IssueTargetOrderById(udg_Bblz_Harvester,852018,tree)
    if GetUnitCurrentOrder(udg_Bblz_Harvester)==852018 then
        call KillDestructable(tree)
    endif
    set tree=null
endfunction

Use this function instead:

JASS:

function Bblz_DestroyTrees takes nothing returns nothing
    local destructable tree=GetEnumDestructable()
    if IssueTargetOrderById(udg_Bblz_Harvester,852018,tree) then
        call KillDestructable(tree)
    endif
    set tree=null
endfunction

JASS:

    call UnitAddAbility(udg_Bblz_Harvester,udg_Bblz_Ability)
    call UnitRemoveAbility(udg_Bblz_Harvester,udg_Bblz_Ability)
    call Preload(udg_Bblz_ShardModel)
    call Preload(udg_Bblz_HitSFX)
    set i=1
    loop
        exitwhen i>udg_Bblz_ExplosionSFXNumber
        call Preload(udg_Bblz_ExplosionSFX[i])
        set i=i+1
    endloop
    call Preload(udg_Bblz_FreezeSFX)

Are those needed? Where do you call that function even?

Phew, it's been a while since the last time I did a code review. Anyway, 4/5 from me

Maker · Jul 8, 2015

^Dummy units can be paused, no problem. Widget life check can be removed completely, UNIT_TYPE_DEAD check is pretty good. One can use native UnitAlive, just declare it first.

Kazeon · Jul 8, 2015

Maker said:
^Dummy units can be paused, no problem. Widget life check can be removed completely, UNIT_TYPE_DEAD check is pretty good. One can use native UnitAlive, just declare it first.

Hm. But he is pausing affected units.

UnitAlive should only be used for vJass since native declaration only valid in JNGP, iirc.

Ofel · Jul 8, 2015

@Quilnez

Lets just say its for safety.

JASS:

    call UnitAddAbility(udg_Bblz_Harvester,udg_Bblz_Ability)
    call UnitRemoveAbility(udg_Bblz_Harvester,udg_Bblz_Ability)
    call Preload(udg_Bblz_ShardModel)
    call Preload(udg_Bblz_HitSFX)
    set i=1
    loop
        exitwhen i>udg_Bblz_ExplosionSFXNumber
        call Preload(udg_Bblz_ExplosionSFX[i])
        set i=i+1
    endloop
    call Preload(udg_Bblz_FreezeSFX)

Are those needed? Where do you call that function even?

From the configuration trigger.

Be right back

@Maker
Lol, I didn't pause dummies.

Seriously, can I pause affected units? What are the risk?

Daffa · Jul 8, 2015

Globals doesn't have to be Nulled, but it's a good practice.

TriggerAddAction -> TriggerAddCondition

Is it just me or the entire tree stuffs can be optimized? The scan feels unnecessary IMO. Just use circular check should be enough. Unless of course, spell mechanics.

Still, 4/5 from me, nicely done lad

EDIT :
Use SetUnitPropWindow(u,0), SetUnitAnimationSpeed(u,0) and SetUnitPropWindow(u,GetUnitPropWindow(u)*bj_RADTODEG), SetUnitAnimationSpeed(u,100)instead of pause unit. Unless these are for dummies.

The code aren't exact since I don't look for them at editor, but roughly enough to guess what should be done.

Pause unit has some annoying behaviors.

BPower · Jul 8, 2015

Review based on the thread description.

1.) What's the purpose of the preload trigger, use a simple function call in the init trigger instead.

2.) Harvesting trees is optional, but you always create an harvester unit.

3.) Which units should be affected is part of user configuration. In GUI this is impossible,
but you are using a JASS/GUI mix. So make it a customizeable function call.

4.) IsUnitType(u,UNIT_TYPE_DEAD) is enough to check if a unit is alove or not.

5.) Pausing non dummy units is very unprofessional. It comes along with unwanted side effects.

6.) GetRandomReal(0,360)*bj_DEGTORAD --> GetRandomReal(0, 2*bj_PI)

Ofel · Jul 9, 2015

@Daffa the Mage
Yes, using SetUnitPropWindow makes unit unable to move, but it makes them 'turnable' which is a bit unrealistic.

@BPower

6.) GetRandomReal(0,360)*bj_DEGTORAD --> GetRandomReal(0, 2*bj_PI)

I have bad score on math :3
Fixing ASAP

Daffa · Jul 9, 2015

Modify it's animation speed as well, and we get a good PauseUnit.

Ofel · Jul 11, 2015

Added terrain deformation, the final version, hopefully.

You wouldn't believe me that terrain deformation can also make windows crash. My laptop screen turned blue with some kind of debug messages.

Daffa · Jul 11, 2015

Isn't terrain deformation causes permanent leak?

Ofel · Jul 11, 2015

~~It's a temporary terrain deformation. But idk, let's wait~~

Edit:
Nope, I guess not. I've played about 15 minutes with maybe about 50 casts and seems like not much lagging.

IcemanBo · Jul 11, 2015

At the moment the channeling is not mimicked quite well.
The animation is played once, but is completly independant of the spell duration.
This is not really characteristic for a channel ability.
What also would be characteristic is that spell gets canceled if newOrder/onDeath for example.
Well, this is my opinion how it would make mimic the original spell some better.
Quillnez made a good suggestion when he said to change the player function into variable.
It can be just part of normal GUI configuration. Nothing will be lost.
.
.
-------- Determines how many explosion effect will be used --------

Set Bblz_ExplosionSFXNumber = 2
^Not neessarily needed. Could be avoided if you check in loop exitwhen effect[index] == null.
In loop you don't need 2 seperate if statements to filter and to check if unit is alive:
if alive(u) and filter(u) then ...
Add harvest ability to your worker via trigger to ensure he can.
You start the "create effect loop" with initial value of 0,
but in init you start with 1 with assignment. Align them.
If you want optimization, one global rect could be used for enum destrucables.
TerrainDeformation is meant to leak a reference, but it can't be avoided,
and it's also used in wc3 default abilities, so I don't ban to use it.
Methods: 3 -> 1 -> 2 -> 4. Why not make it just increasing? ^^
onDeindex also decrement the loop index. Else you potentialy forget about 1 instance.
Spell makes really fun and looks nice. Freezing damaged unit looks coool

About this preload. Ofel has to ensure the config trigger runs before the Preload runs.
So a simple call in his JASS init might not be enough. (timer must be used)
But I don't see why you didn't move the preload in GUI so it just can run after the config part in same thread.

Ofel · Jul 11, 2015

@IcemanBo

Methods: 3 -> 1 -> 2 -> 4. Why not make it just increasing? ^^

I feels like its a little faster. See method 1 is the most often block that will be run periodically since it handles many shards to be moved, so it is putted on top, method 2: freezing units, takes the 2nd place as the most often and so on.

IcemanBo · Jul 11, 2015

What must be changed is:
If unit gets freezed it gets into group.
Now the freezing time won't be refreshed onNewHit, because there is a condition
to filter units out that already are in freezing group. But they should be freezed again.

Also this:
You interrupt channeling now, it's good. For most cases it works, but there exists one mistake.
You only check if orderId is correct castOrder of spell. But now, when a unit is channeling already,
and then starts the same effect again it is still the same orderId, and so the first one won't be interrupted.
-> 2 blizzdards can be casted at same time by caster, or more.

JASS:

if GetUnitAbilityLevel(udg_Bblz_Harvester,'Ahar')==0 then
    call UnitAddAbility(udg_Bblz_Harvester,'Ahar')
endif

Just add ability, nothing bad will happen

. ->
call UnitAddAbility(udg_Bblz_Harvester,'Ahar')

Global rect can be used over re-constructing one local over and over again.

Preload function is not necessarily needed. Could run right after user config.

Ofel · Jul 12, 2015

What must be changed is:
If unit gets freezed it gets into group.
Now the freezing time won't be refreshed onNewHit, because there is a condition
to filter units out that already are in freezing group. But they should be freezed again.

Should I use hashtable or Unit Indexer?

Global rect can be used over re-constructing one local over and over again.

How?

Preload function is not necessarily needed. Could run right after user config.

Well sorry, I thought you said it's optional.

IcemanBo · Jul 12, 2015

1. I'm not sure why you would need it. Unit just be just affected from new by the freeze.

2. SetRect does the waned job.

3. I still note possible optimizations. I only see my first two text paragarphs in the review as mandatory.

IcemanBo · Jul 17, 2015

A bit delay, but here we go.

Concept: 3/5

Idea is not unique.
The idea is to mimic the "Blizzard" default ability in warcraft III, which succeeded.
It's an AoE spell with falling shards that damage and freezes affected units for certain duration.

Code: 3.5/5

The code is a mix of GUI and JASS.
The user has possibility to config most of the spell in GUI, which is positive for most beginners.
Core system is written in plain JASS.

- One global dummy could be used for freezing affected units. Creating/removing is not needed.
- I find your scan trees functions okay with loc, as well, but here too one global could be used.
- Imo "IsYInMap" and "IsXInMap" don't have to be seperated here.

(rating is tending to 4 for me here atm)

Visuals: 4/5

I like visuals actually, but for my personal taste the second effect looks to spammy with such a big appearance rate.

Conclusion: 3.5 -> 4/5

Approved

Brutal Blizzard v1.5

Description

Trigger

Changelog

Credits

Brutal Blizzard (Map)

Moderator

Moderator

Similar threads