[core][lua] Physical Mobskill Refactor#9738
Conversation
| -- Fallback if we somehow got this far. | ||
| return false, shadowsToRemove | ||
| end | ||
|
|
There was a problem hiding this comment.
I see a few issues with current implementation. Most notably, how we delete both blink and utsusemi. Blink may have not triggered and hence shouldnt be consumed.
Proposal:
function utils.shadowAbsorb(target, shadowsToRemove)
local utsusemiMod = target:getMod(xi.mod.UTSUSEMI)
local blinkMod = target:getMod(xi.mod.BLINK)
-- Early return: Target has no shadows.
if
utsusemiMod == 0 and
blinkMod == 0
then
return false, 0
end
local targetShadows = 0
-- Utsusemi takes precedence over blink.
if utsusemiMod > 0 then
targetShadows = utsusemiMod
local effect = target:getStatusEffect(xi.effect.COPY_IMAGE)
if effect then
if targetShadows == 0 then
target:delStatusEffect(xi.effect.COPY_IMAGE)
elseif targetShadows == 1 then
effect:setIcon(xi.effect.COPY_IMAGE)
elseif targetShadows == 2 then
effect:setIcon(xi.effect.COPY_IMAGE_2)
else
effect:setIcon(xi.effect.COPY_IMAGE_3)
end
end
target:setMod(xi.mod.UTSUSEMI, targetShadows)
-- Blink has a random chance of triggering when no utsusemi is present.
elseif blinkMod > 0 then
if math.random(1, 100) <= 20 then
return false, 0
end
targetShadows = blinkMod
if targetShadows == 0 then
target:delStatusEffect(xi.effect.BLINK)
end
target:setMod(xi.mod.BLINK, targetShadows)
end
local actualConsumed = utils.clamp(shadowsToRemove, 0, targetShadows)
local absorbHit = targetShadows >= shadowsToRemove
return absorbHit, actualConsumed
endThere was a problem hiding this comment.
As far as I know those effects are mutually exclusive. You can either have blink or utsusemi
There was a problem hiding this comment.
Cleaned up the code a bit but Blink and Utsusemi are exclusive and can not be on at the same time.
| hitChance = xi.combat.physicalHitRate.getRangedHitRate(mob, target, accuracyModifier + targetSpecialAttackEvasion, false) | ||
| end | ||
|
|
||
| print('hitChance', hitChance) |
There was a problem hiding this comment.
Removed all my debug prints as of now.
| -- Defaults | ||
| local damage = utils.defaultIfNil(skillParams.baseDamage, mob:getWeaponDmg()) | ||
| local numHits = utils.defaultIfNil(skillParams.numHits, 1) | ||
| local fTPScale = utils.defaultIfNil(skillParams.fTP, { 1.00, 1.00, 1.00 }) | ||
| local fTPSubsequentHits = utils.defaultIfNil(skillParams.fTPSubsequentHits, { 1.00, 1.00, 1.00 }) | ||
| local fTPBonus = utils.defaultIfNil(skillParams.fTPBonus, 0) | ||
| local attackMultiplierfTP = utils.defaultIfNil(skillParams.attackMultiplier, { 1.00, 1.00, 1.00 }) | ||
| local accuracyModifierfTP = utils.defaultIfNil(skillParams.accuracyModifier, { 0, 0, 0 }) | ||
| local guaranteedFirstHit = utils.defaultIfNil(skillParams.guaranteedFirstHit and true, false) | ||
| local canCrit = utils.defaultIfNil(skillParams.canCrit, false) | ||
| local criticalChance = utils.defaultIfNil(skillParams.criticalChance, { 0.00, 0.00, 0.00 }) | ||
| local ignoreDefense = utils.defaultIfNil(skillParams.ignoreDefense, { 0.00, 0.00, 0.00 }) | ||
| local attackType = utils.defaultIfNil(skillParams.attackType, xi.attackType.PHYSICAL) | ||
| local damageType = utils.defaultIfNil(skillParams.damageType, xi.damageType.SLASHING) | ||
| local hybridSkill = utils.defaultIfNil(skillParams.hybridSkill, false) | ||
| local hybridSkillElement = utils.defaultIfNil(skillParams.hybridSkillElement, xi.element.NONE) | ||
| local hybridAttackType = utils.defaultIfNil(skillParams.hybridAttackType, xi.attackType.MAGICAL) | ||
| local hybridDamageType = utils.defaultIfNil(skillParams.hybridDamageType, xi.damageType.ELEMENTAL) | ||
| local shadowsToRemove = utils.defaultIfNil(skillParams.shadowBehavior, xi.mobskills.shadowBehavior.NUMSHADOWS_1) | ||
| local skipStoneskin = utils.defaultIfNil(skillParams.skipStoneskin, false) | ||
| local skipYaegasumi = utils.defaultIfNil(skillParams.skipYaegasumi, false) | ||
| local skipFSTR = utils.defaultIfNil(skillParams.skipFSTR, false) | ||
| local skipPDIF = utils.defaultIfNil(skillParams.skipPDIF, false) | ||
| local skipParry = utils.defaultIfNil(skillParams.skipParry, false) | ||
| local skipGuard = utils.defaultIfNil(skillParams.skipGuard, false) | ||
| local skipBlock = utils.defaultIfNil(skillParams.skipBlock, false) | ||
| local primaryMessage = utils.defaultIfNil(skillParams.primaryMessage, xi.msg.basic.DAMAGE) | ||
|
|
||
| -- Initialize return structure | ||
| returnInfo.damage = 0 | ||
| returnInfo.hybridDamage = 0 | ||
| returnInfo.hitsLanded = 0 | ||
| returnInfo.attackType = attackType | ||
| returnInfo.damageType = damageType | ||
| returnInfo.hybridAttackType = hybridAttackType | ||
| returnInfo.hybridDamageType = hybridDamageType | ||
| returnInfo.isCritical = false | ||
| returnInfo.hitData = {} |
There was a problem hiding this comment.
I see these default values are used in both ranged and physical skills but you define them differently.
Example: You have utils.defaultIfNil(skillParams.skipStoneskin, false) in here but in the other one you have utils.defaultIfNil(skillParams.skipStoneskin and true, false)
Since these appear to the be the exact same you can make a local helper function to return all these values which will lower the complexity and remove duplicated code. I am not sure if these need to be different in the future but as its proposed they appear to be identical
76de338 to
a31c635
Compare
| @@ -2827,7 +2827,7 @@ void CBattleEntity::OnMobSkillFinished(CMobSkillState& state, action_t& action) | |||
| if (PSkill->hasMissMsg()) | |||
| { | |||
| result.resolution = ActionResolution::Miss; | |||
| result.param = 0; | |||
| result.param = damage; | |||
There was a problem hiding this comment.
What did you attempt to fix with this change? Miss action packets always have param set to 0.
This is what's causing your tests to fail.
There was a problem hiding this comment.
Test passed after implementing the fix in CBattleEntity::OnMobSkillFinished()
e0e6986 to
963b829
Compare
963b829 to
83441d8
Compare
83441d8 to
7eee102
Compare
I affirm:
What does this pull request do?
Submitting as draft for now. Want to bounce it off automated checks while I sleep and see where it stands/get eyes on it. Will return to fix/consolidate things in morning.
Aware of debug prints being present, will remove/comment out before final undrafted PR but leaving in at the moment for testing purposes.
Did extensive testing on my local but will need to retest based on further changes/adjustments so leaving that box unchecked for now.
1. Reworks how shadow consumption is handled.
Damage is no longer influenced by the shadows consumed.
Based on retail testing, shadow consumption for mobskills was found to not be affected by hit rate checks.
Based on retail testing, shadows are not calculated per hit. A single hit skill that takes 3 shadows will always take 3 shadows.
2. Reworked AoE skill shadow mitigation mechanic based on retail testing.
Physical AoE skills that do not ignore/wipe shadows have a chance to not consume the normal shadow amount.
The amount of shadow consumption mitigated is based on how many shadows a skill would normally take.
The amount of shadows a skill would normally take acts as a counter for how many mitigation attempts will be made. Example: a skill would take 4 shadows will have 4 attempts made for mitigation.
Mitigation can not reduce shadow consumption to 0. 1 shadow will always be taken at a minimum.
Test data can be found here and here.
3. Refactors Physical/Ranged mobskills
Adds/extends support for various parameters.
Retires obsolete xi.mobskills.mobPhysicalHit() function. Superseded by xi.mobskills.processDamage() which checks if the skill's hits landed.
takeDamage() is now wrapped with xi.mobskills.processDamage() to prevent shadow chip damage. Status Effect applications are also gated in the same way. Closes related issue: 🐛 xi.msg.basic.SHADOWS_TAKEN results not functioning properly for mobskills. #8880
Retires obsolete xi.mobskills.mobPhysicalStatusEffectMove() function. Superseded with usage pattern of mobStatusEffectMove() wrapped with xi.mobskills.processDamage().
Retires obsolete mobPhysicalDrainMove() function. Superseded with usage pattern of mobDrainMove() wrapped with xi.mobskills.processDamage().
Physical skills now calculate individual hits and record hit information that can be called later.
Ranged skills now use ranged PDIF, FSTR, and Crit functions.
Fixes ranged skill TP Gain by adjusting the getBaseRangedDelay() for mobs. Based on retail testing, mob ranged delay seems to be the same for all normal mobs recorded. Test Data can be found here.
Added missing trySkillUp() checks for Evasion. This is calculated per hit based on retail.
Adjusted Shield Mastery checks for shield blocks. We previously gated this behind having the Shield Mastery job trait but through retail testing, it was found that it could still proc if you had equipment that had Shield Mastery mods.
Implements skill data from Jimmayus's spreadsheet where applicable. Recent PRs that had updated capture data were kept/took priority.
Added mob families/notes to each skill's script for easy reference.
Added TODO: notes to mobskill scripts where we need capture data or code adjustments.
Steps to test these changes
Tested these steps on my local and were working then. Will retest again if I make further edits.
Test/play around with parameters. Nothing should error out. Each one should affect its related stat as expected.
Test shadow consumption.