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 "Argon" performance points counter #27498

Merged
merged 12 commits into from Mar 8, 2024
Merged

Conversation

frenzibyte
Copy link
Member

@frenzibyte frenzibyte commented Mar 5, 2024

This was supposed to be implemented very long ago but things happened and it fell off my radar. Anyway, here it is.

Preview:

CleanShot.2024-03-05.at.03.56.14.mp4

The counter's design is mostly based off figma, unlike the small number design in #25362. Placement choice is completely arbitrary and likely not going live without expert touches (especially the label flying off to the left as the digits of the PP counter increases).

@frenzibyte
Copy link
Member Author

frenzibyte commented Mar 5, 2024

I don't quite understand this test failure (it's in deserialisation skin test), I'll look at it later/tomorrow unless someone beats me to it.

@peppy
Copy link
Sponsor Member

peppy commented Mar 6, 2024

Should probably have 3 digits placeholder to avoid the shifting label in 98% of cases.

Also I'd probably size it a bit smaller. compared to accuracy if we're putting it there.

@arflyte any input?

@frenzibyte
Copy link
Member Author

I thought about adding that at first for the same reason, I can't remember why I was against it though. I'll add it and see how it feels.

@arflyte
Copy link

arflyte commented Mar 7, 2024

The label must not shift in any circumstances. I'd suggest keeping it to 3 digits and show 999 if it goes beyond it. Put Accuracy and PP in the same line instead of stacked; this will fix left aligned contents when placed on the right side of the screen.

(I just noticed the vibrating combo label, but that's fine; it gives a bit of flavour there.)

@peppy
Copy link
Sponsor Member

peppy commented Mar 7, 2024

show 999 if it goes beyond it

what. i don't think you understand pp? it WILL go above 1,000 in high level play.

@Givikap120
Copy link
Contributor

show 999 if it goes beyond it

what. i don't think you understand pp? it WILL go above 1,000 in high level play.

Well, 4 digit placeholder is good, cuz it won't be higher than 4 digit in a human play
But even tho it will only occurs in auto scores - i don't think clamping it at 9999 is a good idea

@peppy
Copy link
Sponsor Member

peppy commented Mar 7, 2024

I guess I have to @arflyte highlight every time to actually get a response? @arflyte

@arflyte
Copy link

arflyte commented Mar 7, 2024

Yeah lol, I can't keep up with the PP thing now. I thought reaching 4 digits was inhuman or something lol.

Then put both Accuracy and PP in line with 4 digits. It'd be best if it comes with toggle, because it feels weird to have 4th digit mostly unused; but probably that'd give motivation for beginners to hit 4 digits i guess.

@peppy
Copy link
Sponsor Member

peppy commented Mar 8, 2024

I'm going to disagree on this one then. Please use 3 digits of placeholder for now and allow the label to move on hitting four.

@peppy peppy self-requested a review March 8, 2024 01:35
@peppy
Copy link
Sponsor Member

peppy commented Mar 8, 2024

I don't quite understand this test failure (it's in deserialisation skin test), I'll look at it later/tomorrow unless someone beats me to it.

3 days later so I'm taking over this PR.

The test failure is because you renamed a previous non-abstract type to now be a base type. This is a breaking change and what that test is intended to catch. It means that skins which used this type will actually be broken by this PR.

I'll look at fixing.

@peppy peppy enabled auto-merge March 8, 2024 02:32
@peppy peppy disabled auto-merge March 8, 2024 03:16
@peppy peppy merged commit 6323189 into ppy:master Mar 8, 2024
11 of 17 checks passed
@arflyte
Copy link

arflyte commented Mar 8, 2024

Hmm, it's probably wouldn't be that jarring if the label moves if both stats are inline. I guess it's time to explore grouping options for the skin editor.

@frenzibyte frenzibyte deleted the argon-pp-counter branch March 8, 2024 08:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants