Best Implementation of Self-Targeted Skills

[0x05010705]

Posting this as a discussion since Zach asked for it here.

I fixed the ability of mobs to use self-targeted skills regardless of initial target being out of range (IE: Self-Destruct - Even if a player runs out of range while the skill is readying, the skill should still go off and the bomb would still blow up). Everything is tested and working as expected now, but I have some questions since this needs to go up to LSB.

This is a bit tricky to explain, but basically because of skills that cause the mob to die (Self-Destruct, Meltdown, Thermal Combustion, etc...) are doing so within the skill script with mob:setHP(0), I am forced to call the luautils::OnMobWeaponSkill() function on itself (See Code Segment Below) otherwise, the mob wouldn't die when it should be. In doing so, this causes an additional check to be needed within the mob script where a if mob ~= target check before applying additional effects (like stuns) and damage from happening on the mob itself.

Referenced Code Segment:

    // If we have no targets, but ALWAYS_FIRE is enabled, we call the skill script to execute
    if (targets == 0 && (PSkill->getFlag() & SKILLFLAG_ALWAYS_FIRE))
    {
        // We stage the animation so that even if the target is out of range, everyone else who is in range
        // will still see the skill go off normally (This has been tested locally and works)
        // No mob dmg messages are sent to clients and only animation effects are observed.
        actionList_t& list     = action.getNewActionList();
        actionTarget_t& target = list.getNewActionTarget();

        list.ActionTargetID = this->id;
        target.reaction     = REACTION::HIT;
        target.speceffect   = SPECEFFECT::HIT;
        target.animation    = PSkill->getAnimationID();

        luautils::OnMobWeaponSkill(this, this, PSkill, &action);
    }

self-destruct.lua Update:

+    if target ~= mob then
+        target:takeDamage(dmg, mob, xi.attackType.MAGICAL, xi.damageType.FIRE)
+    end

     mob:setHP(0)
-    target:takeDamage(dmg, mob, xi.attackType.MAGICAL, xi.damageType.FIRE)
     return dmg

I can get around this by hard-coding the skill ID's that do insta-kill the mob into core (As much as I hate to do that, there is already hard-coded things within that same file for certain families and a specific skillID for ix'ion). If I do it this way, I can get around having to call OnMobWeaponSkill on itself and this all becomes a nil issue.

Depending on how the above handled, I would have to either update each self-kill skill script to remove the mob:setHP(0) since its handled in core -OR- any skill that makes use of the new flag (ALWAYS_FIRE) would need to be updated in the skill script to check for the if mob ~= target so that the skill is not inflicting damage or status effects on the actual mob (the case where the original target has moved out of range of the skill and now its calling OnMobWeaponSkill on itself)

Hard-coding is the "easy" way to do it since there is a limited number of skillID's that kill the mob, but this also means that if SE ever comes up with a new self-kill skill, then it has to go into core and not the mob script. The alternative is leaving things as is, but it would require checks on the mob skill scripts that are self-targeted and would be a decent amount of file changes.

Updating the one skill script has the effect of updating all associated skills (IE: Multiple skill ID's for self-destruct_1 - So its entirely feasible to just update the associated skills on this front.

However, the number of non-kill skills that are self targeted (such as Typhoon, Thunderbolt, etc..) that don't kill the mobs would also have to be updated since the skill script is potentially being called on itself.
Ex: If the initial target for thunderbolt manages to get out of range before it fires, the skill script it being called on the mob and that skill then needs to have the check to prevent the skill from stunning/damaging the mob as well. So the number of self-targeted skills is greater than the number of just self-killing skills that could be hard-coded.

In theory, its probably better if nothing is hard-coded in core (even if there is already stuff hard coded), but the work of updating each skill is also ten-fold in comparison.

Additionally, the mob_skill_flag values in the database would also need to be updated to use this new flag (in addition to figuring out which skills should fire regardless of targets in range).

So, while I have the core fix ready, I am not sure which is the best way to implement this
Hard Code Self-Death Skills vs Update Mob Script files?
What should go into the initial PR for this?
Just the core changes?
DB updates and Skill Script Updates in another?
Everything all in one?

[WinterSolstice8]

I feel like changing targetfind to allow self for targeting with a new flag (or perhaps no new flag?) is probably the way to go
there's more than just mobskills that need to self target occasionally and it just fails

The only caveat I could see is "what if the mob dies during the ability and its the first target"
iirc mobskills like self destruct will order with the mob as the main target first and could make them die and thus not target the rest
At least, that's what the action packet looks like... doesn't mean we couldn't just resolve in reverse order.

[WinterSolstice8]

Apparently mobskills don't check if the user is alive during the middle of a mobskill when changing targets so that last point might not matter.

[0x05010705]

So, there are actually two flags/enums being used - the mob_valid_target flag and the mob_skill_flag
targetfind.h

enum FINDFLAGS
{
    FINDFLAGS_NONE            = 0,
    FINDFLAGS_DEAD            = 1,  // target dead
    FINDFLAGS_ALLIANCE        = 2,  // force target alliance
    FINDFLAGS_PET             = 4,  // force target pet
    FINDFLAGS_UNLIMITED       = 8,  // unlimited distance
    FINDFLAGS_HIT_ALL         = 16, // hit all targets, regardless of party
    FINDFLAGS_IGNORE_BATTLEID = 32, // ignore battle id check
    FINDFLAGS_ALWAYS_FIRE     = 64, // Skill will always fire off, regardless of targeting
};

mobskill.h

enum SKILLFLAG
{
    SKILLFLAG_NONE           = 0x000,
    SKILLFLAG_ASTRAL_FLOW    = 0x002, // Player's Avatar Astral Flow blood pacts. TODO: give player pet skills their own separate enum, move avatar stuff there.
    SKILLFLAG_NO_TP_COST     = 0x004, // Don't auto deduct TP
    SKILLFLAG_HIT_ALL        = 0x008, // Strike players even if not in party/alliance
    SKILLFLAG_ALWAYS_FIRE    = 0x010, // Skill will always fire off, regardless of targeting
    // unused                = 0x020,
    SKILLFLAG_BLOODPACT_RAGE = 0x040,
    SKILLFLAG_BLOODPACT_WARD = 0x080,
 };

I initially had thought to change the targeting flag, but it actually further complicated things because its only being used for deciding an initial target before it goes into the readying skill state. The other side, the skill flag, is used for special conditions, such as targeting dead players, hitting everyone regardless of party, etc. I am using the skill flag as the primary means of flagging these skills because despite it being a skill flag, its also still used to modify the targetFlags / m_targetFlags variables inside of targetfind.cpp
Ex:

    if (PSkill->getFlag() & SKILLFLAG_HIT_ALL)
    {
        findFlags |= FINDFLAGS_HIT_ALL;
    }

    if (PSkill->getFlag() & SKILLFLAG_ALWAYS_FIRE)
    {
        findFlags |= FINDFLAGS_ALWAYS_FIRE;
    }

    // Mob buff abilities also hit monster's pets
    if (PSkill->getValidTargets() == TARGET_SELF)
    {
        findFlags |= FINDFLAGS_PET;
    }

The first two are using the skillFlags to modify targetFlags, then this is followed by other checks on PSkill->getValidTargets() which is using the mob_valid_targets value from the database.
Notably though, the getValidTargets() values are only checked for equality, and not necessarily bitwise safe. If I were to modify a TARGET_SELF flag value from the database, this would require a whole separate implementation of that new value. The skill flag values are bitwise safe and can have multiple values OR'd together. So this was the cleaner way to implement this.

Also, there is a fine line of differentiation on skills - Some skills should be allowed to be ranged, but still permitted to finish despite the target being in range, while other skills should be able to be completely ranged and the mob fails because the "target is too far away".
This fix itself for allowing which direction a skill goes is fairly minor, minus one set of flagging, so if this was needed on something other than mobskills (Spells??) I can implement that side too.

The big issue here is this

If this is implemented AS-IS, then special cases need to be accounted for in the mob-skills that receive this flagging due to the fact that if the initial target is outside of range of the skill, the skill script is called upon the mob (to ensure that it fires, and if other targets were in range other than the initial target, they are still hit with the skill).
If this is limited to suicide skills and hard-coded within core, then these checks are not needed because the main reason for calling the skill on the mob is because things like Self-Destruct are killing the mob within the script. So if that script doesnt fire, the mob never dies.

To get a feel for the difference between hard-coding the values vs the number of scripts/skill entries in the DB that need to have flagging done to them, Kanryu and myself have created a spreadsheet for tracking. This is a core piece of functionality that is missing from the P Server community. All of these skills will end up getting validation before they are added since this is for LSB and that will be a huge undertaking, but in the interim, this is going to be for implementing the core side of this change which functionally does not modify anything with regards to skills without the new flag enabled on the skill.
https://docs.google.com/spreadsheets/d/15x27OVCQypxPj6iK9WpINCtn25PK9STqei1MbLQh_4U/edit?gid=0#gid=0

There are hundreds of skills that should not necessarily be allowed to be ranged and end up with the "too far away" message.
Things like Stomps from dragons should always be going off, but it still needs to have an initial (proper) target that is in range for that skill before it attempts to ready that skill, otherwise they would ready skills before a target was even initially in range.

RE: Target dieing
Yea, I'll make specific test cast to check for what happens if the initial target dies during the skill to check behavior.
I'll report back my findings here

[UmeboshiXI]

A little late post but this older PR might be of relevance. #5745