Rock Dance

Moderator · Jan 9, 2011

Magtheridon96:
Approved.

Your current handling of the locations is pretty ugly :/
Still, I read through it and found no leaks.
The triggers are okay.

Just a tip, in the cast trigger, since you're repeating the angle calculation, you might as well cache it into a variable and reuse it.
Also, instead of using those location arrays, use normal variables.

Also, use Map initialization instead of the 0 second game time event.

Moderator: Pharaoh_
Date: 03:47, 24th Apr 2012

Review

Fix your indexing. Use Hanky's or Bribe's templates, located here, in the Spells section.

29 Oct 2011
Bribe: Use Hanky's Dynamic Indexing. Looping 1-50 is "MUI" but the logic you're using via that method is absolutely terrible and the load on the CPU is way higher than necessary.

http://www.hiveworkshop.com/forums/spells-569/gui-dynamic-indexing-template-144325

Nightmare Moon · Jan 9, 2011

Suggestions

- don't use Unit - A unit Begins casting an ability, instead of that use
Unit - A unit Starts the effect of an ability;
- don't use custom value of unit;
- the way you make it MUI isn't to good (use better indexing system), also the loop trigger is always on;
- instead of Casting unit use Triggering unit;
- remove Point Value from dummy in OE;
- create another variable for Target point of ability being cast;
- you don't remove TempPoint2 in Rock Dance;
- use parabola equation to make the flying of ball smooth;
- in action where you change Height you use 1000000000.00 value use 0;
- in second trigger in TempGroup you pick dead units;
- tutorial how to improve your spell: [self="http://www.hiveworkshop.com/forums/trigger-gui-editor-tutorials-279/multi-instancible-gui-spell-making-34393/"]MUI[/self]
- when the ball must die you first remove him then kill, why? Better kill him;
- i also don't understand what is with that RD_TargetGroup and you never destroy it;
- why you use those actions

Multiple FunctionsIf (All Conditions are True) then do (Then Actions) else do (Else Actions)

If - Conditions

(Distance between TempPoint and RD_TargetPoint[TempValue]) Less than or equal to (<=) 10.00

Then - Actions

Set RD_TargetPoint[TempValue] = RD_CasterPoint[TempValue]

Unit Group - Remove all units from RD_TargetGroup[TempValue]

Else - Actions

if you already have same action at beginning of loop? Just clear the RD_TargetGroup in first If/Then/Else, where you should turn off loop.

I guess that's all, and btw this spell is very similar to mine... So you could something that mine doesn't have so it would be more original idea.

Voting

3/5 Haven't decided for what to vote, for now

leonguyen112 · Jan 9, 2011

Stanley said:
Suggestions

- don't use Unit - A unit Begins casting an ability, instead of that use
Unit - A unit Starts the effect of an ability;
- don't use custom value of unit;
- the way you make it MUI isn't to good (use better indexing system), also the loop trigger is always on;
- instead of Casting unit use Triggering unit;
- remove Point Value from dummy in OE;
- create another variable for Target point of ability being cast;
- you don't remove TempPoint2 in Rock Dance;
- use parabola equation to make the flying of ball smooth;
- in action where you change Height you use 1000000000.00 value use 0;
- in second trigger in TempGroup you pick dead units;
- tutorial how to improve your spell: [self="http://www.hiveworkshop.com/forums/trigger-gui-editor-tutorials-279/multi-instancible-gui-spell-making-34393/"]MUI[/self]
- when the ball must die you first remove him then kill, why? Better kill him;
- i also don't understand what is with that RD_TargetGroup and you never destroy it;
- why you use those actions

Multiple FunctionsIf (All Conditions are True) then do (Then Actions) else do (Else Actions)

If - Conditions

(Distance between TempPoint and RD_TargetPoint[TempValue]) Less than or equal to (<=) 10.00

Then - Actions

Set RD_TargetPoint[TempValue] = RD_CasterPoint[TempValue]

Unit Group - Remove all units from RD_TargetGroup[TempValue]

Else - Actions

if you already have same action at beginning of loop? Just clear the RD_TargetGroup in first If/Then/Else, where you should turn off loop.

I guess that's all, and btw this spell is very similar to mine... So you could something that mine doesn't have so it would be more original idea.

Voting

3/5 Haven't decided for what to vote, for now

Ah, my bad, I have fixed it. But somethings you said I don't understand (sorry because my bad English, I'm VietNamese) I'll keep learning, thanks for your help.

Klingo · Jan 9, 2011

RD_Group don't leak because you haven't set it... so you don't need to destroy it.

in "Rock Dance Move" trigger, would be nice if you could put "picked unit" into a variable.

In the first trigger , you set two times the positions of triggering unit when only one could do the job

In the first trigger , you set this but you never use it...

Set RD_Unit[MUI] = (Last created unit)

88WaRCraFT3 · Jan 9, 2011

Suggestions

It would be better if you make the rock follow you! Jumps whole time until reachs you, cause it jumps 3 times to target point, and then back to variable Position of Caster...

gl
Cool spell anyway, it can be used like system too. 5/5 +rep

tjordell · Jan 9, 2011

The first thing I see in your code that can be better is the RD_TempPoint1/2/3. To make your spell as small in bytes as possible, you should make the RD_TempPoint into one variable with Array to store the Locafions into. Also, it's easier to work with less Triggers if you or someone else want to modify it in the future.

Secondly, you shall avoid changing Custom Values of units because thats not the right way to handle with a 'bunch' of units. Instead of using this Custom Value you should create another Variable (Integer) and do as other people do it MUI.

Now I cant see anything more than that u should place every action in all the (If/Then/Else) with the condition "If Distace between 'variable1' and 'variable2' is less than or equal to 10.00" into ONE (If/Then/Else). Otherwise you check the same thing over and over again when u dont need to.

Now I'm dine here. I wrote all this from my iPhone 4 so I hope that you really appreciate it. Have a good day/night fella'!

MortAr · Jan 11, 2011

An array variable would take more space than 3 non-array, since they have a default size of 8192, which needs to be initialized in the main script when the map loads.

tjordell · Jan 11, 2011

Are you really sure of that? I heard it from a person with super high reputation that array is less space...

MortAr · Jan 12, 2011

My spell was rejected because of using an array for 3 points instead of using 3 separate variables.
Yes I'm pretty sure it is better.

leonguyen112 · Jan 12, 2011

Thanks all, now I have some great experiences for my next spell

Adiktuz · Oct 28, 2011

Make this use dynamic indexing... so that it would be able to support more than 50 rocks (by utilizing all the possible indexes)...

vuongkkk · Oct 29, 2011

An array variable would take more space than 3 non-array?

@MortAr; tjordell:
GUI array allows you change the size. But this size is used for initiating default values. In the fact, the array of variable RD_TempPoint is still 8192
~ Then the answer is YES.

jass array is a other situation, if creating like this: udg_RD_TempPoint[3]
~ Then the answer is that they are no different. They take the same space

You should know that even you not use, it will take some space after the initiation

RakEnRoL · Oct 29, 2011

I like the spell but something tells me that it lacks something.

leonguyen112 · Oct 30, 2011

RakEnRoL said:
I like the spell but something tells me that it lacks something.

What is lack? Can you tell me?

xIceShotx · Oct 30, 2011

It means something bad is happening and it needs to be kind of improved:
Online Dictionary:
Deficiency or absence of something needed, desirable, or customary.
2:
Something missing or needed.

hope that helps

Adiktuz · Oct 30, 2011

I think the posts above already told you what was lacking in this spell...

Tank-Commander · Apr 22, 2012

This spell really needs a proper configuration, Atm it's rather difficult to modify for /anyone/ considering you have to go through all the triggers to change stuff, also you should use variables for the dummy unit/spell/effects.

leonguyen112 · Apr 24, 2012

^
I fixed and updated, thanks you for your advices.

Tank-Commander · Apr 24, 2012

Your jump damage modification in the config won't actually work for levelling, considering it'll be set once (i.e. the level of the ability of triggering unit == 0) and thus regardless of if the ability is level 3 it'll be the same strength as level 1 (60 damage) even though level 1 should deal 90 and level 3 should deal 150
I'd suggest having two variables of the each configurable: One for BASE stat, and one for PER LEVEL stats, I.e. Jump_Damage_Base = 60, Jump_Damage_Per_Level = 30 and then do the calculation for it at the start of the spell being run:

Set RD_Damage[RD_MUI] = RD_Jump_Damage_Base + (Jump_Damage_Per_Level * (Level of Ability for (Triggering unit)))

Repeat the process for all the other configurables as well (even if the Per_Level version is 0.00/0 so that it gives a greater freedom to all the users of the spell, this'd be in your case, AOE, Jump Times and prehaps jump speed) as the others there is no purpose in having them change per level to increase/decrease the overall power of the spell.

EDIT: you should also use that Caster Variable in your casting trigger to replace all the (Triggering Unit) brackets which'll appear if you do that formula I gave above for all your configurations

leonguyen112 · Apr 24, 2012

^ I don't think so. There is still one trigger named Rock Setting that I didn't post. That trigger have event "Unit begin casting an ability". I mean every time the unit cast spell, trigger will set damage = 60 + 30 x current level of spell. So, the damage is updated every casting times.

EDIT: you should also use that Caster Variable in your casting trigger to replace all the (Triggering Unit) brackets which'll appear if you do that formula I gave above for all your configurations

I'll do this.

Tank-Commander · Apr 24, 2012

um, that really should not be how you set up a configuration, the configuration should be set-up on map initialisation and /never/ change, considering with the current setup if one is "starts casting" and "starts the effect of" then there is a small window for creating bugs, of course since only damage is the one which changes between levels, it'd be harder to notice, but still existent - i.e. this'd make it possible for a level 3 spell to be equally powerful as a level 1 spell if they casted it at the right time (and vice versa)

Setting the variables to alter for the unit should be done /in the same trigger/ as "starts the effect of" and those should only be using configurables and the level of the ability, it's not good to see configurables being set at the same place, as it takes up unecessary processing to run the set variable script when it's trying to change it to the exact same thing.

BloodSoul · Apr 25, 2012

If (All Conditions are True) then do (Then Actions) else do (Else Actions)
- If - Conditions
  - RD_Effect[(Integer A)] Not equal to Unknown
- Then - Actions
- Else - Actions

Or:

JASS:

if ( not ( udg_RD_Effect[GetForLoopIndexA()] != Error ) ) then

"Comparing two variables of differents primitive types (except real and integer) is not allowed"

I think that condition is useless though. Why anyone wouldn't put effects? People cannot import your spell into their map.

leonguyen112 · Apr 25, 2012

@Tank-Commander:
Okay, I see it, I were wrong. I'll update it.

@BloodSoul:
You should make sure that type of RD_Effect is string, not special effect.

BloodSoul · Apr 25, 2012

leonguyen112 said:
@Tank-Commander:
Okay, I see it, I were wrong. I'll update it.

@BloodSoul:
You should make sure that type of RD_Effect is string, not special effect.

That happened when I just downloaded the map and I tried to save it.

leonguyen112 · Apr 25, 2012

BloodSoul said:
That happened when I just downloaded the map and I tried to save it.

You have that bug just because using the different JNGP. This pack will fix that bug. It contain JNGP 1.05d which I used. Maybe I should set this pack in Requirement :vw_wtf:

BloodSoul · Apr 25, 2012

leonguyen112 said:
You have that bug just because using the different JNGP. This pack will fix that bug. It contain JNGP 1.05d which I used. Maybe I should set this pack in Requirement

Probably, because I don't use JNGP. Sorry! xD

Rock Dance

Spell - Rock Dance (Map)

Moderator

Moderator

Similar threads