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

Issue 2 save normal blessings / allow Buff Presets #10

Merged
merged 11 commits into from
Jul 25, 2022

Conversation

SaschaJankowiak
Copy link
Contributor

@SaschaJankowiak SaschaJankowiak commented Jul 19, 2022

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):

L["Preset"] = "Preset"
L["PRESET_TOOLIP"] =  [=[Load the last saved Preset 
and send them to all Pallys if Leader.

|cffffffff[Shift-Left-Click]|r Save a preset 
of all Greater and Normal Blessings 
currently configured.]=]

@Treeston
Copy link
Contributor

one pull request per feature, please

@SaschaJankowiak
Copy link
Contributor Author

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

@Treeston
Copy link
Contributor

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)
@SaschaJankowiak SaschaJankowiak force-pushed the issue-2-save-normal-blessings branch from eb3045a to 7a1a01e Compare July 19, 2022 13:59
@SaschaJankowiak
Copy link
Contributor Author

then make a separate PR for the lay on hands change, or remove it from this branch

done

Copy link
Contributor Author

@SaschaJankowiak SaschaJankowiak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

feature cleanup

@AznamirWoW
Copy link
Owner

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.

@SaschaJankowiak
Copy link
Contributor Author

SaschaJankowiak commented Jul 19, 2022

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

@SaschaJankowiak
Copy link
Contributor Author

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

@Treeston
Copy link
Contributor

isn't the significantly easier fix just storing assigned small blessings by unit name?

@SaschaJankowiak
Copy link
Contributor Author

they would change in 10-man where u will have between 1-3 pallys on board - thus overwriting the 25-man assignments :)

@Treeston
Copy link
Contributor

fair, i suppose...

still feels like this is fixing around the actual problem, which is a lack of functional auto-assign

@SaschaJankowiak
Copy link
Contributor Author

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.

PallyPower.lua Outdated Show resolved Hide resolved
@Treeston
Copy link
Contributor

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?

@SaschaJankowiak
Copy link
Contributor Author

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)
@SaschaJankowiak
Copy link
Contributor Author

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

PallyPower.lua Show resolved Hide resolved
PallyPower.lua Outdated Show resolved Hide resolved
PallyPower.lua Outdated Show resolved Hide resolved
PallyPower.lua Outdated Show resolved Hide resolved
PallyPower.lua Outdated Show resolved Hide resolved
PallyPower.lua Outdated Show resolved Hide resolved
PallyPower.lua Outdated Show resolved Hide resolved
PallyPower_Wrath.xml Outdated Show resolved Hide resolved
@SaschaJankowiak
Copy link
Contributor Author

would like to do a push with alle the changes, but i cannot login - serverlist not loading for testing...

@Treeston
Copy link
Contributor

PTR not up either?

@SaschaJankowiak
Copy link
Contributor Author

PTR works, but without another pally to test with nothing will really happen :)

@Treeston
Copy link
Contributor

Treeston commented Jul 21, 2022

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

@SaschaJankowiak SaschaJankowiak left a 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.

@Treeston
Copy link
Contributor

I think the Aura thing makes sense.

@SaschaJankowiak
Copy link
Contributor Author

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.

@SaschaJankowiak
Copy link
Contributor Author

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?

@Treeston
Copy link
Contributor

I'll add it manually before merging the PR.

Copy link
Contributor

@Treeston Treeston left a 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

PallyPower.lua Show resolved Hide resolved
PallyPower.lua Outdated Show resolved Hide resolved
PallyPower.lua Outdated Show resolved Hide resolved
PallyPower.lua Outdated Show resolved Hide resolved
PallyPower.lua Outdated Show resolved Hide resolved
PallyPowerValues.lua Show resolved Hide resolved
@SaschaJankowiak
Copy link
Contributor Author

functionality lgtm, just some minor cleanups needed

did cleanups and tested on ptr

PallyPower.lua Outdated Show resolved Hide resolved
PallyPower.lua Outdated Show resolved Hide resolved
PallyPower.lua Outdated Show resolved Hide resolved
PallyPower.lua Outdated Show resolved Hide resolved
@Treeston Treeston merged commit 8d94b30 into AznamirWoW:master Jul 25, 2022
@Treeston
Copy link
Contributor

v1.4.6-classic-alpha2 package kicked off

@SaschaJankowiak SaschaJankowiak deleted the issue-2-save-normal-blessings branch July 25, 2022 15:26
Treeston pushed a commit that referenced this pull request Jul 25, 2022
EsreverWoW added a commit that referenced this pull request Nov 28, 2022
Blame: 8d94b30 - Buff Presets (PR #10)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants