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: Extend the embedded fake device font with more characters #6224

Merged

Conversation

torokati44
Copy link
Member

@torokati44 torokati44 commented Feb 6, 2022

By also embedding the "Basic Latin", "Latin I", and "Latin Extended A" sections.

This is all I did to the .fla file before exporting the .swf and extracting the DefineFont3 data:
image
image

While this is still just putting out a larger fire temporarily (cc: #1862), and also increases the embedded (uncompressed) data size by about 60 kB, I still think it's worth it.

After a quick glance at the "Latin Extended B" and "Latin Extended Add'l" sections, they seemed less crucial to include to me personally, relative to their size.

If I didn't miss anything, this fixes #1099, #5357, and probably many more related issues that will have to be checked one by one.

By also embedding the "Basic Latin", "Latin I", and "Latin Extended A" sections.
@torokati44
Copy link
Member Author

Hmm, I suppose the failing as3_edittext_leading test could be updated ... ?
Perhaps the Noto Sans font itself was somewhat tuned since it was made, or the existing embedded font was exported with a different version of Adobe Flash ... ?

@adrian17
Copy link
Collaborator

adrian17 commented Feb 6, 2022

Sounds like a very good tradeoff, compared to hundreds of kB we add for some dependencies that we use 1% of ;)

Hmm, I suppose the failing as3_edittext_leading test could be updated ... ?

Maybe run the test SWFs and compare the screenshots?

@torokati44
Copy link
Member Author

torokati44 commented Feb 6, 2022

The screenshot of the mentioned single failed test on master:
image

And on this branch:
image

The difference:
image

I'd say it's within margin of error, so the change can be tolerable.
Probably the font was adjusted with some better kerning/hinting info?

@adrian17
Copy link
Collaborator

adrian17 commented Feb 6, 2022

TBH, to me it looks better in some places and worse in others... not sure what to think about it :D
Also, instead of tweaking the values every time we change the font, we can also move the test into approx-tests list to be more lenient in general.

@torokati44
Copy link
Member Author

torokati44 commented Feb 6, 2022

I don't have a good eye for these things, and I find both versions to be readable (legible?) just fine.
Also, I suspect that the current version didn't go through any particularly rigorous aesthetic judgement either...

Isn't the purpose of approx-tests to handle the uncertainties that are outside of our control? Say, differences across platforms, backends, or slight changes in external dependencies.
And if so, the embedded font, and everything that uses it, is perfectly within our control (but FIXME), so maybe moving the test there is not warranted?

@torokati44 torokati44 added text Issues relating to text rendering/input tests labels Feb 6, 2022
@adrian17
Copy link
Collaborator

adrian17 commented Feb 6, 2022

Isn't the purpose of approx-tests to handle the uncertainties that are outside of our control?

Well, as we can see, a font in a way is outside of our control :) If some other person updates the embedded font and gets slightly different results again, they'll need to update the test again - which kinda defeats the point :) The previous font also wasn't 100% guaranteed to match FP, I think.

(Further, even some of existing tests are are approx because "I implemented this to the best of my ability, but there are still some minor differences from FP - I probably missed something, but let's consider that out of scope for now". You can see some in the comments there, like // TODO AS3 has _width higher by 5.0, probably padding logic mistake)

(though I wish every single one had an explanation comment... if you do decide to make it an approx test, maybe let's add a top-level comment to always document the reason for being approx)

@adrian17
Copy link
Collaborator

adrian17 commented Feb 6, 2022

But then again, if you don't want to do it, just updating the test now and waiting for the next time this happens is a valid approach too :D

@torokati44
Copy link
Member Author

just updating the test now and waiting for the next time this happens

Truth be told, I'm leaning towards this at the moment. :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tests text Issues relating to text rendering/input
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Special characters are sometimes not displayed
2 participants