Amumu the Sad Mummy

Tank-Commander · Jun 19, 2016

Thanks for the submission to the spells section

I made some quick notes:
- Loops should run every 0.03 seconds - 0.01 is needlessly fast
- you only need one index variable (that doesn't need to be an array)
- The triggers in your description are a bit messed up
- Your filtering in bandage toss is sub-optimal (there's a tutorial on how to do this better in the tutorial section)
- Collision size for bandage toss should be configurable
- DummyPoint leaks in bandage toss
- Grouo[0] leaks in bandage toss
- I suggest using SetUntX/Y over move unit instantly
- if all units have the despair buff removed at the same time, then they aren't recycled
- You index all units affected by despair repeatedly - even if they are already indexed
- Real[1] in despair is not set to 0 when a unit is first indexed - this makes the damage dealt by the ability inconsistent
- You can use Triggering Unit instead of learning hero, it's faster
- There's a random pointless condition in "Tantrum refresh"
- There's a lack of uses of temporary variables when appropriate (such as storing calculations of which the results will be used many times)
- Special effects should be configurable
- Dummy units should be owned by a neutral player as to not mess up the score screen
- Running a filter on literally every unit is not terribly efficient - keep track of what units have which abilities instead of constantly filtering from all units
- Each spell has a very different level of code quality, please bring them all up to the same level
- Don't use imported and especially ripped models from games.
- Change the folder name for the pack to an appropriate name
- Standardize your trigger names (apply prefix, be consistent in naming)
- Change your variable names to have a common prefix e.g. ASM_Group[] this prevents other spells from interfering with this pack
- Temporary groups (ones that are cleared and not stored anywhere after the spell ends) do not need to be indexed - they can all be the same group
- The amount of effects for CotSM should be configurable
- Each spell should have a separate configuration trigger - people may not want to import all the spells from the pack

That's all I got from my first read-through, there may be more but it's hard to notice due to these ones.

LimitingBounds · Jun 19, 2016

Thanks for the review. I shall get to work on these...

*gulp*

Wietlol · Jun 20, 2016

I have a few approach suggestions:

First of all, sorry if TC already mentioned something I said... I am not a reader.

Cursed Touch:
Make a trigger that runs whenever a unit has gained a level.
If the level is X, increase the ability level.
The onDamage trigger is quite nice using a DDS, but "Index[4]" should be "Index[0]" at the very least and maybe renamed to something like "TempInteger". I use TempBlaBla for every global temporary variable that I use if I use GUI.
If however, Index[4] is somehow related to the spell, the name should be changed.
Also the damage calculation is a bit odd... shouldnt it be "Set PDD_Amount = Max(PDD_Amount - CursedTouch_DamageReduction[Index], 0)"?
Where it will take the largest number out of those so there wont be negative damage.
There is a nice GUI function that does this... probably under the math category.
Also shouldnt it only apply when taking "PDD_SpellDamage" or something like that?

Bandage Toss:
Again name your global variables (pretty much constant variables) something relative to the spell.
"AbilityIndex[0]" is not really informative.
"BandageToss_AbilityId" tells you what it is (the ability id) and of what ability it is "Bandage Toss".
Such prefixes like "BandageToss_" or "BT_" or whatever just prevent global variable naming collisions. (I hope TC mentioned that at the very least.)

As the human eye wont notice the difference between 0.01 and 0.03 intervals and the cpu does, it is better to stick to 0.03.
Some people prefer 0.03125 as it is a proper division of 1, but they probably dont actually know why they prefer it

For accuracy reasons, go for 0.03, for performance reasons go for 0.03125 (as you save 1.3 frame per seconds out of 33.3).
For just idgaf reasons, go for 0.03 as I do.

If you deal with time related stuff such as speed when you have to move a unit or something like that, you should multiply the speed per second by the interval duration. So: "Set DistanceTraveled = DistanceTraveled + SpeedPerSecond * 0.03".
If you deal with acceleration and stuff, you might go a level too high though.

For complete collision ignore, you can also add an ability called Ghost (visible) to the unit.
In this case, it doesnt matter, but other units will try to walk around your unit if only that unit's collision is turned off.
If you also give them ghost, other units will walk right through it.
In this case it doesnt matter as I said because this unit will be moving quite fast.

Despair:
Can you turn it off?

When a unit turns it on, add that unit to a unit group "Despair_UnitGroup".
Every 0.03 seconds, you pick all units in that unit group.
No need to go through all units on the map right?
When they turn it off, die or go out of mana, you remove them from the group.

Tantrum:
Very simple approach:
Make a trigger that runs on a DDS.
If the damage is physical and the level of Tantrum for the targeted unit is larger than 0 (aka he has learned it), then reduce damage by Tantrum_DamageReduction[Level of Tantrum for targeted unit], again using that amazing max function to not let it go below 0.

As for the active:
Make a trigger that runs on cast.
Pick all nearby enemy living non-structure units and damage them.
Cooldown reduction is pretty hard in WC3 unfortunately... would have been fun.

Curse of the Sad Mum:
The current trigger is pretty good but you should be able to do the SFX without loops... just get a proper SFX.

LimitingBounds · Jun 21, 2016

Wietlol said:
I have a few approach suggestions:

First of all, sorry if TC already mentioned something I said... I am not a reader.

Cursed Touch:
Make a trigger that runs whenever a unit has gained a level.
If the level is X, increase the ability level.
The onDamage trigger is quite nice using a DDS, but "Index[4]" should be "Index[0]" at the very least and maybe renamed to something like "TempInteger". I use TempBlaBla for every global temporary variable that I use if I use GUI.
If however, Index[4] is somehow related to the spell, the name should be changed.
Also the damage calculation is a bit odd... shouldnt it be "Set PDD_Amount = Max(PDD_Amount - CursedTouch_DamageReduction[Index], 0)"?
Where it will take the largest number out of those so there wont be negative damage.
There is a nice GUI function that does this... probably under the math category.
Also shouldnt it only apply when taking "PDD_SpellDamage" or something like that?

Bandage Toss:
Again name your global variables (pretty much constant variables) something relative to the spell.
"AbilityIndex[0]" is not really informative.
"BandageToss_AbilityId" tells you what it is (the ability id) and of what ability it is "Bandage Toss".
Such prefixes like "BandageToss_" or "BT_" or whatever just prevent global variable naming collisions. (I hope TC mentioned that at the very least.)

As the human eye wont notice the difference between 0.01 and 0.03 intervals and the cpu does, it is better to stick to 0.03.
Some people prefer 0.03125 as it is a proper division of 1, but they probably dont actually know why they prefer it
For accuracy reasons, go for 0.03, for performance reasons go for 0.03125 (as you save 1.3 frame per seconds out of 33.3).
For just idgaf reasons, go for 0.03 as I do.

If you deal with time related stuff such as speed when you have to move a unit or something like that, you should multiply the speed per second by the interval duration. So: "Set DistanceTraveled = DistanceTraveled + SpeedPerSecond * 0.03".
If you deal with acceleration and stuff, you might go a level too high though.

For complete collision ignore, you can also add an ability called Ghost (visible) to the unit.
In this case, it doesnt matter, but other units will try to walk around your unit if only that unit's collision is turned off.
If you also give them ghost, other units will walk right through it.
In this case it doesnt matter as I said because this unit will be moving quite fast.

Despair:
Can you turn it off?

When a unit turns it on, add that unit to a unit group "Despair_UnitGroup".
Every 0.03 seconds, you pick all units in that unit group.
No need to go through all units on the map right?
When they turn it off, die or go out of mana, you remove them from the group.

Tantrum:
Very simple approach:
Make a trigger that runs on a DDS.
If the damage is physical and the level of Tantrum for the targeted unit is larger than 0 (aka he has learned it), then reduce damage by Tantrum_DamageReduction[Level of Tantrum for targeted unit], again using that amazing max function to not let it go below 0.

As for the active:
Make a trigger that runs on cast.
Pick all nearby enemy living non-structure units and damage them.
Cooldown reduction is pretty hard in WC3 unfortunately... would have been fun.

Curse of the Sad Mum:
The current trigger is pretty good but you should be able to do the SFX without loops... just get a proper SFX.

Welp. I really do need to fix the triggers in the description, but that along with a hectic load this week is lmao.

Replies per spell comments said:
Cursed Touch:

Yes, though it's quite a lot easier to trigger it via the 'Unit Gains a Level' event, I made in such a way that it's retroactive. (Try it with the -levelup command in the testmap. )

And yes, I'll change the 'index' variables to just one, since I ridiculously thought before that a global variable used for multiple loops will affect each in a way...heh.

Also, I followed the PDD application like in the author's sample map. I hope I'm doing it correctly.

Bandage Toss:

Alright. Following that 0.03125 sec interval. Kinda gotten insecure about it being too slow so I set it to the fastest interval possible.

Already organizing the variables with the use of prefixes. Sorry for being pretty messy. Eheheheh~

Thanks for the suggestions for speed calculation and the use of Ghost instead of turning off collision. Will apply.

Despair:

Basically, my trigger is a Buff Detection System adds all units with the buff, at current, to the index, removes all units in the index without the buff, and executes the effect for the units in the index. Don't wanna add more triggers to the map for a spell I can code this way alone. The buff goes away the unit anyway once it dies or gets the spell turned off.

You're right. I was thinking that I should also include units that obtained the buff w/o the skill, and it doesn't make sense for a skill with a level-dependent damage output. Changing this appropriately then~

Tantrum:

What you said is actually it.

Simulated the cd reduction with a pseudo-cd timer trigger that should reset as the spell is cast.

Curse of the Sad Mummy

Once I find a nice spell effect that seems fitting and has no need for attaching and rescaling a dummy unit for it.

Wietlol · Jun 21, 2016

Ghost doesnt replace the collision turn off.
You need both to do it properly.
However, SetUnitX() and SetUnitY() ignore collision anyway iirc.

About Tantrum... I feel 5 triggers can be reduced to 2.
1, damage reduction and cooldown reduction.
2, active.

Amumu the Sad Mummy

Introduction

Spells

Cursed Touch

Bandage Toss

Despair

Tantrum

Curse of the Sad Mummy

Spell Description

Level

Manacost

Cooldown

AoE

Effect

Triggers

Cursed Touch Levelup

Cursed Touch Magic Damage

Spell Description

Level

Manacost

Cooldown

Range

Effect

Triggers

Bandage Toss Cast

BT Cast Loop

Spell Description

Level

Manacost (per Second)

Cooldown

AoE

Effect

Triggers

Despair Toggle

Despair Damage Tick

Spell Description

Level

Manacost

Cooldown

AoE (Active)

Effect

Triggers

Tantrum Level

Tantrum Refresh

Tantrum Physical Reduction

Tantrum CD Simulator and Indexer

Tantrum Active

Spell Description

Level

Manacost

Cooldown

AoE

Effect

Triggers

CotSM Spell

Miscellaneous In-coded Spell Triggers

Amumu Init (Configuration Trigger)

Credits

Changelog

Amumu the Sad Mummy (Map)

Similar threads