Split Fireball V 1.01

[Moderator]

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

13:16, 4th Mar 2011
Bribe:


My review

Status: Rejected until updated

 

[hell gate]

looks nice but why you don't move

• Unit Group - Remove (Dying unit) from Fireballs

into the loop after killing the unit?

 

[Laserdude]

looks nice but why you don't move

• Unit Group - Remove (Dying unit) from Fireballs

into the loop after killing the unit?

I knew someone would mention that.
Because that is unneccessary, since I still need the Fireball Death trigger, bacause of the Generic Expiration Timers, so without it, all the Fireballs which does not hit anything would never get out of the unit group, so then I don't add the same action into the same event, because it would be unneccessary.

 

[Maker]

Always put two or more casters in the test map so it's easier to test MUI stuff.
Learn tooltip has lots of typos/bad grammar. For example "untill" -> "until" , "ammount" -> "amount" , "inscreases" -> "increases".
I also prefer the standard Blizzard way of displaying ability level data.
The dummy doesn't need invulnerability ability, it has locust.
Generally if you use something more than once, save it to a variable and use the variable. Like angle from point a to point b and triggering unit. This increases performance.
Split Fireball Move should be initially off. And turned on/off when needed.
Here again, set picked unit to a variable, and use the variable instead of picked unit.
Leak:

• (Number of units in (Units within 50.00 of FireMovePoint matching...

Leak:

• Unit - Create 1 Fireball for (Owner of (Picked unit)) at (FireMovePoint offset by 53.00...

FireLoad, FireSpacing and FireHits should be loaded to a variable.
Fireball Death is unnecessary. Remove the unit from the group in the looping trigger, after you kill it.
The whole if/then/else structure could be better organized. You shouldn't have clear hashtable and kill unit twice there.
The caster won't get the credit from the kill, might be a problem.
The spell isn't easily configurable.

 

[Laserdude]

Always put two or more casters in the test map so it's easier to test MUI stuff.
Learn tooltip has lots of typos/bad grammar. For example "untill" -> "until" , "ammount" -> "amount" , "inscreases" -> "increases".
I also prefer the standard Blizzard way of displaying ability level data.
The dummy doesn't need invulnerability ability, it has locust.
Generally if you use something more than once, save it to a variable and use the variable. Like angle from point a to point b and triggering unit. This increases performance.
Split Fireball Move should be initially off. And turned on/off when needed.
Here again, set picked unit to a variable, and use the variable instead of picked unit.
Leak:

• (Number of units in (Units within 50.00 of FireMovePoint matching...

Leak:

• Unit - Create 1 Fireball for (Owner of (Picked unit)) at (FireMovePoint offset by 53.00...

FireLoad, FireSpacing and FireHits should be loaded to a variable.
Fireball Death is unnecessary. Remove the unit from the group in the looping trigger, after you kill it.
The whole if/then/else structure could be better organized. You shouldn't have clear hashtable and kill unit twice there.

I Will fix grammar and make the ability tooltip look better.
I Will make the offset in the trigger below and the condition to a real variable.

• Unit - Create 1 Fireball for (Owner of (Picked unit)) at (FireMovePoint offset by 53.00...

I will fix the turnoff to reduce lag.
Things I dissagree with:
Sometimes it happens that units with only locust CAN take damage, therefore I prefer to stick to having invulnerability there to take away that possibility.
No. Hashtable data is better than using a hashtable combined with variables, and I rather use a hashtable then using only variables.
As mentioned in my post above, Fireball Death MUST remain there, or the whole spell will leak and some units will stay in the unit group forever, since the fireballs uses generic expiration timers. This is why I also must add a Clear Hashtable Data to that trigger, which will NOT leak in the move trigger if the kill picked unit action is moved to the end of the trigger.

How would you want me to change the if/then/else structure? A looping action can not be added since there is 2 different angles.

EDIT: Fixed things I promised to fix.

 

[Maker]

Sometimes it happens that units with only locust CAN take damage, therefore I prefer to stick to having invulnerability there to take away that possibility.

Really? When is that? Damage target and damage area won't affect units with locust.

No. Hashtable data is better than using a hashtable combined with variables, and I rather use a hashtable then using only variables.

I'm sorry to say but that's nonsense. I mean this:

• set r1 =Load (Key FireFacing) of (Key (Picked unit)) from Firetable
• set r2 = Load (Key FireSpeed) of (Key (Picked unit)) from Firetable
• set i1 = Load (Key FireHits) of (Key (Picked unit)) from Firetable
• Unit - Create 1 Fireball for (Owner of (Picked unit)) at (FireMovePoint offset by SpawnRange towards r1 + 90.00) degrees) facing r1 + 90.00) degrees
• set u1 = Last created unit
• Hashtable - Save r1 + 90.00) as (Key FireFacing) of (Key (Last created unit)) in Firetable
• Hashtable - Save r2 as (Key FireSpeed) of (Key (Last created unit)) in Firetable
• Hashtable - Save i1 - 1) as (Key FireHits) of (Key (Last created unit)) in Firetable
• Unit - Set level of Fireball Death for (Last created unit) to (Level of Fireball Death for (Picked unit))
• Unit - Add a 3.00 second Generic expiration timer to u1
• Unit Group - Add u1 to Fireballs

As mentioned in my post above, Fireball Death MUST remain there, or the whole spell will leak and some units will stay in the unit group forever, since the fireballs uses generic expiration timers.

You could add "unit is alive check". You can leave the dying trigger there if you want to. It's just that all units will trigger the A unit dies trigger.

Keep on making the spell better, and remember to fix the leaks

 

[Laserdude]

Really? When is that? Damage target and damage area won't affect units with locust.


I'm sorry to say but that's nonsense. I mean this:

• set r1 =(Load (Key FireFacing) of (Key (Picked unit)) from Firetable
• set r2 = Load (Key FireSpeed) of (Key (Picked unit)) from Firetable
• set i1 = Load (Key FireHits) of (Key (Picked unit)) from Firetable
• Unit - Create 1 Fireball for (Owner of (Picked unit)) at (FireMovePoint offset by SpawnRange towards r1 + 90.00) degrees) facing r1 + 90.00) degrees
• set u1 = Last created unit
• Hashtable - Save r1 + 90.00) as (Key FireFacing) of (Key (Last created unit)) in Firetable
• Hashtable - Save r2 as (Key FireSpeed) of (Key (Last created unit)) in Firetable
• Hashtable - Save i1 - 1) as (Key FireHits) of (Key (Last created unit)) in Firetable
• Unit - Set level of Fireball Death for (Last created unit) to (Level of Fireball Death for (Picked unit))
• Unit - Add a 3.00 second Generic expiration timer to u1
• Unit Group - Add u1 to Fireballs

You could add "unit is alive check". You can leave the dying trigger there if you want to. It's just that all units will trigger the A unit dies trigger.

Keep on making the spell better, and remember to fix the leaks

1. When creating dummy units, sometimes units will start attacking them, this has happened to me sometimes when I am creating spell swith dummy units.

2. When it comes to hashtables, people seem to have different views of what is best. I made another spell, which was basically what you are saying, but then, people said it was useless to combine variables with hashtable data when you could just use one of them, and since indexing systems is MUCH harder to make with spells like these, I'd rather use hashtables.

3. Unit is alive check? Why? It allready checks if the unit is a Fireball, and if it is, it is allways dead, since the acion is - A unit dies.

4. All the leaks is fixed, as far as I can see.

 

[Maker]

1. Weird. Units with locust can't be targeted.
2. This has nothing to do with indexing vs. hashtables. I'm not talking about indexing. It's all about not needing to load the same data over and over, when you only have to load it once.
3. Unit is alive check in the looping trigger.
4. Both of the leaks I pointed out are still there.

 

[Laserdude]

1. Weird. Units with locust can't be targeted.
2. This has nothing to do with indexing vs. hashtables. I'm not talking about indexing. It's all about not needing to load the same data over and over, when you only have to load it once.
3. Unit is alive check in the looping trigger.
4. Both of the leaks I pointed out are still there.

1. They can not be targeted no, but auto attack (which occours if you attack a creep) can still hit them.
2. And if I decided to do as you wish, how exactly would you mean I should change the triggers?
3. It is not required if I allready turn off the trigger when there are no fireballs in there (which I do now), right?
4. I notice that one of them is there. But the condition one is gone, since the Real has been replaced with a variable.

 

[Maker]

2. Look at the trigger example I posted.
3. That's a good solution.
4. There's still a group created in the condition, and it is not cleared.

 

[Laserdude]

2. Look at the trigger example I posted.
3. That's a good solution.
4. There's still a group created in the condition, and it is not cleared.

4. Number of units? That is just a condition, it does not really create a unit group, it just matches a unit group, which should not leak.
EDIT: 2.

• Hashtable - Save (Key (string))

should have the same effect as

• Hashtable - Save (key (variable))

 

[Maker]

4. The condition from GUI -> JASS conversion: CountUnitsInGroup(GetUnitsInRangeOfLocMatching(udg_ExplodeRange, udg_FireMovePoint:

function GetUnitsInRangeOfLocMatching takes real radius, location whichLocation, boolexpr filter returns group
    local group g = CreateGroup()
    call GroupEnumUnitsInRangeOfLoc(g, whichLocation, radius, filter)
    call DestroyBoolExpr(filter)
    return g
endfunction

 

[Laserdude]

4. The condition from GUI -> JASS conversion: CountUnitsInGroup(GetUnitsInRangeOfLocMatching(udg_ExplodeRange, udg_FireMovePoint:

function GetUnitsInRangeOfLocMatching takes real radius, location whichLocation, boolexpr filter returns group
    local group g = CreateGroup()
    call GroupEnumUnitsInRangeOfLoc(g, whichLocation, radius, filter)
    call DestroyBoolExpr(filter)
    return g
endfunction

I don't do JASS, but I understand enought to see that in JASS, the condition actually picks the units.

 

[Bribe]

Hashtable

Time - Elapsed game time is 0.01 seconds

- Event should be "Map Initialization".

Split Fireball Move

Number of units in (Units within ExplodeRange of FireMovePoint

- Leaks

Unit - Create 1 Fireball for (Owner of (Picked unit)) at (FireMovePoint offset by SpawnRange

- Leaks

Fireball Death

Not sure what you mean by Picked unit but I'm guessing you meant "Dying unit". Not only, but you should be using "Triggering unit" as it is faster than "Dying unit".