• 🏆 Texturing Contest #33 is OPEN! Contestants must re-texture a SD unit model found in-game (Warcraft 3 Classic), recreating the unit into a peaceful NPC version. 🔗Click here to enter!
  • It's time for the first HD Modeling Contest of 2024. Join the theme discussion for Hive's HD Modeling Contest #6! Click here to post your idea!

Cluster Rockets v1.1.0

  • Description

Tinker deploys his rocket pads and fire all the rockets to any targeted unit or any units near the targeted location. Each rocket will explode if collide with any enemy units. The explosion causes damage to nearby units.
Level 1 - 32 rockets, 60 explosion damage.
Level 2 - 48 rockets, 65 explosion damage.
Level 3 - 64 rockets, 70 explosion damage.

Channeling
  • Changelog

--- v1.0.0 ---
First public release version.

--- v1.0.1 ---
Optimized scripts based on some review.

--- v1.0.2 ---
Optimized scripts and fixed a small leak based on reviews.

--- v1.0.3 ---
Optimized scripts and a little performance fix based on review.
Changed tooltip and hotkey.
New ability icon.

--- v1.0.4 ---
Fixed one fatal problem: spell freezes due to bad indexing.

--- v1.0.5 ---
Fixed another fatal problem: can't cast at the center of map.

--- v1.0.6 ---
Optimized unit target acquiring.

--- v1.0.7 ---
Fixed bug: rockets stop when casting rapidly.

--- v1.1.0 ---
Adapted to game patch 1.30. No more dummy units.
New property settings.
Improved ground targeting.
Added target effect on units.
Changed configuration variables.​
  • Credits

Some of the code are adapted from vJASS libraries and Wurst.
Credits to Nestharus, D.O.G, and Wareditor.

Icon by darkdeathknight.
Target effect model by ChirusHighwind.​
  • Other

You can find importing instruction and some Q&A inside the map.
Thanks also to Tank-Commander, Almia, Meatmuffin, BPower, Kam and ILH for their reviews that helped me discover things need to be fixed.​

Keywords:
Rocket, Explosive, Collision, Cluster.
Previews
Contents

Cluster Rockets (Map)

Reviews
11:09, 31st May 2016 Tank-Commander: Thanks for the great submission, just have a few notes on some issues. See my post
Just went over very quickly (haven't quite finished going through)

- You never flush your hashtable
- Your check unit exists function is flawed: a unit can have x and y co-ordinates of 0 and still exist albeit very unlikely, you can check if the UnitTypeId is null (which will be the case if it doesn't exist)
- This is a very weird hybrid of using a Linked List and a Hashtable, why?
- You only need one initialisation function, they can be combined
 
Level 33
Joined
Apr 24, 2012
Messages
5,113
GetWidgetLife cannot detect whether or not a unit(widget) is alive in some circumstances.
You can just check if the UnitTypeId is 0

Check whether the unit exist in the map should be removed. It is just equal to UnitAlive

FlyAbilityID is not recommended to become a constant. Users might change it and break the spell :V
set rFacing = GetRandomReal(0, 360) * bj_DEGTORAD
->
set rFacing = GetRandomReal(-bj_PI, bj_PI)

As for Tank-Commander's comment
- The reason his hashtable is not flushed is because it is being used for unit collision values. But I think the keys used for channeling check shall be flushed.

- " This is a very weird hybrid of using a Linked List and a Hashtable, why?" Hashtable was used for collision check(Again) and for channeling check. He doesn't use a Unit Indexer so he doesn't want to do a linear search for it :V

Overall, I liked it :D

I should test this tomorrow

opinion : I think you should use the Barrage missile model :V
 
Level 13
Joined
Mar 29, 2012
Messages
530
- You only need one initialisation function, they can be combined
You mean the custom script inside the configuration?
Nah, I think it's not neat :3
It's also made for if the user want to reconfigure the spell for some reason, they can just put it at the end of their new configuration (that's the bonus maybe).

GetWidgetLife cannot detect whether or not a unit(widget) is alive in some circumstances.
You can just check if the UnitTypeId is 0

Check whether the unit exist in the map should be removed. It is just equal to UnitAlive
Done.

FlyAbilityID is not recommended to become a constant. Users might change it and break the spell :V
I was thinking that in worst case, that the default crow ability has been somehow edited. So the user can use this to rescue the spell. *\o/*

set rFacing = GetRandomReal(0, 360) * bj_DEGTORAD
->
set rFacing = GetRandomReal(-bj_PI, bj_PI)
Forgot this one. Done.

But I think the keys used for channeling check shall be flushed.
It's just boolean and integer. Right?
 
Level 13
Joined
Mar 29, 2012
Messages
530
You leak a local group handle in function ClsR_AcquireNewTarget also in function ClsR_TerminateRocket. and in the periodic function.

You eventually leak a local unit when you exit the FoG iteration once a proper
target unit was found.
I did destroy the group below endloop.

But I thought local unit is already nulled when FoG return null, isn't?
 
I've gone over the code a bit more thoroughly now and noted a few things:

- You use a lot of local unit groups and destroy them, since you always utilise them in a way that leaves them empty you do not need to do this and can use one global unit group for all uses (adding one global but removing all the lines creating/destroying local groups)
- Please give your local variables meaningful names using single letters is very unhelpful; while you do explain what they are in most functions (some are not) having to scroll back up every two seconds isn't that great
- You have the number for bj_DEGTORAD (57.2957795) instead of the constant bj_DEGTORAD in your code, seems a bit pointless
- When you recycle with a linked list you don't need to null all the unit variables, it will not leak regardless (as the variable will be reused)
- Having a GUI configuration does not a GUI spell make; this is a pure JASS submission
- udg_ClsR_trig can be a local making
JASS:
    if udg_ClsR_trig != null then
        call DestroyTrigger(udg_ClsR_trig)
    endif
redundant
- this makes the spell uncastable on the line x == 0 of any map (again unlikely but it is a problem)
JASS:
if GetOrderTarget() != null or GetOrderPointX() != 0 then

I'd note not all of these are mandatory (the ones towards the top particularly) but it would be helpful and would be slight improvements in my opinion (notably in terms of readability) The spell is great and I'd like to approve it once these things have been solved
 
Level 20
Joined
Aug 13, 2013
Messages
1,696
And still theres a bug occuring ( cant post screenshot atm ), the current missiles just paused the whole spell instances ( the debug expiration timer didnt also work on missiles ), it is frequently visible by lowering the missile speed and rapidly casting it alternatively on the ally and enemy unit.
 
Level 13
Joined
Mar 29, 2012
Messages
530
And still theres a bug occuring ( cant post screenshot atm ), the current missiles just paused the whole spell instances ( the debug expiration timer didnt also work on missiles ), it is frequently visible by lowering the missile speed and rapidly casting it alternatively on the ally and enemy unit.
Completely missed this. :(
I forgot what I was doing with the code back then but I already had this happened.
Gonna check this soon.
 
Level 20
Joined
Aug 13, 2013
Messages
1,696
Don't worry I know you can fix it ^^.

I'll rate it 5/5

The effects are simple but neat and clean, the motion of the rockets are impressive and smooth. Some functions are not needed but overall the spell is great!
 
KILLCIDE and I had a look at the specific issue the other day; after KILLCIDE confirmed that the event runs but runs with a different order ID we wondered why the targetX check was even necessary. Then we noticed that udg_CLsR_AbilityOrder is never set to anything at all which would rather make the "GetUnitCurrentOrder(GetTriggerUnit()) == udg_ClsR_AbilityOrder" condition rather ineffectual, we both assumed this is supposed to be set to the order ID of the ability, the order ID reported on learn was 1(lots of 0s) while all spell cast IDs are between 851970 & 852,672

I can't in good faith approve an ability with a known fatal flaw, though I will do my best to assist solve the issue since I do want to see this approved
 
Level 13
Joined
Mar 29, 2012
Messages
530
Updated to 1.0.5
we noticed that udg_CLsR_AbilityOrder is never set to anything at all
It is set to "firebolt" order ID inside the GUI configuration trigger.

Anyway, I've removed GetOrderPointX() == 0 line and any other 'if's that checks for x == 0. Seems no problem now.
I've tested casting the spell at the very center of the map using trigger and it works.
 
Would anyone have the 1.0.7 version of this still?
Edit: if anyone's interested in a dummy unit version, I started some work on it except I can't figure out how to get SetUnitLookAt to work with this current systems angles.
I was playing around with the latest original version 1.1.0 and discovered why angles weren't working for me, seems sometimes the missiles don't update where they're looking?
 
Top