Holy Deliverance Aura

[Moderator]

Holy Deliverance Aura | Reviewed by Maker 17.11.2014 [COLOR="gray" [COLOR="gray" [COLOR="gray" [COLOR="gray" [COLOR="gray" Concept[/COLOR]] The concept is based on a standard aura but it has lots of triggered effects which reflects nicely on its originality Triggers[/COLOR]] The spell is easily configurable and easy to import if *boolean expression* == true then -> if *boolean expression* then GetWidgetLife(GetFilterUnit()) > 0.405 is not the best way to detect if a unit is alive. In some rare cases dead units acn have more life than that not IsUnitType(unit, UNIT_TYPE_DEAD) and GetUnitTypeId(unit) != 0 should work better The way you handle i and increment it is somewhat weird. It would be more logical to start from 0 and use exitwhen when the main action has been executed at least once HD_Setup does not need to return anything Destroy the timer that triggers HD_Setup Some of your global variable names are not so great like A, source and target You could prevent the ability from being cast on units at max life Objects[/COLOR]] No unneded objects The tooltip could be worded better There's an extra bracket and the statistics word is not needed The spell uses an animation the caster unit type does not have You could remove upgrades used from the dummy and reduce its sight radius Effects[/COLOR]] The effects blend in with each other. You chose them wisely I recommend using a custom effect for the healing when used in a custom map Rating[/COLOR]] CONCEPT TRIGGERS OBJECTS EFFECTS RATING STATUS APPROVED

 

[Vinz]

Reserve + Subscribe + Whatever.

 

[GywGod133]

:3 nyc

 

[chobibo]

call TriggerRegisterTimerEvent(t, 0., false)
call TriggerAddCondition(t, Condition(function HD_Setup))

--->

function HD_Setup takes nothing returns nothing
    //On init
    local integer i = -1
    loop
        set i = i + 1
        exitwhen i == 15
        call SetPlayerAbilityAvailable(Player(i), 'A002', false)
    endloop
    set udg_HD_Dummy = CreateUnit(Player(0), udg_Config_HD_DummyID, 0., 0., 0.)
    call UnitAddAbility(udg_HD_Dummy, udg_Config_HD_SalvationDummy)
    call DestroyTimer(GetExpiredTimer())
endfunction

call TimerStart(CreateTimer(),0.0,false,function HD_Setup)

 

[A Void]

Add a description of your spell in the main description..

 

[Tal'Rarity]

That isn't really required, this isn't a map.

Though a simple explanation would be nice I guess. Looks cool, might test later.

 

[A Void]

That isn't really required, this isn't a map.

Though a simple explanation would be nice I guess. Looks cool, might test later.

This is a map that contains a spell or a system (triggers). No, it is required to create a description. There is a rule called: Use Common Sense, in the rule section.

 

[Tal'Rarity]

This is a map that contains a spell or a system (triggers). No, it is required to create a description. There is a rule called: Use Common Sense, in the rule section.

I made quite the mistake, sorry. Forgot about one of the new rules.

"Spells must have an in-game screenshot and an adequate description."

Don't let it go to your ego, Common Sense doesn't mean you have to make a description though. There isn't any other way to show spells/systems as well.

 

[chobibo]

This is a map that contains a spell or a system (triggers). No, it is required to create a description. There is a rule called: Use Common Sense, in the rule section.

Would you please stop insulting people. That's childish...

EDIT:
@CakeMaster: Yeah, he's right. Add a description and don't forget next time.

 

[Vinz]

I was, and am still on mobile. I forgot to put the tooltip when I uploaded, so I just edited the first post. It's hard to update.

 

[A Void]

Would you please stop insulting people. That's childish...

EDIT:
@CakeMaster: Yeah, he's right. Add a description and don't forget next time.

Can you show me the part where I actually insulted him? I haven't.. I just told him that it is required to have description and quoted a rule from the rules. You can't see difference between insults and counseling? Thats your problem.

Edit: Don't spam threads in regards to me being childish or I will have to start reporting you for useless spam. If you have something to say, please private message me.

I was, and am still on mobile. I forgot to put the tooltip when I uploaded, so I just edited the first post. It's hard to update.

No problem, just don't forget to put a description. It helps people to see what they are downloading.

 

[chobibo]

I quoted it... and I can't seem to find the string "common sense" on the spell submission rules thread. I did find this one though:

• JASS code must be written to follow common JASS convention. To find out more about JASS convention, click here.

@Cakemaster: Take your time bro , but don't forget to update with the details.

 

[Heinvers]

Could you all stop this thread derailing and give advices on what can be improved in his spell? >.>

Also don't throw insults and imaginary problems and shit at each other.

 

[GywGod133]

Could you all stop this thread derailing and give advices on what can be improved in his spell? >.>

Also don't throw insults and imaginary problems and shit at each other.

>.> yeah

 

[chobibo]

The local groups g and g2 could be replaced by a single global group, that would eliminate the need to create and destroy 2 groups.

 

[Bannar]

You wanted this :S
I hope that I'm not doin all the job for judges. If I shouldn't have posted the improvements, then let me know - I'll delete this post and report it after the constest is over.

At first, as stated: call TimerStart(CreateTimer(), 0, false, function HD_Setup) instead of trigger + condition call.

Lines: call SetPlayerAbilityAvailable(Player(i), 'A002', false) and if ( GetUnitAbilityLevel(udg_target, 'B001') > 0 ) then should have thier id fields configurable.

    if GetLearnedSkill() == udg_Config_HD_Ability then
        loop
            set i = i + 1
            exitwhen i > udg_HD_Index
            if udg_HD_Caster[udg_A] == u then
                set udg_HD_Level[udg_A] = udg_HD_Level[udg_A] + 1
            else
                set c = c + 1
            endif
        endloop

Thats some bad design. You do not need two iterators for that.

        loop
            exitwhen i == udg_HD_Index

            if udg_HD_Caster[udg_A] == u then
                set udg_HD_Level[udg_A] = udg_HD_Level[udg_A] + 1
				set u = null
				return
            endif

			set i = i + 1
        endloop

With such solution you both, avoid unnecessary operations for already allocated instance and still have clear code.

SetUnitState and GetUnitState into SetWidgetLife and GetWidgetLife in all the cases. There is no excuse not to do that.

        if IsUnitInGroup(udg_target, udg_HD_Group[a]) == true then
            set udg_HD_HealCD[a] = udg_HD_HealCD[a] - udg_Config_HD_HealCDRed[udg_HD_Level[udg_A]]
        endif

        if IsUnitInGroup(udg_source, udg_HD_Group[a]) == true then
            set udg_HD_HealCD[a] = udg_HD_HealCD[a] - udg_Config_HD_HealCDRed[udg_HD_Level[udg_A]]
        endif

I believe this could be simplified to:

        if IsUnitInGroup(udg_target, udg_HD_Group[a]) or IsUnitInGroup(udg_source, udg_HD_Group[a]) then
            set udg_HD_HealCD[a] = udg_HD_HealCD[a] - udg_Config_HD_HealCDRed[udg_HD_Level[udg_A]]
        endif

Within onCast function, you do not need to create/destroy group within each iteration. Instead provide one group per call or just use global group. GroupEnum clears the group prior enumeration thus you can get rid of those function calls, without dropping safety or leaving code dirty.

In Holy_Deliverance the group issue is also present. You even added additional group while is it can be completely avoided. Get rid of create/destroy spam. Remove GroupClear too.

if IsUnitType(u, UNIT_TYPE_HERO) == true then you do not need to check if it's true. Leave the function alone, the if statement will evaluate condition properly anyway. Similar issues are seen in other lines too.
call IssueTargetOrder(udg_HD_Dummy, "bloodlust", u2)You should be really using ById equivalent here.
"u" local (Holy_Deliverance) does not need to be nulled. Also, you can easily avoid such high amount of reals within that loop function. Just reuse some of them, simplify the approach - I hope you know how to do it.

General:
Some variables are named inappropriately e.g global integer A - this is too generic.
Add proper documentation and make code actually readable - enter line breaks, check identation, don't fill every line break with comment, add like break before given comment instead.

When you fix those, we shall continue.

 

[Vinz]

Some variables are named inappropriately e.g global integer A - this is too generic.
Add proper documentation and make code actually readable - enter line breaks, check identation, don't fill every line break with comment, add like break before given comment instead.

It's a temp variable anyways.

Thanks for the review, appreciated.

Though I do not think I will be able to fix this anytime soon due to my PC needing a repair.

 

[Deleted member 219079]

I wonder when poll comes up...

 

[Vinz]

When we've died of old age.

 

[Deleted member 219079]

Lol btw how does your spell handles hero respawning? Does it give him the aura back?

Edit:
You should check out my tutorial's chapter 5 to get your loops as efficient as possible.

You exit the loop after there is no units in your temp groups g, g2. So, you don't need to null FoG unit as it's null to begin with, secondly you could use global group so you can avoid 2 native calls per temp group. You can see example at my tut's "5.5 : Use variables efficiently".

The tutorial's not approved yet but it's good. There's only one tutorial mod and I wouldn't want to bother him with reviewing the tut again, but yeah I've put a lot to it...

Edit 2:

Add proper documentation and make code actually readable - enter line breaks, check identation, don't fill every line break with comment, add like break before given comment instead.

The code looks readable to me (that's maybe because I write messy code myself lolol ), expect for function names. But Bannar is right you should make it even more readable; the code readability affects the rating of your resource

 

[Vinz]

I'm not sure if I understand you, but it checks if the hero is alive every run and proceeds if so.

 

[Bannar]

Poll comes on Wednesday.

 

[Vinz]

Updated.

 

[yram44]

Hi Mythic your spells are always awesome. I really love your work!

 

[Wasabee]

Hello would this create problems if I use a different damage engine?