Master Branch: CRITICAL_DAMAGE_POS

[RogerSmith]

I was browsing out of curiosity, and found a post dated a while back
viewtopic.php?f=103&t=31576&p=189658&hi ... as#p189658

Looking over the formulas, I question the that the intended effect is achieved.

If Focus Death and Focus Power are meant to add only half of their effect to blows, then
double criticalModPos = (((attacker.calcStat(Stats.CRITICAL_DAMAGE_POS, 1, target, skill) - 1) / 2) + 1);

Is not a good way to achieve that. It may work OK now, because there are not many buffs with POS damage mods, but it is not a future-proof implementation.

Here is why:
assume you have 2 buffs that give 1.6x position damage. So you expect that on normal hit its 1.6*1.6, and on blow hit is 1.3*1.3 (because both are multiplying function).
but
calcStat will do 1.6*1.6 in both cases first, then the rest.
(1.6*1.6 - 1) = 1.56
1.56 / 2 = 0.78
0.78 + 1 = 1.78
------------------
1.3*1.3 = 1.69 != 1.78

The difference is minor (5% == 1.78/1.69) but it will exacerbate the more buffs you have.

------------------
My suggestion is

double criticalModPos = (((attacker.calcStat((Stats.CRITICAL_DAMAGE_POS - 1)/2 + 1, 1, target, skill)); //count the brackets, might be wrong amount

(Heh, you CAN'T actually do that. Why can't you do computation in method argument?)

Which will ensure that the stat pulled from XML is always halved, i.e. (1.6 - 1)/2 + 1 = 1.3.

Some questions:
there used to be a physicalBlowDamage param for XML skills, same way there is physicalSkillDamage for Final Secret (917). Why did the physicalBlowDamage get removed? It's a very useful stat. For the above topic, could have fixed without Core changes.
Example:
Focus death gives 1.9x crit dmg.
But for physicalBlowDamage 0.765x.
Total for blow skills 1.9*0.765 = 1.45x


Overall not very happy about the refactor (just opinion). Skill structure seems to be less abstract. I remember Cubic Summon skills had cubic cast skills attached. Now I have to look in Core for what cubic skills have. Loads of other changes that make it more difficult to change skills to get desired effect.

Kind Regards

P.S. all the formula is ****ed. Why do you include critical damage twice in calcBlowDamage()? Once when you pull criticalMod, then again criticalModPos. Won't both exist in XML skill?

[RogerSmith]

Ok...

So, you cannot do computation inside argument for attacker.calcStat().

You obviously can't put both critDmg and critDmgPos in the XML, because it will calculate more critical damage than it should.

I think it's best to change
double criticalModPos = (((attacker.calcStat(Stats.CRITICAL_DAMAGE_POS, 1, target, skill) - 1) / 2) + 1);
to
double criticalModPos = (attacker.calcStat(Stats.CRITICAL_DAMAGE_POS, 1, target, skill)

and do in XML
critDmg = 1.9
critDmgPos = 0.765
It might not look like retail, but it will have the same effect, and scales fine with multiple effects. You just have to calculate critDmgPos value yourself. (1.45/1.9 = 0.765)

So the problem is really that you calculate the L2Character stats so early that you cannot manipulate them further...