Krappa322 / arcdps_healing_stats

An addon for ArcDPS that shows personal healing stats
MIT License
71 stars 10 forks source link

Barrier #39

Closed jackpoz closed 1 month ago

jackpoz commented 2 months ago

This PR includes the work done at https://github.com/bear-on-the-job/arcdps_healing_stats plus some cleanup to reduce branch differences and make the review easier. Credits go to @bear-on-the-job bear-on-the-job , the original author of the code. I have been using this for a few weeks and I have had no issues that I could find.

Changes to the original version:

Suggestions not included:

See https://github.com/bear-on-the-job/arcdps_healing_stats/releases/tag/v2.8.1.1 for details about the original version.

Krappa322 commented 2 months ago

My concerns with this are the same as my concerns in https://github.com/Krappa322/arcdps_healing_stats/pull/33 :

Some thoughts:

  1. Could we make the option to include/exclude barrier generation per window rather than global? I imagine this would be much more useful since then you can keep both windows up at the same time and you could separate the two. Perhaps the proper place in the UI is under "stats exclude" and there you have an option for "healing" and one for "barrier generation" 1.b If we do that, maybe we could also paint the bars in the window based on how much you have of each? So if you do 70% healing and 30% barrier generation, we paint a bar that's 70% green then 30% beige? 1.c Let's maybe also add an option under the "Sort by" option where you can select to sort by barrier generation (ascending/descending)? And I guess one for healing + barrier as well ("Total outgoing" maybe?) 1.d Let's maybe also add a new window preset which is "Peers Barrier Generation"?
  2. Can we try to consistently call it "barrier generation" rather than just "barrier" as I think it's unclear what means what. You can obviously pad your own stats quite a bit by just spamming barrier off cooldown but that's hardly being a good player, and it should be obvious to everyone reading the stats that they are seeing the barrier generation, not the used barrier (i.e. the generated and consumed barrier)

For the actual implementation on 1., I imagine you have to tag the events whether they are healing or barrier when storing them, but that shouldn't be too hard. Then aggregate all the barrier into a separate variable during the aggregation

jackpoz commented 2 months ago

As you know the code the best, are these changes something that you could implement, using this PR as a draft ? Barrier is used a lot in wvw, where your healing addon is a must-have for healers and support classes, so it would make a lot of players happy to have barrier support.

jackpoz commented 1 month ago

Moved exclusion per window: image

image

jackpoz commented 1 month ago

About

paint the bars in the window based on how much you have of each? So if you do 70% healing and 30% barrier generation, we paint a bar that's 70% green then 30% beige ?

I don't think this makes sense. It's a lot easier to see what is healing and what is barrier by keeping the same color for each: image

Even without reading the title of each window, keeping green for heal and yellow for barrier makes it very easy to recognize what each window is showing.

jackpoz commented 1 month ago

As for

Can we try to consistently call it "barrier generation" rather than just "barrier"

image should we call it "barrier gen" in places with little space, like the details window ?

jackpoz commented 1 month ago

Added preset "Peers barrier generation": image

By default "Peers outgoing" excludes barrier (unless it should be changed): image

jackpoz commented 1 month ago

Implemented sorting.

Sort by heal only, descending: image

Sort by barrier only, descending: image

Sort by total outgoing: image

Krappa322 commented 1 month ago

paint the bars in the window based on how much you have of each? So if you do 70% healing and 30% barrier generation, we paint a bar that's 70% green then 30% beige ?

I think I just badly worded it - I meant what you have here. The first PR colored them all green iirc.

Thanks so much for this, will try to look into it asap!

jackpoz commented 1 month ago

Would you prefer barrier generation to be excluded by default or included by default ? Right now it's excluded by default.

jackpoz commented 1 month ago

Merged master into this branch and fixed the merge conflicts.

Krappa322 commented 1 month ago

Would you prefer barrier generation to be excluded by default or included by default ? Right now it's excluded by default.

I think it probably makes the most sense to have it excluded by default. If nothing else because that way it doesn't change all existing windows for users after they upgrade.

Krappa322 commented 1 month ago

Anything else you'd like to have done? If not I'll merge this and create a pre-release with it

jackpoz commented 1 month ago

It's ready for me too, there is plenty of space for more pull requests in the future if we want to change something 😃