Spell Contest #3 | Results

[Scotty]

i'm not too sad being last for my first ubmision, the only way to go now is up!

overall i only know trigger basics, i mean basics of the basics, i fell though that since i've been here things are getting better and, hopefully, i'll be doing alot better next time around

 

[Viikuna]

SetWidgetLife/SetUnitState works on dead units, which would result in a GetWidgetLife check to confirm if the target is dead may be false (the unit would have more than .405 health, but it'd still be dead). IsUnitType (whichUnit, UNIT_TYPE_DEAD) works even if you have modified a dead unit's life

No one is stupid enough to modify units life, if it is less or equal to .405. Thats why we write our own healing functions and stuff like that,

Then again: IsUnitType (whichUnit, UNIT_TYPE_DEAD) fails, if unit has decayed, so you cant call it safe.

You can easily stop yourself from modifying dead units hp, but decaying is something you might need in your map, so removing it for sake of UNIT_TYPE_DEAD is kinda stupid.

Actually, I dont use either of these. I just do IsUnitInGroup(u,DeadGuys), which is part of UnitRecycling system.

But if you dont have system like this, GetWidgetLife works just fine.

 

[Andrewgosu]

I did not take points away for using "GetWidgetLife" instead of "IsUnitType", to let you know.

 

[Romek]

> Then again: IsUnitType (whichUnit, UNIT_TYPE_DEAD) fails, if unit has decayed, so you cant call it safe.
If the unit has decayed, it's basically removed. So not that, that makes a difference.
And if the unit is decaying, I'm pretty sure that returns true. After all, when a unit dies, it begins decaying. And that would be a pretty useless function if it returned false on decaying units. (It'd be the equivalent of doing "if false then")

 

[Viikuna]

If the unit has decayed, it's basically removed. So not that, that makes a difference.

It does.

If unit is removed, by decaying or by some other means, its unit type cant be UNIT_TYPE_DEATH , because it no longer exists.

edit.

GetWidgetLife does not have this problem.

So if you use UNIT_TYPE_DEATH you also need some other check to make sure that unit actually exists.

 

[Flare]

It does.

If unit is removed, by decaying or by some other means, its unit type cant be UNIT_TYPE_DEATH , because it no longer exists.

That begs the question - why would you be handling non-existant units in the first place? You can't enumerate them (if you could, I would be highly suprised...) and is there anything you could do with a non-existant unit that would affect gameplay (unless modifying the properties of a non-existant unit causes some behind-the-scenes errors )?

Anyway, a single mistake in enumeration could easily result in SetWidgetLife being used on a dead unit, so don't go saying that nobody is stupid enough to do it - mistakes happen, which doesn't make GetWidgetLife perfect either.

I won't be replying to this thread anymore about this whole UNIT_TYPE_DEAD business, so don't direct anything towards me, unless it's via PM

 

[Romek]

> No one is stupid enough to modify units life, if it is less or equal to .405.
AoE healing effects maybe?

Will null units ever get passed to a filter function?

 

[Viikuna]

AoE healing effects maybe?

Thats why we use UntiHealUnit styled functions, which check units current hp before healing it.

And yes, you can group non existing units, but you can have some variable pointing to them. You could even have non existing units stored in some unit group before we had GroupRefresh.

Hm ok, maybe its unlikely to fuck up with UNI_TYPE_DEATH, but GetWidgetLife is safer, because you dont have to worry about using it to non existing unit.

 

[Builder Bob]

Sorry for barging in here, but isn't this discussion a bit like "what's best of apples and oranges"?

Both functions have problems which require extra care when used. As already pointed out, GetWidgetLife() will say a dead unit is alive, if it has been given back life after it's death. IsUnitType(, UNIT_TYPE_DEAD) will return false for removed units, and can be problematic in some code.

I can say that I've had game breaking bugs because of both. Neither are safe on their own. It always depend on what we're coding. Stupid has nothing to do with it in my opinion. Sometimes it's just very hard to see what you might code later, which could break the code you have now.

 

[Viikuna]

Or you can take advantage of cool Jass feature called custom functions and save yourself from doing stupid things.

Ok, I admit that it is apples and oranges thing. I just dont wanna see anyone saying that oranges are better than apples, like it is a fact or something..

edit.

Sometimes it's just very hard to see what you might code later, which could break the code you have now.

Meh. This is true of course.

 

[bOb666777]

Anyway, don't take these comments personally, as an attack towards you.

We try to give as much contructive criticism as we can to help you improve.

Which is really one the key aspects why I host contests.


We have nothing against you, at all. The comments are just based on the code you submitted.

Oh I know that... The main reason for that comment is pretty much only because Romek thinks my code is ugly, what can I do to improve it?

 

[Daskunk]

the more intense the effects are with my spell, the less laggy

the less intense, more laggy unless you want it to look clippy

i spent ages trying to balance it, they are some hard effects to work with

though, in my testing i've had a tonne of instances up without any lag, and my computer is nothing special

grats @ winners, fun contest, new one later today?

Well, none of the other spells, not even the more spammy effect ones, lagged for me, so I don't think it was becuase of that.

 

[~GaLs~]

Lol, pretty satisfy for my spell, as I put really less and less effort on it.

Bump Shock - Gals

* Code: 12/20
o CSData and CSSafety are ancient. They're unused, and there are better alternatives available.
o You use ABC as well as CSData and CSSafety? Seems very pointless.
o Some of your indentation is really bad. It makes it difficult to read.
o The code itself is messy, and I found it difficult to read.
o "set gg_trg_BumpShock = CreateTrigger( )" local triggers are better in this situation. At the moment, the trigger must be called BumpShock, nothing else.
o You should work in radians, instead of converting RADTODEG, then DEGTORAD.
o 'local integer max = 6' is quite useless as it could be inlined. Max should actually be a constant...
o Why are you using a local BSStruct in functinon Actions?
o struct BSConstant is utterly useless and inefficient.
o In CNedCond, the booleans could all be inlined.
o I gave up reading the code because it was so messy, that I was getting a headache.

* Eye Candy: 18/20
o Very, Very nice. I like every aspect of the eyecandy! Certainly a good job on this.
o It seems a tad overdone.

* Theme: 7/10
o Fits the icon well, though it's not the best combination

* Other: 8/10
o Lags on first cast - You could preload the effects
o It's original - I've never seen anything like this.

Total: 45/60

>>You use ABC as well as CSData and CSSafety?
I was initially using ABC, but then I decided to try out CSSafety. (As I'm first time using it)
Then, it needs CSData, so then I copy out CSData there too.
Somehow, I'm not using CSData.

>>Some of your indentation is really bad. It makes it difficult to read.
Maybe it differs from people?
I find it easier to read by this. lol

It is catagorised by spacing though.

>>"set gg_trg_BumpShock = CreateTrigger( )" local triggers are better in this situation. At the moment, the trigger must be called BumpShock, nothing else.
Why set gg_trg_BumpShock = CreateTrigger() is bad, I wonders?
Even, if I don't create a trigger for the variable gg_trg_BumpShock, it self is declared in the global block, by the Wc3Editor.
Why not make use of it but declare another 4 byte(Is it 4 byte?) of handles while leaving the previous 4 byte unused?
*Since they are not going to be destroy throughout the game.

>>You should work in radians, instead of converting RADTODEG, then DEGTORAD
I have been criticized with this matter a few times.
The problem here is, I'm not intelligence enough to know what is radians.
My school, teaches degree in the past, now, and even the future 2 years.
My local sillybus uses Degree to educate people rather than teaching the newly odd Radians. (Maybe not odd in different places )
What to do?
I don't know radians, and I can't use degree, I should be silence for deduction of marks.

>>'local integer max = 6' is quite useless as it could be inlined. Max should actually be a constant...
Yeah it should be a constant.
Constant in Jass code means value that shouldn't be changed even implemented to other people's map.

So, there shouldn't be a rule says that constant must be written in globals blocks or others...
What's wrong when I define a constant value in local? Is it guilty?

>>Why are you using a local BSStruct in functinon Actions?
What's wrong with it?
How should I declare it if I don't local it? Using globals? (It will be SUI thought)
[Single Unit Instanceable]

>>struct BSConstant is utterly useless and inefficient.
I think this matter differs from each people's mind. I think it is efficient though. xD

Why useless and inefficient?
It seperate up those must-not-be-changed variables in another struct.
Extending struct are bad? (Then why vex made it?)

>>In CNedCond, the booleans could all be inlined.
For the sake of tidyness and readability.

~GaLs~ - Bump Shock

No first-cast lag. Why are the constant inside the „BSConstants“ struct? They can be in a globals block. You should have added a constant for the max. index size in the BSStruct for the arrays. All those local booleans in the „CnedCond“ method are pointless. You don't need the „elseif“ statement in the „TargKBTimerAct“ method, the sliding actions will get ran if the first condition won't return true, anyway. Don't think you need to null timers and groups if you recycle them. The „Actions“ function is funny. You should edit the targets in the Object Editor to not allow the caster to target itself. Anyway, the code is so long that it was quite tedious to look through it. Here are some more mistakes: you don't need to convert angles to radians and then back to angles again. Currently you jsut do that, in the „chanTimerAct“ and in the „create“ method. Alot of the code can be more compact.

12/20

The usage of different special effects and lightnings make it look very cool! I especially like the orb that grows bigger in time and the swirling purge effects. Excellent!

19/20

As the icon shows lightning being summoned or conjured, I find the effects fit very well with the theme.

10/10

A knockback spell, but it was quite fun to play around with it. Wasn't boring at all, quite opposite!

8/10

>>They can be in a globals block.
I think they should not be in globals block though.
Ya know, people's who knew little, but not much in Jass.
When they're implementing some (v)Jass spells to their map, they'll straight find those configuration blocks A.K.A global blocks.
They'll try and act cool and tweak what should be changed in the block.
So I believe that placing those much-not-be-changed variable in some difference place would prevent they from touching it.
(That's my point of view though)

>>Don't think you need to null timers and groups if you recycle them.
Forgotten.

>>you don't need to convert angles to radians and then back to angles again.
Mentioned above.
I don't know radians.

>>The usage of different special effects and lightnings make it look very cool! I especially like the orb that grows bigger in time and the swirling purge effects. Excellent!
Thankz, hehe. ^^
[I'm 1 of the participient who get the highest mark in this category (19)]

>>As the icon shows lightning being summoned or conjured, I find the effects fit very well with the theme.
Thanks.
[I also get the highest mark in this category. ]

 

[Romek]

> Why not make use of it but declare another 4 byte(Is it 4 byte?) of handles while leaving the previous 4 byte unused?
If it's unused, there is no handle assigned to the variable. Though I'm clueless as to how much space an unused variable uses, I'm sure it's not a lot.
Personally, between "Make it easy for the user to change the trigger name" and "oh noes, 4 bytes or less", I'd choose the former.

> My school, teaches degree in the past, now, and even the future 2 years.
I never learned about radians in school either. A while ago, I did a quick google, and found that there are Pi radians in 180 degrees, and 2Pi radians in a full circle. That's all you need, really.

> So, there shouldn't be a rule says that constant must be written in globals blocks or others...
You can do "constant integer max = 4" and you can't do "local constant integer max = 4".
Constant globals are faster than locals, and are inlined.

I think you're misunderstanding the point of constants. They're constant variables. They cannot be changed (Note the cannot, not shouldn't) in game. Structs are basically arrays. So what you're doing, is creating a bunch of (for example) string arrays, and setting them all to the same values every time you create an instance.
If they were static, it'd be a different story. If they were constant and static, it'd be even better.

> What's wrong with it?
You're declaring a local, and setting it to a value once. And then not using it at all.
You could've easily just done "call BSStruct.create(...., ..., ..)"

> For the sake of tidyness and readability.
What happened to all those poor bytes you wanted to save? Even so, you could've used a single boolean: "local boolean b = (SomeCond)"; "set b = b and (SomeOtherCond)"; "set b = b and (YetAnotherCond)"; etc.