wc3campaigns
WC3C Homepage - www.wc3c.netUser Control Panel (Requires Log-In)Engage in discussions with other users and join contests in the WC3C forums!Read one of our many tutorials, ranging in difficulty from beginner to advanced!Show off your artistic talents in the WC3C Gallery!Download quality models, textures, spells (vJASS/JASS), systems, and scripts!Download maps that have passed through our rigorous approval process!

Go Back   Wc3C.net > Resources > Code Resources > vJass Spells
User Name
Password
Register Rules Get Hosted! Chat Pastebin FAQ and Rules Members List Calendar



Reply
 
Thread Tools Search this Thread
Old 05-14-2009, 08:04 PM   #16
Rising_Dusk
Obscurity, the Art


Projects Director
Project Leader: OD
 
Join Date: Feb 2006
Posts: 9,729

Submissions (27)

Rising_Dusk has a reputation beyond repute (1192)Rising_Dusk has a reputation beyond repute (1192)Rising_Dusk has a reputation beyond repute (1192)Rising_Dusk has a reputation beyond repute (1192)Rising_Dusk has a reputation beyond repute (1192)Rising_Dusk has a reputation beyond repute (1192)Rising_Dusk has a reputation beyond repute (1192)Rising_Dusk has a reputation beyond repute (1192)Rising_Dusk has a reputation beyond repute (1192)

Hero Contest #3 - 1st PlaceApproved Map: Desert of ExileApproved Map: Advent of the ZenithHero Contest #2 - 1st PlaceHero Contest - Third place>

Send a message via AIM to Rising_Dusk Send a message via MSN to Rising_Dusk
Default

This has numerous problems and is nowhere near ready for approval.
  • InitTrigs are deprecated, use a scope initializer.
  • Your functions are private, they do not need Live_Distortion_ prefixes.
  • Your indenting and code style is very poor and makes the code very messy. You should use shorter, terse variable names, align things, and try to work in blocks.
  • Come on, shorter variable names, this is titanic: set Essences.Data[Essences.Index-1].MissileScale[a] = ...
  • 2*bj_PI, just use the actual value, don't do the math.
  • Destroying and creating groups just to enumerate instantaneously has been long deprecated. Just use the single, global group from GroupUtils. Modularity is good.
  • GetUnitState(data.Caster, UNIT_STATE_LIFE), use GetWidgetLife()
  • call DestroyGroup(data.GotBuff), again, GroupUtils.
  • Please clean up the mess that is your 'callback' function, it's impossible to sort through the lines of messiness. Something that hard to read cannot be approved.
  • private constant function NullFilter takes nothing returns boolean return true endfunction Use BoolexprUtils.
  • Clean up your entire configuration constant global block. Right now it is a total mess and is nigh impossible for users to figure out.
__________________

Last edited by Rising_Dusk : 05-14-2009 at 08:04 PM.
Rising_Dusk is offline   Reply With Quote
Sponsored Links - Login to hide this ad!
Old 05-16-2009, 04:58 PM   #17
JonNny
User
 
JonNny's Avatar
 
Join Date: Jun 2007
Posts: 51

Submissions (2)

JonNny has little to show at this moment (9)

Default

Quote:
Originally Posted by Rising_Dusk
This has numerous problems and is nowhere near ready for approval.
  • InitTrigs are deprecated, use a scope initializer.
  • Your functions are private, they do not need Live_Distortion_ prefixes.
  • Your indenting and code style is very poor and makes the code very messy. You should use shorter, terse variable names, align things, and try to work in blocks.
  • Come on, shorter variable names, this is titanic: set Essences.Data[Essences.Index-1].MissileScale[a] = ...
  • 2*bj_PI, just use the actual value, don't do the math.
  • Destroying and creating groups just to enumerate instantaneously has been long deprecated. Just use the single, global group from GroupUtils. Modularity is good.
  • GetUnitState(data.Caster, UNIT_STATE_LIFE), use GetWidgetLife()
  • call DestroyGroup(data.GotBuff), again, GroupUtils.
  • Please clean up the mess that is your 'callback' function, it's impossible to sort through the lines of messiness. Something that hard to read cannot be approved.
  • private constant function NullFilter takes nothing returns boolean return true endfunction Use BoolexprUtils.
  • Clean up your entire configuration constant global block. Right now it is a total mess and is nigh impossible for users to figure out.

-Changed to a scope initializer
-Removed useless prefixes
-shortened some variable names
-changed 2*bj_PI back to the values , why is everyone telling me different things ? and in my opinial its insanely nonrelevant
-created a global group instead of destroying them each time Edit: Hmm.. right.. now i remember why i did that, whyever it bugs with a global one... got no idea why
-forgot to change this GetUnitState , done it now
-Why is my callback unreadable ? there are some comments what the next lines are for and so on .. imo its not such a great messiness.. anyway.. tried to make it more clearly
-cleaned up the constant global block..

*Updated*

to be honest, i dislike using others systems. also i think e.g. BoolexprUtils will not make this code much better

btw, my last Spellpack was not approved and i was told i should learn vJass and use struct to get it approved
If i now look at these spells i can understand that its coding could have been made more efficient.
Now that im using vJass im getting told im useing too long variable names ect. which are "problems" to get this approved.. (btw , in my Swine Flu Spell i got told i should use longer variable names)
I really attach importance to a clean, leakfree and efficient code but im not a huge perfectionist.. but because of that i seem to be wrong here
__________________

Last edited by JonNny : 05-16-2009 at 05:15 PM.
JonNny is offline   Reply With Quote
Old 05-16-2009, 06:57 PM   #18
akolyt0r
In Flames
 
akolyt0r's Avatar
 
Join Date: Jan 2006
Posts: 1,154

Submissions (3)

akolyt0r has a spectacular aura about (120)

Default

Quote:
Originally Posted by JonNny
Now that im using vJass im getting told im useing too long variable names ect. which are "problems" to get this approved.. (btw , in my Swine Flu Spell i got told i should use longer variable names)
I really attach importance to a clean, leakfree and efficient code but im not a huge perfectionist.. but because of that i seem to be wrong here

Wc3c is for perfectionists ... and obviously RisingDusk really got anal about speed .. (already noticed that in IRC ..huh ?!! xD)
But as a matter of fact, the moderators which "work" for wc3c...do this for free.
Of course you can write correct code, which works well, but is a complete mess to read, ...but maybe you should spend some thoughts for the people who HAVE to read it afterwards aswell ... In my opinion the moderators here do a quite good job... (of course it could be better, but hey they do it for FREE)...

- - -

And since your spell already uses features which are provided by Boolexpr aswell... go use it ... modularity is a _good_ thing ...
Even when it seems to be code bloat for single spells ...in the long run, (e.g. for a whole map) the code will be much shorter (...and easier to maintain.)
__________________
akolyt0r is offline   Reply With Quote
Old 05-16-2009, 08:00 PM   #19
Rising_Dusk
Obscurity, the Art


Projects Director
Project Leader: OD
 
Join Date: Feb 2006
Posts: 9,729

Submissions (27)

Rising_Dusk has a reputation beyond repute (1192)Rising_Dusk has a reputation beyond repute (1192)Rising_Dusk has a reputation beyond repute (1192)Rising_Dusk has a reputation beyond repute (1192)Rising_Dusk has a reputation beyond repute (1192)Rising_Dusk has a reputation beyond repute (1192)Rising_Dusk has a reputation beyond repute (1192)Rising_Dusk has a reputation beyond repute (1192)Rising_Dusk has a reputation beyond repute (1192)

Hero Contest #3 - 1st PlaceApproved Map: Desert of ExileApproved Map: Advent of the ZenithHero Contest #2 - 1st PlaceHero Contest - Third place>

Send a message via AIM to Rising_Dusk Send a message via MSN to Rising_Dusk
Default

I use short, but descriptive variable names. I recommend that to everyone. It's not speed I'm after when I ask you to do that, it's readability. As clear as "MissileScale" might be, you can shorten it to "mscl" and it carries the same idea, while being 8 letters shorter and making your code substantially easier to read on the whole.
Quote:
Originally Posted by JoHnny
to be honest, i dislike using others systems. also i think e.g. BoolexprUtils will not make this code much better
It will, because it abstracts away your unnecessary 'return true' boolexpr.
__________________

Last edited by Rising_Dusk : 05-16-2009 at 08:00 PM.
Rising_Dusk is offline   Reply With Quote
Old 05-16-2009, 08:27 PM   #20
JonNny
User
 
JonNny's Avatar
 
Join Date: Jun 2007
Posts: 51

Submissions (2)

JonNny has little to show at this moment (9)

Default

Quote:
Originally Posted by Rising_Dusk
I use short, but descriptive variable names. I recommend that to everyone. It's not speed I'm after when I ask you to do that, it's readability. As clear as "MissileScale" might be, you can shorten it to "mscl" and it carries the same idea, while being 8 letters shorter and making your code substantially easier to read on the whole.

It will, because it abstracts away your unnecessary 'return true' boolexpr.

Importing 16 lines of script to replace my "unnecessary 'return true' boolexpr" with another one ? Of course it would be worth if there are many other spells / systems which use it
Also cannot imagine any case in which a constant BOOLEXPR_FALSE could be usefull...
of course it could spare some small amounts of code if every other spell /system uses BoolexprUtils

If you are taking everything that insanely precise you could have quoted my name correctly :P

Anyway , ill change that with adding a useful filter function and fix the rest of my Swine Flu spell tomorrow when i got time
__________________

Last edited by JonNny : 05-16-2009 at 08:28 PM.
JonNny is offline   Reply With Quote
Old 05-16-2009, 09:39 PM   #21
Rising_Dusk
Obscurity, the Art


Projects Director
Project Leader: OD
 
Join Date: Feb 2006
Posts: 9,729

Submissions (27)

Rising_Dusk has a reputation beyond repute (1192)Rising_Dusk has a reputation beyond repute (1192)Rising_Dusk has a reputation beyond repute (1192)Rising_Dusk has a reputation beyond repute (1192)Rising_Dusk has a reputation beyond repute (1192)Rising_Dusk has a reputation beyond repute (1192)Rising_Dusk has a reputation beyond repute (1192)Rising_Dusk has a reputation beyond repute (1192)Rising_Dusk has a reputation beyond repute (1192)

Hero Contest #3 - 1st PlaceApproved Map: Desert of ExileApproved Map: Advent of the ZenithHero Contest #2 - 1st PlaceHero Contest - Third place>

Send a message via AIM to Rising_Dusk Send a message via MSN to Rising_Dusk
Default

Quote:
Originally Posted by JonNny
Also cannot imagine any case in which a constant BOOLEXPR_FALSE could be usefull...
It would not be approved if it were not useful. It is because a null boolexpr used in group enumeration native function calls leaks (substantially). Therefore, if you simply want all units to be grouped up, you can't use a null boolexpr and must use a TRUE boolexpr. The FALSE boolexpr is in the library for completeness.

The point of vJass is to be modular. Let us consider for a moment that a great use of my GroupUtils library is the ENUM_GROUP global group that it gives. Even if you just want to use that one group, it is worth it to use the whole library because other systems or spells may then put it to use. The added modularity is one of the greatest aspects of vJass.
Quote:
Originally Posted by JonNny
of course it could spare some small amounts of code if every other spell /system uses BoolexprUtils
Precisely.
Quote:
Originally Posted by JonNny
If you are taking everything that insanely precise you could have quoted my name correctly :P
It's my job to be a precise and nitpicky reviewer - I'm not running this site so that I can quote your name correctly! I fixed it in this post, though. :p
__________________
Rising_Dusk is offline   Reply With Quote
Old 05-17-2009, 11:55 PM   #22
JonNny
User
 
JonNny's Avatar
 
Join Date: Jun 2007
Posts: 51

Submissions (2)

JonNny has little to show at this moment (9)

Default

Updated... changed that Nullfilter and implented GroupUtils... :/
__________________
JonNny is offline   Reply With Quote
Old 07-01-2009, 02:40 PM   #23
Rising_Dusk
Obscurity, the Art


Projects Director
Project Leader: OD
 
Join Date: Feb 2006
Posts: 9,729

Submissions (27)

Rising_Dusk has a reputation beyond repute (1192)Rising_Dusk has a reputation beyond repute (1192)Rising_Dusk has a reputation beyond repute (1192)Rising_Dusk has a reputation beyond repute (1192)Rising_Dusk has a reputation beyond repute (1192)Rising_Dusk has a reputation beyond repute (1192)Rising_Dusk has a reputation beyond repute (1192)Rising_Dusk has a reputation beyond repute (1192)Rising_Dusk has a reputation beyond repute (1192)

Hero Contest #3 - 1st PlaceApproved Map: Desert of ExileApproved Map: Advent of the ZenithHero Contest #2 - 1st PlaceHero Contest - Third place>

Send a message via AIM to Rising_Dusk Send a message via MSN to Rising_Dusk
Default

The character limit for posts has been fixed, you can put your 'Mind Burst' code in the first post now. Also, you need to link to BoolExprUtils in the first post since you now use it in Live Distortion. Also, I'd venture that you need to organize your first post better so people know which libraries apply to which spell and so forth.

The code for both spells looks good, though, with one exception. Your configuration variables at the top of both spells should be constant variables so that they get inlined.
__________________
Rising_Dusk is offline   Reply With Quote
Old 07-02-2009, 12:49 PM   #24
JonNny
User
 
JonNny's Avatar
 
Join Date: Jun 2007
Posts: 51

Submissions (2)

JonNny has little to show at this moment (9)

Default

Quote:
Originally Posted by Rising_Dusk
The character limit for posts has been fixed, you can put your 'Mind Burst' code in the first post now. Also, you need to link to BoolExprUtils in the first post since you now use it in Live Distortion. Also, I'd venture that you need to organize your first post better so people know which libraries apply to which spell and so forth.

The code for both spells looks good, though, with one exception. Your configuration variables at the top of both spells should be constant variables so that they get inlined.

Ok, added the code to my first post
But i wont link BoolExprUtils because i dont use it, i changed these nullfilters into usefull ones

I also see i forgot to update it here, globals were already constant and i fixed other small things
__________________
JonNny is offline   Reply With Quote
Old 07-02-2009, 01:25 PM   #25
Viikuna-
User
 
Viikuna-'s Avatar
 
Join Date: Feb 2009
Posts: 203

Viikuna- will become famous soon enough (44)Viikuna- will become famous soon enough (44)

Default

You are creating dummies dynamicly which is kinda terrible..

Its better to create one dummy caster at map init and use it to cast those rejuvinations and unholyfrenzies.
__________________
No Marlo, no game.
Viikuna- is offline   Reply With Quote
Old 07-02-2009, 03:55 PM   #26
Rising_Dusk
Obscurity, the Art


Projects Director
Project Leader: OD
 
Join Date: Feb 2006
Posts: 9,729

Submissions (27)

Rising_Dusk has a reputation beyond repute (1192)Rising_Dusk has a reputation beyond repute (1192)Rising_Dusk has a reputation beyond repute (1192)Rising_Dusk has a reputation beyond repute (1192)Rising_Dusk has a reputation beyond repute (1192)Rising_Dusk has a reputation beyond repute (1192)Rising_Dusk has a reputation beyond repute (1192)Rising_Dusk has a reputation beyond repute (1192)Rising_Dusk has a reputation beyond repute (1192)

Hero Contest #3 - 1st PlaceApproved Map: Desert of ExileApproved Map: Advent of the ZenithHero Contest #2 - 1st PlaceHero Contest - Third place>

Send a message via AIM to Rising_Dusk Send a message via MSN to Rising_Dusk
Default

Quote:
Originally Posted by JonNny
Ok, added the code to my first post
But i wont link BoolExprUtils because i dont use it, i changed these nullfilters into usefull ones

I also see i forgot to update it here, globals were already constant and i fixed other small things
Okay, cool. See below for a few last notes.
Quote:
Originally Posted by Viikuna-
You are creating dummies dynamicly which is kinda terrible..

Its better to create one dummy caster at map init and use it to cast those rejuvinations and unholyfrenzies.
This is a very, very good point. I would say that you should do this, JonNny.
__________________
Rising_Dusk is offline   Reply With Quote
Old 07-03-2009, 04:57 PM   #27
0zyx0
Perfectionist noob
 
0zyx0's Avatar
 
Join Date: Mar 2009
Posts: 255

0zyx0 will become famous soon enough (38)0zyx0 will become famous soon enough (38)

Default

Collapse JASS:
    unit array EssenceDummy [15] //The array must be the max. value of missiles which are created each cast (in this case , EssencesPerLevel(3) = 3*3+3 = 12)
    real array angle [15] //same as above
    group array DamagedUnits [15] // same as above
    real array MissileScale [15] // same as above

Replace 15 with an integer constant, that can be configured.

SquareRoot(Sin(d)*Sin(d)). What is that supposed to mean!?
(You almost made me use a word there).

Collapse JASS:
private function StealHealthCondition takes unit a , unit b returns boolean
    return  IsPlayerEnemy(GetOwningPlayer(a),GetOwningPlayer(b))  // The condition if a unit is damaged in addition to the UnitFilter
endfunction
Which unit is the caster, and which one is the target? You should probably rename the arguments.
__________________
My new signature - Now easier to understand than ever!

Last edited by 0zyx0 : 07-03-2009 at 05:06 PM.
0zyx0 is offline   Reply With Quote
Old 07-07-2009, 05:21 PM   #28
Rising_Dusk
Obscurity, the Art


Projects Director
Project Leader: OD
 
Join Date: Feb 2006
Posts: 9,729

Submissions (27)

Rising_Dusk has a reputation beyond repute (1192)Rising_Dusk has a reputation beyond repute (1192)Rising_Dusk has a reputation beyond repute (1192)Rising_Dusk has a reputation beyond repute (1192)Rising_Dusk has a reputation beyond repute (1192)Rising_Dusk has a reputation beyond repute (1192)Rising_Dusk has a reputation beyond repute (1192)Rising_Dusk has a reputation beyond repute (1192)Rising_Dusk has a reputation beyond repute (1192)

Hero Contest #3 - 1st PlaceApproved Map: Desert of ExileApproved Map: Advent of the ZenithHero Contest #2 - 1st PlaceHero Contest - Third place>

Send a message via AIM to Rising_Dusk Send a message via MSN to Rising_Dusk
Default

Agreed with all of 0zyx's comments.
__________________
Rising_Dusk is offline   Reply With Quote
Old 07-07-2009, 11:01 PM   #29
JonNny
User
 
JonNny's Avatar
 
Join Date: Jun 2007
Posts: 51

Submissions (2)

JonNny has little to show at this moment (9)

Default

Quote:
Originally Posted by Viikuna-
You are creating dummies dynamicly which is kinda terrible..

Its better to create one dummy caster at map init and use it to cast those rejuvinations and unholyfrenzies.

done...



Quote:
Originally Posted by 0zyx0
Collapse JASS:
    unit array EssenceDummy [15] //The array must be the max. value of missiles which are created each cast (in this case , EssencesPerLevel(3) = 3*3+3 = 12)
    real array angle [15] //same as above
    group array DamagedUnits [15] // same as above
    real array MissileScale [15] // same as above

Replace 15 with an integer constant, that can be configured.


done...

Quote:
Originally Posted by 0zyx0
SquareRoot(Sin(d)*Sin(d)). What is that supposed to mean!?
(You almost made me use a word there).

i prefer using that instead of RAbsBJ(Sin(d)) i dislike RAbsBJ

Quote:
Originally Posted by 0zyx0
Collapse JASS:
private function StealHealthCondition takes unit a , unit b returns boolean
    return  IsPlayerEnemy(GetOwningPlayer(a),GetOwningPlayer(b))  // The condition if a unit is damaged in addition to the UnitFilter
endfunction
Which unit is the caster, and which one is the target? You should probably rename the arguments.

done...

I hope thats all of fixing..
im feeling like Duke Nukem, for each fixed thing two new issues appear
__________________
JonNny is offline   Reply With Quote
Old 07-08-2009, 03:10 AM   #30
Rising_Dusk
Obscurity, the Art


Projects Director
Project Leader: OD
 
Join Date: Feb 2006
Posts: 9,729

Submissions (27)

Rising_Dusk has a reputation beyond repute (1192)Rising_Dusk has a reputation beyond repute (1192)Rising_Dusk has a reputation beyond repute (1192)Rising_Dusk has a reputation beyond repute (1192)Rising_Dusk has a reputation beyond repute (1192)Rising_Dusk has a reputation beyond repute (1192)Rising_Dusk has a reputation beyond repute (1192)Rising_Dusk has a reputation beyond repute (1192)Rising_Dusk has a reputation beyond repute (1192)

Hero Contest #3 - 1st PlaceApproved Map: Desert of ExileApproved Map: Advent of the ZenithHero Contest #2 - 1st PlaceHero Contest - Third place>

Send a message via AIM to Rising_Dusk Send a message via MSN to Rising_Dusk
Default

All good, except one thing:
Quote:
Originally Posted by JonNny
i prefer using that instead of RAbsBJ(Sin(d)) i dislike RAbsBJ
RAbsBJ(Sin(d)) seems to me like it would be faster than SquareRoot(Sin(d)*Sin(d)). Although, since I have no conclusive evidence (Benchmarks), I will go ahead and approve this. If someone wants to change it themselves, they can; it is a configuration function after all.
__________________
Rising_Dusk is offline   Reply With Quote
Reply


Thread Tools Search this Thread
Search this Thread:

Advanced Search

Posting Rules
You may not post new threads
You may not post replies
You may not post attachments
You may not edit your posts

vB code is On
Smilies are On
[IMG] code is On
HTML code is Off


All times are GMT. The time now is 05:34 PM.


Affiliates
The Hubb The JASS Vault Clan WEnW Campaign Creations Clan CBS GamesModding Flixreel Videos

Powered by vBulletin (Copyright ©2000 - 2019, Jelsoft Enterprises Ltd).
Hosted by www.OICcam.com
IT Support and Services provided by Executive IT Services