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

Character alignment issue when CJK glyphs are mixed with other characters. #1051

Open
peaceshi opened this issue Mar 5, 2022 · 12 comments
Open
Labels
bug Something isn't working enhancement New feature or request PR is more than welcomed Extra attention is needed

Comments

@peaceshi
Copy link

peaceshi commented Mar 5, 2022

maplibre-gl-js version: 2.1.7

browser: Microsoft Edge 99.0.1150.30(latest stable)

I use localIdeographFontFamily to load the font.

{
            type: "FeatureCollection",
            features: [
              {
                type: "Feature",
                properties: {
                  description: "璃月港A8璃月港\nGgLlIi\n寄了JjPpQqYy寄了"
                },
                geometry: {
                  type: "Point",
                  coordinates: [0, 0]
                }
              }
            ]
          }

image

Seems like mapbox fixed it in a way. But I can find it.
https://www.mapbox.com/blog/efficient-pitched-tile-loading-precise-cjk-labels-and-js-promises-mapbox-gl-js-v2-1-1
image

Some useful link:
mapbox/mapbox-gl-js#10390
mapbox/mapbox-gl-js#8041
mapbox/mapbox-gl-js#8041 (comment)

@HarelM
Copy link
Collaborator

HarelM commented Mar 5, 2022

We can't copy code from mapbox...
I'm not sure how to properly solve this, a PR would help a lot in this case.

@peaceshi
Copy link
Author

peaceshi commented Mar 5, 2022

mapbox/mapbox-gl-js#10298
I will try PR later.

@HarelM
Copy link
Collaborator

HarelM commented Aug 21, 2022

@peaceshi any updates on this?

@HarelM HarelM added bug Something isn't working enhancement New feature or request PR is more than welcomed Extra attention is needed labels Aug 21, 2022
@spatialillusions
Copy link

Ping on this.
I have built an offline web app that you can run from an USB stick, and if would be awesome if I could use local fonts instead of including all font ranges.
Maybe it would be as simple as returning true from _doesCharSupportLocalGlyph if a localFont property is set.

@HarelM
Copy link
Collaborator

HarelM commented May 11, 2023

I'm not sure I understand how local fonts are related to this issue... Can you please explain?

@spatialillusions
Copy link

Sorry, I might have been unclear.

maplibre today got the property localIdeographFontFamily, for locally overriding generation of glyphs to reduce the number of requests for font ranges. This uses a local font to generate the glyphs instead. But like @peaceshi posted, it looks like it creates misalignment between glyphs from local generation and server based glyph ranges.

In the screenshot posted it shows that the other map library introduced another property called localFontFamily, that if used overrides all font settings from the style, and forces generation of all glyphs using the local font, and this solves the misalignment.

As far as I can see in the code, there is a check here

if (!this._doesCharSupportLocalGlyph(id)) {
to see if the glyph requested is in CJK ranges, and if not, it simply returns. If I could force that to return true for other glyphs, it would render them using the local font that we have set with localIdeographFontFamily as well.
To get it to work in the same way as localFontFamily does, that also uses font weight and other font properties would require some additional work, but would make it possible to render styles without access to any other font ranges at all.

Please let me know if I can give you any additional information.

@HarelM
Copy link
Collaborator

HarelM commented May 11, 2023

There is a work being done as part of #2458 which will allow this I believe, it might solve this problem as well, IDK...
CC @wipfli

@wipfli
Copy link
Contributor

wipfli commented May 12, 2023

@spatialillusions if I understand you correctly, you would like to use the CJK code path for text in all languages and writing systems. #2458 is a proof-of-concept for this. It works OK for latin but I would love to have a bit sharper text first, maybe by doubling the TinySDF resolution as @bdon has experimented with in the past.

@wipfli
Copy link
Contributor

wipfli commented May 12, 2023

@bdon you say the easiest would be to double the resolution of everything, right?

@1ec5
Copy link
Contributor

1ec5 commented May 19, 2023

@spatialillusions if I understand you correctly, you would like to use the CJK code path for text in all languages and writing systems. #2458 is a proof-of-concept for this. It works OK for latin but I would love to have a bit sharper text first, maybe by doubling the TinySDF resolution as @bdon has experimented with in the past.

#2458 goes beyond using TinySDF for all text. This library could remove the CJK-only constraint without worrying about the text segmentation issue. The only regression would be a combining dotted circle under combining diacritics that are already laid out incorrectly anyways.

Doubling TinySDF resolution would be good for parity with Mapbox GL JS, which made that change in v2.1.0 along with adding the localFontFamily option (mapbox/mapbox-gl-js#10298, PR only, no issue available). Apparently some performance optimizations were necessary to offset the cost of the larger sprites.

@bdon
Copy link
Contributor

bdon commented May 21, 2023

@bdon you say the easiest would be to double the resolution of everything, right?

@wipfli I think the easiest would be to double the resolution of all glyph textures - it means server-rendered glyphs (read from a 0-255.pbf etc) would be upsampled - this would quadruple the texture memory required for text atlases, but I don't have a good sense if that is a dealbreaker or not.

Otherwise, we could make the texture size conditional on whether the SDF was generated via tinySDF (PoC here: https://github.com/bdon/maplibre-gl-js/tree/bigger-tinysdf) but that complicates the logic elsewhere.

@wipfli
Copy link
Contributor

wipfli commented May 22, 2023

I think if the map runs OK on an older phone, then it should be fine doubling the glyph resolution globally. We can define what old phone means, but for example a galaxy S6 might be a good choice.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request PR is more than welcomed Extra attention is needed
Projects
None yet
Development

No branches or pull requests

6 participants