Improve CJK text rendering [#2990] #3006

bdon · 2023-08-21T07:21:35Z

Addresses proposal in #2990

Double TinySDF-rendered glyphs with new texScale glyph property.
Change topAdjustment/leftAdjustment to make 2x glyphs match 1x positions.

Video clip of before/after:

cjk-pixel-align.mp4

This required adding 0.5 to both topAdjustment and leftAdjustment, otherwise the alignment was slightly off from the old glyphs, potentially being a "breaking" visual change (an extremely small one).

Launch Checklist

Confirm your changes do not include backports from Mapbox projects (unless with compliant license) - if you are not sure about this, please ask!
Briefly describe the changes in this PR.
Link to related issues.
Include before/after visuals or gifs if this PR includes visual changes.
Write tests for all new functionality.
~~Document any changes to public APIs~~ - no changes to public APIs.
Post benchmark scores.
Add an entry to CHANGELOG.md under the ## main section.

codecov-commenter · 2023-08-21T07:26:08Z

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (9f7e2a9) 75.11% compared to head (f81059f) 75.22%.
Report is 7 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3006      +/-   ##
==========================================
+ Coverage   75.11%   75.22%   +0.10%     
==========================================
  Files         240      241       +1     
  Lines       19170    19244      +74     
  Branches     4325     4338      +13     
==========================================
+ Hits        14400    14476      +76     
+ Misses       4770     4768       -2

Files	Coverage Δ
src/render/glyph_manager.ts	`91.26% <100.00%> (+0.17%)`	⬆️
src/symbol/quads.ts	`60.58% <100.00%> (+0.23%)`	⬆️

... and 6 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

src/style/style_glyph.ts

src/render/glyph_manager.test.ts

HarelM · 2023-08-21T08:42:37Z

Is it possible to fix the alignment as part of this PR as well (you mentioned changing it)? see #1051.

bdon · 2023-08-21T08:46:18Z

Is it possible to fix the alignment as part of this PR as well (you mentioned changing it)? see #1051.

I don't believe so. I would intend this PR to make it into a minor version release or even a patch (no API enhancements).

If we change those hardcoded alignment numbers, we will almost certainly change the visual appearance of multilingual labels, which would be a "breaking" change for v4 possibly. We're in a bit of a bind from what I understand, because those numbers are hardcoded to be appropriate for the fonts Mapbox provided as part of their API - DIN Pro for latin and Arial Unicode for fallbacks.

Because MapLibre users are using whatever fonts they like, we don't have data to support an improvement there. The best we could we do is make recommendations on a fallback font - see maplibre/font-maker#16 - and then make multilingual labels work well for that recommendation.

In any case, it's beyond the scope of this PR.

HarelM · 2023-08-21T08:53:22Z

Ok, thanks for the info!

bdon · 2023-08-21T10:03:24Z

@HarelM what's the best way example for the Post benchmark scores. task?

HarelM · 2023-08-21T10:39:27Z

Try reading this:
https://github.com/maplibre/maplibre-gl-js/blob/95d6f43ed396555ca710445cb9cb25a378c84cc4/test/bench/README.md
I hope it's up to date...
You should be able to run this from command line and also through the browser.

bdon · 2023-08-21T13:44:58Z

HarelM · 2023-08-21T20:53:53Z

I think there are a few failing render tests due to this change.

HarelM · 2023-08-22T10:27:44Z

This looks like a fairly small change, so I think it can go in, I don't see and issues in the code.
I don't get a notification when changing a PR from draft to ready for review, so please request a review once you change the status of this PR.

bdon · 2023-08-22T10:30:58Z

I'm currently experimenting with refactoring the code to move textureScale out of metrics, because that breaks all the test fixture JSONS in the integration tests.

wipfli · 2023-09-23T09:28:56Z

Made a demo: https://github.com/wipfli/double-resolution-cjk-maplibre

wipfli · 2023-09-23T22:10:49Z

I added to my demo what would happen if we quadruple the resolution. Quite fun, it would allow MapLibre GL JS to render some quite large text labels without too much round corners

wipfli · 2023-09-23T22:12:15Z

I don't want to bring this pull request off track, but it is very appealing what you can do with local glyph generation. Will think about the options

src/style/style_glyph.ts

HarelM

I've left a small comment, approved otherwise.

HarelM · 2023-09-28T12:30:41Z

THANKS!!

HarelM · 2023-10-02T05:38:05Z

Can you resolve conflicts for the changelog file please?

HarelM · 2023-10-02T13:37:39Z

THANKS!
I would expect the CJK browser test to fail, shouldn't it?
https://github.com/maplibre/maplibre-gl-js/blob/0e9a4843c91b137c3681aa8379eb2e3c8a85fe6f/test/integration/browser/browser.test.ts#L168C7-L168C7
It basically checks image equality of what is rendered in the browser, but I don't see it failing here and I don't see a change in the test as part of this PR.
What am I missing?

bdon · 2023-10-03T02:52:06Z

not sure about this test case - at least locally I can delete the entire code block at

maplibre-gl-js/test/integration/browser/browser.test.ts

Line 170 in 0e9a484

await page.evaluate(() => {

and the test still passes. compareByPixelmatch always returns 0 (for all platforms). Can you verify you see the same?

bdon · 2023-10-03T03:09:01Z

Is this parse used in other test suites?

maplibre-gl-js/test/integration/browser/browser.test.ts

Line 240 in 0e9a484

actualPng.parse(actualBuff);

maybe we need to use these? https://github.com/pngjs/pngjs#sync-api

wipfli · 2023-10-03T05:28:56Z

Maybe the pixel diff threshold of the test is too low to detect the improvement of this pull request ...

HarelM · 2023-10-03T05:31:39Z

Nope, the CJK map is empty, so nothing is drawn.
I'm not sure how much time this test is broken. :-(

HarelM · 2023-10-03T05:40:09Z

Wait, not sure it is empty, I'll check if I can add a background...

HarelM · 2023-10-03T06:18:00Z

I'll open a PR shortly with the relevant fixes. Seem like this test is broken for a long time... :-(

HarelM · 2023-10-03T08:37:44Z

I've opened a PR: #3162.
Looking at what I did there and the fact that we are running render test in the browser now, I would expect to see some failures in render tests that are using CJK characters.
If this is not the case, it might be because the threshold is OK as this is a subtle change in the font.
Can you run the render test on this branch with UPDATE env parameter to see how these changes look in the render test actual vs expected pngs?
If the change is subtle, as can be seen in the PR comment I can rest assure that we are testing what we expect.
Sorry for nagging, I just want to make sure our tests are actually testing what they should.

HarelM · 2023-10-03T09:20:48Z

I've decided to remove the CJK test there and count on the render tests.
So the only question left for this PR is if the render tests related to CJK are behaving as expected - please verify this and I'll merge this PR.
Thanks for all the hard work!

bdon · 2023-10-05T08:05:55Z

@HarelM all the render tests may be passing because the test harness disables local ideograph rendering (the exact code path this PR affects) by default:

https://github.com/maplibre/maplibre-gl-js/blob/main/test/integration/render/run_render_tests.ts#L538

I don't think there are any instances where this code is tested by render tests. If we enable the option in a render test, the exact pixel output will depend on platform.

bdon · 2023-10-05T10:54:04Z

Created a new test using the old code at #3166 we can merge first before this PR

HarelM · 2023-10-05T11:29:16Z

Sure, why not.

* Port CJK browser test to render test [#3006, #3162] * add platform-specific expected images for local ideographs render test. [#3166] * Ubuntu expected is tofu * github actions ubuntu runner: apt install CJK fonts for render tests [#3166] * Commit expected Ubuntu local ideographs image with noto fonts installed from apt [#3166]

wipfli · 2023-10-06T21:22:34Z

Thanks bringing higher resolution CJK to MapLibre GL JS, @bdon. Can you open an issue in MapLibre Native that we remember to do the same there?

* localIdeographs render test set pixelRatio = 2 [#3006] * localIdeographs render test commit 2x size expected images [#3006]

* Double TinySDF-rendered glyphs with new texScale glyph property * Change topAdjustment/leftAdjustment to make 2x glyphs match 1x positions

CHANGELOG.md

HarelM · 2023-10-08T11:04:42Z

Can we push this forward?
I would like to have this in, in the next version.
Let me know what else is needed here in order to merge this.

HarelM · 2023-10-10T13:59:20Z

@bdon any chance to fix that last few comments?

bdon · 2023-10-11T10:25:50Z

The default allowed diff of 0.00025 is not sensitive enough to catch the difference here:

visually it looks like the correct change in one of the 3 platforms (not sure what's going on in the other 2) with a diff of 0.00006614583333333334

(gif attached)

diff.mp4

bdon · 2023-10-11T11:07:15Z

I changed the test to use much larger text sizes over the default, which should make it more sensitive to pixel changes

wipfli · 2023-11-21T16:26:40Z

Can you open an issue in MapLibre Native that we remember to do the same there?

@bdon did you open a ticket in MapLibre Native?

bdon · 2023-11-22T02:55:46Z

@wipfli I haven't. I can open one when I have a plan to work on it, but I don't right now.

wipfli · 2023-11-22T05:14:51Z

Ah easy. @louwers can you open a ticket for double resolution cjk in native? It is just that we keep track of parity tasks. @bdon you don't have to work on this in native, it is just for us to track...

HarelM reviewed Aug 21, 2023

View reviewed changes

src/style/style_glyph.ts Outdated Show resolved Hide resolved

bdon commented Aug 21, 2023

View reviewed changes

src/render/glyph_manager.test.ts Show resolved Hide resolved

bdon marked this pull request as ready for review September 28, 2023 09:51

HarelM reviewed Sep 28, 2023

View reviewed changes

src/style/style_glyph.ts Outdated Show resolved Hide resolved

HarelM approved these changes Sep 28, 2023

View reviewed changes

bdon mentioned this pull request Oct 5, 2023

Port CJK browser test to render test [#3006, #3162] #3166

Merged

8 tasks

bdon mentioned this pull request Oct 7, 2023

localIdeographs render test set pixelRatio = 2 [#3006] #3186

Merged

8 tasks

HarelM pushed a commit that referenced this pull request Oct 7, 2023

localIdeographs render test set pixelRatio = 2 [#3006] (#3186)

9f7e2a9

* localIdeographs render test set pixelRatio = 2 [#3006] * localIdeographs render test commit 2x size expected images [#3006]

bdon added 7 commits October 7, 2023 16:01

Improve CJK text rendering [#2990]

deb0e5b

* Double TinySDF-rendered glyphs with new texScale glyph property * Change topAdjustment/leftAdjustment to make 2x glyphs match 1x positions

rename texScale to textureScale [#2990]

37704bf

set textureScale to 1 for image SDFs [#2990]

48c6711

Simplify implementation to an optional boolean [#2990]

ccdc603

resolve CHANGELOG.md conflict [#3006]

a690ed2

move comment [#3006]

93915c7

set allowed diff to 0 for localIdeographs [#3006]

2236c16

HarelM reviewed Oct 8, 2023

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

fix duplicate entry in CHANGELOG

3a57a57

bdon added 2 commits October 11, 2023 06:31

Make local ideographs test font sizes much bigger [#3006]

1f938af

add expected images for ubuntu/windows [#3006]

f81059f

HarelM approved these changes Oct 11, 2023

View reviewed changes

HarelM enabled auto-merge (squash) October 11, 2023 11:25

HarelM merged commit c162f49 into maplibre:main Oct 11, 2023
14 checks passed

bdon deleted the cjk-glyph-texscale branch November 22, 2023 02:55

louwers mentioned this pull request Nov 22, 2023

Double Resolution Local Glyph Rasterization for CJK maplibre/maplibre-native#1887

Open

Improve CJK text rendering [#2990] #3006

Improve CJK text rendering [#2990] #3006

Conversation

bdon commented Aug 21, 2023 • edited

Launch Checklist

codecov-commenter commented Aug 21, 2023 • edited

Codecov Report

HarelM commented Aug 21, 2023

bdon commented Aug 21, 2023

HarelM commented Aug 21, 2023

bdon commented Aug 21, 2023

HarelM commented Aug 21, 2023

bdon commented Aug 21, 2023 • edited

HarelM commented Aug 21, 2023

HarelM commented Aug 22, 2023

bdon commented Aug 22, 2023 • edited

wipfli commented Sep 23, 2023

wipfli commented Sep 23, 2023

wipfli commented Sep 23, 2023

HarelM left a comment

Choose a reason for hiding this comment

HarelM commented Sep 28, 2023

HarelM commented Oct 2, 2023

HarelM commented Oct 2, 2023

bdon commented Oct 3, 2023 • edited

bdon commented Oct 3, 2023

wipfli commented Oct 3, 2023

HarelM commented Oct 3, 2023

HarelM commented Oct 3, 2023

HarelM commented Oct 3, 2023

HarelM commented Oct 3, 2023

HarelM commented Oct 3, 2023

bdon commented Oct 5, 2023

bdon commented Oct 5, 2023

HarelM commented Oct 5, 2023

wipfli commented Oct 6, 2023

HarelM commented Oct 8, 2023

HarelM commented Oct 10, 2023

bdon commented Oct 11, 2023

bdon commented Oct 11, 2023

wipfli commented Nov 21, 2023

bdon commented Nov 22, 2023

wipfli commented Nov 22, 2023

bdon commented Aug 21, 2023 •

edited

codecov-commenter commented Aug 21, 2023 •

edited

bdon commented Aug 21, 2023 •

edited

bdon commented Aug 22, 2023 •

edited

bdon commented Oct 3, 2023 •

edited