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

Add adjustment to rendering glyphs #1005

Merged
merged 6 commits into from
Feb 17, 2022

Conversation

Kanahiro
Copy link
Contributor

@Kanahiro Kanahiro commented Feb 16, 2022

close #1002

In based on discussion in the issue and mapbox/tiny-sdf#44 (comment), adjustment codes are added, results are following, it seems good. Someone who review my codes, please judge whether these codes are not backport to mapbox-gl-js.

before

スクリーンショット 2022-02-16 20 49 22

after(topAdjustment = 27)

スクリーンショット 2022-02-16 20 48 54

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.
  • Include before/after visuals or gifs if this PR includes visual changes.
  • Write tests for all new functionality.
  • Document any changes to public APIs.
  • Post benchmark scores.
  • Manually test the debug page.
  • Suggest a changelog category: bug/feature/docs/etc. or "skip changelog".
  • Add an entry inside this element for inclusion in the maplibre-gl-js changelog: <changelog></changelog>.

@Kanahiro Kanahiro changed the title #1002 CJK fonts adjustment Add adjustment to rendering glyphs Feb 16, 2022
@HarelM
Copy link
Collaborator

HarelM commented Feb 16, 2022

Thanks for taking the time to open this PR!
Do you think you can try and add a render test so that this won't happen in the future?
I think the style you linked in the issue can be a great input to the render tests.
See here for more info:
https://github.com/maplibre/maplibre-gl-js/tree/main/test/integration#writing-new-tests

CHANGELOG.md Outdated Show resolved Hide resolved
@github-actions
Copy link
Contributor

github-actions bot commented Feb 16, 2022

Bundle size report:

Size Change: -184 B
Total Size Before: 194 kB
Total Size After: 194 kB

Output file Before After Change
maplibre-gl.js 185 kB 185 kB +3 B
maplibre-gl.css 9.43 kB 9.25 kB -187 B
ℹ️ View Details
Source file Before After Change
src/render/glyph_manager.ts 946 B 949 B +3 B

@Kanahiro
Copy link
Contributor Author

Kanahiro commented Feb 17, 2022

I've tried to make a test with same style in the issue but the issue is not reproduced in test-render, I'm not sure but is this only on browser problem...?

// style.json
{
    "version": 8,
    "glyphs": "local://glyphs/{fontstack}/{range}.pbf",
    "sources": {
        "sample": {
            "type": "geojson",
            "data": {
                "type": "FeatureCollection",
                "features": [
                    {
                        "type": "Feature",
                        "geometry": {
                            "type": "Point",
                            "coordinates": [0, 0]
                        },
                        "properties": {
                            "name_en": "abc",
                            "name_ja": "あいう",
                            "name_ch": "阿衣乌"
                        }
                    }
                ]
            }
        }
    },
    "layers": [
        {
            "id": "sample-text-left",
            "type": "symbol",
            "source": "sample",
            "layout": {
                "text-anchor": "top",
                "text-field": "{name_ja}\n{name_en}",
                "text-font": ["NotoCJK"],
                "text-offset": [-10, 0]
            }
        },
        {
            "id": "sample-text-center",
            "type": "symbol",
            "source": "sample",
            "layout": {
                "text-anchor": "top",
                "text-field": "{name_ja}{name_en}{name_ch}",
                "text-font": ["NotoCJK"],
                "text-offset": [0, 0]
            }
        },
        {
            "id": "sample-text-right",
            "type": "symbol",
            "source": "sample",
            "layout": {
                "text-anchor": "top",
                "text-field": "{name_en}\n{name_ch}",
                "text-font": ["NotoCJK"],
                "text-offset": [10, 0]
            }
        }
    ]
}

Even though use completly same style to https://jsbin.com/yocutiwata/edit?html,output, the problem is not reproduced.

@HarelM
Copy link
Collaborator

HarelM commented Feb 17, 2022

This might explain why we didn't see this issue... :-/
Can you fix lint?
Can you share the images that were created? You see the text correctly in the generated image? Can you share the image here?

@Kanahiro
Copy link
Contributor Author

expected

This image rendered with above style.json, this is correct.
Positions of texts are not changed even whenconst top Adjustment = 27 is not written.

@HarelM
Copy link
Collaborator

HarelM commented Feb 17, 2022

OK, I'll open an issue to see how this can be improved in the future in terms of testing in the browser maybe and not using node execution, as it seems that there are changes between the outputs (which is not that surprising to be honest).
In any case, this issue still needs to be fixed regardless, so please fix the lint errors and I'll merge this.
Thanks again for all your work!

@HarelM HarelM merged commit 964bbef into maplibre:main Feb 17, 2022
wipfli added a commit that referenced this pull request Feb 24, 2022
* 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] <support@github.com>

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] <support@github.com>

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] <support@github.com>

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/icon-text-fit-1x@2x.json

* restore files

Co-authored-by: acalcutt <acalcutt@worcester.edu>

* 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 <74932975+birkskyum@users.noreply.github.com>
Co-authored-by: Yuri Astrakhan <yuriastrakhan@gmail.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Harel M <harel.mazor@gmail.com>
Co-authored-by: Andrew Calcutt <acalcutt@techidiots.net>
Co-authored-by: acalcutt <acalcutt@worcester.edu>
Co-authored-by: vanilla-lake <95251033+vanilla-lake@users.noreply.github.com>
Co-authored-by: Astrid <astridx@users.noreply.github.com>
Co-authored-by: Kanahiro Iguchi <mediterranean1769@gmail.com>
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.

CJK-fonts text offset ignored
2 participants