search

valkyrnstudios / RankSentinel

MIT License
4 stars 3 forks source link

Rebuild Ability tables #7

Closed SabreValkyrn closed 2 years ago

SabreValkyrn commented 2 years ago

Rebuild ability tables scraping TBC databases.

Similar to https://github.com/Gogo1951/GogoWatch/issues/4

Pull directly from class lists to avoid mob spell IDs.

Might also yield pet abilities https://github.com/Gogo1951/GogoWatch/issues/13

Gogo1951 commented 2 years ago

Pet abilities I think had been included, but there was an issue where it didn't pair pets with their owners, so it didn't send messages to the owners.

Building in some sort of "Hey, your growl is on and we're in an instance..." for locks and hunters would be nice too. (=

Shame this never made it over to Classic. https://www.curseforge.com/wow/addons/badpet

Road-block commented 2 years ago

Part of the reason is perhaps that this particular niche is covered by some tank-oriented addons, like for example Simple Taunt Announce (Does warlock/hunter pets and shaman totem)

Gogo1951 commented 2 years ago

STA doesn't tell the hunter though. It just lets you, as the tank, know when someone else growls. I have it play a murlock sound. Ha. And when the hunter leaves growl on I just get spammed with "Mmmrrrggglll" over and over. Then I have to tell the hunter. But it'd be great to have something that automatically reminded the hunter.

I never want to do announcements like that in party / raid, much better as just a private tell. Last I used STA I don't think it had that output option like Bad Pet had.

SabreValkyrn commented 2 years ago

From Discord DM with Gogo, https://docs.google.com/spreadsheets/d/1jtx1WyfChzACzh0WBWANtrqkRtS3D-zPWqs3eOnyVvY/edit#gid=0

Working on skimming that data, massaging, and scripting conversion logic.

SabreValkyrn commented 2 years ago

Got the data itself in a good place, at least seems like a good place until I start trying to use it. PR draft at #13

Still need to rework combat parse logic.

Should I be concerned about the memory footprint of such a large table?

Not that this is a perfect comparison or representation of RAM footprint, but DataTables is 185,500 characters whereas the the current AbilityData https://github.com/valkyrnstudios/SpellSentinel/pull/13/files#diff-8ae75864741e465c0f802a487ecef578c9c8a5dee4f3f577e6f71bc1d5560609 is 189,400

"Foo (Rank N)" feels redundant and not useful, I'll remove it once I update combat log parsing to make sure it's not useful.

SabreValkyrn commented 2 years ago

Converted combat parsing to new data! Rudimentary testing seems good.

https://github.com/valkyrnstudios/SpellSentinel/pull/13/commits/3afc37f11dcd455b8dcebe54e34a5ccd1bb9cc9e

I'm not sold on my use of self.db.profile.isMaxRank or if that should be an in-session variable. Evaluating rank is a lot more processing intensive compared to GogoWatch's MaxLevel property, as it's a few dictionary lookups and comparisons so it felt like caching the results would save a non-trivial amount of processing time in a raid with the same people casting their abilities every 3 seconds.

Road-block commented 2 years ago

I haven't looked at the code but static memory is not a problem (other than longer loading screens depending when that data is put in memory)

The problem is cpu usage spikes either because of doing heavy operations or - and this is where memory usage can be an issue - memory churn, lots of garbage created (for example through frequent creation of tables that are then left hanging).
This can cause the Lua garbage collector to fire more often which also causes a cpu spike and can make the game "stutter" or show FPS drops.

TL;DR: High but static memory usage is not an issue, Heavy computation (especially in combat) or memory increase spikes are. Any time the choice is between for example compression to save memory at the cost of unpacking or packing data before they can be used, larger (but static) memory footprint is always the correct answer.

SabreValkyrn commented 2 years ago

(Over)Optimized a bit more, removed unused name entirely since there's no human curation on AbilityData.lua, doesn't need to be very discoverable. All data changes should happen in the GSheet and table be regenerated from that csv.

Also changed AbilityGroup from a "Class - Ability Name" string to a number for more efficient lookups during runtime. A number takes less space than a string and an indices lookup must be faster than a dictionary key lookup. https://github.com/valkyrnstudios/SpellSentinel/pull/13/commits/9494a035d50fdb35336ff92669f58c4396a98079

Not that this is a perfect comparison or representation of RAM footprint, but DataTables is 185,500 characters whereas the the [string lookup] AbilityData is 189,400

AbilityData.lua is now 99,900 characters using numerical lookup and seems as optimized as I can realistically get.