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

Some text improvements including falling back to system fonts #4721

Open
wants to merge 17 commits into
base: develop
Choose a base branch
from

Conversation

andydotxyz
Copy link
Member

Fixes are in separate commits, the fallback code is basically first-pass.
We include lookup for OS font for the current language and whatever font the OS has for japanese, which typically includes most script languages.

This provides good (tested) coverage of rendering out of the box for macOS, iOS, Linux and Android (windows still to be tested).
Memory increase is currently 5% - 20% depending on how many fonts the app uses.
There are more optimisations to come that will counteract the increase.
The code is now faster than before so that's the current tradeoff

Fixes #(issue)

Checklist:

  • Tests included.
  • Lint and formatter run with no errors.
  • Tests all pass.

@andydotxyz
Copy link
Member Author

Still some tests to revise, but this is pretty much code complete I think.

image

@andydotxyz andydotxyz marked this pull request as ready for review March 21, 2024 18:53
@Jacalz
Copy link
Member

Jacalz commented Apr 30, 2024

How much work is left for this to be ready?

@dweymouth
Copy link
Contributor

dweymouth commented May 9, 2024

Same question as Jacalz, also wondering if this will cover Chinese and Korean as well even if the locale is EN. (Those along with Japanese are the most common non-Latin scripts in peoples' music libraries ;) - though the ideal of course would be lookup for all scripts that may need to be rendered regardless of locale setting.

@andydotxyz
Copy link
Member Author

andydotxyz commented May 10, 2024

Yes. It falls back on a rune basis so unidentified characters should all be looked up.
Functionally the only thing missing is when you need locale info to shape the glyphs in a run - but that is a step later than looking up the font.

I will try and address the test issues this weekend to get the PR landed.

@dweymouth dweymouth added this to the "Elgin" release, early 2024 milestone May 11, 2024
@andydotxyz
Copy link
Member Author

On Windows and macOS (not Linux, iOS or Android) sometimes the symbol fonts align slightly differently on the sub-pixel.
I have disabled the test for now, it makes no difference to the functionality and I did not want to hold up people checking this out...

Copy link
Member

@Jacalz Jacalz left a comment

Choose a reason for hiding this comment

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

I wonder if it perhaps would be easier to test this if we could easily disable the built in fonts similar to #2637?

@andydotxyz
Copy link
Member Author

``

I wonder if it perhaps would be easier to test this if we could easily disable the built in fonts similar to #2637?

I don't see why that makes it easier - if we disable the builtin fonts then it will be down to the OS/setup as to what fonts are found - basically guaranteed to be different unexpectedly on different computers - and may even break on CI.

@dweymouth
Copy link
Contributor

I think @Jacalz means that if you can build with no bundled fonts, at least for testing, you don't need an app that has CJK or other less common characters to test the lookup

@dweymouth
Copy link
Contributor

Plus now that lookup is in place we probably want to give apps the option to opt out of having any bundled fonts in the binary

@Jacalz
Copy link
Member

Jacalz commented May 18, 2024

I think @Jacalz means that if you can build with no bundled fonts, at least for testing, you don't need an app that has CJK or other less common characters to test the lookup

Indeed. I figured it would make things not only easier to test but also allow us to test more parts of the font lookup.

Plus now that lookup is in place we probably want to give apps the option to opt out of having any bundled fonts in the binary

Yup. That would be nice as well down the road as more of a feature than a testing helper :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants