Advanced KnockBack 3D System V.3.2b

[Moderator]

BPower: Once again I've wrote a review for you system ( link ), so you can adjust your code in your own map.
I don't get the feeling that this develops towards approval, not even close.
Furthermore we have many high quality resources in our database,
which cover 2D and 3D unit motion.

old

BPower: I wrote a few questions into the comments.
Removed the rating I gave in the last review.
Otherwise refer to my old review.

13:47, 19th Feb 2016
BPower: Feature bloated code abomination.
I rather recommend using one of the approved well structured
knockback systems from our database.

13:51, 3th Feb 2016
BPower: Set back to Need Fix until [self=http://www.hiveworkshop.com/forums/spells-569/gui-knockback-3d-jump-system-beta-v1-0-a-269237/#post2783284] you wrote a changelog[/self]


10:00, 6th Oct 2015
Bribe:

Pros
+ Has a lot of features

Cons
- Pauses the unit being knocked back and there is no configurable to change this
- Pathing check is done with an item, so any unit with collision size greater than 16 can get stuck
- Paladon's deprecated Knockback had a better implementation of. knockback
- Uses unit groups and hashtables, making it extremely slow (knockbacks need to be as fast as possible and this is among the slowest)

Recommendation
? Build your stuff on top of one of the many approved knockback systems.

20.01.2016, BPower:
I say it directly. I don't see a chance for approval for this system. Only with a radical change in code.
Reasons:

• Knockback / Missile systems must be fast, hence FroGroup + hashtable is a very bad choice.
  ---
• Unit groups lose units which are removed, therefore you have high potential for leaks.
  ---
• I found handles leaks in your code ( group and location )
  ---
• Couldn't find where you flush a childkey in the hashtable.
  ---
• The API is terrible. A user shouldn't save into a hashtable. Instead he / she assigns globals and the system transfers them to the table internally.
  ---

 

[Chaosy]

Post triggers. If you don't know how, there is a tutorial for it.

 

[BPower]

There is a Jump system. Afaik there are really a lot of different knockback/jump systems
uploaded here in the spell section. We will have to list those and compare advantages/disatvantages towards yours during
the moderation process.

Thanks for submitting.

 

[Daffa]

There are lots of Jump and KB3D Systems, in all forms (GUI, Jass, and vJass).
Still, a little addition won't hurt. Show an example trigger that represents how to use the system would be great.

 

[Skyflash]

thanks for reply

Actually i was very eager to find the same system in GUI I`ve downloaded a lot of jump systems here but unfortunately most of them were in jass and i didn`t like the GUI method that they used to execute a jump.here is my system by using my system you can make 2D and 3D knockbacks (launching knockback later) in one system! i think this is a new idea and maybe useful and i will add new features to this system like launching movement attach effects to jumper maybe some circular movement ...
anyway there are 3 type of spells in the map which is showing the system`s function
here is the simple leap spell :

Triggers

• Jump2
  • Events
    • Unit - A unit Starts the effect of an ability
  • Conditions
    • (Ability being cast) Equal to Jump2
  • Actions
    • -------- Adding Caster to Group for Calling it later --------
    • Unit Group - Add (Triggering unit) to KnockBack_Group
    • -------- Define Points and save in system`s Table --------
    • Set Jump_p1 = (Position of (Triggering unit))
    • Set Jump_p2 = (Jump_p1 offset by 1200.00 towards (Facing of (Triggering unit)) degrees)
    • Hashtable - Save Handle OfJump_p1 as (Key kb3d_P_A) of (Key (Triggering unit)) in KnockBack_Hashtable
    • Hashtable - Save Handle OfJump_p2 as (Key kb3d_P_B) of (Key (Triggering unit)) in KnockBack_Hashtable
    • -------- Set Velocity of Moving and Save it in Hashtable --------
    • Hashtable - Save 70.00 as (Key kb3d_V) of (Key (Triggering unit)) in KnockBack_Hashtable
    • -------- Set Velocity of Flying and Save it in Hashtable --------
    • Hashtable - Save 40.00 as (Key kb3d_Vz) of (Key (Triggering unit)) in KnockBack_Hashtable
    • -------- Execute the jumping Function --------
    • Trigger - Run Knockback 3D <gen> (checking conditions)

 

[btdonald]

http://www.hiveworkshop.com/forums/spells-569/jump-system-v1-0-0-a-210994/

 

[BPower]

Knockback3D/Jump systems after a quick search:

• Knock-Back3D - MMS 3D
• Jump System

There are multiple knockback2D systems in database aswell.
Furthermore we have Knockback3D by Cokemonkey in the JASS section.

I will compare yours to them, then I can tell more. Maybe yours is better

 

[Tank-Commander]

I tested the latest version of this and went through the code (skim reading) and some things were very notable

- The movement is very choppy (particularly the jumping, feeling generally mechanical and unclean)
- Using unit - move is not recommended over the use of set unit x/y as it can support many more units without generating lag
- There's no parabola when a unit is in-flight, you can correct this either using a fixed height arc (typically done via application of the formula Current Height = ((4 * MaxHeight) / Max Distance) * (Max Distance - Current Distance) * (Current Distance / Max Distance) if I'm remembering it right (been a long time since I used it) or it can be done via velocity calculations, notably z velocity which is subtracted by a force of gravity each step (consult "SUVAT" equations on how to do this also called the equations of motion)
- There's a lot of bloat to the system - unless referenced more than once, when you extract all the data from the hashtable into variables you are wasting processing, it makes the code easier to write but doesn't make it any better (rather the opposite)
- The variable names aren't particularly clear (effect 1 - 4 doesn't tell us anything about what they're for aside from being effects) without any real reason to do this since the code is not made any more efficient by doing so thankfully the comments do help with this somewhat
- You only need to check if the knockback group is empty when you remove a unit (with the trigger initially off) there's no point in checking every loop
- Crow form can be immediately removed from a unit after is has been applied and it will still allow you to manipulate the fly height
- You reference (Picked unit) many times without setting it to a variable in order to reduce processing
- There is no point having "do nothing" in your code (they do nothing), you can delete all of them and make the code shorter
- when you deallocate a unit from your system you should flush the child in order to not waste memory space
- you leak points while the unit is in transit (only cleaning the ones created/used in an instance when a unit has landed)
- There are unit group leaks in the system (when you pick units to do line damage with being the first I came across though there are more)

The big issue is that these are all the problems you have before beginning to address the issue raised by others - there are loads of these systems and they do it better, your system's strength is having many features but these would be better suited to be built on top of the existing better ones as add-on features. As such it may be better to think of this system as more in an experience in improving your own coding skills rather than as a submission you expect to get approved (as to get it approved it'd need to have merits to approve it alongside the others or to outright supersede them). I'd suggest fixing up the system and taking away the personal improvement which you can then be applied to future projects and submissions but it may never be approved due to the nature of the system you've produced

you seem to have good logical abilities so we may see some great things from you in future
I wish you luck in your coding endeavors

 

[BPower]

Can you enlighten us what changed in your latest update?
It's a huge system and I wish not to spend hours to evaluate every aspect
again, without knowing what you actually fixed in the latest update.

There is a lot of criticism about your code in the comments.
A quick answer like "Yes you are right. Fixed" or "No you're wrong because ..."
would show that you value those you took their time to rate your system.

Also something like a changelog would be appreciated by me and others.

It's a very special reasoning, but I will put your resource back to Need Fix until
you make a statement about what is new in this version.

 

[BPower]

Criticism:


set udg_kb3d_DummyG=CreateGroup()
^
Here you create a unit group for no obvious purpose

The dummy unit type id should be stored in a global integer variable.

For units using crow form by default, this will remove the ability from their ability list.

call UnitAddAbilityBJ( 'Amrf', udg_kb3d_Unit[udg_kb3d_TempInt] )
call UnitRemoveAbilityBJ( 'Amrf', udg_kb3d_Unit[udg_kb3d_TempInt] )

Paused units don't take order commands, therefore that order fixer block does nothing
for this condition.

The unit time scale is hardcoded to a base value of 140 and 120. Why is that so?
I guess it relates to a map/resource of yours. If so, such a feature is not wanted for public systems
which should be working for all users.

You have effect handle leaks. A lot.
Using 4 variables for periodic effects appears to be a random setup for me.

In a 3D projectile system a pitching angle could be considered for dummy units.

 

[Skyflash]

Criticism:

Thanks to at least looking at the spell and evaluate the code

set udg_kb3d_DummyG=CreateGroup()
^
Here you create a unit group for no obvious purpose

I have an obvious Purpose for sure, you can see what would be happened if you disable this action , the Group will be removed forever after Destroying it(well you know if you don`t it will leaks) and there is no way to add any unit or using the dummy units again this little jass script is fixing this bug like a charm!

The dummy unit type id should be stored in a global integer variable.

sorry i don`t understand whats the advantage of using CreateUnitAtLoc and storing UnitID over using the original GUI method? (since this spell assumed to be GUI why using jass function in unnecessary situations?) but if you tell me the reason of that i will do what you want to see

For units using crow form by default, this will remove the ability from their ability list.

call UnitAddAbilityBJ( 'Amrf', udg_kb3d_Unit[udg_kb3d_TempInt] )
call UnitRemoveAbilityBJ( 'Amrf', udg_kb3d_Unit[udg_kb3d_TempInt] )

yes that`s right i will add a condition to filter those units thanks to inform me
i didn`t in hell considered that scenario

Paused units don't take order commands, therefore that order fixer block does nothing
for this condition.

No, Not at all , the bug fixer is one of the most important parts of my system in my opinion , to find out why that`s so important just disable this condition and test it for example use dash spell to run through enemy units the only thing you would see is that the jumper (that assumed to be paused!) turns back and trying to attack units while he is being knockbacked !
i don`t think a little condition harms at all instead of having a unit attacks and changes his face during a jump that's totally make knockbacks inaccurate
but as always its configurable maybe someone likes to create those kind of knockbacks

The unit time scale is hardcoded to a base value of 140 and 120. Why is that so?
I guess it relates to a map/resource of yours. If so, such a feature is not wanted for public systems
which should be working for all users.

you can`t be serious ,why i ever define kb3d_AnimationSpeed ? first of 120 and 140 values were completely experimental achievements by testing animation speeds with different Initial Velocities then i find out this value is perfectly suits my system and why even i did some thing like that instead of just suffice the AnimationSpeed Variable ?
well the answer is to make it more user-friendly that would save time for user to use animation feature instead of testing more than 10 animation speeds to find the proper animation speed but it`s still configurable.
about 120 value this a custom made animation called sliding animation and it has a -1 index(which is not exist in any unit`s animations) and it`s completely optional if some one don`t like it simply don`t use -1 as animation index pretend that it`s not existing .

You have effect handle leaks. A lot.
Using 4 variables for periodic effects appears to be a random setup for me.

they are not periodic those 4 variables are just for preventing leaks and removing the user`s special effect correctly
because once they created you lose access from them unless you store them into variables so user can create 4 static effect (means they have noting to do with periodic loop) and once they allocated user can make sure the effects will be destroyed after the knock-back
periodic special effect which is called dynamic effect will be removed immediately when ever they created ( except effect duration feature that won`t work due to leaking that`s horrible i will fix it in next update

In a 3D projectile system a pitching angle could be considered for dummy units.

i don`t understand the pitching angle i have defined a Angle offset for creating circular motion(too simple but still has room to develop )




any comment about changes and new features?

 

[Deleted member 247165]

This one actually works! I've just checked it on a testing map,made by myself and it worked with no problems! Good job! ^_^

 

[BPower]

The system is bloated with features which should not be in there.
Also they are bad impelemted for example the sound or order pluggin.

I have no intention to approve this system as it's of much less
quality than others we have in our database.

Obviously you have a flawed cleanup in you code.
The handle id counter explodes when you test the map for some time.

 

[Skyflash]

The system is bloated with features which should not be in there.
Also they are bad impelemted for example the sound or order pluggin.

I have no intention to approve this system as it's of much less
quality than others we have in our database.

Obviously you have a flawed cleanup in you code.
The handle id counter explodes when you test the map for some time.

I will fix them surly the handle id got really low when i disable other resources in map and use only the system without any feature and it`s leakless or very very minor leak like 1 count per jump i guess most of the time its constant maybe it`s the error of the debugging method but i will trim and clean my system from leaks and unwanted features

 

[BPower]

It's not clear to me when effect 1 - effect 4 happen.
Knockback systems normally come along with an on motion effect, eventually
an on collide. Please explain when effect 1, 2, 3, 4 take place.
---

• Set kb3d_GetEffect_Dy_model = <Empty String>

^ This line is exists twice in your main trigger.
---
You're missing a setup trigger, where a user can define i.e. the default dummy unit.
---
What is the idea behind a minimum and maximum collision radius?
I think it's enough to use one collision member.
---

• Set kb3d_GetTrigger = Visibility <gen>

^Change it to = null or kb3d_GetTrigger = kb3d_GetTrigger
---

• kb3d_AnimationIndex[kb3d_TempInt] Greater than 0

^ 0 is also a valid animation index. Use -1 instead for no animation.

• Animation - Change kb3d_Unit[kb3d_TempInt]'s animation speed to (140.00 + (kb3d_Velocity[kb3d_TempInt] x (Cos(kb3d_Angle[kb3d_TempInt]))))% of its original speed

^Why is 140% the default offset instead of 100%?
---

• Custom script: set udg_kb3d_Effect_Dy[udg_kb3d_TempInt] = AddSpecialEffectTarget(udg_kb3d_Effect_Dy_model[udg_kb3d_TempInt],udg_kb3d_Unit[udg_kb3d_TempInt], udg_kb3d_Effect_Dy_attach[udg_kb3d_TempInt])

This can be one line

• Custom script: call DestroyEffect( AddSpecialEffectTarget(udg_kb3d_Effect_Dy_model[udg_kb3d_TempInt],udg_kb3d_Unit[udg_kb3d_TempInt], udg_kb3d_Effect_Dy_attach[udg_kb3d_TempInt]))

 

[Skyflash]

It's not clear to me when effect 1 - effect 4 happen.
Knockback systems normally come along with an on motion effect, eventually
an on collide. Please explain when effect 1, 2, 3, 4 take place.
---

okey here is an example
i want to manage a knockback that the target point marked with a flag
i create an flag with special effects and set it as one of the effects for example effect 1 the system will remove it when the jump completes eventually the created effect must be removed after a knock back there is no way to access it later and remove it because no one can estimate how long the jump would be (at least needs a lot of calculation) the system will remove all of created effects i gave user more room to set up to 4 of these effects

• Set kb3d_GetEffect_Dy_model = <Empty String>

^ This line is exists twice in your main trigger.

i will fix it
---

You're missing a setup trigger, where a user can define i.e. the default dummy unit.

I don`t understand what is the default dummy unit
if you mean the type of creating dummy unit there is a kb3d_GetDummyType variable to set type of dummy unit
all of the default settings done inside of Knockback3d else it`s null the user have to specify what would the Dummy unit`s type be
i fixed it now the default unit type is null user must specify the type of dummy
---

What is the idea behind a minimum and maximum collision radius?
I think it's enough to use one collision member.

the maximum Collision Radius is the picking range and the Minimum Radius Refers to minimum radius that the collision happens in the reality a collision happens when an object reaches the nearest object in it`s way this is my method and its highly configurable as it`s should be.
i think 2 inputs make this feature more Accurate for picking a unit or a group of units
---

• Set kb3d_GetTrigger = Visibility <gen>

^Change it to = null or kb3d_GetTrigger = kb3d_GetTrigger

this custom script was included in clean up section of KnockBack3d trigger tha

• set udg_kb3d_GetTrigger = null

I will fix that okey i found the problem

---

• kb3d_AnimationIndex[kb3d_TempInt] Greater than 0

^ 0 is also a valid animation index. Use -1 instead for no animation.

I didn`t know that okey

• Animation - Change kb3d_Unit[kb3d_TempInt]'s animation speed to (140.00 + (kb3d_Velocity[kb3d_TempInt] x (Cos(kb3d_Angle[kb3d_TempInt]))))% of its original speed

^Why is 140% the default offset instead of 100%?

because it was to slow this is the third time i explain that i`v tested a lot of values with different velocities they were slow or to fast 140 was the best choice anyway i will remove that and the user must consider a proper animation speed and use kb3d_GetAnimationSpeed

---

• Custom script: set udg_kb3d_Effect_Dy[udg_kb3d_TempInt] = AddSpecialEffectTarget(udg_kb3d_Effect_Dy_model[udg_kb3d_TempInt],udg_kb3d_Unit[udg_kb3d_TempInt], udg_kb3d_Effect_Dy_attach[udg_kb3d_TempInt])

This can be one line

• Custom script: call DestroyEffect( AddSpecialEffectTarget(udg_kb3d_Effect_Dy_model[udg_kb3d_TempInt],udg_kb3d_Unit[udg_kb3d_TempInt], udg_kb3d_Effect_Dy_attach[udg_kb3d_TempInt]))

i don`t think its necessary i will change the method of allocating Special effect since i found there is no problem with GUI`s Effect Creation

i will fix these issues as soon as possible
thanks

 

[BPower]

I would like to requote my colleague:

Pros: It has a lot of features
Cons: It has a lot of features​

So let's discuss what you implemented.

"Order Bug Fixer":​

• -------- Order Bug Fixer --------

• (Current order of kb3d_Unit[kb3d_TempInt]) Equal to (Order(smart))

^First of all some nitpicking. There is no "bug" at least I'm not aware of it.
If OrderInterupt is true the unit motion stops, if the current order equals "smart".
Why exactly smart? In my opinion you have to compare the current unit order to the one
which triggered the knockback in the first place. If they are unequal the knockback motion should stop.
On knockback start set UnitOrderSaved[index] = GetUnitCurrentOrder(Source[index])
Then periodically if UnitOrderSaved[index] != GetUnitCurrentOrder(Source[index]) then

• If (All Conditions are True) then do (Then Actions) else do (Else Actions)
  • If - Conditions
    • kb3d_Pause[kb3d_TempInt] Equal to True
  • Then - Actions
    • Unit - Order kb3d_Unit[kb3d_TempInt] to Stop
  • Else - Actions

^I'm not very happy with what you did here.
It will not work for all unit orders, because there are instant orders like windwalk.
In my understanding a paused unit is either really paused PauseUnit(whichUnit, true)
or unable to change the trajectory meaning you base your unit motion on cached x/y/z values, while ignoring the actual unit x/y and fly height value.
For example: set x = x + velocity*Cos(angle) vs set x = GetUnitX(whichUnit) + velocity*Cos(angle)
Can you explain your idea behind constantly ordering "stop"?

"Animation feature":​

• If (All Conditions are True) then do (Then Actions) else do (Else Actions)
  • If - Conditions
    • kb3d_Animation[kb3d_TempInt] Equal to True
  • Then - Actions
    • If (All Conditions are True) then do (Then Actions) else do (Else Actions)
      • If - Conditions
        • kb3d_AnimationIndex[kb3d_TempInt] Greater than -1
      • Then - Actions
        • Custom script: call SetUnitAnimationByIndex( udg_kb3d_Unit[udg_kb3d_TempInt], udg_kb3d_AnimationIndex[udg_kb3d_TempInt])
        • Animation - Change kb3d_Unit[kb3d_TempInt]'s animation speed to kb3d_AnimationSpeed[kb3d_TempInt]% of its original speed
      • Else - Actions
  • Else - Actions

^The upper condition, animation == true is not required. You could only use the lower condition animation index greater than - 1.
It's a bit odd as you update the animation index every 0.03 seconds. Do animations override each other or does the function do nothing
if the previous animation is still running and equals the one you wish to display? I'm curious, because I never tested it myself.

"Effect feature":​

Just a quick hint towards variable naming practise. "kb3d_GetEffect_Dy_model" is the file path, which is used for a periodically special effect.
For example kb3d_GetPeriodicEffectModel, kb3d_GetPeriodicFx, kb3d_PeriodicEffect ...would be much more descriptive, hence easy to understand for users.
From the shortcut _Dy_ I couldn't associate a periodic effect feature.
Also some consistency in lower and upper case, in general naming convention is appreciated.
Compare kb3d_GetEffect_Model vs kb3d_GetEffect_model vs kb3d_GetEffectModel. Which one do you think is the best?

"Damage feature":​

• Custom script: call RemoveLocation(udg_kb3d_TempLoc)

^This line provokes internal Warcraft III mechanisms to compensate a potential bug.
A there is a chance that you remove the location handle multiple times while executing the for group actions and
B you use just that location in further features like the tree killer.

"TreeKiller feature":​

• kb3d_Z[kb3d_TempInt] Less than 70.00

^I'm not a fan of hardcoded values, but as long as it's just about trees a value between 50 and 200 seems suitable.
Sidenote: Finally it's still inaccurate in a few special cases, imagine a tree stands on a cliff and the
knockback unit is 2 terrain levels below but in collision range in plane x / y.

"Collision feature":​

• Custom script: set udg_kb3d_tempgroup1=CreateGroup()

^Why not use the kb3d_tempgroup variable instead?

• ((Picked unit) belongs to an enemy of kb3d_Player[kb3d_TempInt]) Equal to True

^Unit collision should be detected for all units, except the knocback unit itself.
In that context I don't really understand the naming Collide_MinRadius.
Can you explain where the difference between minimum and maximum collision radius is?

• Set kb3d_TempLoc2 = (Position of (Picked unit))

^Here is a potential location handle leak.

Dummy creation feature​

• Unit Group - Add (Last created unit) to kb3d_DummyG

^Does kb3d_DummyG has a specific purpose? I couldn't find it elsewhere in your code.
What's the general idea behind the dummy creation feature?

Cleanup​

• Unit - Unpause kb3d_Unit[kb3d_TempInt]

^Here you unpause the knockback unit, however I don't see where you pause it in the first place?

• Animation - Reset kb3d_Unit[kb3d_TempInt]'s animation

That line shouldn't be there by default.

• If (All Conditions are True) then do (Then Actions) else do (Else Actions)
  • If - Conditions
    • kb3d_Index Equal to 0
  • Then - Actions
    • Trigger - Turn off (This trigger)
  • Else - Actions

^This statement should only run after the cleanup process and not for each periodic timer interval.

Furthermore:

• You allocate an hashtable, but the system doesn't require it at all.
  ---
• Set kb3d_GetAngle = 45.00 --> That one should be named "arc".

Conclusion:
For a single map your system might work out ok. For a public resource I see a lot of weaknesses.
In general it's much better to seperate the unit motion part from other "optional features".
In GUI the trigger variable event helps you to modularize code very easy.
It let's the user inject the motion process during important events.

For example the dummy creation or the four special effects appear to be very random toggle to me.

I can only recommend to base you spells on already existing knockback systems, we have a lot well working submissions.
For you personal usage I outlined most weaknesses, so you can improve your own code.

 

[maxodors]

The question I have is whether it is possible to change the angle of the bounce? For instance, I want target-units to bounce in the opposite direction of the missile's direction.