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.

Sign up for GitHub

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

Jump to bottom

CJK-fonts text offset ignored #1002

Closed
Kanahiro opened this issue Feb 14, 2022 · 11 comments · Fixed by #1005
Closed

CJK-fonts text offset ignored #1002

Kanahiro opened this issue Feb 14, 2022 · 11 comments · Fixed by #1005

Comments

@Kanahiro
Copy link
Contributor

Thanks for your great contributions. CJK fonts become shown in v2.1.6 but another problem occurs.

maplibre-gl-js version: v2.1.6

browser: Chrome 97.0.4692.99

Steps to Trigger Behavior

  1. add layer to show text in CJK fonts and set text-offset to it
  2. text-offset looks ignored

Link to Demonstration

same style apllied to two versions maplibre-gl-js

v2.1.6: https://jsbin.com/regowuluwo/edit?html,output
v1.15.2: https://jsbin.com/duzinusoda/edit?html,output

Expected Behavior

Actual Behavior
@HarelM
Copy link
Collaborator

HarelM commented Feb 14, 2022 •
edited
Loading

Thanks for taking the time to report this!
Here's the screenshots from the js bin:
1.15.2:
image
2.1.6:
image
I don't recall changing anything in the area of text-offset though :-(

Since the style file is very large, is there any change you could create a style file that only contains this issue?
If you could do that it will help easing the process of creating a render test in order to make sure this won't break in the future.

@Kanahiro

This comment was marked as outdated.

Sign in to view
@HarelM
Copy link
Collaborator

HarelM commented Feb 14, 2022

Thanks!! Sorry for nagging, but can you create a small geojson source with limited features (1-2) and use that so the entire code will be in that page (I.e. Inline style)?

@Kanahiro
Copy link
Contributor Author

Kanahiro commented Feb 15, 2022 •
edited
Loading

v2.1.6
https://jsbin.com/cuqalowavi/1/edit?html,output

v1.15.2
https://jsbin.com/kiwuvudubi/edit?html,output

Are these good?
And I realize text offset ignored in title may be wrong because CJK-text is misplaced when vertical text-offset is zero.
You can see in above samples CJK fonts are anytime placed somewhat upper than alphabets.

@HarelM
Copy link
Collaborator

HarelM commented Feb 15, 2022

@Kanahiro Great! Thanks!

@Kanahiro
Copy link
Contributor Author

Kanahiro commented Feb 16, 2022 •
edited
Loading

maplibre-gl-js/src/render/glyph_manager.ts

Line 206 in 925b85e

top: char.glyphTop || -8,

This looks caused by this line.
Although I'm not familiar with tiny-sdf, I think tiny-sdf metrics structure has been changed, is right?

We could adjust in interger-value manually but must be careful for backport mapbox-gl-js.

@HarelM
Copy link
Collaborator

HarelM commented Feb 16, 2022

Thanks for digging into this.
We have upgraded tiny-sdf from version 1.x to 2.x and I think this is what introduced these breaking changes.
unfortunately we can't backport the code from mapbox.
I'll see if I can open an issue in the tiny-sdf repo to see if they can guide us on how to solve this properly.
Thanks again for pushing this forward!

@HarelM HarelM mentioned this issue Feb 16, 2022
Closed
@HarelM
Copy link
Collaborator

HarelM commented Feb 16, 2022

I've opened the following issue.
mapbox/tiny-sdf#44
Worst case, we can revert to version 1.x I guess...

@HarelM
Copy link
Collaborator

HarelM commented Feb 16, 2022

Quoting the relevant part of the above issue:
The adjustment GL JS uses is basically top = glyphTop - 27.
@Kanahiro can you check if this solves this issue and maybe create a PR with the fix?

@Kanahiro Kanahiro mentioned this issue Feb 16, 2022
Merged
9 tasks
@Kanahiro
Copy link
Contributor Author

I've just published my PR, please check it.

@HarelM
Copy link
Collaborator

HarelM commented Feb 16, 2022

Thanks!!! I've added my comments there.

Kanahiro added a commit to Kanahiro/maplibre-gl-js that referenced this issue Feb 17, 2022
@Kanahiro
add alphabet and cjk text test maplibre#1002
1a67cda
@HarelM HarelM mentioned this issue Feb 17, 2022
Closed
HarelM pushed a commit that referenced this issue Feb 17, 2022
@Kanahiro
Add adjustment to rendering glyphs (#1005)
964bbef 
* add topAdjustment for glyph

* add topAdjustment for glyph

* update changelog

* add issue number

* add alphabet and cjk text test #1002

* fix lint error
This was referenced Feb 18, 2022
Closed
Merged
wipfli added a commit that referenced this issue Feb 24, 2022
@wipfli @birkskyum
@nyurik @dependabot @HarelM @acalcutt @vanilla-lake @astridx @Kanahiro
[terrain3d] Merge main (#1021)
16bbd6d 
* Migrate expression tests to jest (#965)

* Revert "Move benchmarks to ES modules (#964)" (#969)

This reverts commit aa8ed9d.

* Migrate query tests from puppeteer to playwright (#966)

* Fix web worker in watch mode (#968)

* Fix web worker in watch mode

* Add webworker support throughout

* Don't change rollup for style-spec and benchmarks

* Revert "Don't change rollup for style-spec and benchmarks"

This reverts commit 62a7e14.

* Remove duplicate node-resolve configuration

* Simplify build pipeline (#961)

* Remove build-tsc compile step

* re-enable failing style-spec test

* cleanup

* cleanup

* fix style-spec-test

* Create codeql-analysis.yml

* Bump simple-get from 3.1.0 to 3.1.1 (#971)

Bumps [simple-get](https://github.com/feross/simple-get) from 3.1.0 to 3.1.1.
- [Release notes](https://github.com/feross/simple-get/releases)
- [Commits](feross/simple-get@v3.1.0...v3.1.1)

---
updated-dependencies:
- dependency-name: simple-get
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Bump nanoid from 3.1.30 to 3.2.0 (#973)

Bumps [nanoid](https://github.com/ai/nanoid) from 3.1.30 to 3.2.0.
- [Release notes](https://github.com/ai/nanoid/releases)
- [Changelog](https://github.com/ai/nanoid/blob/main/CHANGELOG.md)
- [Commits](ai/nanoid@3.1.30...3.2.0)

---
updated-dependencies:
- dependency-name: nanoid
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Bump cached-path-relative from 1.0.2 to 1.1.0 (#972)

Bumps [cached-path-relative](https://github.com/ashaffer/cached-path-relative) from 1.0.2 to 1.1.0.
- [Release notes](https://github.com/ashaffer/cached-path-relative/releases)
- [Commits](https://github.com/ashaffer/cached-path-relative/commits)

---
updated-dependencies:
- dependency-name: cached-path-relative
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Move browser test to playwright (#974)

* Bump ts-node for 3x faster postinstall (#975)

* Remove web worker replacement (#976)

* Attribution default open for osm (#795)

* Update CHANGELOG.md

* Revert "Update CHANGELOG.md"

This reverts commit 0b81a41.

* attribution fixes (from astridx)

a1272d9

b9b0370

* Update .gitignore

* fix missing new line lint complained about

* Revert "attribution fixes (from astridx)"

This reverts commit 2031d8e.

* Default compact to be open by default

Default compact button to be open by default, to make OpenStreetMap attibutions to show by default like mentioned in ( #205 )

This uses an idea from ( #205 (comment) ) to show compact open by default, but close it when dragged.

* remove test string I forgot to remove

* fix compact button showing when there are no attributions

* update whitespace trim to es6 syntax

* fixes after testing various states of compact

* revert back so it is open when starting in fullscreen

* revert .gitignore

* fix lint errors

* fix tests

Most of these fails because I made the compact button not show if attributes are empty.

The other test fail because when switching between >640 and <=640 the open attribute is now added

* lint

* Update package-lock.json

* Update package-lock.json

* Update CHANGELOG.md

* delete problematic files

* put back deleted files

* test/integration/assets/sprites/[email protected]

* restore files

Co-authored-by: acalcutt <[email protected]>

* Fix benchmarks (#984)

* Move bench under `test` folder (#979)

* Add typeof guard to performance variable (#986)

* Fixes #768

* Fix comment and add changelog

* Release 2.1.2 Version (#987)

* Release 2.1.2 Version

* Fix changelog according to comments

* Fix release.yml due to bench folder move (#988)

* lint function-url-quotes/ (#983)

* Bump to v2.1.3 - Fix postinstall error `ts-node` not found in non-dev installs (#991)

* Bump to v2.1.4 - Fix missing `postinstall.js` file in npm publish (#992)

* Bump to v2.1.5-pre.1 - Publish empty `postinstall.js` file (#994)

* Bump to v2.1.5 (#996)

* Bump to v2.1.6-pre.1 - Fix `dist/package.json` (#998)

* Bump to v2.1.6 (#999)

* prefix (#1004)

* correct done (#1006)

* Add adjustment to rendering glyphs (#1005)

* add topAdjustment for glyph

* add topAdjustment for glyph

* update changelog

* add issue number

* add alphabet and cjk text test #1002

* fix lint error

* Simplify render tests (#1003)

Fixes #1008 

* Initial commit - half of the tests are failing

* Missing file

* Remove template rendering

* Fix usage of localizeUrls

* Move harness and server to render folder, simplify render code to be sepcific.

* Move function outside and add some typings

* lint

* Add typings, move compare results outside

* Remove server usage and replace it by file system reading.

* Move xhr mocking to main flow

* Move functions outside the render function

* Move creation of tests to render.ts, added more typings

* Move suite implementation into main flow file, add some typings.

* lint

* Moved all logic to a single file that runs the render tests

* remove color from harness

* Remove jest run command

* Small fixes

* lint

* Cleanup

* More cleanup

* Remove harness and move it to a single file. Remove the jest version.

* Remove ignore related code and tests

* Remove ignore, add types

* Add more types

* lint

* Migrate actual image generation to not use Buffer

* lint

* Fix CI run

* Fix CI properly this time...

* Revert png change

* Revert only buffer usage

* Revert png and buffer

* Simplify png in mock file

* Move to use uint8array and change throshold

* Added diff info

* remove specific threshold from test as it fails

* Fix typo

* Fix according to code review

* Husky pre-commit do not fix lint (#1019)

* Fix css

* Use main package-lock.json

Co-authored-by: Birk Skyum <[email protected]>
Co-authored-by: Yuri Astrakhan <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Harel M <[email protected]>
Co-authored-by: Andrew Calcutt <[email protected]>
Co-authored-by: acalcutt <[email protected]>
Co-authored-by: vanilla-lake <[email protected]>
Co-authored-by: Astrid <[email protected]>
Co-authored-by: Kanahiro Iguchi <[email protected]>
@bdon bdon mentioned this issue Aug 16, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Add adjustment to rendering glyphs Kanahiro/maplibre-gl-js
2 participants
@HarelM @Kanahiro