Swirling Death v1.5

Moderator · Oct 20, 2014

Swirling Death v1.4 | Reviewed by BPower | 01.08.2015

[COLOR="gray"

Code[/COLOR]]

	The spell is MUI, leakless and working. There is a small mistake in your code ( See below )
	I sure Damage2 should be Damage1. Unit - Cause SD_Caster[SD_TempIndex] to damage SD_PickedUnits, dealing Cnfg_SD_Damage2[SD_AbilityLevel[SD_TempIndex]] damage of attack type Cnfg_SD_AttackType1 and damage type Cnfg_SD_DamageType1

Objects[/COLOR]]

Objects are ok. Nothing to complain here.

Effects[/COLOR]]

	Effects are ok, however you spam them quite a lot.
	@Map makers: Always be aware that effect spamming could be a reason for unwanted fps drops, especially in multiplayer seasons. Be careful!

Rating[/COLOR]]

CODE	OBJECTS	EFFECTS	RATING	STATUS
3/5	4/5	2.5/5	3.1/5	APPROVED

IcemanBo · Oct 20, 2014

Could you make it support multiple levels?

Set SD_TempPoint = ((Position of SD_Target[SD_TempIndex]) offset by SD_StartOffsetLoc[SD_TempIndex] towards SD_Degree[SD_TempIndex] degrees)

^This leaks a location.

Could you put the triggers into hidden tags? [hidden="Title"]code[/hidden]

Meatmuffin · Oct 20, 2014

Sorry, I have not yet mastered the making spells support multiple levels. And for the other two things, I'll do them tomorrow. Thanks.

IcemanBo · Oct 20, 2014

You only have to create one more integer[array] variable to support levels.
Then you have to change from some non-array variables to array variables. For Damage, AOE, Duration...

Init will look like this:

Set Damage[1] = 10
Set Damage[2] = 20
Set Damage[3] = 30
(user can change it for each level)

When you index you save the level of ability in your new variable:
Set AbilityLevel[index] = Level of (YourAbiliry for (YourUnit))

And in loop, when you do damage, you use it:
Unit - Cause Damage[AbilityLevel[index]] to unit.

So now the damage depends on the level.

Meatmuffin · Oct 20, 2014

Okay thanks, I'll try. Today I'm pretty beat, so I will put this on hold for a day. Tomorrow, I will finish it. Besides the leak and the missing level support, do you see any leaks?

IcemanBo · Oct 20, 2014

No, I don't see more memory leaks.

But try to make as much configurable as posible. (attack- and damage type for example)
Instead of rolling a random number between 1 and 2, better use a boolean[array] which you always change between true/false.
You also don't need two temp group and no two temp unit variables.

kakuzu · Oct 21, 2014

Nice !

GywGod133 · Oct 21, 2014

In Group...

You need ITE (IF THEN ELSE) for checking the condition instead using the Unt Group for checking the condtion.

In other codes is looks good :grin:

Meatmuffin · Oct 21, 2014

IcemanBo said:
You only have to create one more integer[array] variable to support levels.
Then you have to change from some non-array variables to array variables. For Damage, AOE, Duration...

Init will look like this:

Set Damage[1] = 10
Set Damage[2] = 20
Set Damage[3] = 30
(user can change it for each level)

When you index you save the level of ability in your new variable:
Set AbilityLevel[index] = Level of (YourAbiliry for (YourUnit))

And in loop, when you do damage, you use it:
Unit - Cause Damage[AbilityLevel[index]] to unit.

So now the damage depends on the level.

@Iceman
As I said, I don't quite know how to implement level support. Could you make a simple trigger spell that does, and post it in a reply? Because I tried your method, and I can't seem to get it right. If not, I guess I'll just fix leaks and stuff, and save level implemention for my next spell upload.

IcemanBo · Oct 21, 2014

I tried to explain it. You can have a look in other GUI spells, which support it. If you sill don't understand it you can open a thread about your problem.

Meatmuffin · Oct 21, 2014

Bah, fine, when I'm up for it, I'll dig a little deeper.

BPower · Oct 21, 2014

Please post an ingame screenshot aswell.

Meatmuffin · Oct 22, 2014

Couldn't do that for some reason, I'm on a new computer, will try to fix that along with the next update.

IcemanBo · Oct 22, 2014

Print + Paint.

Meatmuffin · Oct 22, 2014

Yeah, I saved it with paint, then it didn't allow me to upload. Then I changed it to .jpg, and now the image is fucked up.

deathismyfriend · Oct 24, 2014

Please use hidden tags when posting trigger.

Also this leaks a location.

Set SD_TempPoint = ((Position of SD_Target[SD_TempIndex]) offset by SD_StartOffsetLoc[SD_TempIndex] towards SD_Degree[SD_TempIndex] degrees)

After you de-index you need to decrease the looping integer ( SD_TempIndex ) otherwise one index never runs each time another index is removed.

Spells should be able to have multiple levels. Yours currently only has one.

Meatmuffin · Oct 25, 2014

Update, hello world.

GywGod133 · Oct 26, 2014

You have need to change something....

In your Trigger Loop..

Unit Group - Pick every unit in SD_TempGroup and do (Actions)
- Loop - Actions
  - If (All Conditions are True) then do (Then Actions) else do (Else Actions)
    - If - Conditions
      - ((Picked unit) belongs to an enemy of SD_TempPlayer[SD_TempIndex]) Equal to True
      - (Picked unit) Not equal to SD_Target[SD_TempIndex]
  - Then - Actions
    - Set SD_PickedUnits = (Picked unit)
    - Unit - Cause SD_Caster[SD_TempIndex] to damage SD_PickedUnits, dealing Cnfg_SD_Damage2[SD_AbilityLevel[SD_TempIndex]] damage of attack type Cnfg_SD_AttackType1 and damage type Normal
    - Set SD_PickedUnits = No unit
Custom script: call DestroyGroup(udg_SD_TempGroup)

->>>

Unit Group - Pick every unit in SD_TempGroup and do (Actions)
- Loop - Actions
  - Set SD_PickedUnits = (Picked unit)
  - If (All Conditions are True) then do (Then Actions) else do (Else Actions)
    - If - Conditions
      - ((SD_PickedUnits) belongs to an enemy of SD_TempPlayer[SD_TempIndex]) Equal to True
      - (SD_PickedUnits) Not equal to SD_Target[SD_TempIndex]
  - Then - Actions
    - Unit - Cause SD_Caster[SD_TempIndex] to damage SD_PickedUnits, dealing Cnfg_SD_Damage2[SD_AbilityLevel[SD_TempIndex]] damage of attack type Cnfg_SD_AttackType1 and damage type Normal
    - Set SD_PickedUnits = No unit
Custom script: call DestroyGroup(udg_SD_TempGroup)

Meatmuffin · Oct 26, 2014

I beg to differ. Why would I pick every unit in an explosion AoE, instead of picking only the ones that I damage?

GywGod133 · Oct 26, 2014

cuz of multiple units

IcemanBo · Nov 6, 2014

Swirling Death v1.1 - Review by IcemanBo & edo494

Date: November 6th, 2014

Concept:

Swirling Death silences the target, surrounds it with a circular movement, until it reaches it's position, and creates explosions all over it's way.
Afterwards it vanishs the same way back, in a circular/surrounding movement.

In terms of the surrounding circulation, inwards and outwards, and the different explosions on it's way (physical- and magical damage explosions),
it is a good and interesting spell concept.

Code:

In cast trigger:
Set SD_AbilityLevel[SD_MaxIndex] = (Level of Swirling Death for (Triggering unit))
->
Set SD_AbilityLevel[SD_MaxIndex] = (Level of Swirling Death for (SD_Caster[SD_MaxIndex]))
I personaly think it's better to try to stick in one way of coding. For example, in GUI, to use as less custom scripts as possible.
It's not that important, but you can remove this custom script line:
Custom script: set udg_SD_ExplBoolean[udg_SD_TempIndex] = not udg_SD_ExplBoolean[udg_SD_TempIndex]
^And instead, you just could set it to true/false in the matching If/Then/Else block via normal GUI action.
Immediatly store "PickedUnit" into the variable, so you already can use it for the checks in conditions.
Damage type should be configurable.
There is comment in loop:
"^And instead, you just could set it to true/false in the matching If/Then/Else block via normal GUI action."
That's corrent. But you should mention one thing. If user removes the boolean variable, there won't be a switching anymore between these two group enumerations and no witching between physical/magical damage. It follows that only one kind of enumeration and damaging will be executed, and so it will be executed twice as often as before. (-> double phiscal, or double magical damage will be dealt)
Second unit enumeration can be moved in "Else - Actions" part of first If/Then/Else, to avoid a second check for the boolean value.
In deindex part you have forgotten to switch the boolean variable with MaxIndex.
Set SD_PickedUnits = No Unit
^Nulling this global unit variable is not really needed, as you for sure will use it again.
Variable name "SD_StartOffsetLoc[]" could make someone think it's a location, but not a real.

Coding is alright. Aside of these minor mentioned things, it's fine.

Visuals:

Icon for learning and spell icon itself differs. This doesn't really make sense. You should equal them.
Learning tooltip does not privide any spell describing information. You should improve it.
Explosions look good, and it's movement is fairly smooth.

Spell tooltip. You don't have to describe technical aspects in detail:
For example, you don't have to note that you also can target allies. (it's set in object data anyways)
"Occurs at target's cooridinates" is also obviously, if you target a unit with a spell.

Also, you could highlight the name of the ability, as well as the hotkey.

Conclusion:

Swirling death has some nice special effects, that are fitting to the explosions. Also the circular movement towards the target looks good in game.
You definitly should work on spell's tooltip, even it doesn't seem really important.
In coding there is nothing expensive/impressing. It's a simple spell, but useful.

Rating: 3/5 and vote for approval. (after fixes)

edo494 · Nov 6, 2014

as mentioned above, I just so I can vote

Meatmuffin · Nov 24, 2014

Small update, haven't been at WE for a while

IcemanBo · Jan 6, 2015

Deindex if the caster gets removed from game. The spell won't make any effect anymore, because caster can't damage anymore.
You can check if a unit is removed if one's UnitType equals "NoUnitType".
You forgot about "AbilityLevel[]" in your deindex part.
Refer to the variable instead of using "Picked Unit" more often.
No need to check the boolean[index] for true and for false in loop. You can move the whole physical damage block into the 'Else' of the first 'If - statement'.
Make some decent import instructions.

You're on a good way, but I still await an update for approval. :csmile:

Meatmuffin · Mar 29, 2015

Update consists of making the spell triggers smoother, more leakless and flawless.

IcemanBo · Apr 2, 2015

When target is dead/removed you should deindex. It might be tolerated for target's death, but it's not acceptable for unit being removed.
Better just cover both cases, and don't bother with these details no more. Just handle it as you do alredy for caster.

The speed in config doesn't match the actual wc3 movement speed. It's /0.03 in your case. Might be noted.

In import instructions:

4. Copy the variables if possible. If not, make them via Variable Editor.

^You should explain the usual method to use when copying triggers. It's just best for GUI.

Just for future. Try to keep variable names simple and intuitive. For example:

Set Cnfg_SD_CloserDistance = 5.00

User can't really imagine immediatly what you mean with this name, it could be simple "SD_Offset", or "SD_Speed".
And in config the additional "Cnfg_" is not necessarily needed. Here atleast you still have the "SD_" one included, so it's okay, but you also can remove the "Cnfg_" actually.
Hope you see what I mean. When reading a lot of code you start to get nitpicking when it comes to things like readability and good names.

SayazEmil · Apr 6, 2015

Hey the Spell looks cool, can you please add some more Comments for newbies like me? Thank you.

Meatmuffin · Apr 7, 2015

If I'm not too lazy.

Meatmuffin · Jul 30, 2015

Minor update, so that the spell can finally get approved. (I hope)

Chaosy · Jul 30, 2015

edit: nvm.

I got a somewhat debatable suggestion.

Right now you do:
if boolean == false then
unit group stuff
else
slightly difference unit group stuff

However you can do:
Unitgroup
if boolean == false
actions

This will make the code a little shorter since you only need to store the unit group variable at one location in the code. However it will be more inefficient since you check for the boolean once per unit.

Meatmuffin · Jul 30, 2015

That is indeed possible and props to you for figuring out an alternate way, but I'm not going to use that, as you mentioned, there is a downside.

BPower · Jul 31, 2015

Hey, some things you should fix:

1. Use the variable instead of Swirling |cffff0000D|reath.

Set SD_AbilityLevel[SD_MaxIndex] = (Level of Swirling |cffff0000D|reath for SD_Caster[SD_MaxIndex])

2. In any case you deal "damage2" damage to valid unit targets.

Meatmuffin · Jul 31, 2015

BPower said:
2. In any case you deal "damage2" damage to valid unit targets.

Can you elaborate? I didn't get it.

BPower · Aug 1, 2015

There is the function which deals damage to the targets. It's using the wrong variable.

Meatmuffin · Aug 1, 2015

Fixed.

Swirling Death v1.5

Swirling Death v1.05 (Map)

Moderator

Moderator

[COLOR="gray"

[COLOR="gray"

[COLOR="gray"

[COLOR="gray"

Similar threads