Optimized Blessing Auto Assignments #13
Conversation
|
Hello @nmonterroso, thank you for your contribution to pallypower. |
|
@SaschaJankowiak |
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 :) |
|
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 😄 |
|
just remove redundant/fallback stuff so we get the "finished/cleaned up" state as soon as possible :) |
|
@SaschaJankowiak, I removed the fallback in 4688e32. Hope it works for you today! |
|
@SaschaJankowiak any updates? |
There was a problem hiding this comment.
workinmg as intended in last tests. Approving this PR, @EsreverWoW / @Treeston what about the test-files? Moving into /test folder and ignore this for the packager?
PallyPower.lua
Outdated
| if buffer == pallys[i] then | ||
| tremove(pallys, i) | ||
|
|
||
| if not self:BuffSelectionOptimized(pallys, class, prioritylist) then |
There was a problem hiding this comment.
I took a quick look over your code, if I unserstand correctly - you added a new logic for the greater Blessing-Assignments "onTop" of the old AutoAssignment-Logic and you completely fallback to this old assignment-code when your logic "fails".
I think when we do a logic refresh we should get rid of the old code (BuffSelections stuff) entirely
PallyPowerAutoAssignment.lua
Outdated
| light = nil | ||
| end | ||
|
|
||
| local function table_contains(t, val) |
There was a problem hiding this comment.
in the WoW Lua-Api there is a native tContains - but i think you need this for your lua-tests outside the wow-interpreter. Maybe you should move this or polyfill it in your Test-Suite
There was a problem hiding this comment.
The tests are exactly why I made this function, but good idea on the polyfill, done in 237873d
test_auto_assignments_tbc.lua
Outdated
| @@ -0,0 +1,231 @@ | |||
| PallyPower = {isWrath = false} | |||
There was a problem hiding this comment.
if we want to keep the testing-stuff i propose moving them into a"test" subfolder, which can be excluded from the curse-packager.
There was a problem hiding this comment.
i moved these to a test/ folder, but lmk if we want to remove them entirely. i added an ignore section to .pkgmeta in 5d96f75. from a quick google search that's how to ignore directories, but i'm not sure how to test that, so lmk if that's correct or if there's something else that needs to be done for ignoring the tests.
FWIW, i think it's better to have them even if they aren't run as any CI. if there any changes to the auto assignment in the future, those expectations can serve as a starting point or to make sure the changes don't break anything. i also found those test files easier for iteration than having to constantly /reload after code changes, and let me use the debugger rather than relying on print statements.
|
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.) |
|
@SaschaJankowiak @Treeston all changes should be in, happy to remove the tests if that's what you want going forward, just lmk 😄 |
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
PallyPowerValuesin 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.luawhich 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.luaandtest_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.