NovaSystem 2.02 Reforged

[Wrda]

The API really looks like it was made to kill all users who are about to use this system. Man, the function doesn't even fit one line, at the 5th parameter onwards you won't remeber what the next one is. You need to be reminded that you're sharing this for humans, not machines, and this isn't really readable
It's just too god damn long, so I'd suggest to start from there.

 

[Dark-Zalor]

You can post the trigger code into this:

This is a code section

If you want some help or feedbacks
btw it's a nice idea to create base spells code. That will help for sure!

 

[Wrda]

Spoiler: Critical

/*                real x - the center x of the nova AOE                                                     
                                                                                          
                real y - the center y of the nova AOE */

It seems to me if "x" and "y" were renamed to "centerx" and "centery" or some variant (uppercase x and y) would also make it more intuitive.

        static method simpleCreate takes unit caster,unit target,real x, real y, real faoe, real height,/*
        */ real damage, real scale, string path, attacktype at, damagetype dt,integer abil returns thistype
            return thistype.create(caster,null, x, y, 0.0, faoe, height, 1, 1, 0.0, 0.0,damage, scale, false, false, false, path, at, dt, TRUE,abil)
        endmethod
      
        static method simpleCreateDelay takes unit caster,real x, real y, real delay, real faoe, real height,/*
        */ real damage, real scale, string path, attacktype at, damagetype dt,integer abil returns thistype
            return thistype.create(caster,null, x, y, 0.0, faoe, height, 1, 1, delay, delay,damage, scale, false, false, false, path, at, dt, TRUE,abil)
        endmethod
      
        static method simpleCreateDelayTarget takes unit caster, unit target,real x, real y, real delay, real faoe, real height,/*
        */ real damage, real scale, string path, attacktype at, damagetype dt,integer abil returns thistype
            return thistype.create(caster,target, x, y, 0.0, faoe, height, 1, 1, delay, delay,damage, scale, false, false, false, path, at, dt, TRUE,abil)
        endmethod

Why does simpleCreate method has "target" as parameter when it is unused currently? Why does simpleCreateDelay and simpleCreateDelayTarget use the same "delay" parameter in 2 different arguments? The first one is interval between each nova creations, the second is the duration of the spell. So you should just add another parameter "dur" or "duration" instead of the idea of "1-time Nova".
Still, this is the part where these 3 methods, and the create method itself should be reworked. create method should provide the most basic parameters, keeping it simple, with default values:

Spoiler: Methods

static method create takes unit caster, unit target, real x, real y, boolean circular, integer abil returns thistype
    //etc.
endmethod

method setNovaProperties takes real aoe, real faoe,  boolean oncast, boolean channel,  boolean follow return nothing
    //this.aoe = aoe
    //etc.
endmethod

method setNovaEffects takes string path, real height, real scale, integer npil, integer npih returns nothing
    //idem
endmethod

method setNovaDamage takes real ntt, real ni, real damage, attacktype at, damagetype dt returns nothing
    //idem
endmethod

This way, you would warn users to use these functions after creating an instance. It's clearer to use than having 20 arguments on the same function.
You should make the details on the struct members instead on the parameter section since not all members are used there. Some names suffer semiotic issues, in order words, they have no direct meaning. "ni", "ntt", "nxi" give no clue what it is about, only by looking at the explanation or examining the code one understands the meaning. It would be the same thing if you had members as "a" for caster unit and "b" for target unit, not intuitive at all. "aoe and "faoe" renamed to "maxAOE" and "novaAOE" or another variant would do.

call GroupEnumUnitsInRange(NOVA_GROUP, xx, yy, this.faoe, Condition(function NovaSystem.Looper)

Condition(function NovaSystem.Looper) is a leak. Need to destroy boolexpr later.

There should be a warning to the user to comment out the UnitAlive native declaration if they already have it somewhere else.

Spoiler: nitpicking indention

        //The group loop that is run when a nova is created
        static method Looper takes nothing returns boolean
            set filter = GetFilterUnit()
            if onHitTable.handle.has(data.abil) then
              call TriggerEvaluate(onHitTable.trigger[data.abil])
            endif
            if data.OnHitFilter(filter) then
                call UnitDamageTarget(data.caster, filter, data.damage, false, false, data.at, data.dt, null)
                if onDamageTable.handle.has(data.abil) then
            set damagetarget=filter
                    call TriggerEvaluate(onDamageTable.trigger[data.abil])
                endif
            endif
            return false
        endmethod

A little indention problem.

GetWidgetLife(u) >= .405 seems a redundant check, since UnitAlive already does that check internally.
Why is BuffNova a point target ability while it's supposed to target friendly hero units?
I don't understand the example of this spell. It almost feels like an instant ability, and doesn't look like it has anything to do with a nova. Perhaps it's attempting to demonstrate "1)An instant Nova"? But then, what's the difference between an instant nova and a fully, manually triggered spell? I'm curious to see what "7)And some others" could be. The other demo spells look nice.

Overall, it is good but need some fixes.