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

Adjust minor BeatmapSetOverlay details to better match osu-web #7730

Merged
merged 42 commits into from
Feb 12, 2020

Conversation

thewildtree
Copy link
Contributor

@thewildtree thewildtree commented Feb 4, 2020

Follows #7724, aims to bring the BeatmapSetOverlay even closer to its osu-web counterpart. These changes are mostly small details, but they definitely can affect the UX.

A few things worth noting:

  • the scoreboard font size change from mentioned PR has been reversed - while 12 technically matches osu-web, scaling in lazer is a bit different, making 14 bring a more familiar look here
  • scoreboard had wrong order of columns just like TopScoreStatisticsSection (as mentioned in #7725), I applied a similar solution, but I'm not sure if it's the proper one here. maybe instead of this we should sort them before they get passed to any UI components?
  • added some checks to hide ratings / show placeholder instead of success rate if beatmap has no leaderboard, but I'm not sure if its done the right way
  • wanted to add the green colour to the combo if it was the max possible, same as web has, but currently I believe there's no way to check beatmaps maxcombo in this scope

image

image

@bdach
Copy link
Collaborator

bdach commented Feb 4, 2020

Please use .\InspectCode.ps1 to find stylistic issues instead of relying on CI to spot them for you, and please don't commit code using the github interface. If you're away from an IDE, we'll understand, just let it be for the day.

@thewildtree
Copy link
Contributor Author

thewildtree commented Feb 4, 2020

Sorry, I couldn't run InspectCode because of some issues with PowerShell on my PC. Will start doing that as soon as I solve them. Also sorry for the CI-fix commit spam, I'm no longer at my PC and github web edit only allows one file per commit. Yup, sorry. Won't do that again.

@thewildtree
Copy link
Contributor Author

Removed mentioned parts, will create separate PRs for them later.

Copy link
Contributor

@smoogipoo smoogipoo left a comment

Choose a reason for hiding this comment

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

In general, always match osu-web sizings

osu.Game/Overlays/BeatmapSet/Scores/ScoreTable.cs Outdated Show resolved Hide resolved
osu.Game/Overlays/BeatmapSet/Scores/ScoreTable.cs Outdated Show resolved Hide resolved
osu.Game/Overlays/BeatmapSet/Info.cs Outdated Show resolved Hide resolved
osu.Game/Overlays/BeatmapSet/Scores/TopScoreUserSection.cs Outdated Show resolved Hide resolved
@thewildtree
Copy link
Contributor Author

thewildtree commented Feb 7, 2020

size 10 is unreadable due to the font rendering issues

in general, if you take a look at the rest of the overlay, most elements use fonts (or overall dimensions) a few px bigger than web and I followed that direction - it seems correct because matching osu-web 1:1 here looks way more off (I suppose due to some scaling behaviour I can't properly explain) and font sizes like 10px are unreadable due to how lazer renders them in overlays.

currently used font sizes make the overlay look much closer to osu-web than if we used 1:1 font sizes.

@smoogipoo
Copy link
Contributor

Match the font sizes. We can uniformly apply a scale at a later point if needed, inside OsuFont itself.

@thewildtree
Copy link
Contributor Author

Then we'd have to change every single overlay element / font since they're mostly sized a tad bigger than osu-web.

@smoogipoo
Copy link
Contributor

That's correct.

@thewildtree
Copy link
Contributor Author

Alright, I'll try to work on it, I'm not sure how that's gonna end up looking though.

@peppy
Copy link
Member

peppy commented Feb 7, 2020

It will look fine. All metrics should match (font sizes, widths, paddings, margins). Overall adjustments can be applied later on when we decide how large or small we want things to display.

The important part is that everything matches on a numeric and relative basis so it is easy to confirm correctness without manually pixel checking (aka what you've probably been doing until now).

@thewildtree
Copy link
Contributor Author

thewildtree commented Feb 7, 2020

Went back to osu-web font size values as suggested. I'll make a separate PR soon which will aim to bring all the dimensions / sizes 1:1 with osu-web.

@bdach
Copy link
Collaborator

bdach commented Feb 10, 2020

I fixed up a few remaining nits; looks fine otherwise.

Copy link
Collaborator

@bdach bdach left a comment

Choose a reason for hiding this comment

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

Think this PR has run its course - I'd say let's get it in for now. Accuracy with web can be iterated on.

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.

4 participants