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

Add team buffs and bonus stats to stats display #2064

Merged
merged 3 commits into from
May 9, 2024

Conversation

frzyc
Copy link
Owner

@frzyc frzyc commented May 3, 2024

Describe your changes

Issue or discord link

Testing/validation

image

Checklist before requesting a review (leave this PR as draft if any part of this list is not done.)

  • I have commented my code in hard-to understand areas.
  • I have made corresponding changes to README or wiki.
  • For front-end changes, I have updated the corresponding English translations.
  • I have run yarn run mini-ci locally to validate format and lint.
  • If I have added a new library or app, I have updated the deployment scripts to ignore changes as needed

@frzyc frzyc requested review from lantua and nguyentvan7 May 3, 2024 04:07
Copy link
Contributor

github-actions bot commented May 3, 2024

[frontend] [Fri May 3 04:10:46 UTC 2024] - Deployed 04e821c to https://genshin-optimizer-prs.github.io/pr/2064/frontend (Takes 3-5 minutes after this completes to be available)

[frontend] [Fri May 3 06:22:32 UTC 2024] - Deployed c719550 to https://genshin-optimizer-prs.github.io/pr/2064/frontend (Takes 3-5 minutes after this completes to be available)

[frontend] [Sat May 4 05:21:41 UTC 2024] - Deployed ff1bdcb to https://genshin-optimizer-prs.github.io/pr/2064/frontend (Takes 3-5 minutes after this completes to be available)

[Thu May 9 07:14:51 UTC 2024] - Deleted deployment

@nguyentvan7
Copy link
Collaborator

I don't really like the idea of this without some distinction between stats and actual talent values

Or without the ability to hide specific things/collapse them someway? It's a ton of clutter imo

@frzyc
Copy link
Owner Author

frzyc commented May 3, 2024

I don't really like the idea of this without some distinction between stats and actual talent values

Or without the ability to hide specific things/collapse them someway? It's a ton of clutter imo

I agree on the clutter part, I have some ideas on reducing the clutter, since we are likely to increase the amount of content we show in the stat display in the future.

I guess i sort of get what you meant by "distinction between stats and actual talent values", I am just not sure what would be the way to go. We've always had basic stats as a section, so adding 2 more is not too crazy of a stretch. Stats are usually shown with stat icons as well, so its somewhat distinct.

@nguyentvan7
Copy link
Collaborator

I think we should do those clutter reduction steps before doing this

Distinction between stats and talent values was something you had discussed with Kite and agreed with iirc. I forgot what their mockup was for that tho

@frzyc
Copy link
Owner Author

frzyc commented May 3, 2024

I think we should do those clutter reduction steps before doing this

Distinction between stats and talent values was something you had discussed with Kite and agreed with iirc. I forgot what their mockup was for that tho

I understand where you are coming from, I don't agree with the suggested course of action. I don't have a concrete plan with decluttering, and I won't really know if decluttering is the right solution to this concern. I think getting feedback from the general user base will influence which decision to mitigate. My contemplative solutions towards decluttering will be somewhat sizable changes and I need to be convinced it's worth the effort.

@nguyentvan7
Copy link
Collaborator

I still don't really get the purpose of adding all these stats here. They are visible elsewhere, which is why I don't see the need to bulk up this display so much

@nguyentvan7
Copy link
Collaborator

Screenshot_20240503_081943_Chrome
Screenshot_20240503_081921_Chrome

Ignoring all the 0s being shown for some reason, this is just a ton of wasted space once you add something like Zhongli or a 4 element team

@frzyc
Copy link
Owner Author

frzyc commented May 3, 2024

I want to have a centralized display for all the stats and numbers. Bonus stats and team buffs are indeed shown in other parts of the UI, but they are generally somewhat hidden(team setting, uncollapse for each char; loadout setting, edit bonus stats).

All the 0s are unintended, I will address removing them.

@nguyentvan7
Copy link
Collaborator

But you can see those stats in the formula already. By this logic we should be showing distinction between self conditional stats too or even base stat vs arti/wep stats

@frzyc
Copy link
Owner Author

frzyc commented May 3, 2024

Can you explain to me what you think should be part of this display? I think we have a mismatch in terms of what we individually think is the purpose and intention of this piece of UI?

@nguyentvan7
Copy link
Collaborator

I think what it has now is appropriate. Final stats and outcomes/DMG/heals/etc for the current character. Showing teambuffs and bonus stats is not a direct final outcome of the character, and can be viewed elsewhere

@frzyc
Copy link
Owner Author

frzyc commented May 3, 2024

Well, I don't think we've explicitly declared that the stats here are "final outcomes based", I think that statement stands in general, but we do display some intermediary numbers as well, like shenhe's normal attack bonus(which shows up for teammates). If we want to draw the line, then those non-outcome numbers should show in talent tab, but not the "final stat display". I don't think we have the granularity in our system to fully support this, nor do I think it's worth the effort to do so.

I also don't like the argument that because the bonus stats/team buffs are shown in the formula of targets, it shouldn't be shown here. I don't think people will necessarily want to infer what their bonus stats/teambuffs are from the relevant formulas. Showing them here will help users understand at least from the input side what can contribute to their final results, instead of tracing backwards from their final results.

This also allows for more dynamic expression here as well, since users can compare changes to the team buffs due to different(artifact/weapon) builds. If you want to pursue having comparisons across different loadouts, this will be necessary as well.

@nguyentvan7
Copy link
Collaborator

Shenhe DMG bonus is an outcome of shenhe and only shows up on shenhes overview tab

There are numerous other things that contribute to a final DMG stat that still won't be covered by this (artifact sets, weapon stats, self conditionals, talent level, talent mechanics)

The only team buff that changes based on someone elses build is Nahida A1. Otherwise, any other scaling teambuffs will show automatically on the character overview page for the character it scales from (e.g. shenhe teambuff shows on her own overview page)

@frzyc
Copy link
Owner Author

frzyc commented May 4, 2024

There are numerous other things that contribute to a final DMG stat that still won't be covered by this (artifact sets, weapon stats, self conditionals, talent level, talent mechanics)

Artifact set, weapon stats are shown in the build above the stat display, self-conditionals and other conditionals are probably something we can tackle adding once we migrate to pando, since there will be more standardization, character stats like level/talents are something that I want to address as well(I actually have a good idea for that). All these are good points, but I won't be able to add them all in this one PR. I think once these are added in, the stat display will be a very "screenshottable" component, that has all the information.

@frzyc
Copy link
Owner Author

frzyc commented May 4, 2024

Hmm, this is weird, locally the team buffs are fine, but on PR deployment it has all the 0 fields
image

@nguyentvan7
Copy link
Collaborator

image

I still don't really like this because now the distinction between stat and targets is even more blurry. before, stats were always top-left. now they will be mixed in, and we had agreed that stats being mixed here (even in a consistent spot) was confusing for the user. Now that it is mixed in, its even more confusing

@frzyc
Copy link
Owner Author

frzyc commented May 9, 2024

image

I still don't really like this because now the distinction between stat and targets is even more blurry. before, stats were always top-left. now they will be mixed in, and we had agreed that stats being mixed here (even in a consistent spot) was confusing for the user. Now that it is mixed in, its even more confusing

I see. we could make it so that we dedicate the left column just for stats (base, bonus, teambuffs), what about stat displays outside of the 3 just mentioned? like these:
image
image

@nguyentvan7
Copy link
Collaborator

I guess I still considered those different because they are affected by hutaos final stats. but you're right, those could still be confusing to new users

@frzyc
Copy link
Owner Author

frzyc commented May 9, 2024

I guess I still considered those different because they are affected by hutaos final stats. but you're right, those could still be confusing to new users

Eitherway, I understand that from your perspective people may confuse these additions (stats fields) to be confusing, since they are not "outputs". I can understand where you are coming from. My perspective is that these fields were never intended to be only "outputs", but rather a place to put relevant calculated numbers. (not sure if you care about the perspective of the creator of this UI). I am not really agreeing with your enforcement of your perspective(that everything here should be "outcome" numbers), since that was not my intention with this UI in the first place.

I am still for releasing the UI additions in this PR as is, and get feedback on how users perceive their addition. I will pay extra attention to seeing whether people are confused about the type of the number, since its your main concern.

@frzyc frzyc merged commit ea65c99 into master May 9, 2024
6 checks passed
@frzyc frzyc deleted the teambuff_bonusstat_displaysection branch May 9, 2024 07:14
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.

None yet

3 participants