Simple bounce ball

[Moderator]

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

12:33, 23rd Apr 2014
BPower:
Looks cool according to the picture, but there are some things which have to improve. link
Need Fix for now. Feel free to ask if you have any questions.

 

[Tal'Rarity]

Only bad thing I spotted was the timer of 0.01, you should be using 0.0325000 or something like that if I recall correctly for coders.

 

[SinnerPo]

Рашен понимашен?

 

[deathismyfriend]

Never use TriggerAddAction use TriggerAddCondition and run everything from there.
Use a timer of .03125
When using vJass and structs simply make the whole spell in the struct. It is much easier to manage.
The spell id should be easily configurable.
The special effects should be easily configurable.
You should filter out dead / removed units.
The time should be easily configurable.

Look at some of my spells. They are in vJass / Jass. Look at all the configurations there.

 

[ILH]

Рашен понимашен?

hey dude, this is english site, so speak english

 

[War3I4i]

Конечно же я понимаю русс)
And now about your advices:

1. Use a timer of .03125 - oh okay, i'll fix it soon
2. When using vJass and structs simply make the whole spell in the struct. It is much easier to manage. - So you want to tell me that all my spell not in structure or what?
3. Spell id i'll make in global variable, ty
4. Effects too
5. The time should be easily configurable - time of what?

[Fixed]
1. Added to settings 2 effects : final effect and effect after fall
2. Set timer period to .03
3. Added to settings spell id
4. Now use trigg add condition (not add action)

 

[deathismyfriend]

2. When using vJass and structs simply make the whole spell in the struct. It is much easier to manage. - So you want to tell me that all my spell not in structure or what?

These are part of the spell and not in the struct.

    private function Spell_Start_Cond takes nothing returns boolean
        return GetSpellAbilityId() == SpellId
    endfunction

    private function Spell_Start takes nothing returns nothing
        local unit a = GetSpellAbilityUnit()
        local real angle = Atan2(GetSpellTargetY() - GetUnitY(a), GetSpellTargetX() - GetUnitX(a))
        call Bounce.Bounce_Start(a, angle, 4, GetUnitX(a), GetUnitY(a), 0, true)
        set a = null
    endfunction


    private function Init takes nothing returns nothing
        local trigger Cast = CreateTrigger( )
        local integer index = 0
        loop
            call TriggerRegisterPlayerUnitEvent(Cast, Player(index), EVENT_PLAYER_UNIT_SPELL_EFFECT, null)
            set index = index + 1
            exitwhen index == 16
        endloop
        call TriggerAddAction( Cast, function Spell_Start )
        call TriggerAddCondition(Cast, Condition(function Spell_Start_Cond))
        set Cast = null
    endfunction

5. The time should be easily configurable - time of what?

By the time I meant the timer.

 

[War3I4i]

Of course this part of spell is not in the struct because it's spell starting with some arguments, Init function it's a scope initializer function, this part don't need to be in struct)
Also i'm fixed code and now don't use trigg add action, thanks for your advices

 

[deathismyfriend]

Of course this part of spell is not in the struct because it's spell starting with some arguments, Init function it's a scope initializer function, this part don't need to be in struct)
Also i'm fixed code and now don't use trigg add action, thanks for your advices

It makes It a lot nicer in my opinion.

 

[SinnerPo]

hey dude, this is english site, so speak english

My conversion was only for creator)

 

[deathismyfriend]

My conversion was only for creator)

That should have been done through pm or message on his page. Threads and all conversations in them should stay in English.

 

[BPower]

- GetSpellAbilityUnit() could be GetTriggerUnit()
- methods are public by default, if not declared as private.
- Store 'h000' into an integer variable. In other maps 'h000' might be a totally different unit type, hence has to be part of the configuration block.
- local unit F --> local unit u
- There is a missing "call" statement here RemoveUnit(ball)
- Don't use magic numbers, a good rule is that everything except 1, 0, -1 should be configurable and/or stored into variables.
- GetRectMinX(bj_mapInitialPlayableArea), etc ... should be stored on init or you can use a library like WorldBounds/SentinelBounds as requirement for this spell.
- In this case ballx/y is already stored into checkX/Y.

                    set dx = x - GetUnitX(ball)
                    set dy = y - GetUnitY(ball)

You have multiple needless GetUnitX/Y function calls, as those values are already stored into local variables.

- For group enumerations create one global group on map init instead of multiple local ones.
- Your identation is screwed at some points of the script code.
- In general the convention for methods should be camelCase and functions PascalCase
- You may explain the IsSpell condition.
- Chose reasonable variables names instead of random one char names like a.
Furthermore please stick to the JPAG convention, it is mandatory for submitted resources in JASS and vJass.

Feel free to ask for help if you need it.

 

[War3I4i]

Sorry but i'll do all changes tomorrow, now about fixes:
1. ok, i'll change it to GetTriggerUnit()
2. thanks for this advice
3. sorry, i frogoted to save unit id in global variable
4. i'll not change unit F to unit u bcause i'm always use F (as FirstOfGroup())
5. hm... okay
6. i need to check this distance to make a parabola height

 

[deathismyfriend]

4. All locals should be camelCase. Other than that keep it as f. It doesn't really matter but unit u is used a lot more so it is a little odd to see unit f.

 

[War3I4i]

4. All locals should be camelCase. Other than that keep it as f. It doesn't really matter but unit u is used a lot more so it is a little odd to see unit f.

Unit F associate for me with FirstOfGroup function so i use unit F

 

[deathismyfriend]

Unit F associated for me with FirstOfGroup function so i use unit F

Grin all you want. I was just telling you convention. Keep being rude like that though and not many people will want to bother helping you.

You set firstofgroup here twice for no reason.

                        loop
                            set F = FirstOfGroup(g)
                            exitwhen F == null
                            if IsUnitEnemy(F, GetOwningPlayer(a)) and not IsUnitType(F, UNIT_TYPE_DEAD) then
                                call UnitDamageTarget(a, F, dmg, false, false, ATTACK_TYPE_NORMAL, DAMAGE_TYPE_DIVINE, null)
                            endif
                            call GroupRemoveUnit(g, F)
                            set F = FirstOfGroup(g)
                        endloop

Also timer should be 0.03125
.03 is for GUI triggers.

You need to null local groups.
You should make a global group and create it only once. Creating and destroying groups is very costly.