search

AznamirWoW / PallyPower

The official repository of PallyPower for retail version of World of Warcraft Classic/TBC
Other
6 stars 6 forks source link

Optimized Blessing Auto Assignments #13

Open nmonterroso opened 1 year ago

nmonterroso commented 1 year ago

Updates the auto assignment to be a bit more optimized. In my experience the auto assignment feature isn't used too often because we end up with unoptimized assignments. Or, if it's used at all, it's used to just fill stuff in, and then change to what's ideal. Generally, this change aims to respect the templates in PallyPowerValues in group/raid environments with the highest ranks assigned as possible.

For example, if the buff priority is wisdom, might and there are two paladins in the group, with pallyA wisdom = 7, pallyB = 7 and pallyA might = 13, pallyB = 8, if pallyA is evaluated first then they will be assigned wisdom, and pallyB will be assigned might. Ideally in this scenario you'd want pallyA to do might since they have a higher skill, and pallyB on wisdom since they have the baseline skill.

When getting into talented buffs this gets a little messy, but hopefully the result is a step in the right direction 😄. The core changes are in PallyPowerAutoAssignments.lua which exposes one function: PallyPowerAutoAssignments.

I've been trying this out on TBCC the past couple days, and have logged into the wrath PTR to quickly verify that things work. I've also included some test files (test_auto_assignments_tbc.lua and test_auto_assignments_wrath.lua) that set up various scenarios with assertions. LMK if there's something I've missed or a test case I should cover.

SaschaJankowiak commented 1 year ago

Hello @nmonterroso,

thank you for your contribution to pallypower. Your code reads nice and clean and seems to fix one of the greatest current PP-Issues in AutoAssign. I will test this Ingame in my next Raid later this evening. More review-comments to come.

nmonterroso commented 1 year ago

@SaschaJankowiak Thanks, hoping it worked out for you in raid. Any updates?

SaschaJankowiak commented 1 year ago

@SaschaJankowiak Thanks, hoping it worked out for you in raid. Any updates?

yeah, i had no issues as far as i can tell but there was no time left while raiding to check if the new logic did the trick or if it was the fallback-one becouse with our pally comp - there would be no difference in the outcome (as expected) to see i think:

that was working before and now it is with your code too :)

nmonterroso commented 1 year ago

Ah, ok, that makes sense. What's the next step, then? Should I remove the fallback, or add some kind of temporary logging to indicate that it happened? Would love to get this into the next release so I can stop using a development version locally 😄

SaschaJankowiak commented 1 year ago

just remove redundant/fallback stuff so we get the "finished/cleaned up" state as soon as possible :)

nmonterroso commented 1 year ago

@SaschaJankowiak, I removed the fallback in 4688e32. Hope it works for you today!

nmonterroso commented 1 year ago

@SaschaJankowiak any updates?

Treeston commented 1 year ago

tests feel like something that would just lie unused anyway, so unless you want to actually use them properly (add CI integration for it), just ditch them.

ps building an entire new system while leaving the old auto-assign system (which is a colossal mess of terrible coding practices) in place sounds like a bad idea.

(but i don't call any shots here anymore, i stopped playing two months ago.)

nmonterroso commented 1 year ago

@SaschaJankowiak @Treeston all changes should be in, happy to remove the tests if that's what you want going forward, just lmk 😄