Implement "max pp" beatmap difficulty attribute text #30322
Conversation
| /// A working beatmap that caches its playable representation. | ||
| /// This is intended as single-use for when it is guaranteed that the playable beatmap can be reused. | ||
| /// </summary> | ||
| private class PlayableCachedWorkingBeatmap : IWorkingBeatmap |
There was a problem hiding this comment.
This is a hack because DifficultyCalculator only accepts an IWorkingBeatmap and I want to avoid double-fetching the playable beatmap. I'm not sure if there's a future where DifficultyCalculator accepts a playable beatmap, but it can be experimented with when it gets rewritten (something I'm in the progress of).
Though, perhaps we're already doing too many GetPlayableBeatmap() fetches elsewhere in the game (ahem - BeatmapInfoWedge) so this needs a more systematic solution at some point.
|
Would be nice to potentially also add a preview for 99% FC, 98% FC etc. like the pp calculator extension has. |
|
No thanks. You can use other tools for that. |
|
Can you elaborate more about this decision? You can use other tools for the max pp value as well, which makes me confused why my idea is rejected. |
Because it adds a lot of extra complexity for questionable benefit |
|
That's a much better answer |
|
Finally the day I've been waiting for max pp to go live has come. Awesome 🙌 |
osu.Game/Beatmaps/StarDifficulty.cs
Outdated
| Attributes = null; | ||
| DifficultyAttributes = null; |
There was a problem hiding this comment.
Completely irrelevant to anything but I'm not sure what this was even doing, looks 100% redundant to me 😅
There was a problem hiding this comment.
The rename or the property itself? It does appear to be used in a few locations if that's what you were referring to.
There was a problem hiding this comment.
I meant the null initialisation specifically.
| @@ -225,6 +228,9 @@ private LocalisableString getValueString(BeatmapAttribute attribute) | |||
| case BeatmapAttribute.StarRating: | |||
| return (starDifficulty?.Stars ?? 0).ToLocalisableString(@"F2"); | |||
|
|
|||
| case BeatmapAttribute.MaxPP: | |||
| return (starDifficulty?.PerformanceAttributes?.Total ?? 0).ToLocalisableString(@"F2"); | |||
There was a problem hiding this comment.
I find it a bit weird how this is rounded to 2 digits when the rolling pp counter that you can add to HUD rounds to full points. This leads to funky situations wherein you seem to get "more pp than the maximum" which actually is just a rounding artifact:
This is also the only display in the entire game that displays fractional performance points as far as I can tell. HUD counter doesn't, results screen display doesn't, performance breakdown doesn't. Even web views don't.
There was a problem hiding this comment.
That's a good point. I've rounded it.
There was a problem hiding this comment.
I remain in firm ideological opposition to the basic premise of this feature but I have no technical objections. I have one UX objection (see above) but maybe it's minor and can be disregarded.
Will leave final go/no go call to @peppy.
The rounding matches the implementation of `PerformancePointsCounter`.
Prereqs:
BeatmapAttributeTextshow values inclusive of mods #303212024-10-17.20-07-37.mp4
This is the second part of #29151 Whether it's agreed upon is likely up for discussion, and likely needs @peppy 's input.
Because the overhead of computing PP is very small, I've merged
PerformanceBreakdownCalculator's main logic intoBeatmapDifficultyCacheand is now computed alongside star difficulty. It makes a lot of contextual sense and removes the double async logic.Also it now uses the
ScoreProcessorfor score statistics + max combo, which are guaranteed to be accurate. There was a "todo: Get max combo from difficulty calculator" inPerformanceBreakdownCalculator's implementation which has been removed - if this leads to incorrect values then live values would be wrong, hence I believe this is the correct change to make.