Punitive Surge 1.02

Moderator · Oct 18, 2010

12:58, 19th Oct 2010
TriggerHappy:

Neat spell but your coding could use a few improvements.

Your globals which are configurable should be constants (and named IN_ALL_CAPS). Same goes for your functions which are meant to be configured, they should be constant functions.
It would be best if you used some sort of group recycling (see GroupUtils on wc3c).

Overall I thinks it's good enough to get approved.

Mainy · Oct 18, 2010

Very nice Bjless

Mantis_Style · Oct 19, 2010

Well, I'm no jass-pro, so I'm not able to judge your coding. But I like the way you present your spell:

Screenshot
Youtube-Video
Code in Hidden-Tags
Usage of Tables and Boxes, I really like these boxes.

I would love to rate your spell when some other guys checked your code, just write a privat message and I'll check again and rate.

axleion · Oct 20, 2010

nice code...
but:
1. since you're using vJASS, how bout using struct with TimerUtils maybe?
2. for the idea of customizability,

JASS:

        call SetSoundVolume( soundHandle , 200 )

that '200' need to be stored into global perhaps?

JASS:

        local sound s = sounds[GetRandomInt(0 , 2)]

that '2' might need to be changed into global, as if a user add some more sounds, he/she might not know that they need to change that '2'.

JASS:

            call DestroyEffect(AddSpecialEffectTarget( hitEffect , v , "chest" ) )

that "chest" could be stored in global too.

JASS:

            call SetUnitVertexColor(u , 100 , 100 , 100 , i2 )

JASS:

                call SetUnitVertexColor(v , 100 , 100 , 100 , dummyInitialFade )

in those line, you can add few globals to make this more customizable, as some user would like to change the color too, not just fading.

JASS:

        set itemPathing = CreateItem( 'ratc' , 0 , 0 )

that 'ratc' could be stored inside a global too, if the user modify the standard item, does it has no impact to the pathing?

those "store into global" thing are just an idea, as some user will just copy+paste the spell, only look the spell from start to end, then just change the vars you provide on the top.

3. you didn't null the trigger "t" in your init.

well just that, hehe

watermelon_1234 · Oct 20, 2010

Code Review:

You should definitely look into GroupUtils like TriggerHappy mentioned. It would allow you to recycle a unit group for UG instead of creating a new one each time.
Instead of using a local group variable, g, you should either have a global group variable used purely for that group enumeration or use ENUM_GROUP if you're using GroupUtils. This avoid having to use DestroyGroup (which you don't seem to be doing it all to g unless I'm being blind)
I suggest that you use TerrainPathability instead of your own pathability check.
I have yet another library to suggest, SoundUtils. This avoids having all those weird problems with sounds and makes it easier for people to import your spell as they don't need to import the global sounds as well.
If you want to become more skilled with vJass, you should look into using structs. By using structs, you wouldn't need a hashtable for this, unless I'm mistaken about something. A useful tutorial to look at would be this.
You could make more things configurable, such as vertex color of the dummy unit. If you don't want to use TerrainPathability, you should make the raw id of the item configurable.

JASS:

call SetUnitColor( v , GetPlayerColor( GetOwningPlayer(u) ) )

->

JASS:

call SetUnitColor( v , GetPlayerColor( P ) )

You could null the local variable v after call GroupAddUnit(dummyGroup , v ) to avoid nulling it unnecessarily.
You could preload special effects.

The spell looks pretty nice in the video.

Edit: Wow, I'm pretty slow.

baassee · Oct 20, 2010

To add more to this:

the initializing function should be private

the event should be spell effect not cast ( you may recognize this from GUI )

I think you should do the getangle and getdist inside the other functions instead of making separate functions

For me it looks like the height is changing in the video but I can't find such a call made

goody

Maker · Oct 20, 2010

Thaks to everyone for the suggestion. I applied some of them.

I'll look into using external libraries.

baassee said:
the event should be spell effect not cast

Did you check the comment in the trigger? There's casting delay between Begins casting and Starts the effect. By default it is 0,5 seconds. So to get by that 0,5 second delay I use Bagins casting. In the ability which is based on Channel, I have set "Disables other abilities" to true. So Begins casting works better with this spell than Starts the effect.

For me it looks like the height is changing in the video but I can't find such a call made

It's just the animation that makes it look like the height is changing

baassee · Oct 20, 2010

Damn that "move" and "stop" were abilities else I would have been correct there!

HaniBon · Oct 20, 2010

I really like this spell.
+REP
Good job, keep it up!

~ HaniBon ~

Apheraz Lucent · Oct 20, 2010

Thanks for crediting me for idea, name and description :3
Just please, change the my name of my Shannequa to something else. Or use the following text:

Swirling in the darkness, the [HERO] is able to perform a move of intense dexterity, quickly traveling towards targeted point and slashing any enemy foolish enough to stand in [his/her] way. Maximum traveling range slightly increases with each level.

aider · Oct 20, 2010

lol, when I copy the trigger and change the raw codes I get this:

(This is my first time importing a Jass spell)

Thanks!

Kricz · Oct 20, 2010

aider said:
lol, when I copy the trigger and change the raw codes I get this:

(This is my first time importing a Jass spell)

Thanks!

Do you have the JNGP?
If not, get it and use this edtiro instead of the normal Wc3 Editor, which doesn't support vJass.

@Spell:
Looks nice, but the coding could be better, using structs and Systems like TimerUtils, GroupUtils etc.

Armenian Beagle · Oct 21, 2010

lol, "shaniqua"

very nice dude

Zealon · Oct 21, 2010

This is awesome, nice work Maker

Maker · Oct 22, 2010

Thank you for the comments.

Apheraz Lucent said:
Just please, change the my name of my Shannequa to something else.

Sorry about that, I forgot to change it. Now it just says "caster".

EDIT: Slowed the animation speed because it played a bit too fast. It's also configurable.

Pharaoh_ · Oct 23, 2010

Simple spell, but I wouldn't expect anything else on your learning course.
Good job, one thing to mention, I'd like to see the fading images follow the animation sequence of the hero.

Maker · Oct 23, 2010

v1.02
The dummies now play their animation at the same speed as the caster.
Changed default sound volume to 100%.

Pharaoh_ said:
I'd like to see the fading images follow the animation sequence of the hero.

I'm not quite sure what you mean by that.

Pharaoh_ · Oct 23, 2010

You fixed it, nevermind :]

Apheraz Lucent · Dec 16, 2010

As a part of my thanks for showing me JNGP, here is edited, new description, along with several icon links. Yours is stretched for some reason.

Swirling in the Darkness and clashing out the weak, caster is able to perform a dashing move towards the desired destination. While traveling, shadows of hero will be summoned to slash any enemy units they encounter, dealing damage.

It's not ,,Point she's facing", because it's a targetable spell ^^

dhguardianes · Nov 28, 2011

@Maker

you check it, use the skill same time Move unit to another area. the skill has bug constant loop.

nightlynx · Jul 5, 2012

Hmm...

first time importing a spell here, so...

To get the ability, I'd need to manually create the hero/ability/item in my own map first, right? It's not possible to import all object data without wiping my own map, am I right?(I know it sounds kinda lazy of me, but I don't want to accidently import and wipe ;_; )

Barorque · Feb 14, 2019

A good charge spell, and a great installation guide for total noobs like me, gj!

Izzetin · Aug 1, 2022

This Spell is awesome but it doesnt Work over 128x128 Area in my Map. I changed the Code that said ''playable 128'' but it still wont work over 128

help

Pyrogasm · Aug 2, 2022

Izzetin said:
This Spell is awesome but it doesnt Work over 128x128 Area in my Map. I changed the Code that said ''playable 128'' but it still wont work over 128
help

I don't entirely understand what you want to do but:

the "RADIUS" variable you changed from 128 to some new value is not related to the physical size of your map in the editor.
RADIUS's value determines the distance at which targets are found in this line: call GroupEnumUnitsInRange( g , r7 , r8 , RADIUS , function unitFilter)
Because of the way GroupEnumUnitsInRange works, a unit will not be 'picked' by the enum unless the origin of its model is within RADIUS of the center. It does not take unit collision size into account, so it's possible that a unit with a large model/collision size/selection circle looked like it should be close enough but was not actually picked. You can get around this by making RADIUS bigger or altering the code to check multiple times but that's not particularly efficient.

Izzetin · Aug 2, 2022

Hmm i didnt check that but the Spell works perfectly fine. but if i walk my Unit to the Top of the Large Map the Spell doesnt Work anymore. He just does the Animaton and effect but not the forward Dash anymore.

Pyrogasm · Aug 3, 2022

You’re going to have to post an example map. There’s no reason that should be happening with the code in this spell. It uses an item to check pathability which is the only time the spell can end early. You are probably trying to cast the spell into an unpathable area.

Izzetin · Aug 3, 2022

Ok here. I put the Spell in this Test Map and the Spell Works in the Heroes Area. but if you go to the Right Goldmine the Spell stops ''mid-air'' and at a specific point on the Map the Spell wont work anymore. If you go back to the Heroes Start Area it works again.
(I renamed the Sounds)

Pyrogasm · Aug 3, 2022

The item pathability check is wonky. The OP checked to make sure that the item coordinates and the 'anticipated' coordinates were exactly equal. Checking if two reals are exactly equal is a terrible idea because they almost never are. In this case, every step of the way the item x/y position was 0.001 to 0.01 away from the target coordinates, which was causing the spell to end early. Why only those parts of the map caused the position to vary by that amount? No idea. There's other stuff that I would fix in this resource but it's not likely ever to be updated so whatever. Just do this:

JASS:

//in the function casterLoop change this...
if r7 == r5 and r8 == r6 then

//to this...
if (RAbsBJ(r7-r5) < 0.01 and RAbsBJ(r8-r6) < 0.01) then

Izzetin · Aug 4, 2022

It worked!
Thanks alot

)

Icon	Description

Punitive Surge 1.02

Charge (Map)

Moderator

Moderator

Attachments

Similar threads