Blessed Shield V1.11

[Moderator]

12th Dec 2015
IcemanBo: Too long time as NeedsFix. Rejected.

12:15, 23rd Jul 2010
TriggerHappy:

I would prefer that your level support supported more than three levels. Make some equation for each. Also, indent.

 

[watermelon_1234]

I have only checked the spell's code on this site.

Code Stuff:

• Use GetWidgetLife(YourUnit)/SetWidgetLife(YourUnit,NewLife) instead of GetUnitState(YourUnit,UNIT_STATE_LIFE)/SetUnitState(YourUnit,UNIT_STATE_LIFE,NewLife)
  It's a bit faster.
• You use GetTriggerUnit so many times in function Remove and add that it would be better to use a local variable for that.
• You should disable the BOOKID ability for all players in the Init function.
• Take

  call TimerStart(t,COOLDOWN,false,function callback)

  out of that big if-then-else block and put it last since it will always be called regardless of the condition.
• Probably should try preloading BOOKID otherwise there will be a bit of lag upon the first time the ability is added.

Other:

• Indenting is a bit strange.
• Explanation at times was a bit awkward, but at least there's documentation. ;D

I will edit this post once I test this in-game.

Okay I tested it. The visual effects are a bit lacking. It might look better to use the Defend sfx instead of Divine Shield. (Well it can be changed so it's just my opinion.)

You should have multi-level support for this spell in case people would want to use this as a hero spell. That means making more of your configurables scale up with level. =P

This is just about the test-map: You should probably show that tip only once at map init.
You should also make it easier to test the spell by making the Paladin have less than 50% life at the start. (It's not that bad though since he can be instantly healed anyway)

Good luck on vJassing! =D

 

[busterkomo]

• The indenting is terrible, which makes some parts difficult to read.
• In your 'cons' function, the variable isn't needed.
• Instead of Set/GetUnitState use Get/SetWidgetLife.
• Store GetTriggerUnit() in a variable.
• This:

      if CHARGECHANCE > 0 then
          if CHARGECHANCE < rand then
              set currentcharges[GetUnitId(GetTriggerUnit())] = currentcharges[GetUnitId(GetTriggerUnit())] - 1
          endif
      else
          set currentcharges[GetUnitId(GetTriggerUnit())] = currentcharges[GetUnitId(GetTriggerUnit())] - 1
      endif

  can be replaced with this:

      if not (CHARGECHANCE > 0 and CHARGECHANCE > rand) then
          set currentcharges[GetUnitId(GetTriggerUnit())] = currentcharges[GetUnitId(GetTriggerUnit())] - 1
      endif

• Start the outside the if statement.
• Your 'chleft' variable is no needed.

 

[Lambdadelta]

Ergh. I have serious problems with your spell.

THREAD

Hello This is my first spell I post.

Name: Blessed Shield
Code: VJass
Leakless: Think so
Lagless: Should be
Mui: Yes
Customizable: Yes
Discription: When the unit that has this spell gets taken under A% of his total health he has a B% chance to gain C amount of charges that will reduce the damage taken of the next C attacks by D%. Every time you take damage there is a E% chance a charge is removed and the damag reduction is lowered by F%. This effect cannot occure more then once every G seconds.
All capital letters are customizable.
Requires: Damage from J4L, AIDS from J4L, Event from J4L and TimerUtils from Vexorian
all systems are in the demo map

• The "Name" is redundant, all of us can see it from the top of the page.
• Code: vJass, is once again, redundant, firstly, we can see it at the top, and it's not VJass but rather vJASS or vJass.
• Leakless and lagless, MUI should not be even mentioned. Leakless and MUI is what spell code reviewers should automatically assume.
  
  

  "Spells must be free of bugs, leaks, unreasonable lag, and must be fully multiinstanceable."

• "Customizable: Yes" - once again, useless.
  

  Spells must be easy to configure (change their stats) and implement into a map. Documentation must be provided if the spell is not implemented with a simple copy and paste.

• Description should be made easier to read. Right now, it's a wall of text.
• Requirements should be linked, Jesus4Lyf's systems aren't on TheHiveworkshop, so link them.
• "all systems are in the demo map" - obviously. How else would your spell demo function?

Some points are already mentioned above so ignore if so.

DEMO MAP

• The map name should not be "Just another Warcraft III map", it should be "Blessed Shield v1.00" or something presentable.
• Footman are so weak. Add some option to make seeing the map in action better, perhaps stronger units, or a trigger that damages your hero to enable it.
• Spell tooltip is terrible.
  
  

  "When taken under 50% of your max life you will gain 5 blessed shield charges wich will reduce the damage by 100%. Everytime you take damage a charge is removed and the next time you will reduce 10% less damage!"

  
  Should be something to this effect:
  
  

  "When the Paladin's health drops below 50%, he will gain 5 blessed shield charges w[h]ich will reduce future damage. Everytime you recieve damage, a charge is loss and the shield blocks 10% less damage."

• The ability should be a learnable hero ability, with proper tooltips, correct button position (since this is only one ability, (0,2) works fine. Perhaps find a more suitable icon that has a passive border and a learnable bordered icon.
  
• I checked your abilities, you DO realize that Damage and AIDS both require an ability, it's even in the import instructions of them. Allow the objectMerger script to be run... I don't see them anywhere.

CODING

Here is what your header should like like (not exactly, but keep in mind presentation). //\\//\\//\\//\\//\\//\\//\\ looks exceptionally ugly.'

Your configuration is also ugly. You should have lines to seperate them and categorise them, rawcodes should be kept at the top if possible (unless there is some othe reason), and //comment comments should have spaces between the comment delimiter and the actual comment. // comment.

//+-----------------------------------------------------------------------------------------------+
//| BLESSED SHIELD v1.00
//| ¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯
//| Description
//| ¯¯¯¯¯¯¯¯¯¯¯
//| Passive effect that causes the hero to recieve charges when health drops below
//| 50%. Charges will reduce future damage. (Whatever).
//|
//| Requires
//| ¯¯¯¯¯¯¯¯
//| - AIDS by Jesus4Lyf
//| - Damage by Jesus4Lyf
//|   (Event by Jesus4Lyf, as a requirement from Damage)
//| - TimerUtils by Vexorian
//|
//| Credits
//| ¯¯¯¯¯¯¯
//| Romek for helping - blah.
//| Bribe for helping - blah.
//| 
//| How to import
//| ¯¯¯¯¯¯¯¯¯¯¯¯¯
//| Copy this trigger into your map.
//| Copy the "Systems" folder into your map.
//| Go to AIDS and Damage, follow their implementation steps regarding the object ability.
//| 		OR
//| Copy the abilities from this map to your map. Be sure they keep the saw Rawcode.
//|
//+-----------------------------------------------------------------------------------------------+

Ignore any points already mentioned but:

1) This is redundant. You are checking if it is 0. Then resetting it to 0.

.
    if currentcharges[GetUnitId(GetTriggerUnit())] == 0 then
        set currentcharges[GetUnitId(GetTriggerUnit())] = 0

2) Loop through every player onInit and disable it there to save any calls.

call SetPlayerAbilityAvailable(GetOwningPlayer(GetTriggerUnit()), BOOKID, false)

3) You can just use if STACK then

if STACK == true then

Although it's not a big deal.

4) You are using up timers if the if block in function add is not met, and taking up structs as you don't destroy them.

5) You can inline this.

local integer rand = GetRandomInt(1,100)

6) Your function names are terrible, they should be initial capitals and the functions should be grouped. I'm going to post an improved version of your code soon.

7) Rather than have an array, use an AIDS struct, for that is what it was made for, and intended to be used like that.

.
        private integer array currentcharges 
        //to keep track of the current unit charges 
        private boolean array cd

8) Your globals are hard to understand O.O?

THIS is how your code is supposed to look like.

Erm. WTF does TheHiveworkshop jass tags do to the coding? It makes the spacing all messed up

My Scripting

scope BlessedShield initializer Init // requires AIDS, Damage, TimerUtils
//+-----------------------------------------------------------------------------------------------+
//| BLESSED SHIELD v1.00 by dudeim
//| ¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯    ¯¯¯¯¯¯
//| Description
//| ¯¯¯¯¯¯¯¯¯¯¯
//| Passive effect that causes the hero to recieve charges when health drops below
//| 50%. Charges will reduce future damage. (Whatever).
//|
//| Requires
//| ¯¯¯¯¯¯¯¯
//| - AIDS by Jesus4Lyf
//| - Damage by Jesus4Lyf
//|   (Event by Jesus4Lyf, as a requirement from Damage)
//| - TimerUtils by Vexorian
//|
//| Credits
//| ¯¯¯¯¯¯¯
//| Romek for helping - blah.
//| Bribe for helping - blah.
//| 
//| How to import
//| ¯¯¯¯¯¯¯¯¯¯¯¯¯
//| Copy this trigger into your map.
//| Copy the "Systems" folder into your map.
//| Go to AIDS and Damage, follow their implementation steps regarding the object ability.
//| 		OR
//| Copy the abilities from this map to your map. Be sure they keep the saw Rawcode.
//|
//+-----------------------------------------------------------------------------------------------+

globals
//************************************************************************************************
//*								    CONFIGURABLES SECTION										 *
//************************************************************************************************

    private constant integer 	 AID 				= 'A000'	// Rawcodes should take priority, be at the top.
    private constant integer    BOOK_ID         	= 'A001'    // Rawcode of the spellbook, contains the aura which provides the visual buff.

																// Should be made into a function that takes a level. But anyways:
	private constant real 		LIFE_PERCENT		= 50.00		// The unit's health must match this or below for the ability to												
	private constant real		ACTIVATE_PERCENT	= 100.00	// Chance that you will gain a charge.
    private constant real       DMG_REDUCTION       = 100.00	// Percent that the damage is reduced by.
    
    private constant integer    CHARGES         	= 5			// How many instances are blocked.
    private constant integer    KEEP_CHARGE_PERCENT = 0 		// There is a 0% chance the unit will not lose a charge.
    private constant integer    MAX_CHARGES     	= 5			// How many charges can you keep? Useless I think?
    private constant real       CHARGE_REDUCTION    = 0		    // You lose 10% damage reduction for each charge loss.
																// 4 charges = 90% reduction
																// 3 charges = 80% reduction
    
    private constant boolean    STACK           	= false		// If you already have charges and you take damage,
                                                                // will the charges stack or be set to max.

    private constant string     BLOCK_SFX       	= "Abilities\\Spells\\Human\\DivineShield\\DivineShieldTarget.mdl" 
    private constant string     BLOCK_SFX_LOC   	= "origin"

    private constant real       COOLDOWN        	= 0.00
endglobals
//************************************************************************************************
//*							THE ABILITY BEGINS HERE - DO NOT TOUCH								 *
//************************************************************************************************

	globals
		private unit tempUnit 	    = null					// global unit used for filter
		private real sys_PERCENT = LIFE_PERCENT * 0.01	// Just so you don't need to calculate on every damage instance.
	endglobals

    // USE AIDS LIKE IT IS MEANT TO !
	private struct UnitData extends array
		//! runtextmacro AIDS()
		boolean inCooldown
		integer charges
		timer	timer
		
		private static method AIDS_filter takes unit u returns boolean
			return GetUnitAbilityLevel( u, 'Aloc' ) == 0
		endmethod
		
		private method AIDS_onCreate takes nothing returns nothing
			set this.inCooldown = false
			set this.charges	= 0
			set this.timer		= null
		endmethod
	endstruct

private function BlockDamage takes nothing returns boolean
    local UnitData data  = UnitData[GetTriggerUnit()]
    local integer remainingCharges
    local real    reduction
    
    if data.charges > 0 then
        set remainingCharges = CHARGES - data.charges
        
        call DestroyEffect(AddSpecialEffectTarget( BLOCK_SFX, data.unit, BLOCK_SFX_LOC) )
        if CHARGE_REDUCTION > 0 then
            set reduction = DMG_REDUCTION - (remainingCharges * CHARGE_REDUCTION)
        else
            set reduction = DMG_REDUCTION
        endif

		// 0% chance that you will not lose a charge.
		if KEEP_CHARGE_PERCENT < GetRandomInt( 0, 100 ) then
			set data.charges = data.charges - 1
		endif

        if data.charges == 0 then
            call UnitRemoveAbility( data.unit, BOOK_ID )
        endif
        
        call Damage_Block( GetEventDamage() * (reduction / 100 ) )
    endif
    
    return false
endfunction

// "callback" function. Where cooldown is placed.
private function ResetCooldown takes nothing returns nothing
    local UnitData data = GetTimerData( GetExpiredTimer() )
    set data.inCooldown = false
    call ReleaseTimer( data.timer )
endfunction

// Cleaned up a lot in here.
private function ActivateAbility takes nothing returns nothing
	local UnitData 	data = UnitData[GetTriggerUnit()]
	
	if ACTIVATE_PERCENT >= GetRandomInt( 0, 100 ) and data.inCooldown == false then
        set data.inCooldown = true
		if data.charges == 0 then
			call UnitAddAbility( data.unit, BOOK_ID )
		endif
		
        // STACK is a constant boolean. So we use a static if.
		static if STACK then
			// If max charges allowed is greater than 0.
			set data.charges = data.charges + CHARGES
		else
			set data.charges = CHARGES
		endif
		
        if data.timer == null then
            set data.timer = NewTimer()
        endif
		
		call SetTimerData( data.timer, data )
		call TimerStart( data.timer, COOLDOWN, false, function ResetCooldown )
	endif
endfunction

// GetTriggerUnit() was used more than once. Set a global unit (local will leak and will look messy if a local
// boolean is used) to it. lifePercent could be inlined, but it looks cleaner without doing so.
private function AbilityCheckCondition takes nothing returns boolean
	local real lifePercent
	set tempUnit = GetTriggerUnit()
    set lifePercent = GetUnitState( tempUnit, UNIT_STATE_LIFE ) / GetUnitState( tempUnit, UNIT_STATE_MAX_LIFE )
    return 	GetUnitAbilityLevel(tempUnit, AID) > 0 and lifePercent <= sys_PERCENT				
endfunction

private function Init takes nothing returns nothing
    // We don't need t and t2, we can just do it like so.
	local trigger t = CreateTrigger()
    local integer i = 0
    call Damage_RegisterEvent( t )
	call TriggerAddCondition(t, function AbilityCheckCondition )
	call TriggerAddAction(t, function ActivateAbility )
	
	set t = CreateTrigger()
    call Damage_RegisterEvent(t)
	call TriggerAddCondition(t, function BlockDamage)
    
    // Spellbook ID is disabled onInit rather than while the spell takes place.
    loop
        call SetPlayerAbilityAvailable( Player(i), BOOK_ID, false )
        set i = i + 1
        exitwhen i == bj_MAX_PLAYER_SLOTS
    endloop
endfunction

endscope

There are far more things I could comment about.

You shouldn't have posted this spell here in my opinion, you should have posted it in the triggering section to get feedback 'for your first spell'. I don't think it is going to be approved in it's current state - simply fixing everything mentioned won't work. I would take this into consideration, and make sure your next spell is presented at higher quality.

 

[dudeim]

Hmmm ok thanks guys will certainly work on it
Never understood the aids struct thing so didn't use it.
About the timer in the if block I first thought that if the percentage wasn't so high the the spell triggered the cooldown also didn't triggered but this is better I think otherwise the next hit would've triggered it.
@watermelon
why would I make a variable of triggering unit? it's kinda useless I'd say as it's already in some sort of variable why should I make a variable of it and use that instead? < One part I didn't really get
Thanks guys for the critisism will sure help me on my way!

 

[Lambdadelta]

why would I make a variable of triggering unit? it's kinda useless I'd say as it's already in some sort of variable why should I make a variable of it and use that instead? < One part I didn't really get

GetTriggerUnit() is not a variable. It's a function. Setting it to a variable is what I mean't. For effiency's sake.

 

[dudeim]

Does is really matter for efficienty? didn't know that

Updated to V1.1
Features updated code thanks to xBlackRose
The spell is now level based

 

[Lambdadelta]

Does is really matter for efficienty? didn't know that

Should a car use lots of petrol for no reason?

 

[dudeim]

aah wait I get it now I think as it's a function GetTriggerUnit() everytime I use it I call a function and if I use it as a variable I only call it once so it's more efficient thanks

 

[watermelon_1234]

Okay you updated it.

• The way you supported levels in your abilities is inefficient. You should have either used a global array or a function that does calculations like
  

  private constant function Cooldown takes integer lvl returns real
      return 10.5 - 0.5*lvl
  endfunction

• I'm pretty sure you should name the timer variable in your struct to something other than timer or else that will conflict. Also, data.unit? (Then again, that was from xBlackRose.)
• You don't need to null the timer if you use TimerUtils as the timer just gets recycled. That also makes your null check useless.
• Like hvo-busterkomo and I mentioned, "Instead of Set/GetUnitState use Get/SetWidgetLife."

 

[Diehard@Azeroth]

I dont know who the hell put that 1/5 rating; thats just absurd.. anyways.

The code is pretty good cus you are incorporating external libraries in it + doing traditional vJASS.

I think description should be changed, and ofc the coding improvements above should be made.

More functionality should be added and pfft thats about it i think?

gj on the spell mate

 

[dudeim]

The level functions i did where there for easy change so anyone with absolutly no skill in math or anything could easily change em and about the global array how would I do that? Where do I make it editable for each level then? Same way using a formula? Will change the names of the unit and timer variable and will use get/setwidgetlife. And the data.unit it refers to the unit wich the struct instance is about so no need to change that.

@up the first version of the spell lacked alot of things that was why the rating was 1/5 thanks to xblackrose it was updated alot!

V1.11 is up!

 

[Septimus]

aah wait I get it now I think as it's a function GetTriggerUnit() everytime I use it I call a function and if I use it as a variable I only call it once so it's more efficient thanks

you got it

 

[watermelon_1234]

The level functions i did where there for easy change so anyone with absolutly no skill in math or anything could easily change em and about the global array how would I do that?

I like math so I like using functions instead. (Besides, the way you set up your level support was pretty much the same as using multiple ifs...)

Anyway, here's how you would do it if you use a global array variable.

globals
    private real array Cooldown
endglobals

...

private function SetCooldown takes nothing returns nothing //Needs to be called at Init.
    set Cooldown[1] = 10.
    set Cooldown[2] = 9.5
    set Cooldown[3] = 9.
endfunction

And you would refer to Cooldown in your code like so:
Cooldown[GetUnitAbilityLevel...]

 

[dudeim]

Aah ok I get it

 

[Lambdadelta]

I'm pretty sure you should name the timer variable in your struct to something other than timer or else that will conflict. Also, data.unit? (Then again, that was from xBlackRose.)

I didn't intend for him to copy it exactly. And, what's wrong with data.unit?

The code is pretty good cus you are incorporating external libraries in it + doing traditional vJASS.

That makes no sense. So as long as I use 'external' libraries, it is considered good code? And what is traditional vJASS

Also, link all required libraries...

 

[dudeim]

Added links to the required systems

 

[watermelon_1234]

And, what's wrong with data.unit?

I just thought it would conflict since isn't there already unit? It seems like I was mistaken then.

Edit: Emphasis on the last statement.
To the post below, if he ever reads this again: I'm sorry that I didn't experiment with it, which I should have before saying something. However, I don't need someone yelling at me after I admitted I was wrong. >_>

 

[Prince.Zero]

I just thought it would conflict since isn't there already unit? It seems like I was mistaken then.

How the heck does

unit

stack with

data.unit

?
pff....

*/facepalm

unit is a reference of

type unit extends handle

data.unit is a syntax of a struct called data.
we don't see that in JASS but we do in vJASS

This doesn't look like a system, it's more like a spell.
And it needs to be configurable!!! Also why the use of soooo many external libraries? I mean come on, i wouldn't use so many external libraries for just 1 single simple spell. -.-

 

[Lambdadelta]

This doesn't look like a system, it's more like a spell.
And it needs to be configurable!!! Also why the use of soooo many external libraries? I mean come on, i wouldn't use so many external libraries for just 1 single simple spell. -.

• It's not a system, it is a spell?
• It is configurable.
• You might not use so many vJass libraries, but others will. 3 is nothing. AIDS is a common indexing system, Damage is as well, it saves the author from having to code the damage detection himself. Why do what other's have done for you? And TimerUtils to attach the timer data. Hashtables are just damn plain ugly.