[Spell] Bane of Evil

[MyPad]

@Cokemonkey11 Any input on this?

 

[Chaosy]

This is minor, but you do not need @configurable before every line, especially since there are no exceptions.

It would be less bloat to juse
@configurable
// vars here
@endconfigurable

 

[Cokemonkey11]

What is `Missile` really for? Should it be in the standard library? Is it a dependency with its own git uri?

for comparing distance between points, prefer distanceToSq when appropriate

`KNOCKBACK_ANGLE_AIR` what are the units?

Code seems fine overall. @Frotty FYI

 

[MyPad]

Spell Review:

Spell Description:

• It might be helpful to point out the target type of the ability, namely either a unit target, point target, both, or instant (no target).
• Description on what the ability does can be improved, such as the amount of damage dealt, knockback strength, cooldown, etc.

Spell Mechanics:

• Pertaining to the first point raised in the Spell Description, the target type of the spell is point-targeted. This has not been referenced in the description as to what the spell does. (in HIVE) (Malus of 0.15 points)

Spell Logic:

• Quoting the appropriate reviewers for Wurst resources:
  
  
• CokemonKey11:
  
  

  What is `Missile` really for? Should it be in the standard library? Is it a dependency with its own git uri?
  for comparing distance between points, prefer distanceToSq when appropriate
  `KNOCKBACK_ANGLE_AIR` what are the units?

  
  Calculated total malus points by MyPad based on the quote above: (0.45 points)

Rating:

4.4/5.0
​

 

[Frotty]

> The reason I am uploading this is because the number of examples for Wurst is scarce.

Nice Many people said they would make spells and more, but never did!

The wurstbin links could be a bit more prevalent. At first I thought they were resource links or something else.
Also, as coke said, should probably make a git repo so this spells can be imported as dependency?

The "setup" snippet should probably contain a package definition. Also changing the gravity might not be ideal if people just copy it into their maps.

This is minor, but you do not need @configurable before every line, especially since there are no exceptions.

It would be less bloat to juse
@configurable
// vars here
@endconfigurable

I think you are mixing something up. The @configurable annotation has actual purpose in wurst, indicating which variables can be configured, otherwise they will throw a warning.

@MyPad

I would suggest waiting at least a few days for a response from OP before final review? Seems a bit early to me, when feedback has just been provided.

 

[Chaosy]

@Frotty
Then just change the annotation to "config" or something I suppose
Unless I am completely wrong and annotations are not glorified comments (I know compiletime has functionality but does configurable actually do something?)

Me personally, I would rather do:
// Config
stuff
// End Config

Rather than having 20 configurable keywords, to me it is too much bloat, especially in this case where every single variable is used for config so there is no confusion regarding what to change and not.

 

[stn]

Update: added a new version that is more tailored towards the style that is used in the library packages.

Also:

• Removed the @configurable part
• Added units for KNOCKBACK_ANGLE_AIR (degrees)
• Made wurstbin links more visible
• Added a link to a Github repository containing the relevant files
• Added an installation instruction

That's it for now.

 

[Frotty]

@stn

Good job so far
Few notes:

• Shouldn't master be the recent version and old on branch? Otherwise no way to pull via setup tool i think.
  
• In Missile instead of private static let clock = CreateTimer() You could just use the TimedLoopModule
• In Missile private unit u must be initialized "= null" otherwise recycled objects could have residual references
• Don't write static function create when you can just use constructors. They exist for a reason.
• You are still using distanceTo when you could use distanceToSq from what I see
• Often you don't use type inference, prefer let and var
• In general brush up on coding conventions: WurstScript • Manual
• An improvement could be to add multiple levels and use Level-Closures for the config variables

Added units for KNOCKBACK_ANGLE_AIR (degrees)

You misunderstood - the config variable should be of type angle, e.g. `0 .fromDeg()` by default.
Then the user also has to supply an angle, thus avoiding any possible confusion between degrees/radians.
Right now you still have the problem that an angle in radians will produce unexpected results.

Cheers

Then just change the annotation to "config" or something I suppose
Unless I am completely wrong and annotations

@config is used to overwrite @configurable variables. Otherwise it will throw a warning.

 

[Frotty]

Very nice! Looks close to best practice now
A few things still stand imho:

• Keep newest version on master branch of repo so it can be used as dependency more easily
• Hide older version pastes in a spoiler

Presentation-wise a gif showing the spell in action would be cool and you should add an image of the generated tooltip.
I can provide the gif.

Cheers

edit:
maybe a few mroe things

• show AoE target image (it's option on ChannelPreset) I expected the arrow to go max distance.
• Improve the test map to add terrain height differences, cliffs, trees. It will be good to see the arrow interact with it

Thanks again

 

[MyPad]

Want to take over, @Sir Moriarty?

 

[Lizreu]

Want to take over, @Sir Moriarty?

Sure. I'll write a review during the weekend, then.

 

[Lizreu]

Spell Review:
Nice spell. Demonstrates very well how easy it is to make a spell like this using Wurst.
One small nitpick: the missile has a maximum range, but it only travels up to the cast point. I think it'd make more sense if it travelled its entire maximum length regardless of the cast point.

Code Review:
I think this is ready for approval, just a few things that I'd like changed:

1. In `BaneOfEvil_config.wurst`, change `let` bindings to `constant` bindings.
2. In `BaneOfEvil.wurst`:
   Change
   

   let missile = new Missile(FX_MISSILE, caster.getPos3Fly(), target)
   missile
   ..setSpeed(MISSILE_SPEED)
   ..setHeight(MISSILE_HEIGHT)
   ..setScale(FX_MISSILE_SCALE_BASE + FX_MISSILE_SCALE_LVL * caster.getAbilityLevel(abilId))
   ..setTrail(FX_TRAIL)
   ..setTrailAttach(FX_TRAIL_ATTACH)
   ..onHit(-> explode(caster, target))

   to
   

   new Missile(FX_MISSILE, caster.getPos3Fly(), target)
   ..setSpeed(MISSILE_SPEED)
   ..setHeight(MISSILE_HEIGHT)
   ..setScale(FX_MISSILE_SCALE_BASE + FX_MISSILE_SCALE_LVL * caster.getAbilityLevel(abilId))
   ..setTrail(FX_TRAIL)
   ..setTrailAttach(FX_TRAIL_ATTACH)
   ..onHit() ->
       explode(caster, target)

   This is slightly more idiomatic.
   
   Change
   

   static let abilId = compiletime(ABIL_ID_GEN.next())

   to
   

   static constant ABIL_ID = compiletime(ABIL_ID_GEN.next())

3. In `Missile.wurst`
   
   Make sure to destroy the callback object in `ondestroy`:
   
   

       ondestroy
           art.destr()
           cb.callback()
           destroy cb

   
   Not doing so will cause a class instance leak for the closure object.
   
   Use `real.atan2(y)` vs using the Atan2 native directly.

Status:
Awaiting Update

 

[stn]

In `BaneOfEvil_config.wurst`, change `let` bindings to `constant` bindings.

What's the idea behind this?

 

[Frotty]

The idea is that the convention is to use 'constant' for globals, but let is okay too, they are functionally the same.

 

[stn]

One small nitpick: the missile has a maximum range, but it only travels up to the cast point. I think it'd make more sense if it travelled its entire maximum length regardless of the cast point.

The aim is to have control over the position where it lands (a target image has been added).

Other than that the code has been changed according to your comments. Also, atan2 has been replaced with getAngle.

 

[MyPad]

So, @Frotty, is this ready for approval?

 

[Frotty]

Probably is.

Few more things

• Don't hide repo link behind "relevant files" so one can more easily use package manager to install it, e.g. `grill install StN-NL/wurstscript-spell-bane-of-evil`
• AbilityToooltipGenerator should probably just be a dependency instead of copy pasted into this repo
• The repo should be a valid wurst project, not just map & .wurst files
• Change "DefaultName" of example map and don't just upload "WurstRunMap"

These steps:

Should be "Either use `grill install`or the WurstSetup to update the downloaded project".
Also why is there only "Test" and "Edit" ? Should be "Use" via wurst.build dependency first and foremost.

e: That actually brings up another problem. "BaneOfEvil_config.wurst" is not a config package, yet named _config. The actual _config package would be in the user's project, configuring your "Config" package which should have variables annotated @configurable.

 

[MyPad]

There was an adventurer who explored dangerous terrains, encountered loathsome beasts and unspeakable horrors of old and lived to tell the tale, only to get knocked up by some Ranger with a fixation for genocidal princes. There he lies to this day, unable to walk properly again, as the pain torments him and reminds him of the humiliation he encountered at the Ranger's hands.

A word of Caution:
As opposed to the conventional way of importing spells (via object editor and trigger editor copypasta), importing the spell into your map requires a bit more care on the user's end. Please read the import instructions carefully.

 

[Frotty]

wat?