BladeMaster's Den v1.3.9

[Moderator]

22:43, 14th May 2016
Tank-Commander: V1.3.9 - I left this post a while back addressing some issues, since they haven't been resolved I'm setting this back to needs fix. If you didn't notice it was there you can subscribe to your thread using the thread tools and you'll know when there's a new post

Old Reviews

14:16, 1st May 2016
Tank-Commander: V1.3.7 - It's nearly there, but there's still some leaks (see: the same post as before) - I'd also suggest compiling a list of merits of this entry over the other Omnislash abilities on the site since once it's cleaned up we'll need to compare and contrast before we can decide to approve or not and it'd make approving this easier

07:05, 30th Apr 2016
Tank-Commander: V1.3.5 - still leaks as with the previous versions, set back to needs fix

23 April 2016
Bribe

This is loaded with memory leaks. I advise seeking help with this resource in the Triggers & Scripts forum. We also have at least 6 billion omnislash spells.

17:07, 21st Apr 2016
Tank-Commander: V1.2.0 - There still some major issues to be addressed, the ability has some notable memory leaks and confusing behaviours: see my post for details

11:52, 19th Apr 2016
Tank-Commander: V1.0.1 - Thanks for the submission - I gave the ability a quick once-over and found a few things that need changing. Please see my post for details

 

[Tank-Commander]

There's quite a few things to address with this ability but I'll stick to the major issues since other problems may be solved by first solving them:

Major:
- The ability is not MUI (Multi-Unit-Instable)
- The structure is inefficient: The same code is repeated for each blademaster created by the ability, by utilising a proper indexing method and loops, all of the duplication could be removed
- If the ability is used in an undead player's base then all the blight within the base will be removed: Don't modify terrain with an ability
- Lacks a configuration

 

[Rheiko]

Also, putting the triggers on the description with [trigger][/trigger] tags would be great.

 

[Ofel]

Spent more time before uploading your spell.

 

[Arad MNK]

Spent more time before uploading your spell.

And don't copy-paste the code in the tooltip, it applies the color tags as well.

 

[Meatmuffin]

Did you just update to bypass the Needs Fix status?.. If not, please put what you have changed in the description.

 

[BPower]

@The_Dark_One:
Could you provide a changelog, so moderators and reviewers know what is new in the current version.

 

[Tank-Commander]

Alright now the ability is starting to resemble something we can use here's some more pointers over what is still left to do:

Major:
- BMD_Unit[0] is not indexed, a dummy is leaked whenever more than one instance is active
- There's no variable use for values commonly used: for example you used "Owner Of BMD Unit[]" 71 times by my count in the cast trigger, you could store this in a variable and just refer to that and it'll run much faster (the same goes for other values referred to in this way)
- Your destructible killing doesn't differentiate between things like trees and things like bridges: We don't want to kill bridges - there are methods on how to avoid this and systems which do it you can find in the spell section.
- AOE and amount of swords are things that should be configurable (amongst other things)

- You reference "Random Unit from BMD_Group[Main_Index]" however at no point is that Group set to anything unless the index happens to be 1 (also if that's meant to be Group 1 then it would pick random units from the whole group rather than those close enough to damage.

What?

• Unit Group - Pick every unit in BMD_Group[1] and do (Actions)
  • Loop - Actions
    • Animation - Play BMD_Unit[Main_Index]'s Animation[Main_Index] animation
    • Special Effect - Create a special effect attached to the chest of BMD_Unit[Main_Index] using Abilities\Spells\Human\Polymorph\PolyMorphDoneGround.mdl
    • Unit - Move BMD_Unit[Main_Index] instantly to BMD_Point[2], facing (Random angle) degrees
    • Unit - Make BMD_Unit[Main_Index] face (Picked unit) over 0.10 seconds
    • Unit - Order BMD_Unit[0] to Undead Banshee - Curse (Picked unit)
    • If (All Conditions are True) then do (Then Actions) else do (Else Actions)
      • If - Conditions
        • (Distance between BMD_Point[3] and BMD_Point[2]) Less than or equal to 200.00
      • Then - Actions
        • Unit - Cause BMD_Unit[Main_Index] to damage (Random unit from BMD_Group[Main_Index]), dealing (Real(BMD_Damage[Main_Index])) damage of attack type Spells and damage type Normal
        • Special Effect - Create a special effect attached to the chest of (Random unit from BMD_Group[1]) using Abilities\Spells\Other\Stampede\StampedeMissileDeath.mdl
      • Else - Actions
    • -------- Removing Leaks --------
    • Custom script: call RemoveLocation(udg_BMD_Point[2])
    • Custom script: call RemoveLocation(udg_BMD_Point[3])

- You repeatedly move the unit and set information about it's animations (etc.) for every unit in the group (though it is always the same location and creates a new random angle every time - also spamming effects
- All those special effects leak as they're never destroyed
- You check if point 3 and point 2 are close together, and if that's true proceed to damage any unit from the group? not ones that are close to point 3?

- What's the point in selecting units randomly if you want to damage all valid targets? (since you pick every unit in the group and run the actions for each one)
- Why do you have multiple sword dummy types when they're basically identical?

Minor:
- The indexing method you're using is very weird to me I'll need to go over it at another point when I have a bit more time on my hands (this may amount to nothing if all is good but would be a major problem if something is wrong)
- Your variable names don't follow a standard convention, while some of them start with BMD_ a lot of the others don't, please fix this

I may add more to this list once I've had a bit more time to go over the code

 

[mondragon]

can you make the damage is the hero attack ?

 

[Meatmuffin]

You forgot to put the triggers inside trigger tags.
http://www.hiveworkshop.com/forums/miscellaneous-tutorials-456/how-easily-post-triggers-163285/

 

[Bribe]

You forgot to put the triggers inside trigger tags.
http://www.hiveworkshop.com/forums/miscellaneous-tutorials-456/how-easily-post-triggers-163285/

Seconded

 

[Tank-Commander]

While this has undergone a lot of improvements - you're still leaking a lot of special effects

• Special Effect - Create a special effect attached to the chest of (Picked unit) using BMD_AttachmentChest

these effects are created a lot of times and are never get destroyed, these and all other effects you make like this leak and the spell can never be approved until these are all resolved, and that's before considering this one's merits vs the other omnislash type abilities we have

Edit: V1.3.7 - You've cleared up most of them a few persist though

• For each (Integer Dummy_Index[BMD_Main_Index]) from 1 to BMD_Clones[BMD_Main_Index], do (Actions)
  • (...)
  • Special Effect - Create a special effect attached to the weapon of BMD_Blademaster_Dummy using BMD_AttachmentWeapon
  • Set BMD_SpecialEffectDummy = (Last created special effect)

BMD_SpeciaEffectDummy isn't indexed and as such it leaks

you also have

• Special Effect - Create a special effect attached to the chest of BMD_CastingUnit[BMD_Main_Index] using BMD_AttachmentChest

in the loop that never gets destroyed

 

[Tank-Commander]

The spell should be in an approvable condition after these two key things are fixed:

Problem 1

the tree detection doesn't fit requirements (as it can only detect summer tree walls, there's a method we use to detect any kind of tree (including non default ones)

1) Create an invisible, unkillable dummy unit (use the same one as you do for the spell) and the config trigger (as you only need one and you never get rid of it)
2) Give it harvest (ghoul) and store it in a variable (say BMD_TreeDetector or something to that effect)
3) When you want to detect a tree order the unit to harvest the picked destructible
4) Check the current order of the Tree Detector, if it is still harvest then the destructible is a tree

it looks something like this

• Destructible - Pick every destructible within BMD_Aoe[BMD_Main_Index] of BMD_Point[3] and do (Actions)
  • Loop - Actions
    • Set BMD_Tree = (Picked destructible)
    • Unit - Order (BMD_TreeChecker) to (Harvest) (BMD_Tree)
    • If (All Conditions are True) then do (Then Actions) else do (Else Actions)
      • If - Conditions
        • Current Order of (BMD_TreeChecker) equal to Harvest
      • Then - Actions
        • Destructible - Kill BMD_Tree

Problem 2

Your spell isn't truly MUI - that is to say, your spell instances can interfere with each other. When you remove Group 2 units in the spell it's possible that some of those units belong to a different cast of the spell, you need to make sure that this does not happen

You could resolve this a number of ways - indexing all the dummy units, using indexed unit groups, expiration timers (though that would require knowing how long the spell takes in advance), basically any method you can think of which achieves this is fine, but it is necessary

Also I notice you're still leaking with this line:

• Unit - Move (Picked unit) instantly to (Position of (Random unit from BMD_Group[1])), facing (Random angle) degrees

I strongly recommend looking carefully over your own work for leaks before submitting - Mods can't/don't notice everything the first run through the code (particularly when there's a lot of leaks) and it leads to a lot of reviews stating that it leaks still

 

[KILLCIDE]

Some other minor things to add onto Tank's review:

• With Tank's tree detection example, make sure to order the harvester to stop at the end of the loop
• You have two abilities currently hardcoded into your spell. Users will need to manually change all of them if they import this into your map. You can just use 2 ability variables that can be changed from the config trigger
• It's kind of pointless to cache variables like Damage[], Slashes[], Clones[], and AoE when you can just reference them directly like: Damage[Ability_Level[Index]]
• Store the owner of the caster into an indexed variable on cast
• I'd make the attack and damage type configurable

 

[Cokemonkey11]

It's kind of pointless to cache variables like Damage[], Slashes[], Clones[], and AoE when you can just reference them directly like: Damage[Ability_Level[Index]]

I don't think pointless is a fair description. If the extra variable improves readability or maintainability, it is an improvement. Equally, if it makes those properties worse, it is detrimental.

My main concern is that this suggestion might be being made because it's considered a performance penalty. This is dangerous. Don't think about this!

If the ability has some performance hit and you want to optimize it, never start with micro-optimisations. In general, 90% of the execution time is being spent on 10% of the code. In the olden days the term "hot spot" was thrown around a lot - https://en.wikipedia.org/wiki/Hot_spot_(computer_programming)

With I/O bound programs, I/O is usually the expensive part. JASS is not the case. But the 90/10 rule still applies to JASS.

The rest of the suggestions look good.

 

[KILLCIDE]

My main concern is that this suggestion might be being made because it's considered a performance penalty.

I was just stating that I personally thought it was pointless. I never like having to come up with two variable names that represent the configurable + the one used in the code, so I just stick with referencing it directly.