Awkward Scaling involving ImprovedDamage (Bug?)

[Hateborne]

Afternoon ladies/gents/trolls,

Using a spell with effectid1 = 124, effect_base_value1 = 500 as a focus on a spell with base damage 375000, with no other affecting foci, the damage scales weirdly or breaks (goes negative). This is being cast as a wizard.

Base Damage: 375,000
Crit Damage: 900,000
With Focus: 13,500,000
Crit With Focus: -10,549,672

Any ideas?


-Hate


https://github.com/EQEmu/Server/issues/347 Issue Started as well.

[Kayen]

Sounds like an overflow issue.

However, just reviewing the code those numbers aren't really hitting the int32 limits.

Question is it healing the NPC when you negative crit?

[Kayen]

This is the function that handles it. Maybe somebody else will see something obviously wrong.

int32 Mob::GetActSpellDamage(uint16 spell_id, int32 value, Mob* target) {

	if (spells[spell_id].targettype == ST_Self)
		return value;

	if (IsNPC())
		value += value*CastToNPC()->GetSpellFocusDMG()/100;

	bool Critical = false;
	int32 value_BaseEffect = 0;
	int chance = 0;

	value_BaseEffect = value + (value*GetFocusEffect(focusFcBaseEffects, spell_id)/100);

	// Need to scale HT damage differently after level 40! It no longer scales by the constant value in the spell file. It scales differently, instead of 10 more damage per level, it does 30 more damage per level. So we multiply the level minus 40 times 20 if they are over level 40.
	if ((spell_id == SPELL_HARM_TOUCH || spell_id == SPELL_HARM_TOUCH2 || spell_id == SPELL_IMP_HARM_TOUCH ) && GetLevel() > 40)
		value -= (GetLevel() - 40) * 20;

	//This adds the extra damage from the AA Unholy Touch, 450 per level to the AA Improved Harm TOuch.
	if (spell_id == SPELL_IMP_HARM_TOUCH && IsClient()) //Improved Harm Touch
		value -= GetAA(aaUnholyTouch) * 450; //Unholy Touch

		chance = RuleI(Spells, BaseCritChance); //Wizard base critical chance is 2% (Does not scale with level)
		chance += itembonuses.CriticalSpellChance + spellbonuses.CriticalSpellChance + aabonuses.CriticalSpellChance;
		chance += itembonuses.FrenziedDevastation + spellbonuses.FrenziedDevastation + aabonuses.FrenziedDevastation;

	//Crtical Hit Calculation pathway
	if (chance > 0 || (IsClient() && GetClass() == WIZARD && GetLevel() >= RuleI(Spells, WizCritLevel))) {

		 int32 ratio = RuleI(Spells, BaseCritRatio); //Critical modifier is applied from spell effects only. Keep at 100 for live like criticals.

		//Improved Harm Touch is a guaranteed crit if you have at least one level of SCF.
		if (spell_id == SPELL_IMP_HARM_TOUCH && IsClient() && (GetAA(aaSpellCastingFury) > 0) && (GetAA(aaUnholyTouch) > 0))
			 chance = 100;

		if (zone->random.Roll(chance)) {
			Critical = true;
			ratio += itembonuses.SpellCritDmgIncrease + spellbonuses.SpellCritDmgIncrease + aabonuses.SpellCritDmgIncrease;
			ratio += itembonuses.SpellCritDmgIncNoStack + spellbonuses.SpellCritDmgIncNoStack + aabonuses.SpellCritDmgIncNoStack;
		}

		else if ((IsClient() && GetClass() == WIZARD) || (IsMerc() && GetClass() == CASTERDPS)) {
			if ((GetLevel() >= RuleI(Spells, WizCritLevel)) && zone->random.Roll(RuleI(Spells, WizCritChance))){
				//Wizard innate critical chance is calculated seperately from spell effect and is not a set ratio. (20-70 is parse confirmed)
				ratio += zone->random.Int(20,70);
				Critical = true;
			}
		}

		if (IsClient() && GetClass() == WIZARD)
			ratio += RuleI(Spells, WizCritRatio); //Default is zero
	
		if (Critical){

			value = value_BaseEffect*ratio/100;

			value += value_BaseEffect*GetFocusEffect(focusImprovedDamage, spell_id)/100;

			value += int(value_BaseEffect*GetFocusEffect(focusFcDamagePctCrit, spell_id)/100)*ratio/100;

			if (target) {
				value += int(value_BaseEffect*target->GetVulnerability(this, spell_id, 0)/100)*ratio/100;
				value -= target->GetFcDamageAmtIncoming(this, spell_id);
			}

			value -= GetFocusEffect(focusFcDamageAmtCrit, spell_id)*ratio/100;

			value -= GetFocusEffect(focusFcDamageAmt, spell_id);

			if(itembonuses.SpellDmg && spells[spell_id].classes[(GetClass()%16) - 1] >= GetLevel() - 5)
				value -= GetExtraSpellAmt(spell_id, itembonuses.SpellDmg, value)*ratio/100;

			else if (IsNPC() && CastToNPC()->GetSpellScale())
				value = int(static_cast<float>(value) * CastToNPC()->GetSpellScale() / 100.0f);

			entity_list.MessageClose_StringID(this, true, 100, MT_SpellCrits,
					OTHER_CRIT_BLAST, GetName(), itoa(-value));

			if (IsClient())
				Message_StringID(MT_SpellCrits, YOU_CRIT_BLAST, itoa(-value));

			return value;
		}
	}
	//Non Crtical Hit Calculation pathway
	value = value_BaseEffect;

	value += value_BaseEffect*GetFocusEffect(focusImprovedDamage, spell_id)/100;

	value += value_BaseEffect*GetFocusEffect(focusFcDamagePctCrit, spell_id)/100;

	if (target) {
		value += value_BaseEffect*target->GetVulnerability(this, spell_id, 0)/100;
		value -= target->GetFcDamageAmtIncoming(this, spell_id);
	}

	value -= GetFocusEffect(focusFcDamageAmtCrit, spell_id);

	value -= GetFocusEffect(focusFcDamageAmt, spell_id);

	if(itembonuses.SpellDmg && spells[spell_id].classes[(GetClass()%16) - 1] >= GetLevel() - 5)
		 value -= GetExtraSpellAmt(spell_id, itembonuses.SpellDmg, value);

	if (IsNPC() && CastToNPC()->GetSpellScale())
		value = int(static_cast<float>(value) * CastToNPC()->GetSpellScale() / 100.0f);

	return value;
}

[Hateborne]

Kayen,

The healing part I cannot tell as it's a target dummy npc that heals to full every 3 seconds with millions of HP. The code block you sent was one the bit I was pouring over. I'm going to setup a devbox and attach a debugger to trace the values when a crit lands. It seems to be only crits. I should have something by Monday, unless you beat me to it.


-Hate

[Zaela_S]

...
		if (Critical){

			value = value_BaseEffect*ratio/100;

			value += value_BaseEffect*GetFocusEffect(focusImprovedDamage, spell_id)/100;

int value = 375000 * 240 / 100 => 900000
int focus = 3600

value *= focus => 3,240,000,000 <-- overflow for signed 32bit
value /= 100 ....

?

edit:
3,240,000,000 - 2^32 => -1,054,967,296
-1,054,967,296 / 100 => -10,549,672

Seems pretty likely.

[Zaela_S]

If the final damage would be over ~21 million it seems like it would inevitably hit overflow somewhere in there. Dunno if it would be worth bumping the temporaries up to int64 for most servers, though.

[Kayen]

At some point a custom server has to take responsibility for what it sets as values for things.

Given all things are relative there is no logical reason to have numbers that high.

[Hateborne]

Originally Posted by Kayen

At some point a custom server has to take responsibility for what it sets as values for things.

Given all things are relative there is no logical reason to have numbers that high.

I'll be sure to tell Hunter the next time I see him that his custom damage and HP values are too high.

Rude comments aside, I am trying to take responsibility and bring up things I find as potential bugs or awkwardly scaling. This scaled so high because another augment was in my test gear that I was unaware of that added an additional 500% via FcDamagePctCrit (302) which was causing the explosive scaling, which created the potential for -2,700,000,000 in the formula on line 99 in effects.cpp (as shown below). Without the extra 500%, it scaled normally (without code changes) and gave appropriate values.

value += int(value_BaseEffect*GetFocusEffect(focusFcDamagePctCrit, spell_id)/100)*ratio/100;
value += int(-2250000 * 500 / 100) * 240 / 100;

Thanks to those that contributed and this thread can be closed/locked/marked-as-resolved/etc.


-Hate