World damage/healing, destructible buildings, multiplier rewrite, logging improvements

kjack9 commented 1 year ago

Changes Proposed:

This is a fairly major rewrite of large parts of AutoBalance. While the user-facing side of things (the .conf file) is hardly changed at all, the internals behind it are quite different.

Observable changes

Handle damage and healing that doesn't come from a creature (world damage, self-damage, NPC healing, etc)
Destructible buildings have the damage done to them scaled appropriately based on the scaling settings
More reliable boss and boss summon detection - both get treated as bosses for multipliers
Rewrite of in-combat locking for players leaving the instance during combat - if any player in the instance is in combat, the difficulty only goes UP until combat ends
Reduce "level up animation" events on creatures by setting their level before they're spawned into the world
Improved player notifications when entering/exiting dungeons
Add commands and logger info to README.md, update the GitHub bug template

Less-obvious changes

Complete rewrite of the level/player scaling process, separating many pieces of repeated code into functions
Improved handling of GMs entering/exiting the instance
Handle debuffs that do "X% of player health" damage correctly - do not level scale
Performance improvements, with fewer unneeded checks being performed
Fewer incorrectly-scaled creatures (flavor critters, player summons, friendly NPCs, other irrelevant creatures)
Improved .ab mapstat and .ab creaturestat output
Many many many many debug logs added or changed, including the start of splitting the logs into different logging sinks. Detailed multiplier calculation logs, damage adjustment logs, combat locking logs.
Many settings are now stored in the map data itself, paving the way for map-specific settings in the future

Issues Addressed:

Closes #148
Closes #142
Closes #40

SOURCE:

N/A

Tests Performed:

tested on latest AC with four people doing multiple dungeons and raids

Known Issues and TODO List

disabled for Battlegrounds entirely. It already wasn't working correctly and was causing crashes in some cases (#148). I have it in my backlog to try to re-add it eventually.
disabled LevelScaling.EndGameBoost which is currently WAAAAY overtuned (#156). I need to figure out how the original developers of the module came up with the multipliers they did, and if they are still needed given how I'm generating multipliers now.
158

I'm tempted to work on these before submitting this PR but... do you really want this to be any larger?

How to Test the Changes:

Use AutoBalance as normal. The user-facing changes should be minimal. However, there will likely be encounters that were previously scaled incorrectly and may now be possible.

Helias commented 1 year ago

very big changes, hope some authors of the issues is available to test this @podmepodme @WangDianhui @therelus @BarbzYHOOL

jshplayer commented 1 year ago

Good stuff, addressed issues look good

Helias commented 1 year ago

Good stuff, addressed issues look good

perfect, did you test it on your server?

WangDianhui commented 1 year ago

Tested in my server. Faction champions in TOC is fixed. Previously buggy Blood council in ICC also works well now.

kjack9 commented 1 year ago

~~I found a small regression - totems for enemies are not being level scaled currently. I will work on a fix.~~

pangolp commented 1 year ago

Particularly, I prefer to work on smaller pull requests, because if you had to revert any of these changes, you would practically have to revert everything. However, it seems like it's a pretty big deal, so if you guys think it's okay, that's fine by me. I would leave it here, and not add anything else, and at most, once the merger is done, in any case, I would correct any possible errors, or add new improvements in other pull requests. I wouldn't modify this anymore.

kjack9 commented 1 year ago

~I found a small regression - totems for enemies are not being level scaled currently. I will work on a fix.~

Fixed in that last commit.

kjack9 commented 1 year ago

Particularly, I prefer to work on smaller pull requests, because if you had to revert any of these changes, you would practically have to revert everything. However, it seems like it's a pretty big deal, so if you guys think it's okay, that's fine by me. I would leave it here, and not add anything else, and at most, once the merger is done, in any case, I would correct any possible errors, or add new improvements in other pull requests. I wouldn't modify this anymore.

I totally hear you and agree in principal. So far I've been pretty bad at doing small PRs.

In my defense this time, very little is changing from the user's perspective. Some stuff that didn't work will work now, but the operation of the module will largely remain the same.

A ton of the behind-the-scenes work I'm doing is on edge cases, code organization, and fully commenting/debug logging the entire module. When I started there was some pretty cryptic stuff happening in the code that I've done my best to un-cryptify (definitely a word, don't look it up). There was also very little to no logging, which made troubleshooting issues a bear.

So while yeah, this PR is quite large, the mod is doing pretty close to the same things it was doing before, but hopefully more consistently and with code that is easier for someone else to decipher and troubleshoot. If it helps, I have a whole list of new features I'd love to implement that I've held off on until we get this one merged in.

I also struggle with how to maintain momentum working on this thing (I'm hot/cold when it comes to motivation to work on stuff) if I submit small PRs and they take a week each to get approved. There's no way the work I did here would have happened if I'd submitted a PR after the first couple of commits.

So again, sorry - I know it's not ideal. Hopefully the gains are worth the headache, and that you and the module users can see the difference in the results they're getting.

Thank you for attending my TED talk.

WangDianhui commented 1 year ago

Not working on heroic ICC and Ruby sanctum. The map level is zero.

But working on all normal raids and heroic TOC.

kjack9 commented 1 year ago

Not working on heroic ICC and Ruby sanctum. The map level is zero.
But working on all normal raids and heroic TOC.

I will check it out, thank you! I wonder why your LFG doesn't have levels for those? I may need to stop relying on that for level information...

EDIT: Confirmed, heroic ICC doesn't have LFG information. Hrm, I will have to think about how to handle that. I'm using that info for some decision making...

EDIT2: I think I'm going to just pull the info from the normal version. For my purposes, that will be okay for checking creature levels and the like.

kjack9 commented 1 year ago

Not working on heroic ICC and Ruby sanctum. The map level is zero.
But working on all normal raids and heroic TOC.

This is fixed now, can you please test with the latest commit?

If a particular heroic dungeon has no entry in the LFG system, the non-heroic version will be used to get level data.

WangDianhui commented 1 year ago

Not working on heroic ICC and Ruby sanctum. The map level is zero. But working on all normal raids and heroic TOC.

This is fixed now, can you please test with the latest commit?

If a particular heroic dungeon has no entry in the LFG system, the non-heroic version will be used to get level data.

Tested, Heroic ICC looks good now, thanks for the effort!

But there brings a new problem to Halion

Normal procedure:

Damage white Halion to 75% HP
White Halion disappear, black Halion shows up in twilight realm with 75% HP
Smash black Halion to 50% HP
White Halion shows up again with 50% HP
Beat Halion in both realm to death. The HP of white and black Halion is linked.

After the update of the mod, black Halion shows up with only 12% of HP in step2. So step3 is skipped, and in step4, white Halion show up with 75% of HP.

I think the problem is that when black Halion spawn in twilight realm, an extra modifier is applied to it. 0.15 in this case.

kjack9 commented 1 year ago

Tested, Heroic ICC looks good now, thanks for the effort!

But there brings a new problem to Halion

Normal procedure:
1. Damage white Halion to 75% HP

2. White Halion disappear, black Halion shows up in twilight realm with 75% HP

3. Smash black Halion to 50% HP

4. White Halion shows up again with 50% HP

5. Beat Halion in both realm to death. The HP of white and black Halion is linked.
After the update of the mod, black Halion shows up with only 12% of HP in step2. So step3 is skipped, and in step4, white Halion show up with 75% of HP.

I think the problem is that when black Halion spawn in twilight realm, an extra modifier is applied to it. 0.15 in this case.

Got it, will see what I can do.

kjack9 commented 1 year ago

But there brings a new problem to Halion

Well, this was trickier to fix than I expected.

The problem is that when a clone is made, the max health gets reset to the original max health for the new clone. However, the current health value (whatever the creature's actual health value is) is COPIED onto the new clone. That means that the max health was getting (correctly) scaled down but the current health was ALSO getting (incorrectly) scaled down.

Funny enough, it's really hard to tell when this cloning process has happened. To handle it, I created a simple scoring system that checks several properties of the summon and its summoner to determine if it is a clone. If it is, the max health is still scaled but the current health is left unchanged.

I tested both Halion (39863, clone is 40142) and Baltharus the Warborn (39751, clone is 39899) and everything is now working as intended. I also tested some other summoning situations (Putricide's Trap, 38879) and it did NOT break situations where the creature is just summoned, but not cloned.

I updated .ab creaturestat so that it will show if a creature is a clone, a summon with a summoner, or a summon without a summoner.

Please test Halion with this latest version and see if the issue is resolved. I'd also appreciate you thinking about any other situation that uses a similar cloning mechanic that we can test.

Thank you for testing!

WangDianhui commented 1 year ago

Please test Halion with this latest version and see if the issue is resolved. I'd also appreciate you thinking about any other situation that uses a similar cloning mechanic that we can test.

Halion now acts without problem in our 5-man party fight. Then I tested some more bosses with linked hp: Dark council in Black Temple: no issue

Twin Emperor in Ahn'Qiraj: The source of cloned damage is boss itself... So an unintented modifier is applied to it

wrong damage:

right damage:

Twin Val'kyr in TOC: Very strange, their hp is correctly modified when spawned, but will fall to 1 immediately when contacted. And there is no damage log in console.

kjack9 commented 1 year ago

Twin Emperor in Ahn'Qiraj: The source of cloned damage is boss itself... So an unintented modifier is applied to it

Unless I misunderstand (EDIT: turns out I do) , I think it is working as it should. I'm not super familiar with the fight, so correct me if I'm wrong.The boss' damage to itself SHOULD be scaled, right? Otherwise you'd run into a situation where the boss hits itself for the original value (3837) even though his health is drastically reduced. The intention isn't to make the fight go faster - it's to make it feel about the same no matter how many players you have. So unless I'm missing something I think this is Working as Intended.

EDIT: Duh, I see now. That's the spell that ensures their health stays in-sync. I think a spell blacklist is probably called for here.

Twin Val'kyr in TOC: Very strange, their hp is correctly modified when spawned, but will fall to 1 immediately when contacted. And there is no damage log in console.

I will investigate this!

kjack9 commented 1 year ago

@WangDianhui

Twin Emps fixed in https://github.com/azerothcore/mod-autobalance/pull/157/commits/e3a9f80fdd41c2567f92aa8d5db8d95d6b677638 with a better permanent fix proposed to AC core in https://github.com/azerothcore/azerothcore-wotlk/pull/17532 .

I now have a mechanism by which I can list SpellIDs that should never be modified. I expect this probably isn't the last time we'll need it.

Twin Val'kyr is up next. So far I can replicate exactly what you're describing - scaling looks great until they're engaged then they drop to 1 health. Disabling AutoBalance globally (without recompiling it out) fixes the issue.

I classify the issue as "super weird". More to come.

kjack9 commented 12 months ago

@WangDianhui

BAM! Fixed Twin Val'kyr by moving the initial stat scaling to earlier in the creature creation process. The boss script was getting very confused about how much max health the Val'kyr had. Now I alter it before it (or any other boss script) sees the creature.

What else you got?!? :smile:

WangDianhui commented 12 months ago

What else you got?!? 😄

I've played through ICC, TOC, ULD, NAXX, Eyes and many other dungeons, didn't find any problems.

Really a fantastic job!

azerothcore / mod-autobalance