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 font sizes and spacing in BeatmapSetOverlay to match osu-web values #7863

Merged
merged 5 commits into from
Feb 21, 2020

Conversation

thewildtree
Copy link
Contributor

@thewildtree thewildtree commented Feb 16, 2020

Summary

Brings all font and drawable sizes in BeatmapSetOverlay in line with osu-web, laying ground for potential OsuFont scale fix to fully match web without the need to manually pixel-check every single part of the overlay.

Most font sizes were a pixel or two bigger - that made them look closer to osu-web, but as mentioned in #7730, after this PR, we could experiment with scaling inside OsuFont itself to match osu-web look and feel while also having correct size values in the code.

I tried my best to make all the boxes (details, top scores etc.) match osu-web dimensions (where possible) 1:1, +- 1px. This can be checked by comparing values from the draw visualizer with osu-web values from your web browser's dev tools, and I'd be glad if someone verified that because I had to manually check every single thing and probably missed something during that process.

Explanation

The reason why the sizes were off is because fonts are rendered differently than other drawables when is comes to size. It seems like they are just visibly smaller when compared to a drawable of a theoretical same size:

image

As visible here, rendered font doesn't actually fill all the vertical space that it occupies - effectively reducing its size.

This is especially visible in the scoreboard - with the row height set to 22 and the text size set to 12, it looks different (more spaced) than web with same dimensions set:

lazer scoreboard:
image

web scoreboard:
image

Remarks

  • changed ScoreTable's column dimensions to closer match web, but left them a bit wider in some cases, because lazer scales the table horizontally while in osu-web it's fixed width. Changes were made based on how it looked in my opinion, so I'd appreciate feedback in that matter.

  • changed AdvancedStats spacing a bit - side effect is that it's also been affected in song select, but I think that's acceptable

  • header still needs changes - in osu-web the amount of space between the diff picker and the title is scaled dynamically (the artist text and everything below is anchored to the bottom), while right now lazer uses static spacing:

    lazer header:
    image

    web header:
    image

  • there are a couple small differences due to osu-web using different line-height values, I made up for that using margins / paddings around font elements (couldn't find a better way to accomplish this)

  • TopScoreStatisticsSection for example looks off, but that's mainly due to the aforementioned font scaling issue, plus osu-web hard-scales the second statistic row to 18px which I haven't done here (yet? idk if we want this though)

  • I'm not sure about whether I've added spacing properly in a few places (code-quality wise), please let me know if I can improve anything

@bdach
Copy link
Collaborator

bdach commented Feb 16, 2020

Re: font sizes - see ppy/osu-framework#3271.

@peppy
Copy link
Member

peppy commented Feb 17, 2020

I don't think this needs to depend on a potential font size fix. That can come later – being slightly wrong isn't a huge deal for us right now.

@bdach
Copy link
Collaborator

bdach commented Feb 17, 2020

Certainly not, just wanted to leave a link to that issue to centralise discussion/explain the discrepancy.

@thewildtree
Copy link
Contributor Author

Hmm, I added that because I didn't want to push a change which, if not accompanied by font sizing changes, arguably reduces readibility and looks worse than the previous version.

If that's fine for you, then alright - we now have proper values in place and that's what counts really.

@peppy
Copy link
Member

peppy commented Feb 21, 2020

Good job on this one – I think it's fine to go as-is and we can address the font sizing at a later point.

@peppy peppy merged commit ab49f88 into ppy:master Feb 21, 2020
@thewildtree thewildtree deleted the adjust-beatmap-overlay branch February 23, 2020 10: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.

3 participants