Chaos Meteor v1.3.3

[Moderator]

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.

 

[BPower]

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.

 

[NightStalker]

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

 

[Tank-Commander]

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

 

[NightStalker]

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

 

[IcemanBo]

• '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

 

[NightStalker]

• '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

 

[BPower]

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.

 

[Daffa]

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.

 

[NightStalker]

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

 

[Kazeon]

- 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

 

[BPower]

• 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.

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.

 

[Wrda]

Delete this:

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

And add locust to the dummy via OE.

 

[NightStalker]

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

 

[Wrda]

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

 

[Daffa]

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.

 

[Maker]

-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

 

[NightStalker]

-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?

 

[Maker]

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.

 

[NightStalker]

-Would it be a problem if i dont put a sound for casting? Some say that its not very handy to use "sound-play" command in spells

-I've just got rid of the angle problem

-made two loc variables again

-decreased visions to 50 for both dummies, in day and night.

If these are okay for you I can upload the final version.

 

[Maker]

-Would it be a problem if i dont put a sound for casting? Some say that its not very handy to use "sound-play" command in spells

In my opinion it would be a nice touch if the oribiting or the falling projectile made a sound before exploding. But it is not required.

 

[NightStalker]

Well, I think it would be nice too, so how about instead of using sound triggers, I can create a custom buff with only the sound effect and it wont bug.

I created a custom effect, but cant seem to figure it out it wont play any sound :/

Last edit: I found it now it has got a sound too

 

[Maker]

-Ability being cast == VARIABLE
-You are leaking badly in the looping trigger, the locs are not removed if height > 5

 

[NightStalker]

-Ability being cast == VARIABLE
-You are leaking badly in the looping trigger, the locs are not removed if height > 5

Okay I didn't have any time for editing in this semester but I'm slowly getting more time since the finals are almost over.

Isn't it good to have a variable to be equal to ability id? The spell name is stored in the variable and the program won't have to check the spells again and again if it is the Chaos Meteor. It is already in a variable and it just checks the variable.

If i make it

• CM_Height[CM_LoopInteger] Less than or equal to 0.00

instead of 5 would it be leak free?

 

[Maker]

The idea is to not have to edit CM_Execution trigger at all when the spell is imorted. You have "(Ability being cast) Equal to A000" condition there. When imported into a map, the ability will change to whatever spell has the 'A000' raw code in the map. Change that line to use a variable.

I guess you have to study more about removing leaks since you don't seem to completely understand them. You need to remove each location before you overwrite the variable with new data. You are overwriting temp_loc_1 in the "then" part.

 

[NightStalker]

I've updated this spell after a long time, I'm hoping it won't need further updates awaiting harsh moderator feedback

 

[KILLCIDE]

Hey NightStalker, could you update and or fix the trigger tags in your description? While youre at it, please add How to Install instructions (make sure to include this in the map as well).

 

[NightStalker]

Thanks for the feedback Killcide ^^ i will update them all when im back at pc.

Update: Done and updated all!!

 

[KILLCIDE]

Needs Fixed

• Dummy units should not be hardcoded
• The dummy unit should be not the one to deal damage. The caster would get zero credit if they were to land any killing blows

Suggestions

• I'm not a big fan of having a unique dummy unit for every special effect required for a spell. Look into vexorian's dummy and attach special effects via triggers
• It would be a bit better to have the How to Import instructions in a single trigger comment instead of having it up in the header
• FallingSpeed is a pretty important mechanic in my opinion. I would add a multi-level support for it. I would also add a note on how what the value really represents. I obviously know its over a second because you multiplity it by the periodic timeout, but a new user might be confused with the algorithm
• Periodic timeout should be a configurable variable. If a user were to change one, but not everything else dependent with it, the spell will bug out
• The spin speed should be added as a configurable
• (Units In Range) has a minor reference leak
• I think it would be slightly faster to use the "Unit - Unit Classification Check" for magic immune instead of "Unit - Unit-Type Classification Check"
• The spell being able to destroy trees should be configurable
• CM is a bit too generic as a prefix name. Users might run into variable conflicts if they use the same prefix in their map

Status

Awaiting Update​

 

[NightStalker]

Thanks for the detailed feedback, ill end it once and for all this time but can you explain the first point? "The dummy unit should not be hardcoded" what does this mean?

 

[KILLCIDE]

The dummy-unit type* shouldn't be hardcoded. The user would have to go into the code to change the units created on cast rather than just going to the config trigger.

 

[NightStalker]

Oh right i had done the coding way before, i shouldve checked the lines before updating. Ill update once more tonight

 

[KILLCIDE]

Needs Fixed

• Nothing

Suggestions

• Periodic timer should be configurable
• SpinAngle should be configurable
• Instead of using RemoveLocation on tempLoc2, and then storing the position of the dummy unit in tempLoc1 directly after, just keep using tempLoc2 until you dont need it anymore
• This trigger -> CMSS_Loop <gen> because why not

Status

Approved​