Stormcyclone [1.0]

Moderator · Jun 9, 2012

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

00:14, 18th Jul 2012
Magtheridon96:

3 groups per instance is just overkill :/
1 group or a table of units would suffice.
You could attach an integer to each unit so you can separate them
instead of having 3 groups.

Instead of using the old fashioned way of iterating through units
in a group, we now use "First of group" loops.

JASS:

/*
*   First we need a dummy group to use.
*   Users have the option to use bj_lastCreatedGroup,
*   but due to certain bugs involved with nesting 
*   group enumerations, I would highly recommend
*   that you do not.
*/
globals
    private group enumGroup = CreateGroup()
endglobals

/*
*   This function takes a unit and returns a boolean.
*   It's used to filter out unwanted units in our
*   group enumeration.
*/
function filter takes unit filterUnit returns boolean
    return true
endfunction

/*
*   This function takes the unit that we are iterating 
*   over currently in our group enumeration and does 
*   stuff to him. Really cool stuff if you ask me.
*/
function actions takes unit enumUnit returns nothing
    call DoNothing()
endfunction

function Enumerate takes nothing returns nothing
    /*
    *   This is the unit we are going to use
    *   in our group enumeration loop.
    */
    local unit enumUnit

    /*
    *   These are just here so that the code
    *   would compile. They represent the 
    *   coordinates of the point that you want 
    *   to reference when selecting units in
    *   range. The range variable represents
    *   ... range.
    */
    local real x = 0
    local real y = 0
    local real range = 0
    
    /*
    *   This function will clear the group 
    *   and select all units in range of 
    *   point(x,y).
    */
    call GroupEnumUnitsInRange(enumGroup, x, y, range, null)
    
    /*
    *   This is where the actual iteration is done.
    */
    loop
        /*
        *   First we get a unit from the group.
        *   After that, we confirm that this unit is
        *   not null. Getting a null unit indicates 
        *   that a group is empty. Finally, we remove
        *   this unit from the group so that we don't
        *   iterate over him more than once.
        */
        set enumUnit = FirstOfGroup(enumGroup)
        exitwhen enumUnit == null
        call GroupRemoveUnit(enumGroup, enumUnit)
        
        /*
        *   Now that we have our unit, we attempt 
        *   to filter him out. If he passes the filter,
        *   actions will be executed for him.
        */
        if filter(enumUnit) then
            call actions(enumUnit)
        endif
    endloop
endfunction

You don't need to inline TriggerRegisterAnyUnitEventBJ
When comparing the distances, you don't need to use the SquareRoot, you can just multiply the constant by itself and compare your expression with that. It's faster because SquareRoot is a pretty slow function.
Use TimerUtils instead of pausing and destroying the timers only if you want to.
Don't create a local group, then enumerate and destroy it, use a global group instead.

CoLd Bon3 · Jun 9, 2012

The spell is good, i like it! I don't know vJass and i can't say if the code is good but the idea is. Perhaps it will be better if the cyclones appear am.... "smoothly", i think? They just come out of nowhere and disappear from nothing. You should make them kinda "fade in" and "fade out" using transparency. :]

Otherwise the spell is good! I give it 4/5.

zv27 · Jun 10, 2012

JASS:

call GroupEnumUnitsInRange(g,GetUnitX(.caster),GetUnitY(.caster),STORM_AREA,Condition(function thistype.slow_filter))
call GroupEnumUnitsInRange(g,GetUnitX(u),GetUnitY(u),CYCLONE_DAMAGE_AREA,Condition(function thistype.cyclone_damage_add))

This is better solution!

JASS:

call GroupEnumUnitsInRange(g,GetUnitX(.caster),GetUnitY(.caster),STORM_AREA,null)
call GroupEnumUnitsInRange(g,GetUnitX(u),GetUnitY(u),CYCLONE_DAMAGE_AREA,null)

Dr. Boom · Jun 11, 2012

Moin moin =)

Thanks so far for the feedback you gave me:

@CoLd Bon3: Well I thought about this too but there is one problem with these cyclones. I only can make the cyclone itself transparent but the "dust" around it, stays at 100% transparency and you wouldn't notice a "fade in - fade out" that good =(

@zv27: In your way how should I execute the group then without any filter? I can use ForGroup(), but the direct filter is bitter. If there is any other thing I don't know, then please explain.

@Camper9514094: Sorry I don't understand what you mean, please describe!

Greetings and Peace
Dr. Boom

Gregory Tountas · Jun 26, 2012

Good job, this Spell looks nice +5/5

Blackair · Jul 18, 2012

oh f***ing jass again ! , is it possible to create this with gui?

Magtheridon96 · Jul 18, 2012

Yes, it is.

edit
One more tip Dr. Boom, WEAPON_TYPE_WHOKNOWS is equal to null.

edit
Also, effect array efc[1] is redundant.
It's the same as having effect efc

zv27 · Jul 18, 2012

Blackair said:
oh f***ing jass again ! , is it possible to create this with gui?

Why fucking JASS?
Try to learn JASS
It's not that hard
Otherwise, this is vJASS, not JASS
Differences are evident.

Magtheridon96 said:
Yes, it is.

edit
One more tip Dr. Boom, WEAPON_TYPE_WHOKNOWS is equal to null.

edit
Also, effect array efc[1] is redundant.
It's the same as having effect efc

true

IsUnitType(u,UNIT_TYPE_DEAD)is a better solution thanGetWidgetLife(u) < 0.405

This will never be destroyed call GroupEnumUnitsInRange(g,GetUnitX(u),GetUnitY(u),CYCLONE_DAMAGE_AREA,Condition(function thistype.cyclone_damage_add))

Magtheridon96 · Jul 19, 2012

Actually, the IsUnitType death check is often more accurate.

Nestharus · Jul 19, 2012

TimerUtils instead of pausing and destroying the timers.

TimerUtils has more overhead than pausing/destroying timers, and I have yet to see any evidence of handle stack corruption in the latest patch (excluding weird things like TSA in a destroyed trigger).

Magtheridon96 · Jul 19, 2012

But if destroying timers can cause bugs, then it eventually will :/ (Murphy's Law)
Recycling timers is the safest method which is why it's recommended for public resources.

Personally, I don't use timer recycling in my maps because I never destroy triggers or use TriggerSleepAction, but users who import these spells might be doing so :/

I completely agree with you Nes, but in the Spells section, we have to try to save users from themselves often.

Nestharus · Jul 19, 2012

I completely agree with you Nes, but in the Spells section, we have to try to save users from themselves often.

You know that I'm against that philosophy right?

Magtheridon96 · Jul 20, 2012

I know, and I am too, but we have to do it for the sake of the guests that come to this site who happen to be about 11 times more numerous than the members :/

Nestharus · Jul 20, 2012

Magtheridon96 said:
I know, and I am too, but we have to do it for the sake of the guests that come to this site who happen to be about 11 times more numerous than the members :/

We don't have to do anything >.>.

Ok, let's stop derailing this thread. It's up to the author what solution they choose to use, so long as that solution is an acceptable one. Not using TimerUtils is an acceptable solution. You've no right to force them to use it. I sure as hell would never use it : |.

Magtheridon96 · Jul 20, 2012

The lack of timer recycling is not exactly what's getting this resource temporarily rejected though.

I could approve it without the timer recycling.

Dr. Boom · Aug 6, 2012

Moin moin =)

Okay wasn't here a longer time now, but now I'm back and say this. Before I start the work again on this spell, I have some points to say:

1) Attaching integer to units. My first thought about it was using UnitUserData. But this would mess up since I would use it to non-dummy units. Other way I can think about is dealing with hashtables. Anyway I have no idea how to handle hashtables, actually I hate them but even if, is it really worth it?
- If there is any other good way, please tell me since I don't know any good =(

2) The SquareRoot thing: I don't know if my math skills are wrong but as far as I calculated, the result of "with SquareRoot without constant²" and "without SquareRoot with constant²" is different. Maybe SquareRoot is slow (we should define the word slow here anyway..), but it's more accurate.

3) You are kidding me right? In the past I used "First of group" actions for same situations and so many people flamed me for doing that, so I switched to the other way. Now you show up and say I should use it again?
Actually I don't have a problem with switching back, but then you should tell me why "First of Group" is now so much better as it was for like 2 years.

Okay these are the 3 points I really want to know, because I don't understand all and hope I'll learn something from it.

Greetings and Peace
Dr. Boom

ap0calypse · Aug 6, 2012

About 2) SquareRoot is not more accurate. In fact: not using it may actually be more accurate (though Warcraft can handle 8 digits behind the comma I believe, so a slight accuracy-advantage wouldn't matter that much anyway).

The standard way of doing things is something like this:

JASS:

if SquareRoot( x1*x1 + y1*y1 ) >= SquareRoot( x2*x2 + y2*y2 ) then

One thing you should notice is that "SquareRoot" can be cancelled out on both sides (a valid move).

So it becomes:

JASS:

if ( x1*x1 + y1*y1 ) >= ( x2*x2 + y2*y2 ) then

In your case, you use STORM_AREA instead of the second distance, so you could do STORM_AREA * STORM_AREA to get the same results.
This is done by multiplying the numbers by themselves (as you would to get rid of SquareRoot). This may only be done if the number itself is positive (and distances can't be negative).

Why would it increase accuracy? Because using SquareRoot will make the number smaller and may force it to round (though it won't matter much).

Dr. Boom · Aug 6, 2012

Moin moin =)

Wondering what your example has to do with my calculation for the distance check. I would agree to your example, but since I don't check a SquareRoot result with another SquareRoot result, I still see it a bit different:

Edit: Even if I don't round up any value, its still more accurate as the non sqrt way.

Greetings and Peace
Dr. Boom

ap0calypse · Aug 6, 2012

Dr. Boom said:
Moin moin =)

Wondering what your example has to do with my calculation for the distance check. I would agree to your example, but since I don't check a SquareRoot result with another SquareRoot result, I still see it a bit different:

Edit: Even if I don't round up any value, its still more accurate as the non sqrt way.

Greetings and Peace
Dr. Boom

First of all: it makes the number smaller because that's just what square root does

I'm talking about 3 being smaller than 9 etc.
Making a number smaller also means that you have less digits to represent the number, unless you go far beyond the comma (which Warcraft cannot do).

That image shows exactly why SquareRoot is less accurate...

Without SquareRoot (original number), the number is 7351,276106 and it will not automatically round (because there is nothing to round in this case).
SquareRoot number: 85,7395830757 - warcraft cannot remember the last 2 digits of that number, so it will automatically round to 85,73958308 (thus losing a bit of accuracy there).

Now if you check 85,73958308² (the number which is rounded = 7351,276107. It changes the original number by 0.000001, which is both less accurate and completely useless.
All in all: accuracy gets worse, but doesn't matter at all. Even integers are accurate enough for coordinates / distances (in wc3 at least).

FUJI · Oct 30, 2013

You can merge the create method and the condition.

Stormcyclone [1.0]

Stormcyclone 1.0 (Map)

Moderator

Moderator

Similar threads