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

14:07, 2nd Jan 2015
Maker: Check my reply in this thread.

old reviews 11:02, 23th Apr 2014 BPower: - The configuration Trigger could run on map initialization. - You use CM_temploc1, 2 and CM_SpinDummyLocion as location variables, while one would be enough to handle every location needed in the spell. - The variable CM_DamageGroup is not used, still it is part of the loop. Please remove it. 23:48, 7th Feb 2014 You created some new issues 09:14, 3rd Feb 2014 BPower: You leak a unit group. For logical reasons you should normally assign a purpose to index 0, but it is simply inefficient here. So start with 1 for ability level 1 -- > Set CM_Damage[1] = 200.00 and not Set CM_Damage[0] = 200.00 in the configuration. This way you will get rid of the arithmetic --> (CM_Damage[LVL] - 1). Store the ability level into a variable. The wisp explosion effect, could be shown even if you don't hit any target. Also it should be configurable. The shockwave sound is an overkill, the used effects already have sounds that fit. tempUnit, tempLoc, ... should have the prefix CM_ Damagetype and attacktype should be configurable.

 

You should implement a configuration trigger which runs once on map init.
Set variables like ability id, dummy, damage(level based), falling speed, aoe, effects, starting fly height ... to the desired default values.
This is important for proper and easy user configuration.

You kill a so called SpinDummy[index]. There is no unit set to that variable.
Talking about variables, you should label them like this: CM_Xxxx --> CM_Caster (for Chaos Meteor caster) so they do not interfere with other map code.

I'm not a fan of terrain deformations as they remain as permanent leak in data, but it fits to your concept, still you don't have to add a new trigger for it.
Just create the deformation at the end of the loop, once you killed the meteor dummy.

 

Firstly about that spin dummy I was trying something and I changed my mind that's left from before.

Secondly, for the configuration I'll start it now and update ASAP. Thanks for feedback

 

You need to add a configuration trigger to this spell, damage, special effects and such should all be configurable. Your spell must also support multiple levels to abide by the site rules. You leak a unit group.

You need a tree filter, currently it'll destroy bridges as well which are a big no.
Really there should not be terrain deformations - I've not seen them present in any decent spells purely because they leak memory, fit concept or not, leaks are not allowed. Not sure how I feel about playing the sound, probably shouldn't be there.

Just a few pointers for now

Edit: also the map needs a proper name (the downloaded one) not just MUISpellTest3

 

Hope that now it fits every rule and i removed the terrain deformation for performance issues and you said it leaked. And added a more artistic fire ball thing Hope you like this time

 

• 'THEN'-Aktionen
  • Set tempLoc1 = (Position of (Dying unit))
  • Spezialeffekt - Create a special effect at tempLoc1 using Abilities\Spells\Orc\WarStomp\WarStompCaster.mdl
  • Spezialeffekt - Destroy (Last created special effect)
  • Sound - Play Shockwave <gen> at 100.00% volume, located at tempLoc1 with Z offset 0.00
  • Custom script: call RemoveLocation(udg_tempLoc1)

^No need for an extra trigger for this maybe? You could just move the actions tp the deindex part of your loop, or?

I only tested it shortly, but seems a lot better than last time I saw code

 

Thanks i updated it so the extra trigger is gone. Posted the triggers on here too so everyone can view without downloading

 

For logical reasons you should assign a purpose to index 0, but it is simply inefficient here. So start with -- > Set CM_Damage[1] = 200.00 and not Set CM_Damage[0] = 200.00
So you don't have to do (CM_Damage[LVL] - 1).

Also store the level into a variable on spell cast.

 

If you wish to add deformations, give a Boolean that can be configured, then a comment warning users that if you set the Boolean to true, you will get deformation effect but risks get lagging maps.

 

i changed the array issues, and i changed my mind about the deformation for performance and leak issues made it more simple too. I'm gonna try for a fire mage spell pack hope it'll be good. I also created my very own first custom model too I'll post the updates of this spell in a few days

 

- the damage group filtering is not necesary actualy because the ability stopped after the first hit..
- you dont need to remove temploc1 after the meteor hits the ground
- misplaced add unit to damage group. put it inside the picking action
- the damage group leaked. because you always create a new group at the last index without destroying it at the end

 

• Custom script: set bj_wantDestroyGroup = true

has to be right above the group which should be destroyed, because it applies for any next group enumaration not only in your trigger.

Code (vJASS):

function ForGroupBJ takes group whichGroup, code callback returns nothing
    // If the user wants the group destroyed, remember that fact and clear
    // the flag, in case it is used again in the callback.
    local boolean wantDestroy = bj_wantDestroyGroup
    set bj_wantDestroyGroup = false//<-- !!!!!!!!!!!!!

    call ForGroup(whichGroup, callback)

    // If the user wants the group destroyed, do so now.
    if (wantDestroy) then
        call DestroyGroup(whichGroup)
    endif
endfunction

You don't need this group, because the meteor doesn't deal damage over time. Just delete it and all checks related to it.

• Unit Group - Add tempUnit to CM_DamageGroup[CM_LoopInteger]

You don't have to: Custom script: call RemoveLocation(udg_CM_tempLoc1) after meteor hits. This location doesn't even exist.

 

Delete this:

• Custom script: call UnitAddAbility(udg_CM_TreeDestroyer, 'Aloc')

And add locust to the dummy via OE.

 

I didnt want to change the peasant unit from default or create another custom unit for that thats why i gave the ability later to it

 

What if the "default" peasant unit starts to attack other units for an unknown reason? I think you should create a new unit.

 

He won't, because he's Neutral Passive(was it Passive/Extra?)
It's been a common practice for detecting trees over the years for GUI and JASS triggering.
Also, you should add the Harvest ability w/ custom script (forget what's the id, check OE), in case player change their peasant and delete that skill.

 

-Possibly improve the tooltip, fix the hotkey text.
-Possibly add casting sound.
-The rotating projectile is created at 0 degrees, but the spin angle can be whatever. So the projectile jumps from the place it is created to the spin angle during the first loop. Create the unit at the first spin angle.
-The cast trigger has two leaks, you overwrite the temp loc and you use Target point of ability beinc cast and not the variable. Looping trigger also overwrites.
-Dummies should be set to Can't raise, does not decay and have little to no vision.

Needs Fix

 

Firstly, thanks for reviewing my spell and for the feedback.

I have some questions about these though.

-WHat do you mean by casting sound? Is it a sound effect when casting the spell?

-To be honest, I didnt understand the problem about the degrees

-For the point leaks, BPower reviewed this spell months ago, and said that all these could be stored into only one point instead of three or two. I guess I have done wrong or this needs two point variables if I get it right?

-Vision is determined by acqusition range, isnt it?

 

A sound when the spell is cast, yes.

You create the projectile that spins with offset (a, 0). The angle is always 0 from target point.
The first offset angle for the looping trigger is always the opposite angle. It is always 180.
The angles should be the same.

You need two location variables.

Vision is not determined by acquisition range. They are connected when detecting enemies that come into range. The dummies however do not attack anything so we can ignore acquisition range.
Reduce the vision of the dummies, they give as much vision as normal units currently.