[GUI] Omnislash Template

[MyPad]

If there is something wrong with the resource, feel free to tell me.

EDIT: (July 29, 2017)

Removed Bump and first edit!

 

[Reventhous]

I haven't test your system to the fullest yet, but here's my quick suggestions for the next update.

-Please do something with the code, its a bit messy.
-Add some configuration.
-Instead using multiple timer, use integer as countdown for each interval.
-Replace the local group g with global variable.
-null the local variables just before return or endfunction

 

[MyPad]

-Please do something with the code, its a bit messy.

Which part of the code are you pertaining to? Is it the one with the pf__ prefixes and sm__ prefixes or is it in the functions that start with neither?

-Add some configuration.

Configuration, you say? Could you further elaborate on this?

-Instead using multiple timer, use integer as countdown for each interval.

- Don't worry about the timers; it is done internally. About the integer being used as countdown, I would like you to further expand on your suggestion.

-Replace the local group g with global variable.

Done!

-null the local variables just before return or endfunction

I guess you caught some unnulled variables. If it is about the local unit enum_unit, don't worry about it; the FirstOfGroup native will get the first unit and GroupRemoveUnit will remove that unit from the group, effectively setting the first unit to the second unit in the group.

I'll elaborate on this later..

 

[Reventhous]

Which part of the code are you pertaining to? Is it the one with the pf__ prefixes and sm__ prefixes or is it in the functions that start with neither?

Just put some space between the code line. Like this,

function RequestOmnislash takes unit u, unit t, real timeout, real damage, real range, integer i, boolean b returns integer
    local integer this = sm__Omnislash__allocate()
    local group g
    local unit enum_u

    call sm__Omnislash__push(this)

    set udg_omni_Caster[this] = u
    set udg_omni_Target[this] = t
    set udg_omni_Timer[this] = CreateTimer()
    set udg_omni_Interval[this] = timeout
    set udg_omni_Damage[this] = 0
    set udg_omni_Range[this] = range

    if b then

Configuration, you say? Could you further elaborate on this?

Anything that is constant in your code should be configurable. Likes unit filter, attack type, damage type, ...

- Don't worry about the timers; it is done internally. About the integer being used as countdown, I would like you to further expand on your suggestion.

I'm talking about performance, it is something to worry about.
Create a timer when the game start, when there is a index exist, start the timer and pause when there is no index on the list. So, your system will no longer needs to constantly create and destroy timer.

About the integer as countdown, this method is for systems that uses a single timer. Actually you can also use real but I prefer integer.
Its works like this

Create a integer array variable "omni_Countdown
Create a integer array variable "omni_MaxCount
Create a integer array variable "omni_CurrentTick
Create a integer array variable "omni_MaxTick

Given timeOut is 0.30
and omni_Interval is 0.03
>set udg_omni_MaxCount[index] = timeOut/udg_omni_Interval // == 10
>set udg_omni_Countdown[index] = udg_omni_MaxCount[index]
>set udg_omni_CurrentTick[index] = 0
>set udg_omni_MaxTick[index] = i

In the iterating function
>add

if udg_omni_Countdown > 0 then
    set udg_omni_Countdown[index] = set udg_omni_Countdown[index] - 1
else
    set udg_omni_Countdown[index] = udg_omni_MaxCount[index]
    set udg_omni_CurrentTick[index] = set udg_omni_CurrentTick[index] + 1

End the omnislash when CurrentTick >= MaxTick.

Done!

I don't know what you've done. The local group g is still exist in your code.
What I mean by replacing local group g with global variable is
>remove local group g
>create Unit Group variable "omni_tmpGroup"
>replace g with omni_tmpGroup
>remove CreateGroup and DestroyGroup in your code.

 

[MyPad]

Anything that is constant in your code should be configurable. Likes unit filter, attack type, damage type, ...

Oh, that is for the default trigger. It's coded so that everything is configurable if you don't use the default trigger. To make it more clear,

call RequestOmnislash(caster, target, timeout, damage, range, instances, triggerId)

You can plug in any trigger that you like in the triggerId; the only compromise is that you have to put it in Custom Script.
If triggerId is null, it resorts to the default setting, explaining what you see in the trigger now.

So if you have another trigger that catches the moment when the variable udg_omni_Omnislash_RealEvent is set to 1.00 or 2.00, and you want that instance previously mentioned to have the trigger's actions executed, you can use the triggerId in order to identify which is which. (This is ideally done in a triggercondition)

I'm talking about performance, it is something to worry about.
Create a timer when the game start, when there is a index exist, start the timer and pause when there is no index on the list. So, your system will no longer needs to constantly create and destroy timer.

That could be a viable suggestion, but I'll have yet to implement timestamps for it and code it in a priority queue form. As a compromise, I'm keeping it as is for now.

About the integer as countdown, this method is for systems that uses a single timer. Actually you can also use real but I prefer integer.
Its works like this

Create a integer array variable "omni_Countdown
Create a integer array variable "omni_MaxCount
Create a integer array variable "omni_CurrentTick
Create a integer array variable "omni_MaxTick

Given timeOut is 0.30
and omni_Interval is 0.03
>set udg_omni_MaxCount[index] = timeOut/udg_omni_Interval // == 10
>set udg_omni_Countdown[index] = udg_omni_MaxCount[index]
>set udg_omni_CurrentTick[index] = 0
>set udg_omni_MaxTick[index] = i

In the iterating function
>add

if udg_omni_Countdown > 0 then
    set udg_omni_Countdown[index] = set udg_omni_Countdown[index] - 1
else
    set udg_omni_Countdown[index] = udg_omni_MaxCount[index]
    set udg_omni_CurrentTick[index] = set udg_omni_CurrentTick[index] + 1

End the omnislash when CurrentTick >= MaxTick.

In a way, that is what is being done in the iterating function

function pf__Omnislash__onExec takes nothing returns nothing

I don't know what you've done. The local group g is still exist in your code.
What I mean by replacing local group g with global variable is
>remove local group g
>create Unit Group variable "omni_tmpGroup"
>replace g with omni_tmpGroup
>remove CreateGroup and DestroyGroup in your code.

Changed it now. It will now use a variable udg_ENUM_GROUP instead of dynamically creating and destroying groups, (although from what I've heard, groups are actually a type of linked list and (this one is from me) they're quite safe to dynamically create and destroy.)

 

[KILLCIDE]

Needs Fixed

• Filters for the base omnislash should be configurable from the top
• 851972 should be configurable
• Documentation would be appreciated as it will make the review process less tedious and prevent misunderstandings

Suggestions

• Update your code to follow the GPAG & JPAG guidelines
• GetWidgetLife() IsUnitState() when checking if a unit is alive or dead
• It's unnecessary to use GroupClear() before GroupEnumUnitsInRange()

Status

Awaiting Update​

 

[MyPad]

Thanks for the suggestions, though I would like to ask a few questions.

1.) What is your suggestion for the function name instead of RequestOmnislash and ReturnOmnislash?

2.) Should I add protection by allowing only one instance per slasher or pseudo-protection by queuing the following requests for the same slasher, or ignore protection?

 

[KILLCIDE]

What is your suggestion for the function name instead of
RequestOmnislash
and
ReturnOmnislash
?

GetOmnislashId

Should I add protection by allowing only one instance per slasher or pseudo-protection by queuing the following requests for the same slasher, or ignore protection?

I would do the former.

 

[MyPad]

Okay, then. I'm actually recoding this thing from scratch, so that it would ultimately be nicer to look at.

How about AddOmnislash and RemoveOmnislash?
Not only do they provide instances, but they also provide default values, and deallocate instances respectively.

 

[KILLCIDE]

How about
AddOmnislash
and
RemoveOmnislash
?

That works then. If you want to be consistent with how naming conventions work in vJASS, you can call it OmnislashCreate & OmnislashDestroy.

 

[MyPad]

(July 29, 2017)
- Updated the bundle with newer examples! (New example in GUI (but it need not be manually displayed because of the preview triggers option ))

 

[KILLCIDE]

It would be great if you can make the API shorter and more user friendly.

 

[MyPad]

After a year or so, I thought of revisiting the concept, making it more GUI-Friendly in the process. Thus, the revisions were borne, and just as Warcraft 3 gets a Reforged version, so does this get a Remade version.

If there are bugs which I have not spotted, or cases which are not considered by the system, please let me know here.

 

[Dr Super Good]

Required for Approval:

Demonstration map needs some major improvements as it currently is as good as unusable. The player test units start out with practically no life meaning they die near instantly to the test targets. There is no revive or respawn of any of the test units. Even the test omni-slash abilities do not appear to work as the heroes will teleport into allies if cast on them and if cast on enemies will only slash them once. The triggers say the count is set to 40 times for both test abilities so possibly this is a bug.​

Comments and Suggestions:

For each of the configuration variables explain what the variable controls. They do have sensible names but this might not be enough for people who are not fluent in English. Even if they are they might be left guessing if something does not work as expected due to a bug or unintended interaction.

Some of your triggers contain such a high proportion of custom script it might be worth considering converting them to better formatted JASS triggers. This should not be detrimental to usability since it is unlikely a user will be modifying those triggers much and they currently need decent JASS knowledge to do so anyway.​

 

[MyPad]

Required for Approval:

Demonstration map needs some major improvements as it currently is as good as unusable. The player test units start out with practically no life meaning they die near instantly to the test targets. There is no revive or respawn of any of the test units. Even the test omni-slash abilities do not appear to work as the heroes will teleport into allies if cast on them and if cast on enemies will only slash them once. The triggers say the count is set to 40 times for both test abilities so possibly this is a bug.​

Comments and Suggestions:

For each of the configuration variables explain what the variable controls. They do have sensible names but this might not be enough for people who are not fluent in English. Even if they are they might be left guessing if something does not work as expected due to a bug or unintended interaction.

Some of your triggers contain such a high proportion of custom script it might be worth considering converting them to better formatted JASS triggers. This should not be detrimental to usability since it is unlikely a user will be modifying those triggers much and they currently need decent JASS knowledge to do so anyway.​

Thanks for the review. Improvements on the description on what each ability does will be introduced, as well as adding a respawn mechanic for creeps for testing purposes. Variables, if not already explained, will be elaborated upon.

EDIT:

It appears that the bug found was associated with OMNI_INVULNERABLE_FLAG. As this bug was considered in the version where BlzIsUnitInvulnerable returned false when it should've returned true, this likely might have been the case. The update will set OMNI_INVULNERABLE_FLAG to true by default.

Nevertheless, more testing will be performed before an update can properly be released.

 

[MyPad]

Version 1.3.0

Changelog:

• improved the spell description of sample spells.
  
  
• Reconverted the GUI triggers into pure JASS. (Removed triggers that had no apparent
  
  functionality to the user.)
  
• Edited Changelog 1.2.0 (This is due to the "new" native retrieving the invulnerability state of a unit.)|
  
  
• Added some needed comments on some more dubious variables.
  
  
• Updated import instructions to include the system code into the map header.
  
  
• Improved the automatic target searching algorithm.
  
  
• Now includes a more vibrant terrain (though not all), and respawns all dying units (revives dying Heroes).
  
  
• Improved the comments in the demo triggers.
  
  
• Updated screenshot image

 

[Dr Super Good]

Comments and Suggestions:

Potentially useful system for anyone who likes the Final Fantasy 7 style Omnislash concept. Easy to use, import and does a pretty good job at it.

A possible improvement would be settings to allow for smooth unit movement between attacks rather than teleporting. Some Omnislash spells I have seen in the past do this and in some cases it can be more visually desirable depending on the effect the user wants to achieve.​