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

Improve TTF support for pixel-style fonts #11848

Merged
merged 2 commits into from Dec 30, 2021
Merged

Conversation

v-rob
Copy link
Member

@v-rob v-rob commented Dec 9, 2021

This PR improves support for TTF pixel-style fonts, such as Unifont TTF. It introduces a new setting, font_size_divisible_by (and mono_font_size_divisible_by) that forces a font into integer scaling. It takes an integer, which is the number of pixels high the font is (not points, pixels). It works regardless of DPI or GUI scaling.

Please note that font_size is not in pixels but scaled such that 1 unit = 1 pixel at 96 DPI -- hence, higher DPIs will result in higher font sizes.

Alternate to #11767 to supersede bitmap fonts in every regard (see #11611).

image

To do

This PR is Ready for Review.

How to test

Get Unifont. Set it as the font, set font_size_divisible_by to 16 (since that is the height of the font in pixels), and then muck about with font_size, display_density_factor, and gui_scaling. Make sure that Unifont never, ever appears blurry no matter what settings you use for those, but does, in fact, do integer scaling.

@v-rob
Copy link
Member Author

v-rob commented Dec 9, 2021

Some things after a discussion on #minetest:

  • The setting name for font_required_multiple_size is really bad. Perhaps font_snap_to_multiple?
  • Disabling antialiasing isn't actually helpful for replacing bitmap fonts. (It does look cool for normal TTF fonts, though, if you want to relive the Windows 95 days :P But not really useful for this PR, so I'll probably remove)
  • @erlehmann wants the possibility of fonts not rendering color like bitmap fonts. That wouldn't be too difficult.

@sfan5 sfan5 added the Feature ✨ PRs that add or enhance a feature label Dec 9, 2021
src/client/fontengine.cpp Outdated Show resolved Hide resolved
@sfan5
Copy link
Member

sfan5 commented Dec 9, 2021

  • The setting name for font_required_multiple_size is really bad. Perhaps font_snap_to_multiple?

My suggestion is font_size_divisible_by.

erlehmann wants the possibility of fonts not rendering color like bitmap fonts. That wouldn't be too difficult.

I don't think we want knobs to turn off random features. Especially not because of the whole "artistic direction in games" thing, if the game/mod author wants the text colored it's not up to the client to ignore that.

@erlehmann
Copy link
Contributor

erlehmann commented Dec 9, 2021

I don't think we want knobs to turn off random features. Especially not because of the whole "artistic direction in games" thing, if the game/mod author wants the text colored it's not up to the client to ignore that.

This is not about creative direction, it is an accessability feature. I have a hard time reading colored text depending on the color of the sky in Minetest (I can distinguish low contrast worse than other people). And the people who write rainbow text are not game authors, but other players who think it is extremly funny to do so.

The feature to not color text already seems to exist, I just did not know about it … or at least this is how I interpret this PR: #11849

@erlehmann
Copy link
Contributor

The setting name for font_required_multiple_size is really bad. Perhaps font_snap_to_multiple?

My suggestion is font_size_divisible_by.

While that may be mathematically correct (and make it obvious why it can not be zero), IMO what should be conveyed is that this is a pixel font base size, i.e. the smallest size where scaling by an integer will not produce artifacts. Maybe that can be handled in the description.

@erlehmann
Copy link
Contributor

erlehmann commented Dec 9, 2021

I am loading the font /usr/share/fonts/X11/misc/10x20.pcf.gz – i.e. a pixel font.

What value do I have to give to font_required_multiple_size for proper scaling?

I think it definitely looks wrong with font_required_multiple_size set to 20.

(Edit: If this depends on DPI again, you have the same problem as before.)

@sfan5
Copy link
Member

sfan5 commented Dec 9, 2021

This is not about creative direction, it is an accessability feature.

It is game authors/server owners responsibility to include accessibility features (in your example such as stripping color from chat). The engine could even help here.
But including a client feature to unconditionally disable all and every coloring is the wrong way.

The feature to not color text already seems to exist, I just did not know about it

The PR you mentioned is a server-side feature.

@erlehmann
Copy link
Contributor

It is game authors/server owners responsibility to include accessibility features (in your example such as stripping color from chat). The engine could even help here.

I do not get it. If users could enable it, they could enabe it for any game without anyone having to do anything for their game or server.

I use websites with a userstyle CSS that sets background to black, foreground to white, links to blue – because many websites have low contrast, making them hard to read. How would be doing the same thing to text in Minetest bad?

@erlehmann
Copy link
Contributor

erlehmann commented Dec 9, 2021

Something about how Minetest renders pixel fonts in the widely used PCF format seems to be weird.

The setting of font_required_multiple_size seems to only affect line height & not scale the font.

This could mean to get crisp pixel fonts one might use PCF fonts without antialiasing (needs testing).

It makes me doubt this PR is the right approach though. I think that Freetype must know the height of the font in pixels for at least some font formats … which could mean this setting is needed only for scalable fonts? But then the user will probably know the height of the font in pixels, but not in points.

@v-rob
Copy link
Member Author

v-rob commented Dec 9, 2021

After some reflection this morning, there's an issue of compatibility (you knew it). If we use points from this PR, then formspecs that absolutely size their fonts will break since the fonts will instantly be smaller. On the other hand, if we start sizing using pixels (which is, in fact, possible), then text will be too small for high density screens. So it seems that the current fake-96-DPI points have to stay for now.

However, that said, I can still manipulate the multiples setting to make pixel fonts work right, regardless of font measurements. To be precise, multiples has to be measured in pixels, not DPI-specific stuff (which is what the PR does currently, and that's incorrect). I will apply a fix later, which (hopefully, if I understand the Freetype API correctly) should fix the issues people have been having with this PR.

@v-rob v-rob force-pushed the pixel-fonts branch 3 times, most recently from cd4bb44 to 37e5032 Compare December 9, 2021 21:28
@v-rob
Copy link
Member Author

v-rob commented Dec 9, 2021

Done. Fixed it up, reverted to old points, and removed the useless antialiasing option. Read the PR description again and go ahead and test. I did my own DPI testing this time, and I can't get Unifont to appear blurry, but it does do integer scaling, so that's good.

As a note, I'm going to leave decolorization out of this PR so it doesn't prevent it from going forward. Another PR can do that if it's decided to do that.

@erlehmann
Copy link
Contributor

erlehmann commented Dec 10, 2021

@v-rob as mentioned on IRC, I suggest to set the font_size_divisible_by setting automatically for font formats that already include such information (such as PCF).

Also I still think the naming of this setting is not too good, but I guess something like font_base_height_in_pixels or similar would be still too confusing (as it might imply being prescriptive, not describtive) … ?

@sfan5
Copy link
Member

sfan5 commented Dec 10, 2021

Note that PCF is not explicitly supported, if it happens to work that is by chance. Apart from the no longer working Irrlicht format the only other format Minetest intends to support is TTF.

@erlehmann
Copy link
Contributor

Note that PCF is not explicitly supported, if it happens to work that is by chance.

PCF is explicitly suported by FreeType. It doesn't just work by chance, but by choice.

Apart from the no longer working Irrlicht format the only other format Minetest intends to support is TTF.

Excuse me, but is this a threat/announcement to remove PCF support? I find it really unnerving when developers declare stuff that has worked for a long time as „no longer supported“ and want to remove it just because they do not personally use it.

Removal of the Irrlicht pixel font format was (allegedly) about easing the maintenance burden. Are you going to make mtFreetype as a badly maintained fork of FreeType and remove all formats except TTF?

@sfan5
Copy link
Member

sfan5 commented Dec 10, 2021

Excuse me, but is this a threat/announcement to remove PCF support?

No? All I'm saying is that if PCF stops working one day making it work again will not be prioritized.

@v-rob
Copy link
Member Author

v-rob commented Dec 10, 2021

@v-rob as mentioned on IRC, I suggest to set the font_size_divisible_by setting automatically for font formats that already include such information (such as PCF).

Looking through the FreeType2 API, I can see likely candidates as to where to find this information, but since FreeType is localized to CGUITTFont whereas FontEngine has to select font sizes before a CGUITTFont is even created, it might take substantial code restructuring. Since PCF apparently isn't officially supported, it probably isn't advisable to do that then.

Anyway, I feel that anyone who takes the time to install their own font can take an extra minute to find the default height of the font, either by trial and error or by importing a screenshot of it into their favourite image editor. It's what you have to do to use Unifont with your OS, so it's not unreasonable for users to do the same for Minetest.

@v-rob
Copy link
Member Author

v-rob commented Dec 10, 2021

I find it really unnerving when developers declare stuff that has worked for a long time as "no longer supported" and want to remove it just because they do not personally use it.

A side note and not related to the PR, but if something was never officially supported and not documented as supported anywhere, it sounds perfectly justified to remove it. For instance, consider Windows 95's development and the great lengths they went to make every Windows 3.1 and DOS program they could find work on it properly, even though those programs abused the API's undocumented edges and bugs. Then consider the ugly hacks, and problems they caused for future Windows OS's, and wasted time. To be sure, they accomplished their goal, but at what cost? PCF isn't causing anything like that for Minetest of course, but that doesn't justify the rationale of "it's undocumented and/or a bug, but works, so you have to support it."

@sfan5
Copy link
Member

sfan5 commented Dec 15, 2021

This PR is ready, right?

@v-rob
Copy link
Member Author

v-rob commented Dec 15, 2021

Yes, I don't plan on adding any auto-detection features to this PR, so it's as good as ready.

Copy link
Member

@sfan5 sfan5 left a comment

Choose a reason for hiding this comment

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

Tested with unifont

@rollerozxa
Copy link
Member

Tested it with KetchupLand's fork of Minetest which uses Unifont TTF as the default font, more particularily on Android where the text normally gets scaled on my phone, and compared to how it was before. Above is before, below is with this PR applied and with divisible_by being set to 16. You can see how it makes the text perfectly crisp and not blurry at all by doing integer scaling (hence why the screenshots look a bit misaligned even though it is cropped at the same positions, I'd assume the size gets rounded to the nearest full pixel size which would be up in this case), more in line with what you would expect from a pixel font. Awesome!

image

rollerozxa added a commit to KetchupLand/minetest that referenced this pull request Dec 19, 2021
Improve TTF support for pixel-style fonts (minetest/minetest#11848)
@v-rob v-rob merged commit 4a16ab3 into minetest:master Dec 30, 2021
LoneWolfBT pushed a commit to MT-CTF/minetest that referenced this pull request Feb 13, 2022
@v-rob v-rob deleted the pixel-fonts branch January 27, 2024 01:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature ✨ PRs that add or enhance a feature One approval ✅ ◻️
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants