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

core: Include a regular subsetted Noto Sans as the default font #18353

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

Conversation

evilpie
Copy link
Collaborator

@evilpie evilpie commented Oct 22, 2024

The idea is to include a regular TTF font file for font rendering, instead of first creating a SWF with the Noto Sans as an embedded font. I am using pyftsubset together with a list of unicode ranges matching specific Unicode blocks.
This should make it quite easy to extend the included font in the future. Sadly ttf_parser doesn't support any of the newer compressed formats like WOFF. That is why I decided to manually apply deflate compression, which reduces the size by almost 50%.

Fixes #14761

@evilpie evilpie force-pushed the new-font branch 3 times, most recently from 99eedad to ad3254e Compare October 23, 2024 18:28
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not quite sure what happened here. I can't reproduce this when running the test in Firefox using the extension. (The colors seem wrong anyway)

image

Copy link
Member

Choose a reason for hiding this comment

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

What do you mean? Note that this test clicks the "Green Only" button (see its input.json).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh sorry. Somehow I thought it's obvious. But the = sign (e.g. in rows=5) now looks like just a dash (-).

Copy link
Member

Choose a reason for hiding this comment

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

I believe you would have to render it in 1:1 scale and quality 1 to observe that. If we up the quality to 4 the second dash would probably be shown, but it's interesting it's missing on quality 1 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh this looks bad.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not sure what is going on here. The test actually includes a DefineFont3 for Noto Sans, but it seems like we might still be falling back to the device font. I actually get the same rendering on desktop (I don't have Noto Sans installed).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Apparently a known issues: #14713. Thanks @kjarosh

Copy link
Member

Choose a reason for hiding this comment

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

This shouldn't be needed after #18437.

Copy link
Member

Choose a reason for hiding this comment

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

Dropped this change.

@evilpie evilpie marked this pull request as ready for review October 24, 2024 09:18
@evilpie
Copy link
Collaborator Author

evilpie commented Oct 24, 2024

This should be ready to review. I don't see any text rendering change that seems problematic.

@evilpie evilpie added A-core Area: Core player, where no other category fits T-refactor Type: Refactor / Cleanup text Issues relating to text rendering/input labels Oct 24, 2024
@torokati44
Copy link
Member

Sadly ttf_parser doesn't support any of the newer compressed formats like WOFF.

Just referencing harfbuzz/ttf-parser#122, in case it's added in the future.

@torokati44
Copy link
Member

There are also crates like allsorts and webtype, have you considered relying on them (or anything else)? Not that there's anything wrong with simply deflating (it may be better even, as no new dependency brought in), I'm just curious.

@evilpie
Copy link
Collaborator Author

evilpie commented Oct 24, 2024

We need ttf_parser anyway for TLF's DefineFont4, so adding another crate with a different API would just be additional work.

@torokati44
Copy link
Member

(Just a side-note: I keep getting confused whether "regular" means "not light and not bold" or "just a .ttf, not a blob extracted from a .swf"; and whether "subsetted" means "subscript" like ₜₕᵢₛ, or "not all unicode characters included" here. I assume it's the latter for both words though.)

Copy link
Contributor

@Dinnerbone Dinnerbone left a comment

Choose a reason for hiding this comment

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

Looks good to me but I'd really like to consider using metric compatible fonts at some point.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-core Area: Core player, where no other category fits T-refactor Type: Refactor / Cleanup text Issues relating to text rendering/input
Projects
None yet
Development

Successfully merging this pull request may close these issues.

minus sign does not appear
4 participants