Meat Hook System [GUI] v4.2

[Moderator]

24th Mar 2016

Your resource has been reviewed by BPower.
In case of any questions or for reconfirming the moderator's rating,
please make use of the Quick Reply function of this thread.

Review:

Ever since DotA was released Meathook is one of the most popular custom spells in Warcraft III.
As a consequence of its popularity, the spell has been coded multiple times on different modding homepages.
I approve it based on the time you invested to re-create the concept. Good job!
I tend to give you a rating of 3/5.

• For concept(5) and uniqueness(1) ~3.
• For API ~4
• For coding ~3
• For performance ~2
  Reason: Storing the all dummies per timeout into an array via ForGoup trigger execution requires a lot of extra computation.

Troubleshooting:

• • Custom script: if IsPlayerAlly(GetOwningPlayer(udg_MeatHT[udg_MeatHRef]), GetOwningPlayer(udg_MeatHC[udg_MeatHRef])) == false then
  ^Rather use native IsPlayerEnemy.
• Pausing dummies units improves performance.
• Using ClearGroup is not required in your code.
• There is a minimal chance for a group handle leak, but even if it occurs it will only happen once per game season.

 

[Maker]

I spotted a memory leak and unoptimized custom scripts. Unit group emtpy bj -> FirstOfGroup == null for example. Better to optimize the code a bit.

 

[ArgentinianUser]

I dont know to much about spells, but in my map it works fine, thanks!

 

[IcemanBo]

Don' t hardcode. Create a config trigger where user can config the spell to his needs.
Add descriptive comments for the user. This is especially needed in config part.

I don't understand why you sometimes use custom scripts, if the GUI would do it, too. It's anyway a GUI spell.
But anyway, if you use custom script then use the most efficient method. For example avoid BJs or also what Maker mentioned above.

Don't name something like only "CValue" or "Temp_Real". It's okay for your test map,
but consider the user also uses his own variables which might come in conflict with yours.
So just make a standard and make for all your names for example "MHG_variableName".

 

[Daffa]

CountUnitsInGroup
Don't use it, use a counter instead and increment it when unit casts and decrement it when an unit's effect is over

 

[IcemanBo]

Additionaly to my previous post,

- make as much configurable as possible. For example damaga/attack- type.
- "Cur" or "Next" are not really descrptive names for variables of type "point"/"location".
- These one liners in GUI for filters are very unreadable. Using "Multiple - Conditions" block is very recommended.

 

[SayazEmil]

It doesn't work on my map.

 

[IcemanBo]

Well you can explain the submitter what's exactly your problem,
maybe it will help him to fix the bug, if there is one.
This resource is not approved yet.

 

[NightSkyAurora]

Greetings hivers, I fixed the SetUnitX\Y problem. I will now try to eliminate the creation, accession and destruction of locations by storing and doing all calculations in X[] Y[] real variables and only send the results to SetUnitX\Y. I believe that this can boost performance to match the JASS version . This will now also fix the irritating behaviour in the map boundaries because SetUnitX\Y ignores them.

 

[Bribe]

http://www.hiveworkshop.com/forums/...?prev=search=meat%20hook&d=list&r=20&status=a

http://www.hiveworkshop.com/forums/...?prev=search=meat%20hook&d=list&r=20&status=a

http://www.hiveworkshop.com/forums/...?prev=search=meat%20hook&d=list&r=20&status=a

http://www.hiveworkshop.com/forums/...?prev=search=meat%20hook&d=list&r=20&status=a

Those are just some of the approved hooks. As a project to get this resource approved, please take some time to compare yours against the ones which have already been made so as to argue whether or not this one should get approved, too. Events are not enough.

 

[BPower]

I agree with Bribe. Did you compare your code to already existing Meat Hook spells?

short review:

1. Don't do this:

• Custom script: call DestroyGroup(GetLastCreatedGroup())

Instead store the group into a global group variable and use that one.
There can be very special cases, where GetLastCreatedGroup() does not store the
group you wish to destroy.

Anyway, which group handle are you destroying here?

2. Instead of two GameEvent variables ( reals ) you could use just one.
HookEvent == 1, HookEvent == 2, HookEvent = 2.5, ....

3. udg_MeatHCLoc leaks location handles periodically per hook head.

 

[NightSkyAurora]

Thanks BPower, I'll change them right away.

@Bribe:
I know that there are many hook spells that are quite good. Some are simple like in dota and other more advanced like in Pudge Wars. My favourite Meat hook is the one by BPower (also happens to be one of the mods in this spell) but the only thing that offends me is the amount of resources and experience you need to be able to use his spell. I aim to create an efficient GUI and simple version of his spell that the general user can understand. I am also open for ideas to add more features to this spell... anything to get it approved

 

[BPower]

Current review:

boundaryX/Y should be replaced by minX, minY, maxX, maxY.
To get the correct boundary values GUI overs a function Min X Of Region, etc.

In Trigger - initialization you are leaking a location handle.

You do not reset you meat hook event variable real to 0 after the trigger fired.
That's why the trigger can't fire again.

In the grabble part exists a game message. Why so?

 

[ArmeniN]

Hello. how do i set this spell affect only heroes, since i want to make AI hook me trough non-hero units.

 

[NightSkyAurora]

New update! I couldn't find any leaks though... I compared my spell to other meat hooks and find this one quite unique and fun. I know BPower's hook is really similar but this one is one of a kind in GUI. If there is ANY kind of changes or additions you'll like to see then PLEASE let me know!!!

 

[BPower]

You leak 4 rect handle when using (Entire map). That function returns a rect with map size.

There is no configuration trigger added, where users can make level based settings.

 

[NightSkyAurora]

There is no configuration trigger added, where users can make level based settings.

What do you mean? These variables takes spell level into account...

• Set Config_MeatHlvlDistnace = 250.00
• Set Config_MeatHlvlDamage = 50.00
• Set Config_MeatHlvlAoE = 0.00
• ect...

However I've found that I sat spell level to meathook level in the Launch_MH function so when one would use a different spell ID, meathook level would have been 0. I fixed that in the new update ^^

 

[BPower]

You move the chain members then you check for map boundaries.
That's not logical.

There is real onHook somewhere in the code. I guess it's from an older version.

The event real is never reset to 0.00 Therefore two equal events can't fire in a row.
This means that the event can't be fired for two different launching hooks.

 

[Skyflash]

I`v Tested your map Well Done but i found some leaks by Using Handle Counter
after 3 hooks the Handle Counter Showed me Approximately 1000 Leaks and thats not good
Use this Method to find them

• Leak Finder
  • Events
    • Time - Every 0.09 seconds of game time
  • Conditions
  • Actions
    • Set Debug = (Point(0.00, 0.00))
    • Custom script: call BJDebugMsg(I2S(GetHandleId(udg_Debug)-0x100000))
    • Custom script: call RemoveLocation(udg_Debug)

 

[NightSkyAurora]

Thank you for the leak finding trigger. I played around with it and founded the main cause:

leaks

• Set Temp_Group = (Units within MeatHAoE[MeatHRef] of MeatHCurPoint matching ((Matching unit) Not equal to MeatHC[MeatHRef]))
• Custom script: call DestroyGroup(udg_Temp_Group)

AND

• Destructible - Pick every destructible within MeatHAoE[MeatHRef] of MeatHCurPoint and do (Actions)
  • Loop - Actions
    • Unit - Order harvester to Harvest (Picked destructible)
    • If (All Conditions are True) then do (Then Actions) else do (Else Actions)
      • If - Conditions
        • (Current order of harvester) Equal to (Order(harvest))
      • Then - Actions
        • Unit - Order harvester to Stop
        • Destructible - Kill (Picked destructible)
      • Else - Actions

By disabling them, leaks are greatly reduced. However I have no idea how to fix it...

 

[Skyflash]

Thank you for the leak finding trigger. I played around with it and founded the main cause:

leaks

• Set Temp_Group = (Units within MeatHAoE[MeatHRef] of MeatHCurPoint matching ((Matching unit) Not equal to MeatHC[MeatHRef]))
• Custom script: call DestroyGroup(udg_Temp_Group)

AND

• Destructible - Pick every destructible within MeatHAoE[MeatHRef] of MeatHCurPoint and do (Actions)
  • Loop - Actions
    • Unit - Order harvester to Harvest (Picked destructible)
    • If (All Conditions are True) then do (Then Actions) else do (Else Actions)
      • If - Conditions
        • (Current order of harvester) Equal to (Order(harvest))
      • Then - Actions
        • Unit - Order harvester to Stop
        • Destructible - Kill (Picked destructible)
      • Else - Actions

By disabling them, leaks are greatly reduced. However I have no idea how to fix it...

I'm glad that helped you for the second one I have one solution for you the (destructible-pick range) action has leak inside BJ file use this method instead of (range) I used this method to trim my system create a region with the custom script below:

• Custom script: set TempRec = GetRectFromCircleBJ(point,radius)

Then pick destructibles in region and don't forget to remove the region via

• Custom script: call RemoveRect(TempRect)

For the first leak use this method inspired from my system

• Custom script: call GroupEnumUnitsInRange(group, GetLocationX(point), GetLocationY(point), radius, null)

Sry if its not clear enough I don't have worldedit right now

 

[NightSkyAurora]

Thank you Skyflash, the destructables doesn't leak anymore ^^

About the unit group... Should I replace

• Set Temp_Group = (Units within MeatHAoE[MeatHRef] of MeatHCurPoint matching ((Matching unit) Not equal to MeatHC[MeatHRef]))

with

• Custom script: call GroupEnumUnitsInRange(udg_Temp_Group, GetLocationX(udg_MeatHCurPoint), GetLocationY(udg_MeatHCurPoint), udg_MeatHAoE[udg_MeatHRef], null)

?

This doesn't work because the group is always emty.

 

[Skyflash]

Thank you Skyflash, the destructables doesn't leak anymore ^^

About the unit group... Should I replace

• Set Temp_Group = (Units within MeatHAoE[MeatHRef] of MeatHCurPoint matching ((Matching unit) Not equal to MeatHC[MeatHRef]))

with

• Custom script: call GroupEnumUnitsInRange(udg_Temp_Group, GetLocationX(udg_MeatHCurPoint), GetLocationY(udg_MeatHCurPoint), udg_MeatHAoE[udg_MeatHRef], null)

?

This doesn't work because the group is always emty.

Oh my bad I forgot to mention that you must create group first , use this custom script:

• Custom script: set udg_group = CreateGroup()

 

[Bribe]

call GroupEnumUnitsInRangeOfLoc(udg_Temp_Group, udg_MeatHCurPoint, udg_MeatHAoE[udg_MeatHRef], null)

GUI groups are initialized automatically and you do not use custom script to create them. Array groups are the exception.

 

[NightSkyAurora]

For some reason, creating udg_Temp_Group solved the problem. I don't know why lol. Also I figured that if you create the group using Skyflash's method then I must use FirstOfGroup. The GUI version to pick units in group doesn't work. Thank you for your help guys!
updated

 

[Bribe]

Then you are destroying that group on another trigger in your map. It should instead have a unique name to prevent that kind of incident. And the GUI pick-all-units works fine. Debug the real problems before compromising on unnecessary workarounds.

 

[NightSkyAurora]

Thanks for the advice Bribe.
updated

 

[BPower]

The meat hook ability should be stored into an "Ability" variable.
While importing the spell triggers the raw code of the meat hook ability will most likely change.
User should only have to change the value of the variable in the configuration trigger
instead of going into the spell source code.

In GUI it's convenient to provide an array setup via configuration trigger for example Hook_Config_Damage[level] = value
That ensures that the user doesn't have to touch the on cast event trigger at all.

I saw that you make use of ExecuteFunc in your code. I assume you found it in GUI UnitIndexer code and wanted to try it out.
ExecuteFunc is a very powerful native, but comes along with a few downsides for instance it is very slow.
There should be a specific reason, why the usage of ExecuteFunc is required.
That would be for example to avoid running into an OP limit case, wait or like in UnitIndexer to squeeze the code into one trigger.
Otherwise ExecuteFunc should be avoided in public resources.
In your case ExecuteFunc is not required and should be removed.
If I would be you, I would move the code which executes after ExecuteFunc in the on cast trigger.

• -------- > custom properties here < --------

^I don't have to mention that a normal user will not find that line.
As I said, I would strongly recommend an array based configuration via config trigger.

• Custom script: if IsPlayerAlly(GetOwningPlayer(udg_MeatHT[udg_MeatHRef]), GetOwningPlayer(udg_MeatHC[udg_MeatHRef])) == false then

^You don't need a "== false comparison", better go with

• Custom script: if IsPlayerEnemy(GetOwningPlayer(udg_MeatHT[udg_MeatHRef]), GetOwningPlayer(udg_MeatHC[udg_MeatHRef])) then

Sometimes you use custom script for conditions ( see above ), sometimes if then else provided by the GUI.
That's not consistent and makes your code less read-able.

The option to set grapple == true is quite cool. Good job considering that option.

• Custom script: call DestroyGroup(udg_Temp_Group)

^I don't see the purpose of that line.

After deallocating a meat hook index, you still run a widget ( unit, destructable ) detection.
The hook itself however doesn't exist no more.

• Custom script: call RemoveLocation(udg_MeatHCLoc)

^You have this line after evaluating if the spell has ended. It's not required, as you do the same again a few lines below.

 

[WereElf]

• Custom script: if IsPlayerAlly(GetOwningPlayer(udg_MeatHT[udg_MeatHRef]), GetOwningPlayer(udg_MeatHC[udg_MeatHRef])) == false then

^You don't need a "== false comparison", better go with

• Custom script: if IsPlayerAlly(GetOwningPlayer(udg_MeatHT[udg_MeatHRef]), GetOwningPlayer(udg_MeatHC[udg_MeatHRef])) then

To avoid confusion, this should be

• Custom script: if not IsPlayerAlly(GetOwningPlayer(udg_MeatHT[udg_MeatHRef]), GetOwningPlayer(udg_MeatHC[udg_MeatHRef])) then

or

• Custom script: if IsPlayerEnemy(GetOwningPlayer(udg_MeatHT[udg_MeatHRef]), GetOwningPlayer(udg_MeatHC[udg_MeatHRef])) then

 

[NightSkyAurora]

The meat hook ability should be stored into an "Ability" variable.
While importing the spell triggers the raw code of the meat hook ability will most likely change.
User should only have to change the value of the variable in the configuration trigger
instead of going into the spell source code.

I'm not sure how to implement that. What if the end user has a meat hook and grapple ability? Shouldn't there be an ability variable for every spell that use this system?

In GUI it's convenient to provide an array setup via configuration trigger for example Hook_Config_Damage[level] = value
That ensures that the user doesn't have to touch the on cast event trigger at all.

The user can change the level factors in the cast trigger by changing the Config_MeatHlvl.... variables. The cast trigger is really a template or example on setting up a hook spell and it is encouraged for the user to fine tune the spell in that trigger. I will add more "cast" triggers as examples in the next update. Also my method supports infinite (2147483648) levels but in the array method you have to define each level explicitly.

I saw that you make use of ExecuteFunc in your code. I assume you found it in GUI UnitIndexer code and wanted to try it out.
ExecuteFunc is a very powerful native, but comes along with a few downsides for instance it is very slow.
There should be a specific reason, why the usage of ExecuteFunc is required.
That would be for example to avoid running into an OP limit case, wait or like in UnitIndexer to squeeze the code into one trigger.
Otherwise ExecuteFunc should be avoided in public resources.
In your case ExecuteFunc is not required and should be removed.
If I would be you, I would move the code which executes after ExecuteFunc in the on cast trigger.

Actually I saw that in the persistant beam system and I thought it would be cool to have the setup and looping code in one trigger. If that is discouraged then I'll split the code

• -------- > custom properties here < --------

^I don't have to mention that a normal user will not find that line.
As I said, I would strongly recommend an array based configuration via config trigger.

I clearly explained where to find that in the instructions. I see the misunderstanding with the configuration trigger now. "Meathook cast" is actually the configuration trigger. I will work on the documentation to make that more clear.

• Custom script: if IsPlayerAlly(GetOwningPlayer(udg_MeatHT[udg_MeatHRef]), GetOwningPlayer(udg_MeatHC[udg_MeatHRef])) == false then

^You don't need a "== false comparison", better go with

• Custom script: if IsPlayerAlly(GetOwningPlayer(udg_MeatHT[udg_MeatHRef]), GetOwningPlayer(udg_MeatHC[udg_MeatHRef])) then

I didn't know you can do that. Thanks for pointing that out however I'll need to change that to GUI and and I don't think that shortcut will work there...

Custom script: call DestroyGroup(udg_Temp_Group)
^I don't see the purpose of that line.

After fixing the leak that variable is no longer needed. I'll remove that.

After deallocating a meat hook index, you still run a widget ( unit, destructable ) detection.
The hook itself however doesn't exist no more.

I'll look into it, thanks.

• Custom script: call RemoveLocation(udg_MeatHCLoc)

^You have this line after evaluating if the spell has ended. It's not required, as you do the same again a few lines below.

As far as I understand, when the spell ends, the trigger is turned off. That means that the function a few lines below won't be called, causing a memory leak. Please correct me if I am wrong.

Thank you BPower for the review.

@WareElf: I understood BPower but thanks for helping ^^

P.S There will be more features in the next update. I think it would be better to call this a chain system or something because many of the upcomming features don't even require the target to be hooked...

 

[BPower]

I couldn't produce bugs ingame. The coding is with some exceptions here and there ok.

 

[Skyflash]

I`m Going to use it if i could import it
nice system anyway
the handle leaks won`t go further than 900 this means no leaks although i found out the Leakfinder is not working since patch 1.21

 

[NightSkyAurora]

Founded and fixed 2 bugs plus some extra improvements. Let me know if anyone has any ideas regarding this system.

 

[Quentuche]

I can't set the level damage. I mean, even when I'm testing your map, if I change the variable "Config_MeatHlvlDamage" like to 200 instead of 50, it doesn't apply my changes.

Am I doing it wrong ? Or is this variable bugged ? I don't know

 

[Quentuche]

Hi, it's me again

I think I have found the solution.

When I was casting Meat Hook, damage per level were not actualised. So I checked your code, and think you forgot to set the variable "MeatHlvl_Damage" in the spell Meathook" and then "MeatHlvl_Damageloop" too.

Perhaps I'm wrong, I'm not that good, but now it works for me.

What do you think ?

 

[sarumanthecursed]

how would one make it so that the hook only hooks the intended target and not enemies along the way.

 

[NightSkyAurora]

Quentuche I accidentally set the variable twice in the Meathook ini trigger where the second one was incorrect. It works after removing it.

Thanks for the idea sarumanthecursed, I added that feature

 

[Virus-X]

WTF error is that.

 

[gusanomental]

WTF error is that.

I'm having the same issue. Wonder what is wrong here.

 

[NightSkyAurora]

Are you compiling with jassnewgenpack5d?

 

[gusanomental]

Thanks, got it to work now.

 

[Venkolm]

Very Well Done! 5/5

 

[RanCatu1020]

Hi, how can I make the grapple pull the caster once it reached its max distance instead of the hook returning to the triggering unit? I've been trying to figure it out myself but I can't come up with anything. Pls help. Thanks. T-T

 

[V]

EPIC and very readable!

 

[FirstLegacy]

i'm having error with the system..
can anyone help me with this?..

 

[Łapin Viktus]

Where is "distance function" ? i can't find it
I have the same probleme like FirstLegacy.

 

[Pyrogasm]

Where is "distance function" ? i can't find it
I have the same probleme like FirstLegacy.

In the map's Custom Script section you will find these functions:

function distance takes real x1, real x2, real y1, real y2 returns real
    return SquareRoot((x1 - x2) * (x1 - x2) + (y1 - y2) * (y1 - y2))
endfunction

//determine wheather a point is inside the configured map bounds
function IsOutsideMap takes real x, real y returns boolean
    return x < udg_minX or y < udg_minY or x > udg_maxX or y > udg_maxY
endfunction

//If MeatHLockOn != null and MeatHIgnore == true, this function is called to filter the locked unit
function FilterLockedUnit takes nothing returns boolean
    return ( GetFilterUnit() == udg_MeatHLockOn[udg_MeatHRef] )
endfunction

You can either put them in the same place in your map or you can put it all inside a library in any normal JASS trigger:

Hidden

library MeatHookUtils
function distance takes real x1, real x2, real y1, real y2 returns real
    return SquareRoot((x1 - x2) * (x1 - x2) + (y1 - y2) * (y1 - y2))
endfunction

//determine wheather a point is inside the configured map bounds
function IsOutsideMap takes real x, real y returns boolean
    return x < udg_minX or y < udg_minY or x > udg_maxX or y > udg_maxY
endfunction

//If MeatHLockOn != null and MeatHIgnore == true, this function is called to filter the locked unit
function FilterLockedUnit takes nothing returns boolean
    return ( GetFilterUnit() == udg_MeatHLockOn[udg_MeatHRef] )
endfunction
endlibrary

 

[Łapin Viktus]

I have imported the functions into the map's Custom Script section. However I still have an error:

Line 3043: Invalid argument type - invalid argument type 'unknown type' in call on line 3043(unknown type)
call GroupEnumUnitsInRange(udg_Temp_Targets, udg_MeatHChainX[udg_MeatHRef], udg_MeatHChainY[udg_MeatHRef], udg_MeatHAoE[udg_MeatHRef], function FilterLockedUnit)


This line, but the function FilterLockedUnit is on the map's Custom Script, so why that don't work ?

The bug also appears on the test map, when we add a hero to the map.

 

[Pyrogasm]

It should be Filter(function FilterLockedUnit) or Condition(function FilterLockedUnit) in that line.