Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(DB/Core) Fix the CLS damage system and update creature_classlevelstats #4749

Merged
merged 25 commits into from
Mar 19, 2021

Conversation

Hacki95
Copy link
Contributor

@Hacki95 Hacki95 commented Mar 7, 2021

Fix the CLS damage calculation system and update creature_classlevelstats accordingly

Changes Proposed:

  • Reenable the min/max damage CLS system in Creature::SelectLevel in Creature.cpp that has been disabled
  • Fix the underlying calculation issue in Creature::CalculateMinMaxDamage in StatSystem.cpp. AttackMultiplier was wrongfully multiplied with AttackSpeed. I used the reverse engineered CLS calculations TC/vmangos/cmangos use.
  • Correct several wrong values in creaure_classlevelstats. Multiple damage_base and attackpower values were wrong before. I imported those values from TC's database.
  • Correct several wrong DamageModifier values in creature_template. Using TC values for those aswell. The values we had before were mostly a flat 1 or completly off because they were based on the old system. With the new values the calculations lead to the desired result.
  • Drop "mindamage", "maxdamage", "minrangedamage", "maxrangedamage", "attackpower" and "rangedattackpower" from creature_template as those values aren't needed anymore.
  • Fix all code that references those columns.
  • Implement BaseVariance and RangeVariance (mostly for custom mobs).

Issues Addressed:

Tests Performed:

  • Tested "reload creature_template".
  • Used the Spell "Beast Lore (ID=1462) on around 20 different mobs to check their min/max damage values. Those values now correctly change if a mob is for example Level 35 vs Level 36. All values I tested so far have been ~99% to the pre-calculated min/max values so it seems like the new damage_base values are correct or atleast much better than the values we had before.
  • Played for a few hours on my offline realm and everything I tried looks great.

How to Test the Changes:

  • Download the PR, compile the code and apply the DB update.
  • Make a Hunter or simply learn spell 1462.
  • Fly around the world and check the mobs min/max attack values.

Known Issues and TODO List:

  • I have no way to confirm the values I took from TC are 100% accurate. BUT they are way better than the precalculated values we used before and every mob I tested has been correct.

Target Branch(es):

  • Master

How to Test AzerothCore PRs

When a PR is ready to be tested, it will be marked as [WAITING TO BE TESTED].

You can help by testing PRs and writing your feedback here in the PR's page on GitHub. Follow the instructions here:

http://www.azerothcore.org/wiki/How-to-test-a-PR

- Re enable the CLS dmg calculation.
- Fix the error in how min/max dmg are calculated
- Update a bunch of values for damage_base and attackpower which have been wrong before
- Remove min/maxdmg and min/maxrangedmg from creature_template as they aren't used anymore.
@FrancescoBorzi
Copy link
Contributor

@T1ti

@PkllonG
Copy link
Contributor

PkllonG commented Mar 7, 2021

After the test, I felt the damage value was higher
player level 70
.go c id 17711
before 10000-15000
after 15000-20000
Maybe that's the right damage?

@T1ti
Copy link
Contributor

T1ti commented Mar 7, 2021

After the test, I felt the damage value was higher
player level 70
.go c id 17711
before 10000-15000
after 15000-20000
Maybe that's the right damage?

It's still WIP, dmg modifier is not updated yet. This creature you're testing has 35 dmg modifier and modifiers work differently between the two systems

You can try creatures that have x1 dmg modifier for now

Also he hasn't updated BC/Wrath values yet

@Hacki95
Copy link
Contributor Author

Hacki95 commented Mar 7, 2021

Added the missing DamageModifiers for Elite mobs/Bosses etc. And I removed the line from cs_reload()

@T1ti
Copy link
Contributor

T1ti commented Mar 8, 2021

It's looking good. Different levels have different values as it should
image
image

I have tested some creatures dmg like magmadar which are off (even on TC) and will need to be corrected
image
But this system makes it VERY easy to find the right values and blizzard most of the time uses a simple number. (since we now use the same system as blizz)
For example, modifier 17 gives the exact correct value for magmadar
image (I made a sheet formula
image)

-You need to delete the attack power column in creature_template and load the creature's AP from the CLS
-One column which is not entirely necessary but nice to give some flexibility to users, is to add a "variance" column, to let the users having custom damage range and not always max = min*1.5 (I saw someone complaining about that) TC already has that implemented.

But otherwise it looks pretty good atm, is off hands calculated properly ? TC did several commits for it.
Good job !

@Hacki95
Copy link
Contributor Author

Hacki95 commented Mar 8, 2021

I will take a look at the Attackpower column.
Should I fix the value for Magmadar in this PR?

Also about the variance, I might implement this but I am not sure yet. Mainly because I am not 100% sure how this mechanic is supposed to work.
float baseValue = GetFlatModifierValue(unitMod, BASE_VALUE) + (attackPower / 3.5f) * variance;
Variance is multiplied with the baseValue and not directly for the maxdmg. I think this needs some more research before I would blindly copy those lines without understanding if blizz had something like this. Maybe this is something for another PR but I will take a look.

About Off-Hands I am tbh not 100% sure, I assumed our calculations are already correct for this. I will double check!

Thanks for the feedback I appreciate it.

cInfo->lootid = fields[43].GetUInt32();
cInfo->pickpocketLootId = fields[44].GetUInt32();
cInfo->SkinLootId = fields[45].GetUInt32();
cInfo->DifficultyEntry[0] = fields[1].GetUInt32();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or wrong, it should start from 0
cInfo->DifficultyEntry[0] = fields[0].GetUInt32();

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right I had an error in my thinking here, will update now

@PkllonG
Copy link
Contributor

PkllonG commented Mar 8, 2021

It's a good damage calculation system, we should finish it anyway

@Hacki95
Copy link
Contributor Author

Hacki95 commented Mar 8, 2021

It's a good damage calculation system, we should finish it anyway

Are you talking about the variance system or in general? I need some input if I should add that or better leave it out for now.

@PkllonG
Copy link
Contributor

PkllonG commented Mar 8, 2021

I'm talking about current pr

@PkllonG
Copy link
Contributor

PkllonG commented Mar 8, 2021

If you need to test anything, please let me know

@Hacki95
Copy link
Contributor Author

Hacki95 commented Mar 8, 2021

If you need to test anything, please let me know

If you know how we could test if the Off-Hand damage is calculated correctly that would be really useful.

@PkllonG
Copy link
Contributor

PkllonG commented Mar 8, 2021

I'm afraid I can't help you. I'm just a tester. My coding knowledge is too poor

@Hacki95
Copy link
Contributor Author

Hacki95 commented Mar 8, 2021

I meant by testing if the values seem to be correct, I dont even know which NPCs I could check :D But no problem. maybe @T1ti has an idea.

@PkllonG
Copy link
Contributor

PkllonG commented Mar 8, 2021

I think we should restore all previously changed DamageModifier values

@Footman
Copy link
Contributor

Footman commented Mar 16, 2021

Like I said I got feedback from top Raid groups in 25 heroic mode. They were very angry and have never seen such high damage on other servers. A lot of creatures do too much damage that tanks with top Gear can't keep up anymore. I had to revert those changes for now.
I don't know what is causing this. Just telling facts.

@Hacki95
Copy link
Contributor Author

Hacki95 commented Mar 16, 2021

I'm not saying that what you reported is wrong, after all the damage IS higher than before! I am just trying to say that the new values are the actual values how they should be and the values before were very undertuned, making HC too easy.

https://wowwiki-archive.fandom.com/wiki/Lich_King_(Icecrown_Citadel_tactics) (Lich King in 25Normal hits for around 40k)

With the CLS system:
grafik

As you can see for 25normal he now does the expected damage. I sadly can't find a single source which states for how much meele damage the Lich King hits for in 25HC, but all other values seem to be correct (10, 10hc, 25).

It could be that the modificator for 25hc is simply wrong and needs some tuning, but so far I haven't found any information that points to this being the case.

If your players say they never saw such damage before then in my mind, they either only played on other AzerothCore based servers which ofcourse were all undertuned aswell before this PR.

Or if they were on TC based servers, then they must have manually nerfed bosses in HC.

Because with this in place we do exactly the same damage as TC. Which makes me think if the damage would have been wrong in TC for years surely there would be a bug report somewhere but there isn't.

I will try to get into contact with someone who might be able to confirm/deny if the Lich King really does so much damage in HC25.

Until then, this is an issue regarding the the damage_modifier, not the system itself. After this PR gets merged there will probably be quite a handful of mobs which need some sort of tuning.

@Knindzagxg were you able to do some tests yet? No more errors have been found so far.

@Knindzagxg
Copy link
Member

For now it looks good.

What is really great about whole PR, that we can just change / modify damagemodifier if we need to fix some boss.
When it comes to dungeons that i tested, there is no to much changes and i actually like it - Kinda makes it a little harder due class changes.

What i wasn't able to test due my time are Demo Shouts.

Can someone test what happens with any boss damage when you use Bear or Warrior Demoralizing shout.
We had big issue with that as well, and i wonder will this PR make it work as it should.

@Hacki95
Copy link
Contributor Author

Hacki95 commented Mar 16, 2021

For now it looks good.

What is really great about whole PR, that we can just change / modify damagemodifier if we need to fix some boss.
When it comes to dungeons that i tested, there is no to much changes and i actually like it - Kinda makes it a little harder due class changes.

What i wasn't able to test due my time are Demo Shouts.

Can someone test what happens with any boss damage when you use Bear or Warrior Demoralizing shout.
We had big issue with that as well, and i wonder will this PR make it work as it should.

Expected damage without Demoralizing Shout:
grafik
Damage done:
grafik

Expected damage with Demoralizing Shout:
grafik
Damage done:
grafik

Looks like its working great!

@PkllonG
Copy link
Contributor

PkllonG commented Mar 17, 2021

image
Alone, my graduate defense Paladin 25H ICC
I don't know if the damage is high or low

@mpfans
Copy link
Contributor

mpfans commented Mar 18, 2021

image
Alone, my graduate defense Paladin 25H ICC
I don't know if the damage is high or low

ICC老1需要分担伤害.所以这个测试我感觉没实际意义

@Knindzagxg
Copy link
Member

Awesome, finally Shout working.

@pk will check ICC later tonight or tomorrow.

@Hacki95
Copy link
Contributor Author

Hacki95 commented Mar 18, 2021

image
Alone, my graduate defense Paladin 25H ICC
I don't know if the damage is high or low

It looks about right for HC

@Knindzagxg
Copy link
Member

Damage looks fine pk.

Also, if possible when checking those, can u add small ss of your tanking stats, so we can see how it would work on lower geared / not defense capped and things like.

@Hacki95
Copy link
Contributor Author

Hacki95 commented Mar 18, 2021

@Knindzagxg @FrancescoBorzi Can this be marked as "to be merged" now? The system is thoroughly tested.

The only thing left are wrong damage_modifiers and those will be much easier to correct when the general public can get their hands on it. I'm not the right guy to ask for the correct DB values anyway.

@Knindzagxg
Copy link
Member

Give it few more days, if not problem.

I wanna do few cross checking and to make few files about damage in raids mostly.
In case we gonna need to change some values i will have it ready.

@Knindzagxg
Copy link
Member

Had some extra time today so files are ready in case we gonna need them.

This should improve overall damage -> Not much, but looks much better.

PS: In case there is an issue with some Boss, pretty please provide a bit more extra information (SS of your gear, defense, recount or etc - Will help a lot.)

@mpfans
Copy link
Contributor

mpfans commented Mar 19, 2021

My server has 1500 + online players and has been tested for about 2 weeks. I think this PR can be merged. If you encounter incorrect damage, can fix it separately

@Si1ker Si1ker merged commit 2031e55 into azerothcore:master Mar 19, 2021
@Footman
Copy link
Contributor

Footman commented Mar 20, 2021

My server has 1500 + online players and has been tested for about 2 weeks. I think this PR can be merged. If you encounter incorrect damage, can fix it separately

but you dont have them playing wotlk with ICC

@Hacki95
Copy link
Contributor Author

Hacki95 commented Mar 20, 2021

My server has 1500 + online players and has been tested for about 2 weeks. I think this PR can be merged. If you encounter incorrect damage, can fix it separately

but you dont have them playing wotlk with ICC

There is nothing pointing to ICC HC being broken, yes it was easier before but it also wasn't right.

If you want to make 25HC easier for your players simply tune down the damage_modifier of the offending bosses, but it also won't be blizzlike that way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update the creature damage system to CLS, too many issues currently. [$50 awarded]
9 participants