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

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

Merged
merged 51 commits into from
Oct 19, 2023

Conversation

kjack9
Copy link
Collaborator

@kjack9 kjack9 commented Oct 12, 2023

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:

SOURCE:

N/A

Tests Performed:

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

Known Issues and TODO List

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.

- Add worldHealthMultiplier, intended to be used to scale the health of attackable game objects in the instance
- tear out and re-implement all the damage handling to ensure more cases are handled
- use the map's damage modifier when damage happens where the source isn't a creature
- better handle when the source of damage no longer exists (logged out or despawned)
- extensive (optional) logging under the `module.AutoBalance.Damage` logging prefix
- WIP implementattion of game object damage handling (destructible buildings)
- fix a long-standing error in the class name for some logging
- need to apply statmodifiers still
- improved logging
- fix a bug with the enable check case statements (stupid case breaks)
- remove antiquated battleground checks - should never scale in battlegrounds
- better handle "special" creatures (totems, triggers, critters)
- allow combat critters to scale correctly
- allow intentionally-low-level triggers and critters to remain low level
- workaround an issue with summons showing the wrong level client-side (level change ramp up)
- further improve GM and user commands
- erase map AB info from maps when they are created, preventing issues with instance ID re-use
- better handle instances with no non-GM characters (disable scaling temporarily)
- removed unnecessary `std::string`s
- majory re-wrote the multiplicer calculations to increase accuracy and clarity
- take advantage of new `OnBeforeCreatureSelectLevel` hook to set the initial level of creatures before they are added to the world
- separated level-scaled and non-level-scaled multipliers for tracking and display
- move creature relevancy check to new function `isCreatureRelevant`
- rename many variables to standardize and clarify
- add option to force a map stats update, to be used when a player enters the map
- players leaving the map are no longer included in the map's stats
- sweeping improvements in logging of the multiplier calculation process (`AutoBalance.StatGeneration`)
- update `.ab creaturestat` to show new scaled modifiers in an understandable way
- replace kludgy player count detection with an internal player list (more features coming Soon)
- skip scaling on player-owned summons, totems, etc
- skip scaling on flavor critters but not on combat critters
- separate map and global config times, reducing the work to refresh if only a map config changes
- centralize detection of "non-relevant" creatures, use a `skipMe` tag to speed up processing
- correctly handle a GM leaving the instance who was previously a non-GM when they entered
- temporarily disable in-combat checking
- improve logging format to produce the ID needed for `.go creature <UUID>`
- standardize logging format
- moved enums to top the file, added `Relevance` enum
- improve performance of `isCreatureRelevant` but saving the decision from previous runs
- fix issues with summon detection to (hopefully) cover all edge cases
- consolidate and fix StatModifier case selection, add debugs
- more logging improvements all around
- boss summons now also count as a "boss"
- percent-based damage auras will now ignore level scaling (per-player scaling only)
- map multipliers (damage/healing and health) will now honor the level scaling setting
- use a custom struct to store and move around world multipliers
- properly remove LevelScalingEndGameBoost until it can be fixed (azerothcore#156)
- creatures summoned by a boss will now be scaled like bosses
- further refinement of trigger handling logic
- fix active creature not being decremented on creature removal
- fix incorrect announcing of GMs entering/exiting the instance
- improvements for in-game commands
- continued log improvements
@kjack9
Copy link
Collaborator Author

kjack9 commented Oct 13, 2023

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

@pangolp
Copy link
Contributor

pangolp commented Oct 13, 2023

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
Copy link
Collaborator Author

kjack9 commented Oct 14, 2023

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
Copy link
Collaborator Author

kjack9 commented Oct 14, 2023

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
Copy link

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

@kjack9
Copy link
Collaborator Author

kjack9 commented Oct 14, 2023

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
Copy link
Collaborator Author

kjack9 commented Oct 14, 2023

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?

image image

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

- fix display issue with active creatures
- that'll teach me to commit before I build
@WangDianhui
Copy link

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?

image

image

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:

  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.

@kjack9
Copy link
Collaborator Author

kjack9 commented Oct 15, 2023

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.

- track summoner as a part of AutoBalanceCreatureInfo
- detect when a summon is a clone and use the correct current health instead of scaling again
- add summon information to `.ab creaturestat`
- as always, logging improvements
@kjack9
Copy link
Collaborator Author

kjack9 commented Oct 16, 2023

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.

image

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.

image

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
Copy link

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:
35802c2c3822e81c35fa6b93da93fcb

right damage:
79e5c50cea4e7cc554b442d0992b879

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.
250d7878c5bfe71daa2c988fa7c6a0e
4b136e03a63ffc7ebfc88be7e5e8f80
a74fa4a6a0c7e633ec2d778a944dd3d

@kjack9
Copy link
Collaborator Author

kjack9 commented Oct 17, 2023

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!

- spells with SPELL_AURA_SHARE_DAMAGE_PCT effects will now correctly be unmodified
- added `spellIdsToNeverModify` and correctly skip modification of them
- add "Twin Empathy" (1177) to the `spellIdsToNeverModify` list
- convert `_IsAuraPercentDamage` to `_isAuraWithEffectType` since this probably isn't the last I'll need it
- logging changes, naturally
@kjack9
Copy link
Collaborator Author

kjack9 commented Oct 17, 2023

@WangDianhui

Twin Emps fixed in e3a9f80 with a better permanent fix proposed to AC core in azerothcore/azerothcore-wotlk#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
Copy link
Collaborator Author

kjack9 commented Oct 18, 2023

@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?!? 😄

@WangDianhui
Copy link

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!

@Helias Helias merged commit 50a5d5e into azerothcore:master Oct 19, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants