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

Upgrade tiny sdf with latest typings #525

Merged
merged 1 commit into from
Oct 20, 2021
Merged

Upgrade tiny sdf with latest typings #525

merged 1 commit into from
Oct 20, 2021

Conversation

HarelM
Copy link
Collaborator

@HarelM HarelM commented Oct 18, 2021

Launch Checklist

Seems like the tiny sdf upgrade was not done entirely correctly, I have submitted a PR to add typings and after I used it here it seems that the return value was changed too, but the code was not updated here.
I'm not sure how to test this unfortunately... :-(

See: mapbox/tiny-sdf#39

  • 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.

@github-actions
Copy link
Contributor

Bundle size report:

Size Change: +5 B
Total Size Before: 194 kB
Total Size After: 194 kB

Output file Before After Change
maplibre-gl.js 185 kB 185 kB +5 B
maplibre-gl.css 9.49 kB 9.49 kB 0 B
ℹ️ View Details
Source file Before After Change
rollup/build/tsc/src/render/glyph_manager.js 905 B 909 B +4 B

@HarelM HarelM mentioned this pull request Oct 18, 2021
@wipfli
Copy link
Contributor

wipfli commented Oct 18, 2021

I'm not sure how to test this unfortunately...

@JinIgarashi can you help us with this?

bitmap: new AlphaImage({width: 30, height: 30}, tinySDF.draw(String.fromCharCode(id))),
bitmap: new AlphaImage({width: 30, height: 30}, tinySDF.draw(String.fromCharCode(id)).data),
Copy link
Contributor

Choose a reason for hiding this comment

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

In version 1 of TinySDF they had this:

TinySDF.prototype.draw = function (char) {
    return this._draw(char, false).data;
};

See https://github.com/mapbox/tiny-sdf/blob/2b1efa1ba63233c920f1116dbab64dc13a62c7e4/index.js#L152-L154 meaning that this change here seems correct to me.

@HarelM
Copy link
Collaborator Author

HarelM commented Oct 19, 2021

I would still want to see if I can find a way to add a test to make sure this is working as expected.
If I can't find a way in a reasonable time I'll merge.

@HarelM
Copy link
Collaborator Author

HarelM commented Oct 20, 2021

I'll merge this and open an issue...

@HarelM
Copy link
Collaborator Author

HarelM commented Oct 20, 2021

#546

@HarelM HarelM merged commit b7bfaf4 into main Oct 20, 2021
@HarelM HarelM deleted the fix-tiny-sdf-2-0-4 branch October 20, 2021 18:22
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