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

Refactor difficulty icons to not suck #18819

Merged
merged 10 commits into from
Jun 24, 2022

Conversation

peppy
Copy link
Member

@peppy peppy commented Jun 23, 2022

Something I've been putting off for a while. The hierarchy of these icons and the very sparse and different usages made it a nightmare to deal with.

  • DifficultyIcon is now a very simple implementation with a bindable to update the rating externally, and some sane defaults.
  • CalculatingDifficultyIcon is responsible for doing automatic calculation when on-screen.
  • DifficultyRetriever is a stand-alone component that handles the retrieval. I figure this may be useful in other contexts in the future. Or not. At least it's well defined and documented now.
  • I was able to delete/combine some classes along the way (GroupedFilterable and Filterable combined, and some weird MatchModeType nuked).

@peppy peppy force-pushed the difficulty-icon-refactor-pass branch from d7ae590 to 01da6f2 Compare June 23, 2022 10:27
Copy link
Member

@frenzibyte frenzibyte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Upon testing, it seems difficulties that are beyond the song select screen area but not completely offscreen will remain unpopulated, making it look off:

CleanShot.2022-06-24.at.05.36.20.mp4

(notice the difficulty cards at the bottom right of the screen updating SR the moment they enter song select range)


Never mind, I thought this PR introduced the usage of DelayedLoadUnloadWrapper for processing difficulty, while it's actually already present in master.

osu.Game/Beatmaps/Drawables/CalculatingDifficultyIcon.cs Outdated Show resolved Hide resolved
osu.Game/Overlays/BeatmapSet/BeatmapPicker.cs Outdated Show resolved Hide resolved
Comment on lines -145 to +128
ITooltip<DifficultyIconTooltipContent> IHasCustomTooltip<DifficultyIconTooltipContent>.GetCustomTooltip() => new DifficultyIconTooltip();
ITooltip<DifficultyIconTooltipContent> IHasCustomTooltip<DifficultyIconTooltipContent>.
GetCustomTooltip() => new DifficultyIconTooltip();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this automated by rider? I've also seen this happening locally on any file I can touch recently, not sure if something changed in the configuration somewhere.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say so.

peppy added 4 commits June 24, 2022 13:58
The whole component is pointless so I'm just going to nuke for now I
guess. Kind of makes the whole refactor effort pointless but oh well?

To expand on this, the implementation was actually incorrect as pointed
out at ppy#18819 (review).
@peppy
Copy link
Member Author

peppy commented Jun 24, 2022

I've fixed all the reported issues. See 2883769 commit message for important dialogue.

@smoogipoo smoogipoo requested a review from frenzibyte June 24, 2022 06:53
@frenzibyte frenzibyte enabled auto-merge June 24, 2022 07:38
@peppy
Copy link
Member Author

peppy commented Jun 24, 2022

Upon testing, it seems difficulties that are beyond the song select screen area but not completely offscreen will remain unpopulated, making it look off:

Just for documenting purposes, this was fixed with 2883769 by binding directly to the DrawableCarouselBeatmap's difficulty calculation operation.

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.

3 participants