Fix advanced stats in beatmap info overlay showing "key count" on non-mania beatmaps #27380
Conversation
frenzibyte
changed the title
| { | ||
| Ratings.Ratings = BeatmapSet?.Ratings; | ||
| ratingBox.Alpha = BeatmapSet?.Status > 0 ? 1 : 0; | ||
| advanced.Ruleset.Value = rulesets.GetRuleset(beatmapInfo?.Ruleset.OnlineID ?? 0); |
There was a problem hiding this comment.
I dunno. I would have just changed AdvancedStats to have a ctor flag of something like "showConvertedRulesetInfo" rather than changing this whole flow.
The new flow reads very shoddy and is confusing. Like when do you hook up ruleset to beamap and when do you use game? It's not explained anywhere and bound to get used wrong in the future.
There was a problem hiding this comment.
I don't agree with that. I think boolean flags are shoddy and should only be used as a last resort. This seems better because it makes this a more "pure" component in that it has explicit inputs that it's reading rather some implicit resolution process as previously.
On a quick skim the only thing that makes me wince in this diff is the SongSelect change and it looks like that one is caused by the typical song select debounce hell.
There was a problem hiding this comment.
To confirm you're understand this: Ruleset not being set (and thus null) in the online beatmap set overlay is the fix here.
Zero xmldoc explaining this, just a wild ruleset bindable? I don't know.
There was a problem hiding this comment.
Uhhhh... I guess I missed that part. null fixing it is dodgy indeed.
Should be explicitly set in all instances.
There was a problem hiding this comment.
If it's explicitly set on the beatmap overlay to anything that isn't null, it will regress the fix. I'm open to a better structure or xmldoc trying to explain this, but this is why I proposed the ctor argument instead (it's explicit and it gives us a place to document the modes of operation). 🤷
There was a problem hiding this comment.
My API proposal is for AdvancedStats which is used at song select, and bound to song select's local ruleset.
Maybe if we can't easily agree on this the class should just be copypasted into two versions. Because I guarantee the next issue is going to be that it's reading from global mods and displaying "unexpected" on the overlay.
There was a problem hiding this comment.
I'm not sure why this conversation is going in the places it's going so I might just recuse myself from it.
the next issue is going to be that it's reading from global mods and displaying "unexpected" on the overlay
I never wanted that. It should not be reading from global mods. It should be given the correct ruleset to show explicitly in every usage. No inferences.
Hell give it a ruleset bindable instead but one that is explicltly hooked to by every usage. Song select can bind it to the game-global bindable and beatmap info overlay can do its local one.
There was a problem hiding this comment.
There's more issues with AdvancedStats ruleset – it doesn't have knowledge of whether a conversion can happen, so right now it will prefer the bound ruleset even if the conversion is impossible (ie. mania beatmap and osu! ruleset being set). Not sure if this is okay or not. Should it be up to the point of construction to get this right?
There was a problem hiding this comment.
My vision for it was for it to be a dumb component that displays what it's told to. At the point it's told to display an impossible conversion something has gone deeply wrong somewhere higher above I'd say.
|
I've added xmldoc to the bindable, I guess it's fine. |
The component itself was assuming too much. The decision of whether to show the "key count" label should be completely up to each usage instead.