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.

Sign up for GitHub

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

Jump to bottom

Make font hinting user-configurable. #78830

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Make font hinting user-configurable. #78830

wants to merge 9 commits into from
+350 −61

Conversation

CLIDragon
Copy link
Contributor

@CLIDragon CLIDragon commented Dec 29, 2024 •
edited
Loading

Summary

Interface "Make font-hinting user-configurable"

Purpose of change

Fixes the slight blurriness of Terminus on ImGui windows which was caused by not hinting in bitmap mode.

fixes #73120

Describe the solution

The font is loaded through the same fontdata.json file that was always used.

I'm not really happy with the clarity of the error messages, but I am not sure how to make them clearer. I don't think it's possible to use JsonWithPath.throw_error() because if it occurs before the first typeface is loaded the game will crash before load.

Describe alternatives you've considered

None.

Testing

Loaded the game. Checked that error messages were displayed appropriately for incorrect sizes.

Additional context

Current
image

Previous
image

@github-actions github-actions bot added Info / User Interface Game - player communication, menus, etc. [JSON] Changes (can be) made in JSON [C++] Changes (can be) made in C++. Previously named `Code` <Bugfix> This is a fix for a bug (or closes open issue) json-styled JSON lint passed, label assigned by github actions labels Dec 29, 2024
@github-actions github-actions bot added the astyled astyled PR, label is assigned by github actions label Dec 29, 2024
moxian
moxian reviewed Dec 29, 2024
View reviewed changes
data/fontdata.json Outdated Show resolved Hide resolved
@Procyonae
Copy link
Contributor

This could do with some temporary migration for the default fonts otherwise noone keeping their config on update will get the changes unless I'm misunderstanding

@github-actions github-actions bot added the BasicBuildPassed This PR builds correctly, label assigned by github actions label Dec 29, 2024
@Kilvoctu
Copy link
Contributor

Kilvoctu commented Dec 30, 2024 •
edited
Loading

Pulled and tested this, and yeah, seems like anyone with an existing config/fonts.json won't see the changes in this update. I agree some more user-facing documentation would be preferred.

Before, with Terminus
Screenshot 2024-12-29 175319

After I deleted my config/fonts.json and let it rebuild
Screenshot 2024-12-29 175337

Before, using a custom font FiraCode
Screenshot 2024-12-29 180455

Same custom font, hinting bitmap
Screenshot 2024-12-29 174915

The screenshots don't really do it justice; it does look much better in game

db48x
db48x approved these changes Dec 30, 2024
View reviewed changes
Copy link
Contributor

@db48x db48x left a comment

Choose a reason for hiding this comment

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

Overall I like this change, but I have one or two suggestions.

src/font_loader.h Outdated Show resolved Hide resolved
src/cata_imgui.cpp Outdated Show resolved Hide resolved
@github-actions github-actions bot removed the astyled astyled PR, label is assigned by github actions label Dec 31, 2024
@github-actions github-actions bot added <Documentation> Design documents, internal info, guides and help. [Markdown] Markdown issues and PRs astyled astyled PR, label is assigned by github actions labels Dec 31, 2024
@CLIDragon
Copy link
Contributor Author

CLIDragon commented Dec 31, 2024 •
edited
Loading

This could do with some temporary migration for the default fonts otherwise noone keeping their config on update will get the changes unless I'm misunderstanding

I've changed it so that config/fonts.json is automatically updated to the new format. Additionally, if the font is written as a single string terminus hinting will default to bitmap and Roboto-Medium hinting will default to Auto.

@CLIDragon
Make font hinting user-configurable.
48d6286 
* Configure default hinting for Terminus and Roboto to better values.
* Document font options in docs/FONT_OPTIONS.md
* Fix the slight blurriness of Terminus on ImGui which was caused by
  not hinting in bitmap mode.
@github-actions github-actions bot removed the BasicBuildPassed This PR builds correctly, label assigned by github actions label Dec 31, 2024
db48x
db48x suggested changes Dec 31, 2024
View reviewed changes
src/font_loader.cpp
Comment on lines +173 to +176
default:
// This should never be reached.
json.member( "hinting", "Default" );
break;
Copy link
Contributor

Choose a reason for hiding this comment

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

This default case need not be rare. The default hinter is great for most fonts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If a font is default it should have a std::nullopt hinting so it should get assigned before the switch statement.

src/font_loader.cpp
Comment on lines +53 to +54
if( hinting == "Default" ) {
return std::nullopt;
Copy link
Contributor

Choose a reason for hiding this comment

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

This should return zero, not std::nullopt as if it were uninitialized.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

0 isn't a valid ImGuiFreeTypeBuilderFlags value unfortunately, and I don't really want to store a raw integer. Would it be acceptable to modify ImGui to add a default 0 value to ImGuiFreeTypeBuilderFlags?

src/font_loader.cpp Outdated Show resolved Hide resolved
@db48x
Copy link
Contributor

db48x commented Dec 31, 2024

I expanded the documentation and made some other small tweaks. I sent them to you in CLIDragon#2.

@github-actions github-actions bot removed the astyled astyled PR, label is assigned by github actions label Jan 1, 2025
@github-actions github-actions bot added the astyled astyled PR, label is assigned by github actions label Jan 1, 2025
@db48x
Copy link
Contributor

db48x commented Jan 1, 2025

Sent you another tweak in CLIDragon#3.

@github-actions github-actions bot added BasicBuildPassed This PR builds correctly, label assigned by github actions and removed BasicBuildPassed This PR builds correctly, label assigned by github actions labels Jan 1, 2025
@CLIDragon CLIDragon closed this Jan 1, 2025
@CLIDragon CLIDragon reopened this Jan 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@moxian moxian moxian left review comments

@db48x db48x db48x requested changes

Assignees
No one assigned
Labels
astyled astyled PR, label is assigned by github actions <Bugfix> This is a fix for a bug (or closes open issue) [C++] Changes (can be) made in C++. Previously named `Code` <Documentation> Design documents, internal info, guides and help. Info / User Interface Game - player communication, menus, etc. [JSON] Changes (can be) made in JSON json-styled JSON lint passed, label assigned by github actions [Markdown] Markdown issues and PRs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Imgui rendered fonts do not look as crisp as in the other menus with the software renderer
5 participants
@CLIDragon @Procyonae @Kilvoctu @db48x @moxian