vmangos / core

Progressive Vanilla Core aimed at all versions from 1.2 to 1.12
GNU General Public License v2.0
691 stars 491 forks source link

πŸ› [Bug]Wand hit rate formula is wrong #2361

Open ShiyoKozuki opened 10 months ago

ShiyoKozuki commented 10 months ago

πŸ› Bug report

image image

100% hit rate with 0 wand skill against a level 12enemy 100% hit rate with 26 wand skill vs a level 16 enemy Enemies much higher level than your skill(such as +20 or higher) do start resisting you properly, but lower level gaps is not working correctly.

This would be like if a level 0 player had 100% hit rate against a level 12 enemy with fireball. It's actually worse, because the level1 player has innate fire magic skill while wands start at 0. This means wands are too effective against enemies within +10 to your level(if capped wand skill) making wands too effective at killing enemies higher level than they should be.

Wands should be using a hit rate formula similar to spells, i.e. a level 15 mage's wand skill caps at 75 so has the hit rate against enemies equal to casting frostbolt with +0% hit rate if their wand is at level 75.

Expected behavior

https://youtu.be/h3BTbzAeJ1E?t=3328 The person in the video even mentions that he's being spam resisted, and a few minutes before he talked about how he needs to level his wand and it's going to get spam resisted at low level. Wands should be resisted as often as spells against enemies higher level than the wands current skill(i.e. a level 15 mage has 75 wand skill, so wanding a level 18 enemy would have equal hit rate to casting frostbolt on a +3 enemy with +0% hit rate)

GoonBoiBarry commented 10 months ago

Do you have a source for this?

ShiyoKozuki commented 10 months ago

Do you have a source for this?

Wands are the most optimal way to deal damage to enemies ~10 levels above you on this project. Every other form of damage is heavily mitigated by enemies 3 levels higher than you or more. I do not have a source, but it seems pretty obvious something is off if you think of this logically.

GoonBoiBarry commented 10 months ago

That's not why changes are made here lol. The project is exclusively trying to emulate vanilla, Whether something is balanced is irrelevant.

Hunters ranged damage also works this way in classic because it has different hit tables that allow it to hit high level mobs effectively, so I imagine wands are using the same formula.

ShiyoKozuki commented 10 months ago

That's not why changes are made here lol. The project is exclusively trying to emulate vanilla, Whether something is balanced is irrelevant.

Hunters ranged damage also works this way in classic because it has different hit tables that allow it to hit high level mobs effectively, so I imagine wands are using the same formula.

Fair enough, here's a video: https://youtu.be/h3BTbzAeJ1E?t=3328 The person in the video even mentions that he's being spam resisted, and a few minutes before he talked about how he needs to level his wand and it's going to get spam resisted at low level.

GoonBoiBarry commented 10 months ago

That does look like it is probably behaving incorrectly. I doubt a change would be made without a sample size big enough to know what to change it to.

BDuncanson-17 commented 10 months ago

I don't have to time to dig up the code for you but wands behave as ranged attacks in vanilla. Test it by stacking agility and seeing the results to your wand's crit rate.

leontedragos commented 10 months ago

There is no trace of the Wand Skill (228) being used in the code for any calculation.

I set my wand skill to 1/300 then I have the exact chance to hit a boss like with full 300/300. The chance to hit is based on player level vs victim level instead of wand skill. Wanding is treated like a spell and it benefits from spell hit and spell crit.

In this function the hit chance is calculated as a level difference instead of a weapon skill difference. int32 SpellCaster::MagicSpellHitChance(Unit* pVictim, SpellEntry const* spell, Spell* spellPtr)

A potential fix would be to divide the wand's skill level by 5 to replace the attacker's level in the formula.

a

Video of this change in action: https://streamable.com/5g63z4

ShiyoKozuki commented 10 months ago

There is no trace of the Wand Skill (228) being used in the code for any calculation.

I set my wand skill to 1/300 then I have the exact chance to hit a boss like with full 300/300. The chance to hit is based on player level vs victim level instead of wand skill. Wanding is treated like a spell and it benefits from spell hit and spell crit.

In this function the hit chance is calculated as a level difference instead of a weapon skill difference. int32 SpellCaster::MagicSpellHitChance(Unit* pVictim, SpellEntry const* spell, Spell* spellPtr)

A potential fix would be to divide the wand's skill level by 5 to replace the attacker's level in the formula.

a

Video of this change in action: https://streamable.com/5g63z4

Could you do a PR of this? Seems accurate it also needs a lot more code changes than the snippet you gave.

leontedragos commented 10 months ago

I don

There is no trace of the Wand Skill (228) being used in the code for any calculation. I set my wand skill to 1/300 then I have the exact chance to hit a boss like with full 300/300. The chance to hit is based on player level vs victim level instead of wand skill. Wanding is treated like a spell and it benefits from spell hit and spell crit. In this function the hit chance is calculated as a level difference instead of a weapon skill difference. int32 SpellCaster::MagicSpellHitChance(Unit* pVictim, SpellEntry const* spell, Spell* spellPtr) A potential fix would be to divide the wand's skill level by 5 to replace the attacker's level in the formula. a Video of this change in action: https://streamable.com/5g63z4

Could you do a PR of this? Seems accurate it also needs a lot more code changes than the snippet you gave.

I only added those 4 lines of code. You capture the Wand's shoot spell 5019 and instead of using the attacker's character level, you get his Wand's skill level and you divide it by 5. That's it.

ShiyoKozuki commented 10 months ago

attackerLevel is never declared in the current function of int32 SpellCaster::MagicSpellHitChance so I'm confused...

image

leontedragos commented 10 months ago

attackerLevel is never declared in the current function of int32 SpellCaster::MagicSpellHitChance so I'm confused...

image

a

Ah ok, sorry. I just declared those when I was testing to make it easier to understand what was going on.

Wall-core commented 10 months ago

Why are we adding code to MagicSpellHitChance? Wands are not magic spells, they are ranged attacks and treated as such in Vanilla. Their hit chance is calculated already in MeleeSpellHitResult as far as I'm aware. Whether or not there is an issue there is another matter but this is not correct.

leontedragos commented 10 months ago

Because there's no utilization of the Wand weapon skill (228), so you can hit a level 63 boss with 1/300 just the same as with 300/300, you will have 17% resist no matter the weapon skill level. Like the person who reported this showed, if the wand's skill level is accounted for then you get resisted a lot more when attacking enemies that are higher level or when your wand skill is still leveling up. I can't know what the true formula is like, but this seemed to work.

ShiyoKozuki commented 10 months ago

attackerLevel is never declared in the current function of int32 SpellCaster::MagicSpellHitChance so I'm confused... image

a

Ah ok, sorry. I just declared those when I was testing to make it easier to understand what was going on.

Why doesn't your code have

#if SUPPORTED_CLIENT_BUILD > CLIENT_BUILD_1_6_1
    int32 leveldif = int32(pVictim->GetLevelForTarget(this)) - int32(GetLevelForTarget(pVictim));
#else
    int32 leveldif = (!spellPtr && spell->HasEffect(SPELL_EFFECT_PERSISTENT_AREA_AURA)) ?
        int32(pVictim->GetLevelForTarget(this)) - std::max<int32>(1, spell->spellLevel) :
        int32(pVictim->GetLevelForTarget(this)) - int32(GetLevelForTarget(pVictim));
#endif

stuff? Wont your code mess up later logic for leveldiff then?

ShiyoKozuki commented 9 months ago

This works wonderfully, thank you.

mserajnik commented 8 months ago

I recently started leveling a priest on a private instance and can confirm that there are seemingly no resists against enemies around the same level, regardless of the wand skill. I didn't look through the code but leontedragos' mention that wand skill isn't used for the calculation seems to be true from the observed gameplay behavior.

I found another Classic video where someone starts leveling his wand skill as a priest:

I think based on video evidence (as well as various posts in forums and Reddit) it seems safe to assume that the current calculation in VMaNGOS is off, at least when compared to Classic; I can't find any actual Vanilla evidence but I guess that's something that wouldn't have been changed in Classic.

leontedragos' suggestion might not be ideal code-wise (as Wall-core suggests) but if the calculation seems reasonable maybe a "more proper" approximation of blizz-like behavior than what VMaNGOS currently has could be implemented based on it.