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

Fix glyph width for characters in RTL scripts #26

Merged
merged 2 commits into from
Jan 25, 2021
Merged

Conversation

ChrisLoer
Copy link
Contributor

measureText is not directly measuring glyphs -- it's actually measuring a run of text. When we provide a character from an RTL script, it lays the single-character run of text out right-to-left, and the right of the bounding box ends up being less than the left of the bounding box. TinySDF tries to allocate a negative-size array for the glyph and errors out.

I think there may be more work to do to get the left metric correct, but this PR at least avoids the crash and gives something renderable.

@mourner

@ChrisLoer ChrisLoer requested a review from mourner January 25, 2021 07:05
@@ -1,6 +1,6 @@
{
"name": "@mapbox/tiny-sdf",
"version": "1.2.0",
"version": "1.2.1",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reminder to avoid changing the version in the PR, and do it separately with npm version in the main branch.

Copy link
Member

@mourner mourner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 given a fast-forward merge to keep the version commit / tag intact in linear history

@ChrisLoer ChrisLoer merged commit 66658a2 into master Jan 25, 2021
@mourner mourner deleted the metrics_fixes branch January 25, 2021 07:17
@github-pages github-pages bot temporarily deployed to github-pages January 25, 2021 07:17 Inactive
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.

2 participants