-
Notifications
You must be signed in to change notification settings - Fork 8
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
Issue 2 save normal blessings / allow Buff Presets #10
Issue 2 save normal blessings / allow Buff Presets #10
Conversation
one pull request per feature, please |
but it is - it's just 2 commits as the commit "lay-on-hands" has nothing to do with the feature itself - just noticed is was missing in the current master |
then make a separate PR for the lay on hands change, or remove it from this branch |
…nfig and holding shift. Loading is done by just clicking without modifier. Code is done with extendability in mind (e.g. multiple profiles)
eb3045a
to
7a1a01e
Compare
done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
feature cleanup
Just from a usability standpoint, it would make more sense to have two buttons - one for saving and one for loading of presets. Also would be helpful if you could give a name of the preset, new or existing, for saving and be able to choose which one to load. |
Yeah there is a lot of room for improvement when you think of this kind of feature. The Question is: is this an usable Feature which can extend current functionality without much hassle and a big ole "Profile Manager" or would this be enough in the first place to see if the big one will be requested much. About two Buttons: I thought of People who will use this Feature - mostly Class-Leaders/Officers/Raidleaders and their assistants (which i hope they can read Tooltips) and therefore i would like to not blow up this interface with a lot of Buttons. As for multiple Profiles, there would have to be another UI-Layer or some kind of submenu for managing these with good usability in mind |
the main usecase probably will be raiding with mostly the same People over and over again - when the roster changes alot you will have to manually do the setup anyway |
isn't the significantly easier fix just storing assigned small blessings by unit name? |
they would change in 10-man where u will have between 1-3 pallys on board - thus overwriting the 25-man assignments :) |
fair, i suppose... still feels like this is fixing around the actual problem, which is a lack of functional auto-assign |
Auto-Assign in its current form was optimized for classic-wow before normal-blessing-management existed and for a setup of 4-6+ Pallys in AQ40-Raids. And when it was not changed too much - it should do its job quite well in SoM. Doing a really well designed auto-assign would either need some kind of generally accepted static buff priority in each TBC/WOTLK by spec or a custom priority editor. |
LGTM code wise. did you test it, both as leader and not as leader? are you sure there aren't desyncs if you try to restore a preset as non-leader where other paladins' assignments do not agree with current? ex. Bob's currently set to big Kings, your preset has Bob set to big Might, what does your PP show after loading the preset? does it agree with what Bob's PP shows? |
I'll do some enhancements with this in mind (current push)- but that's untested and i cannot test this today |
…c buffs. Checks for being leader to show the Preset Button, prevent setting of buffs, which the pallys may not have skilled anymore, but were previously saved (e.g. kings, sanc)
finished feature (one-button-solution) with preventing async. tested in group and raid environment. Stays backward-compatible because no new comm- or datastructure is needed on foreign pallys |
would like to do a push with alle the changes, but i cannot login - serverlist not loading for testing... |
PTR not up either? |
PTR works, but without another pally to test with nothing will really happen :) |
you can have up to 4 PTR accounts, it's great for testing |
needs further testing ingame to check stability after refactoring
…e Preset-Feature (Auras are mostly GRP-/Encounter-/Situation-Specific
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did a lot of testing (thx to the multiple-ptr-accounts info!) and this worked fine. I excluded the feature of clearing the auras when calling ClearAssignments from the Preset-Function cause it didn't really felt well to have auras respond to the whole feature.
I think the Aura thing makes sense. |
Any more feedback on this? :) when there isn't anything against this we could probably push an alpha version to let the community give feedback. |
how does the localization-Stuff on curse work? Has the Key to be added manually or does curse scan the files to create the keys itself? |
I'll add it manually before merging the PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
functionality lgtm, just some minor cleanups needed
did cleanups and tested on ptr |
|
As written in #2 there is a big request in saving presets for your raid - especially the normalBlessings. I've done a basic Buff Preset function. Works by clicking "Preset" while holding shift to store current configuration and clicking without modifier to restore the buffs.
I've coded this with extendability in mind (e.g. do a submenu with multiple profiles later on). But for now this should work.
For the implementation i used the unused "PallyPower_SavedPresets" Variable allready referenced by the TOCs.
As for Localization there are 2 new Strings to add (my proposals):