Triggered Incinerate v1.2

Aether · Jun 6, 2016

There's nothing in the code in the description. :O

AGD · Jun 6, 2016

Ah, my bad.
Now updated.

Emm-A- · Jun 6, 2016

Hey AGD, why are you using

JASS:

StringHash()

instead of integer numbers? For readability?

Kyrbi0 · Jun 8, 2016

This is pretty nifty. CURSE YOU BLIZZARD HARDCODING!

KILLCIDE · Jun 8, 2016

Cool idea, especially for a passive ability like Incinerate that has a lot of hardcoded values. However, I see this functioning way more as a system rather than multiple abilities. I also wonder how many different types of Incinerates people would actually want in their maps. Anyway, here are some issues I would like for you to tackle:

It's not necessary to null the local triggers in the InitTrig function
The way you index abilities are horrendous. Instead of copy-pasting all the variables inside one function, why not have a function that will do it for you? You set some values to some temp global variables, and then call the function that will index + transfer all the temp global variables to the indexed variables. This will dramatically reduce the size of function TI_Config
Instead of making your own "DDS", please just use any of the reliable DDS in the spell section
TI_SpellCheck & MainAction can be combined into one function
In TI_SpellCheck, replace return true to exitwhen true. Even if the ability is found, the loop will still continue to run with your current setup
The dummy unit's ID is currently hardcoded
It's not necessary to convert level to a real when you calculate things like duration & damage
I would move the filters to a function somewhere near the top of the code so users can easily configure it
In MainAction, why is the dummy only removed if unit u passes the filters?
In MainAction, you should only declare the locals like level nand duration, and then store things into them if the unit passes the filter
Why SaveStringBJ over SavStr?
Personally, I think the damage and death event functions should only register events if there is an instance of the debuff
You can get rid of the locals fulldamagegroup & smalldamage group by just using a single recycled global group
Why is a dummy unit dealing damage to the units instead of the actual caster?

Could you also add an ingame screenshot instead of the spell's code? For now, I will set the spell's status to Awaiting Update.

AGD · Jun 13, 2016

JASS:
Hey AGD, why are you using

JASS:

StringHash()

instead of integer numbers? For readability?

Yes

Why is a dummy unit dealing damage to the units instead of the actual caster?

It is because if the actual caster will be the one to deal damage, it will trigger another incinerate, and thus creating an unending loop. But if it is the dummy, it will not because it has no incinerate ability.

KILLCIDE · Jun 13, 2016

Could you not add an extra condition to your "DDS" to make sure that the target source is not already affected by Incinerate? Or can other incinerates stack on each other?

IcemanBo · Jun 13, 2016

The 'n000' unit type in code should not be hardcoded, as Killcide mentioned.

The StringHash function is not very compared to usage of constants. It's not required to evaluate it again and again.
It would be more performant to use variables for the usage.

Directly give the user access to define attacktype, damagetype, weapntype, instead of defining the index. No need to convert them each call.

Really follow Killcide's suggestion and use a DDS system. Some systems are made for good reasons, and are not easy to do perfectly from scratch.

AGD · Jun 14, 2016

The
'n000'
unit type in code should not be hardcoded, as Killcide mentioned.

The StringHash function is not very compared to usage of constants. It's not required to evaluate it again and again.
It would be more performant to use variables for the usage.

Directly give the user access to define attacktype, damagetype, weapntype, instead of defining the index. No need to convert them each call.

Really follow Killcide's suggestion and use a DDS system. Some systems are made for good reasons, and are not easy to do perfectly from scratch.

Yes I'll include it in the update.

Could you not add an extra condition to your "DDS" to make sure that the target source is not already affected by Incinerate? Or can other incinerates stack on each other?

The source is not affected by the incinerate but the target unit. And yes, other incinerates can stack damage on each other ( additionally ).
I'm not sure if I understood your first question here correctly.

KILLCIDE · Jun 14, 2016

AGD said:
The source is not affected by the incinerate but the target unit. And yes, other incinerates can stack damage on each other ( additionally ).
I'm not sure if I understood your first question here correctly.

I'm aware. However, that was not what I was suggesting. I really dislike the fact that the caster of the spell does not get appropriate credit for the kills. Since the dummy unit is dealing damage, the dummy unit will get credit. If you're afraid of an infinite loop, why not just disable the trigger before the you deal the triggered damage and then turn it back on afterwards?

AGD · Jun 30, 2016

Resource updated.
I still need to use StringHashes because I used the concatenated string with the spell index as the parent key.

KILLCIDE · Jun 30, 2016

It seems you ignored some of my important suggestions.

The brute copy-paste setup you have for TI_SpellsList can be avoided by using temp global variables that a user can setup and then simply calling a function that will index all the variables for them is better than having every single incinerate spell under one function
It would be better to keep the unit filters in TI_MainAction in a seperate function near the top of the code so that users can easily configure it to their liking instead of having to look for it in the code
Out of curiousity, why does the unit that deals the killing blow not get damaged by the AoE?
My same suggestion for the filters in TI_MainAction go for TI_Incinerate
I personally think all the debug messages you have in the code are not needed and just make the code look like a mess. Since you are using hashtables, you can have the debug in some seperate code/trigger and easily look up who is dealing what damage with what spell, right?

I would really like for you to do the first suggestion and maybe the one about the debug messages. This will dramatically reduce the length of your code. I will set this resource back to Awaiting Update.

AGD · Jul 1, 2016

KILLCIDE said:
The brute copy-paste setup you have for TI_SpellsList can be avoided by using temp global variables that a user can setup and then simply calling a function that will index all the variables for them is better than having every single incinerate spell under one function

If I understood correctly, that would mean that they can only setup one set of stats that is the same for all incinerates.

KILLCIDE said:
It would be better to keep the unit filters in TI_MainAction in a seperate function near the top of the code so that users can easily configure it to their liking instead of having to look for it in the code
My same suggestion for the filters in TI_MainAction go for TI_Incinerate

I think these lines in the configurations does it already, no?

JASS:

   //** Determines if the following type of units are allowed as target **//

   // Structures
   set udg_TI_IsUnitStructure[i] = false

   // Mechanical
   set udg_TI_IsUnitMechanical[i] = false

   // Magic Immune
   set udg_TI_IsUnitMagicImmune[i] = false

   // Allies
   set udg_TI_IsUnitAlly[i] = true

   // Illusions
   set udg_TI_IsUnitIllusion[i] = true

KILLCIDE said:
I personally think all the debug messages you have in the code are not needed and just make the code look like a mess. Since you are using hashtables, you can have the debug in some seperate code/trigger and easily look up who is dealing what damage with what spell, right?

Okay I will do it

KILLCIDE said:
Out of curiousity, why does the unit that deals the killing blow not get damaged by the AoE?

I missed this one, I'll fix it

KILLCIDE · Jul 1, 2016

AGD said:
If I understood correctly, that would mean that they can only setup one set of stats that is the same for all incinerates.

No? What makes you think that?

Set SaveAbility = Incinerate v1
Set SaveBaseDamage = 100
Set SaveBaseDamagePerLevel = 50
Set SaveSFXExplosion = Abilities\Spells\Other\Incinerate\FireLordDeathExplode.mdl
Custom script: call TI_SpellsList()
-------- --------
Set SaveAbility = Incinerate v2
Set SaveBaseDamage = 50
Set SaveBaseDamagePerLevel = 100
Set SaveSFXExplosion = Objects\Spawnmodels\Undead\UndeadBlood\UndeadBloodNecromancer.mdl
Custom script: call TI_SpellsList()

JASS:

function TI_SpellsList takes nothing returns nothing
    set udg_Ability[udg_Index] = udg_SaveAbility
    set udg_BaseDamage[udg_Index] = udg_SaveBaseDamage
    set udg_BaseDamagePerLevel[udg_Index] = udg_SaveBaseDamagePerLevel
    set udg_SFXExplosion[udg_Index] = udg_SaveSFXExplosion
    set udg_Index = udg_Index + 1
endfunction

You can even add a "safety" by having default Incinerate that will switch all the global variables to their default values.

AGD said:
I think these lines in the configurations does it already, no?

Ahh yes

my mistake. I would still do it for readability though.

AGD · Jul 3, 2016

KILLCIDE said:
No? What makes you think that?

Set SaveAbility = Incinerate v1

Set SaveBaseDamage = 100

Set SaveBaseDamagePerLevel = 50

Set SaveSFXExplosion = Abilities\Spells\Other\Incinerate\FireLordDeathExplode.mdl

Custom script: call TI_SpellsList()

-------- --------

Set SaveAbility = Incinerate v2

Set SaveBaseDamage = 50

Set SaveBaseDamagePerLevel = 100

Set SaveSFXExplosion = Objects\Spawnmodels\Undead\UndeadBlood\UndeadBloodNecromancer.mdl

Custom script: call TI_SpellsList()

But users still need to do the same copy - paste if they want to add more slot for the spell right?

KILLCIDE · Jul 3, 2016

Yes, but at least they wont be under one insanely long function in the main code.

AGD · Jul 4, 2016

Alright i'll do it, i guess it would also be better for this spell to be more GUI friendly.

AGD · Jul 4, 2016

Now updated

KILLCIDE said:
The brute copy-paste setup you have for TI_SpellsList can be avoided by using temp global variables that a user can setup and then simply calling a function that will index all the variables for them is better than having every single incinerate spell under one function

Done

KILLCIDE said:
Out of curiousity, why does the unit that deals the killing blow not get damaged by the AoE?

The unit named killer there is not actually the killing unit but the one that places the incinerate buff on the dying unit so it should not take damage from the explosion it caused.
However I already changed the variable name from killer => buffplacer to make it clear.

KILLCIDE said:
I personally think all the debug messages you have in the code are not needed and just make the code look like a mess. Since you are using hashtables, you can have the debug in some seperate code/trigger and easily look up who is dealing what damage with what spell, right?

Done

BTW, I also added the credits which I forgot to put previously.

KILLCIDE · Jul 6, 2016

Ugh yes. The code is much shorter

Here are a few suggestions:

You should maybe switch all the global variables to some type of default incinerate just in case users forget to use one of the variables before they call the function. If they do forget, the global variable will be storing the most recent incinerate they made. You could have users configure the default incinerate in some seperate function or a bunch of constant functions
I fail to see the purpose of this call ExecuteFunc( "Trig_Incinerate_SpellsList_Actions" )
You don't have to post your debug trigger in your description

My only gripe with the code is how you keep the actual filters in the code ! It makes the code all wide and ugly. I still stand by my previous suggestion to you before by just calling the function directly like so:

JASS:

function Filters takes unit u, player p returns boolean
    return (not IsUnitType(u, UNIT_TYPE_STRUCTURE) and (not IsUnitType(u,UNIT_TYPE_MECHANICAL) and (IsUnitEnemy(u, p)
endfunction

//=======================================================
function SomeFunction takes nothing returns nothing
    local unit u = GetTriggerUnit()
    local player p = GetTriggerPlayer()

    if Filters(u, p) then
        // do stuff
    endif

Other than that, the code looks fine to me. The submission overall isn't something gamebreaking, nor is it original

I still question how many versions of Incinerate people will actually have in their map, but someone is bound to find this useful. Approved with a rating of 3/5.

AGD · Jul 13, 2016

KILLCIDE said:
You should maybe switch all the global variables to some type of default incinerate just in case users forget to use one of the variables before they call the function. If they do forget, the global variable will be storing the most recent incinerate they made.

This is currently how the indexing works, if they forget to set the value of a temp global, the value that will be passed to the indexed global is the value that the temp global is currently containing which is actually the its value on the latest incinerate they made.

I fail to see the purpose of this
call ExecuteFunc( "Trig_Incinerate_SpellsList_Actions" )

It is because the configuration script could be possibly compiled by WE first before the main script in which case using a normal function call wont work because it is calling a function below it.

Approved with a rating of 3/5.

I'll still be updating this soon, btw I still didn't saw the rating.

KILLCIDE · Jul 13, 2016

AGD said:
I'll still be updating this soon, btw I still didn't saw the rating.

Added

KILLCIDE · Nov 15, 2016

Sorry for the update. Just updating the tags.

posthumousturbo · Sep 7, 2020

Can't use it, too much time needed. In my opinion this is not good idea.

AGD · Sep 8, 2020

@posthumousturbo I'm guessing what took your time are the importing of the requirements like the Damage Engine? Because after that or if you already have that in your map, setting up this system's configuration is actually the same as configuring a GUI spell.

Ugabunda · May 3, 2021

Hi AGD, I want to use this system in my map (which already has Damage Engine), but I have some questions:

Why is the dummy unit required? I searched for "n000", but didn't find this in any part of the code?
Same thing with the 3 custom abilities (A000, A001, A002) - are these needed? Didn't find them referred in the JASS Code...?
Can I delete the TI Variable Creator after importing it into my map? So essentially I leave only "Incinerate SpellsList" and "Triggered Incinerate" in my map

Cheers!

Triggered Incinerate v1.2

Triggered Incinerate v1.2 (Map)

Similar threads